ptuomola commented on a change in pull request #1207:
URL: https://github.com/apache/fineract/pull/1207#discussion_r464024948
##########
File path:
fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/ClientTest.java
##########
@@ -143,4 +143,36 @@ public void testClientAsEntityStatus() {
}
+ @SuppressWarnings("unchecked")
+ @Test
+ public void testPendingOnlyClientRequest() {
+
+ Random rand = new Random();
+ // Add a few clients to the server and activate a random amount of them
+ for (int i = 0; i < 15; i++) {
+ final Integer clientId =
ClientHelper.createClientAsEntity(this.requestSpec, this.responseSpec);
+ if (rand.nextInt(10) % 2 == 0) {
+ // Takes Client to pending status
+ this.clientHelper.closeClient(clientId);
+ this.clientHelper.reactivateClient(clientId);
+ }
+ // Other clients stay in Active status
+ }
+ List<HashMap<String, Object>> clientsRecieved = (List<HashMap<String,
Object>>) clientHelper.getClientWithStatus(50, "pending");
+
Review comment:
Should we not check that we receive at least some clients? As far as I
can see, this test will still pass if the search doesn't actually work and
returns nothing...
Ideally we should check that we receive some of the same clients that we set
to status pending above - but at the very least we should check that we've
received something...
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/portfolio/client/domain/ClientStatus.java
##########
@@ -18,17 +18,17 @@
*/
package org.apache.fineract.portfolio.client.domain;
+import org.springframework.util.StringUtils;
+
/**
* Enum representation of client status states.
*/
public enum ClientStatus {
- INVALID(0, "clientStatusType.invalid"), //
- PENDING(100, "clientStatusType.pending"), //
- ACTIVE(300, "clientStatusType.active"), //
- TRANSFER_IN_PROGRESS(303, "clientStatusType.transfer.in.progress"), //
- TRANSFER_ON_HOLD(304, "clientStatusType.transfer.on.hold"), //
- CLOSED(600, "clientStatusType.closed"), REJECTED(700,
"clientStatusType.rejected"), WITHDRAWN(800, "clientStatusType.withdraw");
+ INVALID(0, "clientStatusType.invalid"), PENDING(100,
"clientStatusType.pending"), ACTIVE(300,
+ "clientStatusType.active"), TRANSFER_IN_PROGRESS(303,
"clientStatusType.transfer.in.progress"), TRANSFER_ON_HOLD(304,
+ "clientStatusType.transfer.on.hold"), CLOSED(600,
"clientStatusType.closed"), REJECTED(700,
+ "clientStatusType.rejected"), WITHDRAWN(800,
"clientStatusType.withdraw");
Review comment:
Hmm... couldn't spot any difference except removal of the formatting
(one status per line) - was there something changed here?
##########
File path:
fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/ClientTest.java
##########
@@ -143,4 +143,36 @@ public void testClientAsEntityStatus() {
}
+ @SuppressWarnings("unchecked")
+ @Test
+ public void testPendingOnlyClientRequest() {
+
+ Random rand = new Random();
+ // Add a few clients to the server and activate a random amount of them
+ for (int i = 0; i < 15; i++) {
+ final Integer clientId =
ClientHelper.createClientAsEntity(this.requestSpec, this.responseSpec);
+ if (rand.nextInt(10) % 2 == 0) {
+ // Takes Client to pending status
+ this.clientHelper.closeClient(clientId);
+ this.clientHelper.reactivateClient(clientId);
+ }
+ // Other clients stay in Active status
+ }
+ List<HashMap<String, Object>> clientsRecieved = (List<HashMap<String,
Object>>) clientHelper.getClientWithStatus(50, "pending");
+
+ for (int i = 0; i < clientsRecieved.size(); i++) {
+ HashMap<String, Object> clientStatus =
ClientHelper.getClientStatus(requestSpec, responseSpec,
+ String.valueOf(clientsRecieved.get(i).get("id")));
+ ClientStatusChecker.verifyClientPending(clientStatus);
+ }
+
+ clientsRecieved = (List<HashMap<String, Object>>)
clientHelper.getClientWithStatus(50, "active");
+
+ for (int i = 0; i < clientsRecieved.size(); i++) {
+ HashMap<String, Object> clientStatus =
ClientHelper.getClientStatus(requestSpec, responseSpec,
Review comment:
Same here as above - shouldn't we check we received at least one active
client?
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/portfolio/client/service/ClientReadPlatformServiceImpl.java
##########
@@ -258,6 +259,13 @@ private String buildSqlStringFromClientCriteria(String
schemaSql, final SearchPa
extraCriteria += " and c.display_name like ? ";
}
+ if (status != null) {
+ ClientStatus clientStatus = ClientStatus.fromString(status);
+ if (clientStatus.getValue() != 0) {
Review comment:
So this would mean you can't search for clients with status = INVALID.
Is this correct behaviour - shouldn't we still allow searching even if the
value you pass in is INVALID? I.e. to find any INVALID entries (if any)...
At the same time it would be good to validate the provided input string to
ensure it has actually mapped to a valid value, and throw
PlatformValidationException if it doesn't.
So you should be able to search for any invalid entries by sending in
clientStatusType.invalid - but if you send in clientStatusType.blah then you
should get a validation exception.
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/portfolio/client/domain/ClientStatus.java
##########
@@ -63,6 +63,33 @@ public static ClientStatus fromInt(final Integer
statusValue) {
return enumeration;
}
+ public static ClientStatus fromString(final String clientString) {
Review comment:
Hmmm... how does this actually work now? Here we are comparing to the
strings as defined in the enum (e.g. clientStatusType.active) but the test is
passing in just "active". So it should not match / return anything. What did I
miss 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:
[email protected]
