Skip to content

FINERACT-2202: Bulk import fix - #4442

Merged
adamsaghy merged 1 commit into
apache:developfrom
carlossortega:fix_bulk_import
Mar 19, 2025
Merged

FINERACT-2202: Bulk import fix#4442
adamsaghy merged 1 commit into
apache:developfrom
carlossortega:fix_bulk_import

Conversation

@carlossortega

Copy link
Copy Markdown
Contributor

Description

Fix for bulk import:
Users
Clients - Entity
Clients - Person

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.

@IOhacker IOhacker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review the imports


final String staffIdParamName = "staffId";
final Long staffId = command.longValueOfParameterNamed(staffIdParamName);
final Long staffId = command.longValueOfParameterNamed(staffIdParamName) != 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is problematic as if staffId was not provided the value will be null and you will get NullPointerException as the side effect of the !=0.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@adamsaghy done.

@adamsaghy adamsaghy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Kindly see my review!

final Long officeId = this.fromApiJsonHelper.extractLongNamed(OFFICE_ID, element);
baseDataValidator.reset().parameter(OFFICE_ID).value(officeId).notNull().integerGreaterThanZero();

if (this.fromApiJsonHelper.parameterExists(STAFF_ID, element)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is there a reason to remove this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@adamsaghy Yes, because the staff name field is optional and should not be validated as mandatory.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was optional check... if the staffId was not part of the request, there was no validation... if staffId is there, it should validate. I dont think removing this is correct course of action!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@adamsaghy Done, during testing, the issue was resolved by simply changing 0L to null. The optional verification was added back.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@adamsaghy
Could you please review again?

@adamsaghy adamsaghy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Kindly see my review!

@adamsaghy
adamsaghy merged commit 9d53d1c into apache:develop Mar 19, 2025
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.

3 participants