You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by "carlossortega (via GitHub)" <gi...@apache.org> on 2025/03/07 22:35:23 UTC
[PR] FINERACT-2202: Bulk import fix [fineract]
carlossortega opened a new pull request, #4442:
URL: https://github.com/apache/fineract/pull/4442
## Description
Fix for bulk import:
Users
Clients - Entity
Clients - Person
Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [x] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [x] 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.
- [x] Create/update unit or integration tests for verifying the changes made.
- [x] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [x] 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
- [x] 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2202: Bulk import fix [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4442:
URL: https://github.com/apache/fineract/pull/4442#discussion_r1987695924
##########
fineract-provider/src/main/java/org/apache/fineract/useradministration/service/AppUserWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -102,7 +102,9 @@ public CommandProcessingResult createUser(final JsonCommand command) {
final Set<Role> allRoles = assembleSetOfRoles(roles);
final String staffIdParamName = "staffId";
- final Long staffId = command.longValueOfParameterNamed(staffIdParamName);
+ final Long staffId = command.longValueOfParameterNamed(staffIdParamName) != 0
Review Comment:
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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2202: Bulk import fix [fineract]
Posted by "carlossortega (via GitHub)" <gi...@apache.org>.
carlossortega commented on code in PR #4442:
URL: https://github.com/apache/fineract/pull/4442#discussion_r1996147432
##########
fineract-provider/src/main/java/org/apache/fineract/useradministration/service/UserDataValidator.java:
##########
@@ -124,11 +124,6 @@ public void validateForCreate(final String json) {
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)) {
Review Comment:
@adamsaghy Done, during testing, the issue was resolved by simply changing 0L to null. The optional verification was added back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2202: Bulk import fix [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4442:
URL: https://github.com/apache/fineract/pull/4442#discussion_r1995521083
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/importhandler/ImportHandlerUtils.java:
##########
@@ -317,7 +317,7 @@ public static Long getIdByName(Sheet sheet, String name) {
return 0L;
}
} else {
- return 0L;
+ return null;
Review Comment:
is there a reason to change this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2202: Bulk import fix [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4442:
URL: https://github.com/apache/fineract/pull/4442#discussion_r1996043348
##########
fineract-provider/src/main/java/org/apache/fineract/useradministration/service/UserDataValidator.java:
##########
@@ -124,11 +124,6 @@ public void validateForCreate(final String json) {
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)) {
Review Comment:
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2202: Bulk import fix [fineract]
Posted by "carlossortega (via GitHub)" <gi...@apache.org>.
carlossortega commented on code in PR #4442:
URL: https://github.com/apache/fineract/pull/4442#discussion_r2001779619
##########
fineract-provider/src/main/java/org/apache/fineract/useradministration/service/UserDataValidator.java:
##########
@@ -124,11 +124,6 @@ public void validateForCreate(final String json) {
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)) {
Review Comment:
@adamsaghy
Could you please review again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2202: Bulk import fix [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4442:
URL: https://github.com/apache/fineract/pull/4442#discussion_r1995521613
##########
fineract-provider/src/main/java/org/apache/fineract/useradministration/service/UserDataValidator.java:
##########
@@ -124,11 +124,6 @@ public void validateForCreate(final String json) {
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)) {
Review Comment:
is there a reason to remove this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2202: Bulk import fix [fineract]
Posted by "carlossortega (via GitHub)" <gi...@apache.org>.
carlossortega commented on code in PR #4442:
URL: https://github.com/apache/fineract/pull/4442#discussion_r1996034287
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/bulkimport/importhandler/ImportHandlerUtils.java:
##########
@@ -317,7 +317,7 @@ public static Long getIdByName(Sheet sheet, String name) {
return 0L;
}
} else {
- return 0L;
+ return null;
Review Comment:
@adamsaghy Yes, it is necessary to change it because the staff name is optional. If no value is provided, it should return null instead of 0L, as 0 could be interpreted as a valid identifier.
##########
fineract-provider/src/main/java/org/apache/fineract/useradministration/service/UserDataValidator.java:
##########
@@ -124,11 +124,6 @@ public void validateForCreate(final String json) {
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)) {
Review Comment:
@adamsaghy Yes, because the staff name field is optional and should not be validated as mandatory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2202: Bulk import fix [fineract]
Posted by "carlossortega (via GitHub)" <gi...@apache.org>.
carlossortega commented on code in PR #4442:
URL: https://github.com/apache/fineract/pull/4442#discussion_r1992208832
##########
fineract-provider/src/main/java/org/apache/fineract/useradministration/service/AppUserWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -102,7 +102,9 @@ public CommandProcessingResult createUser(final JsonCommand command) {
final Set<Role> allRoles = assembleSetOfRoles(roles);
final String staffIdParamName = "staffId";
- final Long staffId = command.longValueOfParameterNamed(staffIdParamName);
+ final Long staffId = command.longValueOfParameterNamed(staffIdParamName) != 0
Review Comment:
@adamsaghy done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2202: Bulk import fix [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #4442:
URL: https://github.com/apache/fineract/pull/4442
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org