FINERACT-1932: Fineract modularization by nidhiii128 · Pull Request #5562 · apache/fineract

nidhiii128 · 2026-02-28T20:02:04Z

Description

This PR modernizes the Staff API by replacing manual JSON Map parsing with a structured StaffRequest DTO and migrating date handling from the deprecated java.util.Date to the modern java.time.LocalDate.

Key Changes

Architectural Refactor: Moved StaffRequest from fineract-provider to fineract-core to allow for better separation of concerns and reuse across modules.
Type Safety: Replaced Map<String, Object> in StaffWritePlatformService with the StaffRequest DTO, enabling compile-time checks and reducing runtime errors.
Modern Date Handling: Updated the Staff entity, commands, and validators to use java.time.LocalDate, aligning with the project's goal to eliminate legacy date objects.
Command Handling: Refactored CreateStaffCommandHandler and StaffCommandFromApiJsonDeserializer to leverage the new DTO structure.
Test Alignment: Updated StaffTest.java and related integration tests (including Feign client tests) to ensure compatibility with the new API structure.

Impact

Improves code readability and maintainability.
Simplifies the validation logic by using DTO-based constraints.
Ensures consistent date/time behavior across different database engines (MariaDB/PostgreSQL).

Checklist

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

Write the commit message as per our guidelines
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 our 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
This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.

Your assigned reviewer(s) will follow our guidelines for code reviews.

airajena · 2026-03-01T04:27:02Z

fineract-core/src/main/java/org/apache/fineract/organisation/staff/domain/Staff.java

+            lastnameChanged = true;
+        }
+
+        // Fixed: Combined the flags you created above


I feel such comments should be avoided.

airajena · 2026-03-01T04:28:51Z

...g/apache/fineract/organisation/staff/service/StaffWritePlatformServiceJpaRepositoryImpl.java

                    "Staff with externalId `" + externalId + "` already exists", "externalId", externalId);
        } else if (realCause.getMessage().contains("display_name")) {
-            final String lastname = command.stringValueOfParameterNamed("lastname");
+            // FIX: Use 'request' getters, not 'command'


This comments belongs to PR description, we should keep code clean.

IOhacker

@nidhiii128 I think the Fineract modularization has another approach. This looks to me to be a Refactor of the Staff. Please check how other modules have been executed. I suggest to have a conversation about this change in the Apache Fineract developer list.

IOhacker · 2026-03-01T07:53:17Z

@nidhiii128 please work on this jira ticket, I am unable to assign this to you, but please pick it: https://issues.apache.org/jira/browse/FINERACT-2515

nidhiii128 · 2026-03-01T09:13:51Z

@nidhiii128 please work on this jira ticket, I am unable to assign this to you, but please pick it: https://issues.apache.org/jira/browse/FINERACT-2515

Hi @IOhacker,

I saw the updates to the ticket. I actually started working on the refactor the moment you posted the task, focusing on the 'bad practice' note regarding the PortfolioAccountType methods.

I have just submitted a Pull Request that addresses exactly what was discussed: I've removed the redundant isSavingsAccount() and isLoanAccount() methods from the Enum and updated all callers across the provider and core modules to use direct enum comparisons.

The build is passing locally after a fresh rebase against develop. Looking forward to your feedback!

PR Link: #5563

vidakovic · 2026-03-01T18:44:42Z

fineract-core/src/main/java/org/apache/fineract/organisation/staff/domain/Staff.java

        return new Staff(staffOffice, firstname, lastname, externalId, mobileNo, isLoanOfficer, isActive, joiningDate);
    }

+    public static Staff fromRequest(final Office staffOffice, final StaffRequest request) {


This is more a comment to us as a community and less for you @nidhiii128 ... we have functions like this all over the place, but I have some doubts about the usefulness of them in general. I don't see where this is actually used and/or where it would be useful. Do we really optimize here anything? Isn't it just easier to return the whole updated object if we really have to (I wouldn't... if the client needs that information really from the backend then it should just call the "read" endpoint). The client initiates the change... if a http 200 is received then it "knows" that the changes are ok, if 4xx or 5xx then the updates didn't succeed... all needed information is already available on the client side.

But let's assume this is really necessary and I'm missing something essential here... then I would really urge to use a library that does diffing way better than any of our handwritten (boilerplate) code: https://javers.org

Hi @vidakovic , I completely agree with your point. The manual boilerplate we have for these updates is definitely not ideal and makes the code harder to maintain. Using a library like Javers would be a much cleaner and more professional way to handle object diffing across Fineract. I've followed the current fromRequest pattern here just to stay consistent with the existing modules for now, but I'm definitely in favor of the community moving toward a better solution like the one you suggested.

All good @nidhiii128 your approach makes sense to get this PR approved; introducing new libs/dependencies "on the fly" without at least a Jira ticket usually doesn't get a PR approved. Will start the build so that we can see where we are with this.

adamsaghy · 2026-03-02T20:36:30Z

  Did you mean 'ClientIdentifierStatus statusEnum = ClientIdentifierStatus.valueOf(statusName.toUpperCase(Locale.ROOT));' or 'ClientIdentifierStatus statusEnum = ClientIdentifierStatus.valueOf(statusName.toUpperCase(Locale.getDefault()));' or 'ClientIdentifierStatus statusEnum = ClientIdentifierStatus.valueOf(Ascii.toUpperCase(statusName));'?
/home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/organisation/staff/domain/StaffRequest.java:19: error: [PackageLocation] Expected package org.apache.fineract.organisation.staff.data to be declared in a directory ending with org/apache/fineract/organisation/staff/data, instead found org/apache/fineract/organisation/staff/domain
package org.apache.fineract.organisation.staff.data;

nidhiii128 · 2026-03-03T07:08:55Z

  Did you mean 'ClientIdentifierStatus statusEnum = ClientIdentifierStatus.valueOf(statusName.toUpperCase(Locale.ROOT));' or 'ClientIdentifierStatus statusEnum = ClientIdentifierStatus.valueOf(statusName.toUpperCase(Locale.getDefault()));' or 'ClientIdentifierStatus statusEnum = ClientIdentifierStatus.valueOf(Ascii.toUpperCase(statusName));'?
/home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/organisation/staff/domain/StaffRequest.java:19: error: [PackageLocation] Expected package org.apache.fineract.organisation.staff.data to be declared in a directory ending with org/apache/fineract/organisation/staff/data, instead found org/apache/fineract/organisation/staff/domain
package org.apache.fineract.organisation.staff.data;

hi @adamsaghy,
I've addressed these two points in the latest push:
Regarding toUpperCase(): I've updated the code to use Locale.ROOT. I understand now that using the default system locale can cause unexpected behavior with certain character sets (like the Turkish 'I'), so using Locale.ROOT ensures consistent, language-neutral behavior for the Enum lookup.
Regarding StaffRequest.java: You were right the package declaration was still pointing to .data while the file was physically located in the .domain directory. I have corrected the package header to package org.apache.fineract.organisation.staff.domain; and updated all importing files to fix the compilation errors.

All local Checkstyle and compilation tasks are now passing.

nidhiii128 force-pushed the feature/staff-modernization-cqrs branch 4 times, most recently from 156e5f7 to 42eb73f Compare February 28, 2026 20:23

airajena reviewed Mar 1, 2026

View reviewed changes

IOhacker requested changes Mar 1, 2026

View reviewed changes

vidakovic reviewed Mar 1, 2026

View reviewed changes

nidhiii128 force-pushed the feature/staff-modernization-cqrs branch from 42eb73f to c69d38b Compare March 2, 2026 05:33

FINERACT-1932: Fineract modularization

079e581

nidhiii128 force-pushed the feature/staff-modernization-cqrs branch from c69d38b to 079e581 Compare March 3, 2026 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FINERACT-1932: Fineract modularization#5562

FINERACT-1932: Fineract modularization#5562
nidhiii128 wants to merge 1 commit intoapache:developfrom
nidhiii128:feature/staff-modernization-cqrs

nidhiii128 commented Feb 28, 2026

Uh oh!

airajena Mar 1, 2026

Uh oh!

airajena Mar 1, 2026

Uh oh!

IOhacker left a comment

Uh oh!

IOhacker commented Mar 1, 2026

Uh oh!

nidhiii128 commented Mar 1, 2026

Uh oh!

vidakovic Mar 1, 2026

Uh oh!

nidhiii128 Mar 1, 2026

Uh oh!

vidakovic Mar 1, 2026

Uh oh!

adamsaghy commented Mar 2, 2026

Uh oh!

nidhiii128 commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

nidhiii128 commented Feb 28, 2026

Description

Key Changes

Impact

Checklist

Uh oh!

airajena Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

airajena Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

IOhacker left a comment

Choose a reason for hiding this comment

Uh oh!

IOhacker commented Mar 1, 2026

Uh oh!

nidhiii128 commented Mar 1, 2026

Uh oh!

vidakovic Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

nidhiii128 Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

vidakovic Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

adamsaghy commented Mar 2, 2026

Uh oh!

nidhiii128 commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants