FINERACT-1095 Added status parameter in Clients API

[thesmallstar]

This PR adds a new parameter in client - onlyPending.
The reason for adding onlyPending and not isPending is because
we are trying to remove the use of
"c.status_enum= 100"
Now that means the status should be "only pending".

Just to remember how we are fixing this:

1. This PR is merged
2. @luckyman20 makes changes in community App
3. We wait for confirmation from the community and remove sqlSearch param in the client.

Next up I will take up the loan module.
After which, if the community agrees we will completely remove the sqlSearch param in all APIs.

[thesmallstar]

Just remembered, @vorburger A bug fixed without integration tests is not a bug fixed.
Adding them ;)

[thesmallstar]

Updated with integration tests, ready for review.

[ptuomola]

Hi - great stuff.

One question: rather than having a boolean flag to check for status 100 only, would it make sense to make this more generic by making this parameter an int and allowing the caller of the API to specify any status code as a search parameter?

Then you could get the same behaviour by calling the API with status = 100, but you could also use the same parameter for any other status if you wanted? Otherwise I think we run the risk of over time having to introduce a new boolean for every status type...

[thesmallstar]

The reason is we are using an enum, then we would also need to specify to the user(developer) what enum we are using for each type will it be a good idea?

[luckyman20]

I second with @ptuomola

It will provide more options to dev if wants to make further changes in frontend.

[thesmallstar]

@ptuomola @vorburger @luckyman20 instead of taking enum code we can take a string and then map them to integers, so that the API user don't need to worry about the numbers.

I hope you would agree, using this.

[thesmallstar]

@luckyman20 @ptuomola I have updated the API and tests.
so now this accepts a parameter status which can take value active, closed, withdrawn, pending, etc :)
Please review this.

[vorburger]

I'm marking this Approved to indicate that this looks roughly about right to me, but I've not reviewed it in full details, seeing that @ptuomola is on it - thank you! I'll let you drive the review merge whenever you're good with it, but won't merge it myself.

[thesmallstar]

@ptuomola I am collectively answering to your doubts here:

1. I am actually adding clients, so there would be some clients in the database for sure. (Check the first for loop).
2. I am adding 15 clients and each time I am assigning him pending or active status randomly, that means

The probability that there are no active/pending clients = 1/pow(2,15) = 0.00003051757, I hope you will agree we can ignore this and consider that there is at least one active/pending client.

3. The client status never actually becomes invalid, it just means that there is no such status. In that case, we as you said we can either give an exception or show all entries.
   @ptuomola as you said, should we instead throw an exception? still confused or we should instead show all the clients if it does not map to any of them.

FYI @vorburger

[ptuomola]

@ptuomola I am collectively answering to your doubts here:

1. I am actually adding clients, so there would be some clients in the database for sure. (Check the first for loop).
2. I am adding 15 clients and each time I am assigning him pending or active status randomly, that means

The probability that there are no active/pending clients = 1/pow(2,15) = 0.00003051757, I hope you will agree we can ignore this and consider that there is at least one active/pending client.

No that wasn't my concern - I know you create active and pending clients. My concern was: what if the query selection doesn't work as expected, and instead returns an empty list without matching any clients? I don't see the test checking for the case where there is no clients returned - it just checks that whatever clients are returned are in the correct status.

So something like assertNotEquals(list.size(), 0) for both cases...

1. The client status never actually becomes invalid, it just means that there is no such status. In that case, we as you said we can either give an exception or show all entries.
   @ptuomola as you said, should we instead throw an exception? still confused or we should instead show all the clients if it does not map to any of them.

My personal view would be that we should throw an exception if the caller sends a status that is not a valid value for ClientStatus. Otherwise the caller thinks that there are no clients matching this status and never finds out that they actually sent the wrong value. Empty list should mean that there are no matches.

[thesmallstar]

@ptuomola thank you so much for the quick response!
I got it now, updating it now.

[thesmallstar]

@ptuomola I have updated this as you suggested, the tests are running for hours now idk why. Would you review this please?

[thesmallstar]

looking into the failed test case.

[thesmallstar]

@ptuomola ready for your review :D

[ptuomola]

@thesmallstar thanks - will take a look tomorrow morning!

[ptuomola]

Sorry for the delay in looking into this - have been a bit busy! Will try to look at this tonight...

[ptuomola]

@thesmallstar Finally had a chance to look at this and to test it out locally. The functionality itself works as expected - great work!

The only concern I have is: I really don't like the fact that we need to maintain two lists of ClientStatus values - one in the enum itself, another one in the validator. It's really easy for these to get out of synch: in fact they have already done so, as there's a spelling mistake in the validator ("transfer_in_progreess") which doesn't match the validator.

Given this, maybe the right thing to do after all is to use the ClientStatus enum itself to validate? I.e. call the fromString method to validate, and throw an error if you get ClientStatus.INVALID? I know that doesn't allow you to search for values that actually have the status INVALID, but I don't think that's a valid status in the first place - so we should not have any such records anyway.

Does that make sense? That would allow us to remove the arrayList of valid values, but otherwise keep the functionality intact. What do you think?

[thesmallstar]

@ptuomola agreed and good point! updating this.

[thesmallstar]

@ptuomola done, Let's see if the test passes.

[ptuomola]

Looks great to me! Thanks for your work on this @thesmallstar