[GitHub] [fineract] galovics commented on a diff in pull request #2731: [FINERACT-1678] Introducing synchronous COB execution for loan account changing APIs

[GitBox <gi...@apache.org>]

galovics commented on code in PR #2731:
URL: https://github.com/apache/fineract/pull/2731#discussion_r1015304993


##########
fineract-provider/src/main/java/org/apache/fineract/cob/service/LoanAccountLockServiceImpl.java:
##########
@@ -51,4 +51,11 @@ public boolean isLoanHardLocked(Long loanId) {
                         || LockOwner.LOAN_INLINE_COB_PROCESSING.equals(loanAccountLock.getLockOwner())) //
                 .filter(loanAccountLock -> StringUtils.isBlank(loanAccountLock.getError())).isPresent();
     }
+
+    @Override
+    public boolean isLoanSoftLocked(Long loanId) {
+        Optional<LoanAccountLock> loanAccountLockOptional = loanAccountLockRepository.findById(loanId);

Review Comment:
   I think it's not needed to fetch the actual entity. Let's write a custom repository method for this.
   Something like `existsByIdAndLockOwner`



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/config/SecurityConfig.java:
##########
@@ -90,7 +89,7 @@ protected void configure(HttpSecurity http) throws Exception {
                 .addFilterAfter(fineractInstanceModeApiFilter, SecurityContextPersistenceFilter.class) //
                 .addFilterAfter(tenantAwareBasicAuthenticationFilter(), FineractInstanceModeApiFilter.class) //
                 .addFilterAfter(loanCOBApiFilter, TenantAwareBasicAuthenticationFilter.class) //
-                .addFilterAfter(twoFactorAuthenticationFilter, BasicAuthenticationFilter.class); //
+                .addFilterAfter(twoFactorAuthenticationFilter, LoanCOBApiFilter.class); //

Review Comment:
   So where do the BasicAuthenticationFilter go?



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/filter/LoanCOBApiFilter.java:
##########
@@ -67,35 +70,51 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
             Iterable<String> split = Splitter.on('/').split(request.getPathInfo());
             Supplier<Stream<String>> streamSupplier = () -> StreamSupport.stream(split.spliterator(), false);
             boolean isGlim = isGlim(streamSupplier);
-            Long loanId = getLoanId(isGlim, streamSupplier);
-            if (isLoanLocked(loanId, isGlim)) {
-                reject(loanId, response);
+            Long loanIdFromRequest = getLoanId(isGlim, streamSupplier);
+            List<Long> loanIds = isGlim ? getGlimChildLoanIds(loanIdFromRequest) : Collections.singletonList(loanIdFromRequest);
+            if (isLoanHardLocked(loanIds)) {
+                reject(loanIdFromRequest, response);
+            } else if (isLoanSoftLocked(loanIds)) {
+                executeInlineCob(loanIds);
+                proceed(filterChain, request, response);
             } else {
                 proceed(filterChain, request, response);
             }
         }
     }
 
-    private boolean isBypassUser() {
-        return context.getAuthenticatedUserIfPresent().isBypassUser();
+    private void executeInlineCob(List<Long> loanIds) {
+        inlineLoanCOBExecutorService.lockLoanAccounts(loanIds);

Review Comment:
   Hmm, I'm thinking about the responsibilities. I think it'd be better if the service itself locks the loan accounts, don't you think?
   Like that's a unit in itself. You instruct the service "let's run the inline COB for these loan accounts" and you should expect that everything is taken care of.



##########
fineract-provider/src/main/java/org/apache/fineract/cob/service/InlineLoanCOBExecutorServiceImpl.java:
##########
@@ -121,7 +121,7 @@ private List<LoanAccountLock> getLoanAccountLocks(List<Long> loanIds) {
         return loanAccountLocks;
     }
 
-    private void execute(List<Long> loanIds, String jobName) {
+    public void execute(List<Long> loanIds, String jobName) {

Review Comment:
   Can't we add this method to the interface with a generic T type which refers to the entity ID type?
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org