[GitHub] [fineract] ptuomola commented on a change in pull request #1207: FINERACT-1095 Added status parameter in Clients API

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

ptuomola commented on a change in pull request #1207:
URL: https://github.com/apache/fineract/pull/1207#discussion_r463389318



##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/client/service/ClientReadPlatformServiceImpl.java
##########
@@ -258,6 +261,19 @@ private String buildSqlStringFromClientCriteria(String schemaSql, final SearchPa
             extraCriteria += " and c.display_name like ? ";
         }
 
+        if (status != null) {
+            final String lowerCaseStatus = status.toLowerCase();
+            Map<String, Integer> statusNumber = new HashMap<String, Integer>();
+            statusNumber.put("active", 300);
+            statusNumber.put("withdrawn", 800);
+            statusNumber.put("closed", 600);
+            statusNumber.put("rejected", 700);
+            statusNumber.put("pending", 100);

Review comment:
       If you look at org.apache.fineract.portfolio.client.domain.ClientStatus, there seems to be more values for this enum than shown here. Was there a reason why only this subset is relevant for this?
   
   Also for mapping from string to value,  would it make sense to put this into the ClientStatus class and make it reusable? The same way as for example for GLAccountType.fromString() or CalendarFrequencyType.fromString()? That way we would be maintaining the logic in one place only...




----------------------------------------------------------------
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.

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