FINERACT-1912 Savings Account Transactions query -Advanced #3158

Mk9894 · 2023-05-09T16:47:08Z

Note

Please refer #3192 for the changes. This PR will be closed

Description

Describe the changes made and why they were made.

Ignore if these details are present on the associated Apache Fineract JIRA ticket.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

Write the commit message as per https://github.com/apache/fineract/#pull-requests
Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
Create/update unit or integration tests for verifying the changes made.
Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

…RACT-1912)

…NERACT-1912)

…ation from env variables

…to FINERACT-1912

…ERACT-1912)

galovics

In addition to the comments, there are zero tests for this new API, please add proper coverage for it.

galovics · 2023-05-10T05:53:01Z

fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm

@@ -29369,6 +29369,242 @@ <h2>Retrieve Savings Account Transaction Template:</h2>
 	    </div>
 	</div>

+	<a id="savingsaccounts_transactions_list" name="savingsaccounts_transactions_list" class="old-syle-anchor">&nbsp;</a>


This is not used anymore.

@galovics Does it mean that we can remove this?

This is a judgement call I think. You should see if what you do breaks the web UI from Mifos.

galovics · 2023-05-10T05:54:42Z

...in/java/org/apache/fineract/portfolio/savings/api/SavingsAccountTransactionsApiResource.java

+        this.context.authenticatedUser().validateHasReadPermission(SavingsApiConstants.SAVINGS_ACCOUNT_RESOURCE_NAME);
+        final SearchParameters searchParameters = SearchParameters.forPagination(offset, limit, orderBy, sortOrder);
+
+        LocalDate fromDate = null;


Why don't we use an argument resolver for this instead? That way we wouldn't have to duplicate this logic anywhere but just go with a plain LocalDate in the parameter list.

@galovics

Thank you for sharing the information regarding the HandlerArgumentResolver. I found that HandlerArgumentResolver is specific to Spring MVC framework and I understand that the HandlerArgumentResolver interface is not directly related to the Jersey framework, which is a separate framework for building RESTful web services.

Also it seems that Fineract is indeed using the Jersey framework for handling REST requests, rather than the MVC framework.

In a Spring Boot application, we have the flexibility to choose either the Spring MVC or Jersey framework for handling REST requests. When using Spring MVC, the HandlerArgumentResolver interface allows us to customize the resolution of method arguments in controller methods. However, in the case of Jersey, an equivalent mechanism called the ParamConverterProvider interface is used to handle method arguments.

In Jersey, if we want to achieve similar functionality to the HandlerArgumentResolver, we can implement the ParamConverterProvider interface. This interface enables us to define custom conversion logic for method parameters in Jersey-based REST endpoints. The ParamConverterProvider is responsible for converting the values from incoming requests into the desired method parameter types.

Regarding your specific concern, if we choose to implement the ParamConverter interface in our custom implementation, we might not have access to the "locale" and "dateFormat" Query parameters from the Request. This could limit our ability to convert the given date string to a LocalDate object based on the specified locale and dateFormat.

Considering this, it seems more appropriate to use the legacy approach as currently implemented in the PR. This approach allows us to retrieve the "locale" and "dateFormat" Query parameters and utilize them for converting the date string to a LocalDate object, satisfying the requirements.

Thanks.
Please let me know if you have any further questions or concerns

That's right, it should be a jersey parameter converter.
It's possible to access other attributes like this:

https://stackoverflow.com/questions/27584161/read-another-parameter-within-jerseys-paramconverter

https://stackoverflow.com/questions/53657358/paramconverter-based-on-http-header-value

Anyhow, let's postpone this and keep the legacy version.

...in/java/org/apache/fineract/portfolio/savings/service/SavingsAccountReadPlatformService.java

galovics · 2023-05-10T05:57:33Z

...ava/org/apache/fineract/portfolio/savings/service/SavingsAccountReadPlatformServiceImpl.java

+    @Override
+    public Page<SavingsAccountTransactionData> retrieveAllTransactions(final Long savingsId, DepositAccountType depositAccountType,
+            SearchParameters searchParameters, LocalDate fromDate, LocalDate toDate,
+            @DecimalMin(value = "0", message = "must be greater than or equal to 0") BigDecimal fromAmount,


What's going to evaluate this validation annotation?

galovics · 2023-05-10T05:58:22Z

...ava/org/apache/fineract/portfolio/savings/service/SavingsAccountReadPlatformServiceImpl.java

+            @DecimalMin(value = "0", message = "must be greater than or equal to 0") BigDecimal fromAmount,
+            @DecimalMin(value = "0", message = "must be greater than or equal to 0") BigDecimal toAmount, String transactionType) {
+
+        final StringBuilder sqlBuilder = new StringBuilder(200);


This is a super red flag, no native queries please unless you absolutely need them. Why don't you simply use a JPQL query with a projection for this?

Hello @galovics , I acknowledge your preference for using JPQL for implementation. However, it's important to note that there is a possibility of compatibility issues with the other APIs of the transaction endpoint. Specifically, the retrieveOne function of the transaction API returns a response of Type SavingsAccountTransactionData, which is a custom type. In contrast, when using JPQL, we need to return SavingsAccountTransaction as the data type, which is the entity itself. This could potentially lead to the return response being incompatible with other transaction endpoints. I would greatly appreciate hearing your thoughts on this.

Thanks

you might wanna consider using jpa projections:
https://www.baeldung.com/spring-data-jpa-projections

https://vladmihalcea.com/spring-jpa-dto-projection/

Please look at the EclipseLink docs:
https://wiki.eclipse.org/EclipseLink/UserGuide/JPA/Basic_JPA_Development/Querying/JPQL#EclipseLink_Extensions_.28EQL.29

Joins between independent entities (EL 2.4)

Another option is to do a theta join between the entities, that should be supported by JPA directly.

galovics · 2023-05-10T05:58:28Z

...ava/org/apache/fineract/portfolio/savings/service/SavingsAccountReadPlatformServiceImpl.java

+
+        if (StringUtils.isNotEmpty(transactionType)) {
+            Integer transcationTypeEnum = null;
+            if ("deposit".equals(transactionType.toLowerCase())) {


magic string

galovics · 2023-05-10T05:58:34Z

...ava/org/apache/fineract/portfolio/savings/service/SavingsAccountReadPlatformServiceImpl.java

+            Integer transcationTypeEnum = null;
+            if ("deposit".equals(transactionType.toLowerCase())) {
+                transcationTypeEnum = SavingsAccountTransactionType.DEPOSIT.getValue();
+            } else if ("withdrawal".equals(transactionType.toLowerCase())) {


magic string

galovics · 2023-05-10T05:58:41Z

...ava/org/apache/fineract/portfolio/savings/service/SavingsAccountReadPlatformServiceImpl.java

+        sqlBuilder.append("select " + sqlGenerator.calcFoundRows() + " ");
+        sqlBuilder.append(this.transactionsMapper.schema());
+        sqlBuilder.append(" where sa.id = ? and sa.deposit_type_enum = ? ");
+        final Object[] objectArray = new Object[12];


Nope, this won't fly.

galovics · 2023-05-10T05:59:22Z

...ava/org/apache/fineract/portfolio/savings/service/SavingsAccountReadPlatformServiceImpl.java

@@ -48,6 +49,7 @@
 import org.apache.fineract.organisation.monetary.data.CurrencyData;
 import org.apache.fineract.organisation.staff.data.StaffData;
 import org.apache.fineract.organisation.staff.service.StaffReadPlatformService;
+import org.apache.fineract.organisation.teller.exception.InvalidDateInputException;


This is a teller exception, why did we import it in the savings module?

galovics · 2023-05-10T05:59:39Z

...ava/org/apache/fineract/portfolio/savings/service/SavingsAccountReadPlatformServiceImpl.java

+        if (fromDate != null || toDate != null) {
+            if (fromDate != null && toDate != null) {
+                if (toDate.isBefore(fromDate)) {
+                    throw new InvalidDateInputException(fromDate.toString(), toDate.toString());


No message, nobody will understand whats the issue without looking at the code.

galovics · 2023-05-10T06:01:09Z

Also, squash your commits pls.

.../org/apache/fineract/portfolio/savings/api/SavingsAccountTransactionsApiResourceSwagger.java

galovics

No tests, please cover the use-cases with integration/unit tests. Thanks.

Also, squash your commits.

galovics · 2023-05-17T07:17:05Z

...in/java/org/apache/fineract/portfolio/savings/api/SavingsAccountTransactionsApiResource.java

+        this.context.authenticatedUser().validateHasReadPermission(SavingsApiConstants.SAVINGS_ACCOUNT_RESOURCE_NAME);
+        final SearchParameters searchParameters = SearchParameters.forPagination(offset, limit, orderBy, sortOrder);
+
+        LocalDate fromDate = null;


That's right, it should be a jersey parameter converter.
It's possible to access other attributes like this:

https://stackoverflow.com/questions/27584161/read-another-parameter-within-jerseys-paramconverter

https://stackoverflow.com/questions/53657358/paramconverter-based-on-http-header-value

Anyhow, let's postpone this and keep the legacy version.

adamsaghy · 2023-05-22T11:03:18Z

@Mk9894 Please squash your commits!

muthu.mk and others added 7 commits May 6, 2023 10:17

Support Pagination and sorting for Savings Account Transactions (FINE…

b646a02

…RACT-1912)

Support Filtering by transaction type,transaction date and amount (FI…

9397e31

…NERACT-1912)

Introduce Savings Account Transactions Swagger Resource(FINERACT-1912)

3f3e864

Merge branch 'develop' into FINERACT-1912

1a5271f

FINERACT-1724: Enhance external event datetime precision

db88892

FINERACT-1724: Ability to override tenant datasource min/max configur…

a94deb7

…ation from env variables

Merge branch 'FINERACT-1912' of https://github.com/Mk9894/fineract in…

f9247ca

…to FINERACT-1912

jdailey requested review from avikganguly01, IOhacker and galovics May 9, 2023 18:02

jdailey added Needs Functional Review PRs which pass build and have no obvious technical problems, but need functional review. java Pull requests that update Java code labels May 9, 2023

Mk9894 added 3 commits May 10, 2023 02:06

Merge remote-tracking branch 'upstream/develop' into FINERACT-1912

31e6fdb

Update Legacy API documentation for Savings account transactions (FIN…

0009f10

…ERACT-1912)

Update apiLive.htm (FINERACT-1912)

0040290

galovics requested changes May 10, 2023

View reviewed changes

adamsaghy reviewed May 10, 2023

View reviewed changes

.../org/apache/fineract/portfolio/savings/api/SavingsAccountTransactionsApiResourceSwagger.java Show resolved Hide resolved

Mk9894 added 3 commits May 17, 2023 05:56

FINERACT-1912 Update Implementation with JPQL based approach

5bd5c33

Merge remote-tracking branch 'upstream/develop' into FINERACT-1912

d50aaab

FINERACT-1912 Add validation for Amount

1543cb3

galovics requested changes May 17, 2023

View reviewed changes

Mk9894 added 2 commits May 18, 2023 19:29

Merge remote-tracking branch 'upstream/develop' into FINERACT-1912

7b0d4c4

FINERACT-1912 Add IntegrationTests and Fix minor issues

6ce6904

Mk9894 closed this Jun 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FINERACT-1912 Savings Account Transactions query -Advanced #3158

FINERACT-1912 Savings Account Transactions query -Advanced #3158

Mk9894 commented May 9, 2023 •

edited

galovics left a comment

galovics May 10, 2023

Mk9894 May 10, 2023

jdailey May 12, 2023

galovics May 10, 2023

Mk9894 May 17, 2023

galovics May 17, 2023

galovics May 10, 2023

galovics May 10, 2023

Mk9894 May 11, 2023

adamsaghy May 11, 2023

galovics May 15, 2023

galovics May 10, 2023

galovics May 10, 2023

galovics May 10, 2023

galovics May 10, 2023

galovics May 10, 2023

galovics commented May 10, 2023

galovics left a comment •

edited

galovics May 17, 2023

adamsaghy commented May 22, 2023

FINERACT-1912 Savings Account Transactions query -Advanced #3158

FINERACT-1912 Savings Account Transactions query -Advanced #3158

Conversation

Mk9894 commented May 9, 2023 • edited

Note

Description

Checklist

galovics left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galovics commented May 10, 2023

galovics left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsaghy commented May 22, 2023

Mk9894 commented May 9, 2023 •

edited

galovics left a comment •

edited