Skip to content

Conversation

@Monica-CodingWorld
Copy link
Contributor

@Monica-CodingWorld Monica-CodingWorld commented Jan 21, 2026

Description

Modified StandingInstructionDataValidator.java to make amount, interval, recurrence frequency, and on month day fields optional when standing instruction type is "DUES". These fields are automatically picked from the loan repayment schedule for DUES type instructions, so they should not be required during validation.

Changes:

  • CREATE validation: Made amount field .notNull() only for FIXED type; recurrence fields optional for DUES type
  • UPDATE validation: Applied same conditional logic for partial updates; fixed variable scope for standingInstructionType
  • Periodic validation: Moved recurrenceInterval.integerGreaterThanZero() inside if (isPeriodic) block where it belongs

JIRA ticket : https://issues.apache.org/jira/browse/FINERACT-2048

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.

.notNull();
baseDataValidator.reset().parameter(StandingInstructionApiConstants.recurrenceFrequencyParamName).value(recurrenceFrequency)
.notNull();
if (standingInstructionType == null || !StandingInstructionType.fromInt(standingInstructionType).isDuesAmoutTransfer()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

standingInstructionType == null || !StandingInstructionType.fromInt(standingInstructionType).isDuesAmoutTransfer()

You are using this logic in repeatedly throughout the code change i think you can extract this into a helper function

baseDataValidator.reset().parameter(StandingInstructionApiConstants.recurrenceFrequencyParamName).value(recurrenceFrequency)
.notNull();
if (standingInstructionType == null || !StandingInstructionType.fromInt(standingInstructionType).isDuesAmoutTransfer()) {
baseDataValidator.reset().parameter(StandingInstructionApiConstants.recurrenceIntervalParamName).value(recurrenceInterval)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a little bit complicated. and if conditions can be merged, also of there is any business rule that is implemented make sure to document that helper function

Business rules should be explicit and self-documenting

while the Github Action is not run here is a pro tip you can run the check on your forked repo and add testcase for more coverage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Aman-Mittal Thank you so much for your guidance and suggestions. I will improve the PR and update shortly.

@Monica-CodingWorld
Copy link
Contributor Author

Hi @Aman-Mittal,
I've addressed all your feedback:

  • Extracted helper methods - isAmountRequired() and isRecurrenceRequired() to eliminate code duplication
  • Added JavaDoc - Explaining business rules clearly
  • Added unit tests - 8 tests covering all scenarios (amount/recurrence requirements for both DUES and FIXED_AMOUNT_TRANSFER types)
    All tests pass locally. Please review the changes. Once you approve, I'll squash the commits into a single clean commit before merging.
    Thank you!

…ng instruction type

- Add isAmountRequired() and isRecurrenceRequired() helper methods
- Replace repetitive conditional patterns
- Add comprehensive JavaDoc
- Add unit tests for validation logic
- Apply Spotless code formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants