Aman-Mittal commented on PR #260:
URL:
https://github.com/apache/fineract-backoffice-ui/pull/260#issuecomment-5227103286
Thanks for picking this up — the diagnosis is right, and letting a
domain-rule 403 through to the stacked-message branch is the correct shape for
the fix. One thing to change before it lands, and it is only visible against a
real platform.
## The stacked branch prefers `developerMessage`
The branch this now routes 403s into already reads:
```ts
const msg = err['developerMessage'] || err['defaultUserMessage'] ||
'Validation error';
```
`developerMessage` first. Reproduced against a running Fineract just now:
```
DELETE /codes/15/codevalues/24 → 403
errors[0].developerMessage : "The request caused a data integrity issue
to be fired by the database."
errors[0].defaultUserMessage : "This code value is in use"
```
So after this change that user stops seeing *"You do not have permission to
perform this action"* and starts seeing *"The request caused a data integrity
issue to be fired by the database."* — which is not an improvement, and is the
same category of problem the comment left in place is warning about. The
identical preference sits in the fallback a few lines below.
Flipping it to `defaultUserMessage || developerMessage` in both places gives
*"This code value is in use"*, which is the message the platform wrote for the
user.
The new test does not catch this because its fixture has no
`developerMessage` on the error entry. Real Fineract bodies always carry one —
the group-closure example in the test looks like this in practice:
```json
{
"errors": [
{
"developerMessage": "A loan with money outstanding cannot be closed",
"defaultUserMessage": "A loan with money outstanding cannot be closed",
"userMessageGlobalisationCode":
"error.msg.loan.close.loan.has.money.outstanding",
"parameterName": "id"
}
]
}
```
A fixture where the two strings **differ** would pin the behaviour that
matters.
## Two smaller notes
**`isDomainRuleViolation` collapses to `hasValidationErrors`.** Since it is
`code === '…' || hasValidationErrors`, and the `FORBIDDEN` branch is skipped
whenever either holds, the globalisation-code half never changes an outcome.
Worth dropping it, or keeping it and dropping the `||` — as written it reads
like it does more than it does.
There is also a gap between the two: when the code matches but `errors[]` is
empty, this skips `FORBIDDEN`, skips the stacked branch, and lands on the
top-level `developerMessage` — *"Request was understood but caused a domain
rule violation."*
**A question rather than a claim:** does the platform's genuine
permission-denied 403 also carry an `errors[]` array? If it does, this change
routes it into the stacked branch too, and the user would be shown the
`NOT_ALLOWED` permission code that the original guard existed to hide. I could
not construct that case here without a restricted user, so I have not asserted
it either way — but it seems worth one test before merge, since it is the only
path that could make things worse rather than better.
Happy to be wrong on any of this; the underlying change is one the app needs.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
