[GitHub] [fineract] ptuomola commented on a change in pull request #1286: Removed string concatenated SQL & sqlSearch from Staff (Fineract-854)

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

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



##########
File path: fineract-provider/src/main/java/org/apache/fineract/organisation/staff/api/StaffApiResource.java
##########
@@ -118,7 +118,6 @@ public StaffApiResource(final PlatformSecurityContext context, final StaffReadPl
             @ApiResponse(responseCode = "200", description = "OK", content = @Content(array = @ArraySchema(schema = @Schema(implementation = StaffApiResourceSwagger.GetStaffResponse.class)))),
             @ApiResponse(responseCode = "200", description = "GET https://DomainName/api/v1/staff?status={ACTIVE|INACTIVE|ALL}", content = @Content(schema = @Schema(implementation = StaffApiResourceSwagger.GetStaffResponse.class))) })
     public String retrieveStaff(@Context final UriInfo uriInfo,
-            @QueryParam("sqlSearch") @Parameter(description = "sqlSearch") final String sqlSearch,
             @QueryParam("officeId") @Parameter(description = "officeId") final Long officeId,

Review comment:
       Are we sure that no client is calling this - i.e. will this change make an impact to one of the UIs? 

##########
File path: fineract-provider/src/main/java/org/apache/fineract/organisation/staff/service/StaffReadPlatformServiceImpl.java
##########
@@ -204,49 +215,38 @@ public StaffData retrieveStaff(final Long staffId) {
     }
 
     @Override
-    public Collection<StaffData> retrieveAllStaff(final String sqlSearch, final Long officeId, final boolean loanOfficersOnly,
-            final String status) {
-        final String extraCriteria = getStaffCriteria(sqlSearch, officeId, loanOfficersOnly, status);
-        return retrieveAllStaff(extraCriteria, officeId);
+    public Collection<StaffData> retrieveAllStaff(final Long officeId, final boolean loanOfficersOnly, final String status) {
+        final SQLBuilder extraCriteria = getStaffCriteria(officeId, loanOfficersOnly, status);
+        return retrieveAllStaff(extraCriteria);
     }
 
-    private Collection<StaffData> retrieveAllStaff(final String extraCriteria, Long officeId) {
+    private Collection<StaffData> retrieveAllStaff(final SQLBuilder extraCriteria) {
 
         final StaffMapper rm = new StaffMapper();
         String sql = "select " + rm.schema();
-        final String hierarchy = this.context.authenticatedUser().getOffice().getHierarchy() + "%";
-        if (StringUtils.isNotBlank(extraCriteria)) {

Review comment:
       I think it would have actually been good to leave looking up hierarchy and adding the condition for it here. That way, regardless of how you call retrieveAllStaff, you can't retrieve things you are not allowed to see. Otherwise we are relying on the caller to add the condition for hierarchy - which they may forget...

##########
File path: fineract-provider/src/main/java/org/apache/fineract/organisation/staff/service/StaffReadPlatformServiceImpl.java
##########
@@ -159,7 +159,18 @@ public StaffData mapRow(final ResultSet rs, @SuppressWarnings("unused") final in
 
     @Override
     public Collection<StaffData> retrieveAllLoanOfficersInOfficeById(final Long officeId) {
-        return retrieveAllStaff(" office_id = ? and is_loan_officer=1 and o.hierarchy like ?", officeId);
+        final StaffMapper rm = new StaffMapper();
+        String sql = "select " + rm.schema();
+        SQLBuilder extraCriteria = new SQLBuilder();
+        extraCriteria.addCriteria(" office_id = ", officeId);
+        extraCriteria.addCriteria(" is_loan_officer= = ", 1);
+        final String hierarchy = this.context.authenticatedUser().getOffice().getHierarchy() + "%";
+        extraCriteria.addCriteria(" o.hierarchy like ", hierarchy);
+
+        sql += " " + extraCriteria.getSQLTemplate();
+        sql = sql + " order by s.lastname ";
+
+        return this.jdbcTemplate.query(sql, rm, extraCriteria.getArguments());

Review comment:
       Would we not want to do this "magic" of adding the condition for hierarchy and then executing the JDBC template only in one place? I.e. would it not be better to keep this code still in retrieveAllStaff and just call retrieveAllStaff with the right SQLBuilder from here? Or is there a reason why we want to duplicate the code here? 




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