You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/08/26 10:37:58 UTC
[GitHub] [fineract] thesmallstar opened a new pull request #1286: Removed string concatenated SQL & sqlSearch from Staff (Fineract-854)
thesmallstar opened a new pull request #1286:
URL: https://github.com/apache/fineract/pull/1286
Removed string concatenated SQL and use of SQLseach parameter from Staff,
I have looked closely in what I am changing but a detailed review would help since we probably don't have good tests coverage on fineract yet.
@awasum how do you think this as the project idea for outreachy this winter? WE NEED MORE TESTS!!
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar closed pull request #1286: Removed string concatenated SQL & sqlSearch from Staff (Fineract-854)
Posted by GitBox <gi...@apache.org>.
thesmallstar closed pull request #1286:
URL: https://github.com/apache/fineract/pull/1286
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on pull request #1286: Removed string concatenated SQL & sqlSearch from Staff (Fineract-854)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1286:
URL: https://github.com/apache/fineract/pull/1286#issuecomment-680815721
Argh! failed due to connection error, closing and opening this PR.
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar closed pull request #1286: Removed string concatenated SQL & sqlSearch from Staff (Fineract-854)
Posted by GitBox <gi...@apache.org>.
thesmallstar closed pull request #1286:
URL: https://github.com/apache/fineract/pull/1286
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on pull request #1286: Removed string concatenated SQL & sqlSearch from Staff (Fineract-854)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1286:
URL: https://github.com/apache/fineract/pull/1286#issuecomment-680960725
Failed again due to connection error.
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on pull request #1286: Removed string concatenated SQL & sqlSearch from Staff (Fineract-854)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1286:
URL: https://github.com/apache/fineract/pull/1286#issuecomment-687663016
@ptuomola updated, thanks for pointing that out.
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on a change in pull request #1286: Removed string concatenated SQL & sqlSearch from Staff (Fineract-854)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1286:
URL: https://github.com/apache/fineract/pull/1286#discussion_r478280829
##########
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:
Yes, we already confirmed that.
It was only used in loans and clients, for which I added extra status param,
----------------------------------------------------------------
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
[GitHub] [fineract] ptuomola merged pull request #1286: Removed string concatenated SQL & sqlSearch from Staff (Fineract-854)
Posted by GitBox <gi...@apache.org>.
ptuomola merged pull request #1286:
URL: https://github.com/apache/fineract/pull/1286
----------------------------------------------------------------
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
[GitHub] [fineract] ptuomola commented on a change in pull request #1286: Removed string concatenated SQL & sqlSearch from Staff (Fineract-854)
Posted by 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
[GitHub] [fineract] thesmallstar commented on pull request #1286: Removed string concatenated SQL & sqlSearch from Staff (Fineract-854)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1286:
URL: https://github.com/apache/fineract/pull/1286#issuecomment-683467331
@ptuomola thanks for a great review, I have updated this!
----------------------------------------------------------------
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
[GitHub] [fineract] awasum commented on pull request #1286: Removed string concatenated SQL & sqlSearch from Staff (Fineract-854)
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #1286:
URL: https://github.com/apache/fineract/pull/1286#issuecomment-680805549
> Removed string concatenated SQL and use of SQLseach parameter from Staff,
> I have looked closely in what I am changing but a detailed review would help since we probably don't have good tests coverage on fineract yet.
>
> @awasum how do you think this as the project idea for outreachy this winter? WE NEED MORE TESTS!!
Improving test overage will be a good Outreachy project. First thing, we need to identify scenarios which require unit or integration test on Fineract. Are there open JIRA issue for test coverage? If not, create one and begin preparing test cases and scenarios so that it will be easy for an intern to work on.. THat is if you are willing to mentor during the upcoming Outreachy session.
@thesmallstar also engage on the Outreachy thread on the Dev list so we have all these ideas in one place and also create awareness as Outreachy and GSoC are two ways we get new contributors and new features.bug fixes on Fineract. CC @vorburger @ptuomola @xurror @percyashu
----------------------------------------------------------------
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
[GitHub] [fineract] ptuomola commented on a change in pull request #1286: Removed string concatenated SQL & sqlSearch from Staff (Fineract-854)
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1286:
URL: https://github.com/apache/fineract/pull/1286#discussion_r479855716
##########
File path: fineract-provider/src/main/java/org/apache/fineract/organisation/staff/service/StaffReadPlatformServiceImpl.java
##########
@@ -257,14 +252,7 @@ private String getStaffCriteria(final String sqlSearch, final Long officeId, fin
// employee who does not belong to his office or a sub office for his
// office.
Review comment:
I think this comment is now in the wrong place and should be moved to retrieveAllStaff(), right?
----------------------------------------------------------------
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