FINERACT-1911 Assign datatable to Savings transaction #3304

Mk9894 · 2023-07-12T08:29:53Z

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.

adamsaghy · 2023-07-12T12:38:35Z

...ache/fineract/infrastructure/core/serialization/DatatableCommandFromApiJsonDeserializer.java

@@ -57,6 +57,7 @@ public class DatatableCommandFromApiJsonDeserializer {
    public static final String NEW_CODE = "newCode";
    public static final String M_LOAN = "m_loan";
    public static final String M_SAVINGS_ACCOUNT = "m_savings_account";
+    public static final String M_SAVINGS_ACCOUNT_TRANSACTIONS = "m_savings_account_transaction";


Please follow the naming convention of the other entries: no plurals

Sure. Will change it accordingly

adamsaghy · 2023-07-12T12:45:49Z

.../org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java

@@ -1709,98 +1709,93 @@ private Long getLongSqlRowSet(final SqlRowSet rs, final String column) {
        return val;
    }

-    private String dataScopedSQL(final String appTable, final Long appTableId) {


why to remove the final from these variables?

It was my mistake. Will correct it. Thanks

adamsaghy

Please add some tests to cover the functionality.

marta-jankovics · 2023-07-13T00:59:45Z

.../org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java

+        switch (appTable.toLowerCase()) {
+            case "m_loan":
+                scopedSQL = "select distinct x.* from ("
+                        + " (select o.id as officeId, l.group_id as groupId, l.client_id as clientId, null as savingsId, l.id as loanId, null as entityId from m_loan l "


This parts of the queries can be reused (the only difference is the alias):
"join m_client c on c.id = l.client_id join m_office o on o.id = c.office_id and o.hierarchy like '" + currentUser.getOffice().getHierarchy() + "%'

" join m_group g on g.id = l.group_id join m_office o on o.id = g.office_id and o.hierarchy like '" + currentUser.getOffice().getHierarchy() + "%'

marta-jankovics · 2023-07-13T01:01:33Z

...ache/fineract/infrastructure/core/serialization/DatatableCommandFromApiJsonDeserializer.java

@@ -57,6 +57,7 @@ public class DatatableCommandFromApiJsonDeserializer {
    public static final String NEW_CODE = "newCode";
    public static final String M_LOAN = "m_loan";
    public static final String M_SAVINGS_ACCOUNT = "m_savings_account";
+    public static final String M_SAVINGS_ACCOUNT_TRANSACTIONS = "m_savings_account_transaction";


You have these constants added, but not used eg. in RWNioncoreDataServiceImpl

marta-jankovics · 2023-07-13T01:02:38Z

.../org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java

-        }
-        if (appTable.equalsIgnoreCase("m_share_product")) {
-            return;
+        switch (appTable.toLowerCase()) {


Better than it was, but you could use SUPPORTED_APPTABLE_NAMES or the best would be to use EntityTables everywhere

marta-jankovics · 2023-07-13T01:09:20Z

.../org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java

        }
-
-        throw new PlatformDataIntegrityException("error.msg.invalid.application.table", "Invalid Application Table: " + appTable, "name",
-                appTable);
    }

    private String mapToActualAppTable(final String appTable) {


There are so many other places regarding datatables where you did not add the new apptable type:
org.apache.fineract.infrastructure.dataqueries.data.EntityTables
org.apache.fineract.infrastructure.dataqueries.service.EntityDatatableChecksReadPlatformServiceImpl
org.apache.fineract.infrastructure.dataqueries.service.EntityDatatableChecksWritePlatformServiceImpl

Could you please provide a full solution.
Integration tests also should be added.

marta-jankovics · 2023-07-19T21:47:57Z

...ache/fineract/infrastructure/core/serialization/DatatableCommandFromApiJsonDeserializer.java

@@ -158,12 +148,9 @@ public void validateForCreate(final String json) {

        final String apptableName = this.fromApiJsonHelper.extractStringNamed(APPTABLE_NAME, element);
        baseDataValidator.reset().parameter(APPTABLE_NAME).value(apptableName).notBlank().notExceedingLengthOf(50)
-                .isOneOfTheseValues(SUPPORTED_APPTABLE_NAMES);
+                .isOneOfTheseStringValues(EntityTables.getEntitiesList());


This will not allow to create and update 'm_center' but as we discussed I'll fix this

marta-jankovics · 2023-07-19T21:48:59Z

...neract/infrastructure/dataqueries/service/EntityDatatableChecksWritePlatformServiceImpl.java

@@ -105,10 +105,11 @@ public CommandProcessingResult createCheck(final JsonCommand command) {
                    throw new EntityDatatableCheckAlreadyExistsException(entity, status, datatableName);
                }
            } else {
-                if (entity.equals("m_loan")) {
+                EntityTables entityTable = EntityTables.fromName(entity);
+                if (EntityTables.LOAN.equals(entityTable)) {


minor: enum equality can be checked by identity

adamsaghy · 2023-07-20T11:53:12Z

@Mk9894 Please check why it is not compiling!

adamsaghy · 2023-07-20T12:00:38Z

build.gradle

@@ -514,7 +515,6 @@ configure(project.fineractJavaProjects) {
                    "EqualsGetClass",
                    "EqualsUnsafeCast",
                    "DoubleBraceInitialization",
-                    "UnnecessaryDefaultInEnumSwitch",


why do you want this?

UnnecessaryDefaultInEnumSwitch does not make sense to cause the build to fail. Most of the time it is even very useful eg. to prepare to the enums being extended.

marta-jankovics · 2023-07-20T14:43:32Z

build.gradle

@@ -514,7 +515,6 @@ configure(project.fineractJavaProjects) {
                    "EqualsGetClass",
                    "EqualsUnsafeCast",
                    "DoubleBraceInitialization",
-                    "UnnecessaryDefaultInEnumSwitch",


UnnecessaryDefaultInEnumSwitch does not make sense to cause the build to fail. Most of the time it is even very useful eg. to prepare to the enums being extended.

adamsaghy reviewed Jul 12, 2023

View reviewed changes

adamsaghy requested changes Jul 12, 2023

View reviewed changes

marta-jankovics reviewed Jul 13, 2023

View reviewed changes

Mk9894 force-pushed the FINERACT-1911-datatable branch from 39b4724 to 5d8bc5d Compare July 19, 2023 17:21

Mk9894 requested review from marta-jankovics and adamsaghy July 19, 2023 17:23

marta-jankovics approved these changes Jul 19, 2023

View reviewed changes

Mk9894 force-pushed the FINERACT-1911-datatable branch from 5d8bc5d to 28a97d4 Compare July 20, 2023 05:11

marta-jankovics approved these changes Jul 20, 2023

View reviewed changes

Mk9894 requested a review from marta-jankovics July 20, 2023 11:58

adamsaghy reviewed Jul 20, 2023

View reviewed changes

marta-jankovics approved these changes Jul 20, 2023

View reviewed changes

Mk9894 added 2 commits July 21, 2023 15:15

FINERACT-1911 Assign datatable to Savings transaction

458dbce

FINERACT-1911 Disable UnnecessaryDefaultInEnumSwitch

b9c7e89

Mk9894 force-pushed the FINERACT-1911-datatable branch from 6ab7bb5 to b9c7e89 Compare July 21, 2023 09:45

adamsaghy merged commit a439952 into apache:develop Jul 21, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FINERACT-1911 Assign datatable to Savings transaction #3304

FINERACT-1911 Assign datatable to Savings transaction #3304

Mk9894 commented Jul 12, 2023

adamsaghy Jul 12, 2023

Mk9894 Jul 13, 2023

adamsaghy Jul 12, 2023

Mk9894 Jul 13, 2023

adamsaghy left a comment •

edited

marta-jankovics Jul 13, 2023

Mk9894 Jul 19, 2023

marta-jankovics Jul 13, 2023

marta-jankovics Jul 13, 2023 •

edited

marta-jankovics Jul 13, 2023 •

edited

marta-jankovics Jul 19, 2023

marta-jankovics Jul 19, 2023

Mk9894 Jul 20, 2023

adamsaghy commented Jul 20, 2023

adamsaghy Jul 20, 2023

marta-jankovics Jul 20, 2023

marta-jankovics Jul 20, 2023

FINERACT-1911 Assign datatable to Savings transaction #3304

FINERACT-1911 Assign datatable to Savings transaction #3304

Conversation

Mk9894 commented Jul 12, 2023

Description

Checklist

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

adamsaghy left a comment • edited

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

marta-jankovics Jul 13, 2023 • edited

Choose a reason for hiding this comment

marta-jankovics Jul 13, 2023 • edited

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

adamsaghy commented Jul 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsaghy left a comment •

edited

marta-jankovics Jul 13, 2023 •

edited

marta-jankovics Jul 13, 2023 •

edited