You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by "Monica-CodingWorld (via GitHub)" <gi...@apache.org> on 2026/01/19 06:29:53 UTC
[PR] FINERACT 263 : "Add Notes" for Centers permission is missing [fineract]
Monica-CodingWorld opened a new pull request, #5346:
URL: https://github.com/apache/fineract/pull/5346
## Description
This PR updates the NotesApiResource to handle NoteType resources more explicitly.
Changes made:
- Removed the fallback to INVALIDNOTE in the default case of getResourceDetails().
- Added a fast-fail mechanism by throwing NoteResourceNotSupportedException for unsupported note types.
- Ensures that unsupported note types no longer silently pass through and provides a clear API error response.
Jira issue:- https://issues.apache.org/jira/projects/FINERACT/issues/FINERACT-263
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Write the commit message as per [our guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#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](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made.
- [ ] Follow our [coding conventions](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
- [ ] [This PR must not be a "code dump"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute).
Your assigned reviewer(s) will follow our [guidelines for code reviews](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-263 : Add Notes for Centers permission is missing [fineract]
Posted by "Monica-CodingWorld (via GitHub)" <gi...@apache.org>.
Monica-CodingWorld commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3796414099
> @Monica-CodingWorld could you please take a look at https://github.com/apache/fineract/actions/runs/21319148523/job/61366891412?pr=5346
working on it, will update the PR shortly.
--
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 263 : "Add Notes" for Centers permission is missing [fineract]
Posted by "Monica-CodingWorld (via GitHub)" <gi...@apache.org>.
Monica-CodingWorld commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3769298391
Hi reviewers,
This change updates the default handling in NotesApiResource#getResourceDetails to throw NoteResourceNotSupportedException instead of falling back to INVALIDNOTE, as discussed in FINERACT-263.Please let me know if there are any other files or cases you’d recommend updating as part of this fix.
I’d be happy to incorporate your feedback.
Thanks!
--
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-263 : Add Notes for Centers permission is missing [fineract]
Posted by "IOhacker (via GitHub)" <gi...@apache.org>.
IOhacker commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3795712409
@Monica-CodingWorld could you please take a look at https://github.com/apache/fineract/actions/runs/21319148523/job/61366891412?pr=5346
--
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-263: Add Notes for Centers permission is missing [fineract]
Posted by "Monica-CodingWorld (via GitHub)" <gi...@apache.org>.
Monica-CodingWorld closed pull request #5346: FINERACT-263: Add Notes for Centers permission is missing
URL: https://github.com/apache/fineract/pull/5346
--
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-263 : Add Notes for Centers permission is missing [fineract]
Posted by "Monica-CodingWorld (via GitHub)" <gi...@apache.org>.
Monica-CodingWorld commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3799742055
> Kindly see my concerns + you need to add proper testing (integration test / E2E).
Hi @adamsaghy,
Thanks for the detailed feedback - really appreciate you taking the time to go through this thoroughly.
You're absolutely right on all counts. Looking back, I took the wrong approach with the centerId field - I'll refactor to use entityId + entityType like you suggested.
Also totally missed the permission migrations, my bad. I'll add those properly also will add the integration tests.
Quick question on the SpotBugs fix - I actually already removed the exclusion and fixed the switch statement with a default case. Hope that's the right way to handle it?
Will push the fixes shortly. Thanks again for the guidance.
Regards,
Monica
--
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-263: Add Notes for Centers permission is missing [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3920938112
@Monica-CodingWorld What's up here?
--
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 263 : "Add Notes" for Centers permission is missing [fineract]
Posted by "Monica-CodingWorld (via GitHub)" <gi...@apache.org>.
Monica-CodingWorld commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3772994728
> PR content has nothing to do with PR title...
Really sorry all changes did not got commited. I'll update it shortly.
--
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 263 : "Add Notes" for Centers permission is missing [fineract]
Posted by "IOhacker (via GitHub)" <gi...@apache.org>.
IOhacker commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3795086926
:) always remember to squash and commit. Please help us with that.
--
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-263 : Add Notes for Centers permission is missing [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #5346:
URL: https://github.com/apache/fineract/pull/5346#discussion_r2727494552
##########
config/spotbugs/exclude.xml:
##########
@@ -102,4 +102,9 @@
<Match>
<Bug pattern="UR_UNINIT_READ" />
</Match>
+ <Match>
+ <Class name="org.apache.fineract.commands.domain.CommandWrapper" />
+ <Field name="centerId" />
+ <Bug pattern="URF_UNREAD_FIELD" />
+ </Match>
Review Comment:
We dont want this rule to exclude!
--
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-263: Add Notes for Centers permission is missing [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3977404963
@Monica-CodingWorld Are you planning of opening a new PR or did you change your mind with 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-263: Add Notes for Centers permission is missing [fineract]
Posted by "Monica-CodingWorld (via GitHub)" <gi...@apache.org>.
Monica-CodingWorld commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3977413330
> @Monica-CodingWorld Are you planning of opening a new PR or did you change your mind with this?
Hi @adamsaghy , I am planning to open a new PR for this as it's kind of messy here and it's getting harder to reworking the solution with a cleaner implementation based on your feedback.
will shortly open a PR for this issue :)
--
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 263 : "Add Notes" for Centers permission is missing [fineract]
Posted by "Monica-CodingWorld (via GitHub)" <gi...@apache.org>.
Monica-CodingWorld commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3772916126
> PR content has nothing to do with PR title...
i am really sorry wrong file got commited. I'll change it shortly.
--
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 263 : "Add Notes" for Centers permission is missing [fineract]
Posted by "Monica-CodingWorld (via GitHub)" <gi...@apache.org>.
Monica-CodingWorld closed pull request #5346: FINERACT 263 : "Add Notes" for Centers permission is missing
URL: https://github.com/apache/fineract/pull/5346
--
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-263: Add Notes for Centers permission is missing [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3834953781
@Monica-CodingWorld Please make sure to rebase this PR with latest develop branch as well!
--
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-263 : Add Notes for Centers permission is missing [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #5346:
URL: https://github.com/apache/fineract/pull/5346#discussion_r2727410751
##########
fineract-core/src/main/java/org/apache/fineract/portfolio/note/domain/NoteType.java:
##########
@@ -28,6 +28,7 @@ public enum NoteType {
CLIENT(100, "noteType.client", "clients", "Client note"), //
LOAN(200, "noteType.loan", "loans", "Loan note"), //
LOAN_TRANSACTION(300, "noteType.loan.transaction", "loanTransactions", "Loan transaction note"), //
+ CENTER(400, "noteType.center", "centers", "Center note"), //
Review Comment:
What is the difference between Center and Group?
--
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-263 : Add Notes for Centers permission is missing [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #5346:
URL: https://github.com/apache/fineract/pull/5346#discussion_r2727410751
##########
fineract-core/src/main/java/org/apache/fineract/portfolio/note/domain/NoteType.java:
##########
@@ -28,6 +28,7 @@ public enum NoteType {
CLIENT(100, "noteType.client", "clients", "Client note"), //
LOAN(200, "noteType.loan", "loans", "Loan note"), //
LOAN_TRANSACTION(300, "noteType.loan.transaction", "loanTransactions", "Loan transaction note"), //
+ CENTER(400, "noteType.center", "centers", "Center note"), //
Review Comment:
What is the difference between Center and Group?
--
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-263: Add Notes for Centers permission is missing [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3804289745
@Monica-CodingWorld Please make sure to rebase this PR with latest `develop` branch as well!
--
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 263 : "Add Notes" for Centers permission is missing [fineract]
Posted by "IOhacker (via GitHub)" <gi...@apache.org>.
IOhacker commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3773243097
@Monica-CodingWorld squash and commit your changes, make sure that only 1 commit is in the PR. Also you have to test it in your local dev env.
--
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-263 : Add Notes for Centers permission is missing [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #5346:
URL: https://github.com/apache/fineract/pull/5346#discussion_r2727499981
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/note/api/NotesApiResource.java:
##########
@@ -228,7 +229,11 @@ private CommandWrapper getResourceDetails(final NoteType type, final Long resour
resourceNameForPermissions = GROUPNOTE;
resourceDetails.withGroupId(resourceId);
}
- default -> resourceNameForPermissions = INVALIDNOTE;
+ case CENTER -> {
+ resourceNameForPermissions = CENTERNOTE;
Review Comment:
There is no such permission... you must add it before it can be used!
--
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-263 : Add Notes for Centers permission is missing [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #5346:
URL: https://github.com/apache/fineract/pull/5346#discussion_r2727408397
##########
fineract-core/src/main/java/org/apache/fineract/commands/domain/CommandWrapper.java:
##########
@@ -33,6 +34,8 @@ public class CommandWrapper {
private final Long clientId;
private final Long loanId;
private final Long savingsId;
+ @SuppressFBWarnings(value = "URF_UNREAD_FIELD", justification = "Will be used for center-specific permissions")
+ private final Long centerId;
Review Comment:
Center is a group no? I would rather not introduce new field. Kindly asking you to use the entityId + entityName is needed instead.
--
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 263 : "Add Notes" for Centers permission is missing [fineract]
Posted by "Monica-CodingWorld (via GitHub)" <gi...@apache.org>.
Monica-CodingWorld commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3795016457
> @Monica-CodingWorld could you please check the output https://github.com/apache/fineract/actions/runs/21178884898/job/61230181309?pr=5346
@IOhacker Thanks for the review. I've looked into the CI build failure. The error occurs because the addition of the centerId parameter to the CommandWrapper constructors requires updates in a few more places that instantiate the class. I have now identified and fixed these remaining instances (added null for the centerId parameter where appropriate) and have pushed the corrections. The build should pass now.
--
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-263 : Add Notes for Centers permission is missing [fineract]
Posted by "Monica-CodingWorld (via GitHub)" <gi...@apache.org>.
Monica-CodingWorld commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3796842932
> @Monica-CodingWorld could you please take a look at https://github.com/apache/fineract/actions/runs/21319148523/job/61366891412?pr=5346
@IOhacker @adamsaghy SpotBugs is failing on SF_SWITCH_NO_DEFAULT in NoteWritePlatformServiceJpaRepositoryImpl.getNoteForDelete() (missing default case in switch statement). This appears to be an existing issue that SpotBugs is now catching after the rebase. Should I:
1. Fix the code - Add a default case to the switch statement, or
2. Add exclusion - Add SF_SWITCH_NO_DEFAULT exclusion to config/spotbugs/exclude.xml?
--
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 263 : "Add Notes" for Centers permission is missing [fineract]
Posted by "Monica-CodingWorld (via GitHub)" <gi...@apache.org>.
Monica-CodingWorld commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3773793964
> @Monica-CodingWorld squash and commit your changes, make sure that only 1 commit is in the PR. Also you have to test it in your local dev env.
@adamsaghy Thank you for the guidance, i made all the changes.
The PR is now ready with a clean commit history. Could you please review when you have a moment?
Best Regards,
Monica
--
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 263 : "Add Notes" for Centers permission is missing [fineract]
Posted by "IOhacker (via GitHub)" <gi...@apache.org>.
IOhacker commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3788143260
@Monica-CodingWorld could you please check the output https://github.com/apache/fineract/actions/runs/21178884898/job/61230181309?pr=5346
--
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-263: Add Notes for Centers permission is missing [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #5346:
URL: https://github.com/apache/fineract/pull/5346#issuecomment-3883458556
@Monica-CodingWorld Please make sure to rebase this PR with latest develop branch as well!
--
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