fix: WEB-862 Localise the error as per the selected language#3407
Open
saksham869 wants to merge 1 commit intoopenMF:devfrom
Open
fix: WEB-862 Localise the error as per the selected language#3407saksham869 wants to merge 1 commit intoopenMF:devfrom
saksham869 wants to merge 1 commit intoopenMF:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Cohort / File(s) | Summary |
|---|---|
Error handler src/app/core/http/error-handler.interceptor.ts |
Reworked error-message selection to prefer response.error.errors[0] (using defaultUserMessage/developerMessage fallbacks), consolidated userMessageGlobalisationCode lookup to prefer nested key, apply translations via TranslateService.instant(...) when available, simplified parameterName extraction, limited OAuth-related 400 handling to requests with /oauth/token or /authentication, and updated 403 OTP-invalid detection to check the globalisation code. |
Translations src/assets/translations/en-US.json |
Added multiple top-level error translation keys (error.auth.*, error.token.invalid.*, error.bad.request.*, error.unauthorized.*, error.resource.not.found.*, error.server.internal.*, error.resource.notImplemented.*, error.unknown.*) and adjusted comma placement in a validation message. |
Env removal src/environments/.env.ts |
Deleted the auto-generated environment module that exported mifos_x (version/hash) and allow_switching_backend_instance configuration. |
Sequence Diagram(s)
mermaid
sequenceDiagram
participant Client as Client (UI)
participant HTTP as HttpClient
participant Interceptor as ErrorHandlerInterceptor
participant Translate as TranslateService
participant Alert as AlertService
Client->>HTTP: send request
HTTP-->>Client: error response
HTTP->>Interceptor: intercept(error)
Interceptor->>Interceptor: pick message from response.error.errors[0] or fallbacks
Interceptor->>Translate: instant(userMessageGlobalisationCode)
Translate-->>Interceptor: translatedText or key
Interceptor->>Alert: show(type, message)
Alert-->>Client: display alert
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- WEB-456: Add exception message for HTTP 501 #2840 — Modifies the same error-handler interceptor and adds TranslateService usage and a 501 handling branch.
- WEB-430: Fix Dead “Forgot Password” Button and Replace TODO Handlers #2811 — Redirects centralized API error handling into an ErrorHandlerService; touches related error-flow code.
- fix: WEB-860 Present Globalisation Code translated to local languages … #3452 — Alters nested
userMessageGlobalisationCodehandling and message-selection behavior in the interceptor.
Suggested reviewers
- alberto-art3ch
- IOhacker
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately describes the main objective: localizing error messages based on the selected language by extracting and using Fineract-provided localization codes in the ErrorHandlerInterceptor. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
saksham869
force-pushed
the
fix/WEB-862-final
branch
from
0624a89 to
f656f64
Compare
IOhacker
requested changes
Mar 19, 2026
Contributor
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
@saksham869 please review that only one commit is expected and make sure that the title is following the syntax required
saksham869
changed the title
saksham869
force-pushed
the
fix/WEB-862-final
branch
3 times, most recently
from
2c1f4a9 to
196f3e0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/assets/translations/pt-PT.json (1)
11-24: Consider normalizing the 501 key naming style for consistency.New keys use dotted segments (e.g.,
error.resource.not.found.*), while the nearby 501 key useserror.resource.notImplemented.*. Keeping both styles aligned reduces future lookup mismatches.♻️ Optional consistency patch
+ "error.resource.not.implemented.type": "Erro não implementado", + "error.resource.not.implemented.message": "Funcionalidade não implementada!",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/assets/translations/pt-PT.json` around lines 11 - 24, The 501-related keys use a different naming style (e.g., error.resource.notImplemented.type/message) which conflicts with the dotted-segment convention used elsewhere (e.g., error.resource.not.found.type/message); rename the 501 keys to follow the dotted convention (for example error.resource.not.implemented.type and error.resource.not.implemented.message) and update any lookup sites that reference the old keys to use the new keys to avoid runtime lookup mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/core/http/error-handler.interceptor.ts`:
- Around line 50-64: The interceptor currently lets
response.error.userMessageGlobalisationCode overwrite a more-specific nested
code and may have initialized errorMessage from developerMessage before checking
nested errors; update the logic in error-handler.interceptor.ts so that you
first prefer and assign nested values
(response.error.errors[0].userMessageGlobalisationCode and
.defaultUserMessage/.developerMessage) to globalisationCode and errorMessage,
and only set globalisationCode from response.error.userMessageGlobalisationCode
if globalisationCode is still null/undefined; ensure errorMessage is not
overwritten by a top-level developerMessage once a nested defaultUserMessage was
chosen.
- Around line 133-136: The catch-all else branch currently discards any resolved
backend message and always uses the generic unknown message; update the else
branch in the error-handling flow (the code that calls alertService.alert and
translate.instant) to prefer the resolved backend message (e.g. the
previously-derived defaultUserMessage or the translated
userMessageGlobalisationCode) when present, and only fall back to
translate.instant('error.unknown.message') if no backend message exists; ensure
you use the same translation helper (translate.instant) for the globalization
code path and pass that resolved string into alertService.alert's message field.
- Around line 109-114: The current branch where isClientImage404 returns a
successful Observable with null swallows the original 404; instead rethrow the
original error so downstream image service can handle it. Replace the return new
Observable(...) in the isClientImage404 branch with a rethrow using
throwError(() => err) (or throwError(() => originalError)) so the interceptor
emits the error rather than a null value; keep the isClientImage404 condition
and use the same err reference available in the interceptor's catch block.
- Around line 67-72: In src/app/core/http/error-handler.interceptor.ts the
hardcoded translation keys (e.g., 'error.auth.type', 'error.token.invalid.*',
'error.bad.request.*', etc.) lack the required "errors." prefix and therefore
fail to resolve; update all those hardcoded keys to include the "errors." prefix
(for example 'error.auth.type' -> 'errors.error.auth.type') and mirror the
existing pattern used around globalisationCode: use translate.instant(...) then
verify the returned value !== key before assigning to errorMessage (referencing
globalisationCode, translate.instant and errorMessage in the interceptor) so
only valid translations replace the fallback.
---
Nitpick comments:
In `@src/assets/translations/pt-PT.json`:
- Around line 11-24: The 501-related keys use a different naming style (e.g.,
error.resource.notImplemented.type/message) which conflicts with the
dotted-segment convention used elsewhere (e.g.,
error.resource.not.found.type/message); rename the 501 keys to follow the dotted
convention (for example error.resource.not.implemented.type and
error.resource.not.implemented.message) and update any lookup sites that
reference the old keys to use the new keys to avoid runtime lookup mismatches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5d483fe-52d3-4aaa-8f28-18ad8552bde4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**/*
📒 Files selected for processing (15)
src/app/core/http/error-handler.interceptor.tssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.jsonsrc/environments/.env.ts
Contributor
Author
|
Hi @IOhacker
All CI checks are now passing. Please re-review.. |
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/app/core/http/error-handler.interceptor.ts (4)
109-114:⚠️ Potential issue | 🟠 MajorRe-throw the client-image 404 instead of converting it to
null.This branch turns the 404 into a successful emission, so downstream handlers never see an error. If the image service is supposed to decide how to handle a missing photo, it needs the original
HttpErrorResponse.In Angular HttpInterceptor with RxJS catchError, does returning an Observable that emits `null` swallow the original `HttpErrorResponse`, and should `throwError(() => error)` be used instead to keep downstream error handlers working?🔧 Suggested fix
-import { Observable } from 'rxjs'; +import { Observable, throwError } from 'rxjs'; @@ - return new Observable((observer) => { - observer.next(null); - observer.complete(); - }); + return throwError(() => response);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/http/error-handler.interceptor.ts` around lines 109 - 114, The branch that handles isClientImage404 should re-throw the original HttpErrorResponse instead of returning an Observable that emits null; update the logic in the error handling path (in the interceptor method where isClientImage404 is checked) to return throwError(() => error) (or rethrow the error via throw) so downstream subscribers and the imaging service receive the original HttpErrorResponse rather than a successful null emission.
50-64:⚠️ Potential issue | 🟠 MajorKeep the first nested Fineract error as the winner.
Line 63 still overwrites the
errors[0]code selected on Lines 55-57. When Fineract sends both shapes, that replaces the more specific nested message with a generic top-level one and breaks the intended nested-first fallback.🔧 Suggested fix
- let errorMessage = response.error?.developerMessage || response.message; + let errorMessage = response.error?.defaultUserMessage || response.error?.developerMessage || response.message; let globalisationCode: string | null = null; @@ - if (response.error?.userMessageGlobalisationCode) { + if (!globalisationCode && response.error?.userMessageGlobalisationCode) { globalisationCode = response.error.userMessageGlobalisationCode; + errorMessage = response.error?.defaultUserMessage || errorMessage; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/http/error-handler.interceptor.ts` around lines 50 - 64, The current logic sets globalisationCode from response.error.userMessageGlobalisationCode even after selecting the nested winner from response.error.errors[0], which overwrites the more specific nested code; update the conditional that checks response.error?.userMessageGlobalisationCode so it only assigns to globalisationCode (and does not change errorMessage) if globalisationCode is still null/undefined after inspecting response.error.errors[0] — reference the variables response, globalisationCode, errorMessage and the nested response.error.errors[0] when making this change.
133-137:⚠️ Potential issue | 🟠 MajorDon't discard the resolved backend message in the catch-all branch.
Statuses like 409, 422, or 503 can already populate
errorMessageabove, but Line 136 always replaces it with the generic unknown text. That undoes the localization work for every non-enumerated status.🔧 Suggested fix
} else { this.alertService.alert({ - type: this.translate.instant('error.unknown.type'), - message: this.translate.instant('error.unknown.message') + type: this.translate.instant('errors.error.unknown.type'), + message: errorMessage || this.translate.instant('errors.error.unknown.message') }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/http/error-handler.interceptor.ts` around lines 133 - 137, The catch-all else branch in error-handler.interceptor.ts currently overwrites any resolved backend message by always using translate.instant('error.unknown.message'); update the else logic to only use the generic translated message when the local variable errorMessage (the resolved backend message computed earlier in the function) is empty/undefined, otherwise pass errorMessage through to this.alertService.alert; locate the branch around the this.alertService.alert call and conditionally choose between errorMessage and this.translate.instant('error.unknown.message') so backend/localized messages are preserved for statuses like 409/422/503.
66-72:⚠️ Potential issue | 🟠 MajorPrefix these HTTP error lookups with
errors..Line 68 and the status branches below are resolving keys from the root namespace, but the locale files store them under
errors(for examplesrc/assets/translations/lt-LT.json). With ngx-translate's default parser,instant('error.auth.type')misses and falls back to the key string, so alerts can render raw keys instead of localized text.Verification: confirm there is no custom parser remapping these keys, then verify that only the
errors.-prefixed paths resolve in the locale JSON.#!/bin/bash set -e # Check whether the app installs a custom ngx-translate parser / missing handler. rg -n "TranslateParser|MissingTranslationHandler|TranslateModule\\.forRoot" src/app python3 <<'PY' import json from pathlib import Path def get_value(target, key): keys = key.split(".") buf = "" cur = target while keys: buf += keys.pop(0) if isinstance(cur, dict) and buf in cur and (isinstance(cur[buf], (dict, list)) or not keys): cur = cur[buf] buf = "" elif not keys: cur = None else: buf += "." return cur data = json.loads(Path("src/assets/translations/lt-LT.json").read_text(encoding="utf-8")) for key in ( "error.auth.type", "errors.error.auth.type", "error.bad.request.type", "errors.error.bad.request.type", ): print(f"{key} => {get_value(data, key)!r}") PYExpected result: the unprefixed keys resolve to
None, while theerrors.-prefixed paths resolve to translated strings.🔧 Suggested fix pattern
- const translated = this.translate.instant(globalisationCode, response.error?.errors?.[0] || response.error || {}); - if (translated !== globalisationCode) { + const translationKey = globalisationCode.startsWith('errors.') + ? globalisationCode + : `errors.${globalisationCode}`; + const translated = this.translate.instant(translationKey, response.error?.errors?.[0] || response.error || {}); + if (translated !== translationKey) { errorMessage = translated; } @@ - type: this.translate.instant('error.auth.type'), + type: this.translate.instant('errors.error.auth.type'),Apply the same prefixing to the other hardcoded status keys as well.
Also applies to: 83-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/http/error-handler.interceptor.ts` around lines 66 - 72, The translation lookups are using root keys but your locale JSON nests messages under "errors", so update all calls that build or pass translation keys to prefix them with "errors." (e.g., the translate.instant call that uses globalisationCode and every branch that constructs status-based keys in the interceptor) so that translate.instant receives "errors.<key>" instead of "<key>"; locate uses of globalisationCode, translate.instant, and the response.status handling in error-handler.interceptor.ts and prepend "errors." to those keys (and to any hardcoded keys in the 83-137 range) ensuring you don't alter other logic like fallback handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/app/core/http/error-handler.interceptor.ts`:
- Around line 109-114: The branch that handles isClientImage404 should re-throw
the original HttpErrorResponse instead of returning an Observable that emits
null; update the logic in the error handling path (in the interceptor method
where isClientImage404 is checked) to return throwError(() => error) (or rethrow
the error via throw) so downstream subscribers and the imaging service receive
the original HttpErrorResponse rather than a successful null emission.
- Around line 50-64: The current logic sets globalisationCode from
response.error.userMessageGlobalisationCode even after selecting the nested
winner from response.error.errors[0], which overwrites the more specific nested
code; update the conditional that checks
response.error?.userMessageGlobalisationCode so it only assigns to
globalisationCode (and does not change errorMessage) if globalisationCode is
still null/undefined after inspecting response.error.errors[0] — reference the
variables response, globalisationCode, errorMessage and the nested
response.error.errors[0] when making this change.
- Around line 133-137: The catch-all else branch in error-handler.interceptor.ts
currently overwrites any resolved backend message by always using
translate.instant('error.unknown.message'); update the else logic to only use
the generic translated message when the local variable errorMessage (the
resolved backend message computed earlier in the function) is empty/undefined,
otherwise pass errorMessage through to this.alertService.alert; locate the
branch around the this.alertService.alert call and conditionally choose between
errorMessage and this.translate.instant('error.unknown.message') so
backend/localized messages are preserved for statuses like 409/422/503.
- Around line 66-72: The translation lookups are using root keys but your locale
JSON nests messages under "errors", so update all calls that build or pass
translation keys to prefix them with "errors." (e.g., the translate.instant call
that uses globalisationCode and every branch that constructs status-based keys
in the interceptor) so that translate.instant receives "errors.<key>" instead of
"<key>"; locate uses of globalisationCode, translate.instant, and the
response.status handling in error-handler.interceptor.ts and prepend "errors."
to those keys (and to any hardcoded keys in the 83-137 range) ensuring you don't
alter other logic like fallback handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20fc9573-c71c-411c-ada2-55aeee33e391
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**/*
📒 Files selected for processing (15)
src/app/core/http/error-handler.interceptor.tssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.jsonsrc/environments/.env.ts
✅ Files skipped from review due to trivial changes (5)
- src/assets/translations/es-MX.json
- src/assets/translations/de-DE.json
- src/assets/translations/cs-CS.json
- src/assets/translations/en-US.json
- src/assets/translations/fr-FR.json
🚧 Files skipped from review as they are similar to previous changes (7)
- src/assets/translations/ne-NE.json
- src/assets/translations/sw-SW.json
- src/assets/translations/pt-PT.json
- src/assets/translations/lv-LV.json
- src/assets/translations/ko-KO.json
- src/assets/translations/it-IT.json
- src/assets/translations/es-CL.json
saksham869
force-pushed
the
fix/WEB-862-final
branch
from
196f3e0 to
80a0e0d
Compare
Contributor
Author
|
Hi @IOhacker and — I have addressed all 4 review comments: |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/app/core/http/error-handler.interceptor.ts (1)
43-56:⚠️ Potential issue | 🟠 MajorPreserve the top-level
defaultUserMessageas the fallback.Responses that only populate top-level
userMessageGlobalisationCode/defaultUserMessagestill start fromdeveloperMessage, so a missing frontend key falls back to technical English instead of the server-localized text.🔧 Suggested fix
- let errorMessage = response.error?.developerMessage || response.message; + let errorMessage = response.error?.defaultUserMessage || response.error?.developerMessage || response.message; @@ if (!globalisationCode && response.error?.userMessageGlobalisationCode) { globalisationCode = response.error.userMessageGlobalisationCode; + errorMessage = response.error?.defaultUserMessage || errorMessage; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/http/error-handler.interceptor.ts` around lines 43 - 56, The current logic sets errorMessage from developerMessage then overrides with nested defaultUserMessage, but ignores a top-level defaultUserMessage fallback, causing developerMessage to be used when a top-level defaultUserMessage exists; update the initialization and fallback logic in error-handler.interceptor.ts (variables errorMessage, globalisationCode, response) so errorMessage is initialized as response.error?.developerMessage || response.error?.defaultUserMessage || response.message, and when nested errors are absent or their defaultUserMessage is missing, fall back to response.error?.defaultUserMessage before using developerMessage or response.message, while preserving the existing nested-priority and globalisationCode selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/core/http/error-handler.interceptor.ts`:
- Around line 72-89: The OAuth-specific 400 handling is too broad: narrow the
condition (currently using environment.oauth.enabled && status === 400) to only
apply to OAuth/token endpoints by also checking the request URL (e.g.,
request.url or req.url) for the token/auth path (like contains '/token' or the
app's auth route). Update the conditional in error-handler.interceptor (the
interceptor method that inspects status === 401 / 400) so it only shows the auth
alert when status === 400 AND the request URL matches the OAuth token endpoint;
leave the generic 400 branch (errors.error.bad.request) to handle other
validation/business errors.
- Around line 77-80: Replace the brittle check that compares
response.error?.errors?.[0]?.defaultUserMessage to an English string inside the
error handling branch with a check against the stable machine-readable field
response.error?.errors?.[0]?.userMessageGlobalisationCode (as already extracted
earlier in error-handler.interceptor.ts); specifically update the 403 OTP branch
to match the known globalization code (e.g., "error.msg.otp.token.invalid")
instead of the localized defaultUserMessage so the OTP logic in the interceptor
reliably triggers across languages.
---
Duplicate comments:
In `@src/app/core/http/error-handler.interceptor.ts`:
- Around line 43-56: The current logic sets errorMessage from developerMessage
then overrides with nested defaultUserMessage, but ignores a top-level
defaultUserMessage fallback, causing developerMessage to be used when a
top-level defaultUserMessage exists; update the initialization and fallback
logic in error-handler.interceptor.ts (variables errorMessage,
globalisationCode, response) so errorMessage is initialized as
response.error?.developerMessage || response.error?.defaultUserMessage ||
response.message, and when nested errors are absent or their defaultUserMessage
is missing, fall back to response.error?.defaultUserMessage before using
developerMessage or response.message, while preserving the existing
nested-priority and globalisationCode selection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c244537-dbdf-4553-8283-97c3eabb91ba
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**/*
📒 Files selected for processing (3)
src/app/core/http/error-handler.interceptor.tssrc/assets/translations/en-US.jsonsrc/environments/.env.ts
💤 Files with no reviewable changes (1)
- src/environments/.env.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/assets/translations/en-US.json
saksham869
force-pushed
the
fix/WEB-862-final
branch
from
80a0e0d to
e52988b
Compare
Contributor
Author
|
Hi @alberto-art3ch Hernández sir Merge Conflict Resolved + CodeRabbit Issues AddressedRebased onto the latest Changes Made1. Scoped OAuth 400 to auth endpoints only 2. OTP check now uses Verification
Ready for re-review. Happy to make any further adjustments! |
saksham869
force-pushed
the
fix/WEB-862-final
branch
from
e52988b to
21e90a4
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/core/http/error-handler.interceptor.ts (1)
98-105:⚠️ Potential issue | 🟡 MinorHardcoded English fallback string should be translated.
Lines 100-101 contain the hardcoded string
'Invalid parameters were passed in the request!'which violates i18n guidelines. Additionally, the fallback|| this.translate.instant('errors.error.bad.request.message')on line 104 is unreachable sincemessagewill always be truthy due to the hardcoded fallback.🔧 Suggested fix
} else if (status === 400) { + const fallbackMessage = this.translate.instant('errors.error.bad.request.message'); const message = parameterName - ? `[${parameterName}] ${errorMessage || 'Invalid parameters were passed in the request!'}` - : `${errorMessage || 'Invalid parameters were passed in the request!'}`; + ? `[${parameterName}] ${errorMessage || fallbackMessage}` + : errorMessage || fallbackMessage; this.alertService.alert({ type: this.translate.instant('errors.error.bad.request.type'), - message: message || this.translate.instant('errors.error.bad.request.message') + message: message });Based on learnings: "Use proper i18n variables from
ngx-translate/corefor all user-facing strings instead of hardcoded text".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/http/error-handler.interceptor.ts` around lines 98 - 105, The code uses a hardcoded English message and an unreachable fallback; replace the literal 'Invalid parameters were passed in the request!' with a translated string and build the message so the alert fallback is reachable: compute a baseMessage = errorMessage || this.translate.instant('errors.error.bad.request.message') and then set message = parameterName ? `[${parameterName}] ${baseMessage}` : baseMessage, and pass that message into this.alertService.alert without the extra "|| this.translate.instant(...)" fallback; update the block handling status === 400 (variables: status, parameterName, errorMessage, this.translate.instant, this.alertService.alert) accordingly.
🧹 Nitpick comments (1)
src/app/core/http/error-handler.interceptor.ts (1)
137-137: Consider usingreturn throwError()for consistency with line 113.Line 137 uses
throw responsedirectly while line 113 usesreturn throwError(() => response). Both work in acatchErrorcontext, but usingthrowErrorthroughout would be more consistent and idiomatic for RxJS error handling.♻️ Suggested refactor for consistency
} - throw response; + return throwError(() => response); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/http/error-handler.interceptor.ts` at line 137, In the catchError block where the interceptor currently does "throw response", replace that direct throw with "return throwError(() => response)" to match the pattern used earlier (line 113) and keep RxJS error handling consistent; look for the ErrorHandlerInterceptor (or the intercept method / catchError callback) and change the final throw to a return of throwError(() => response).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/app/core/http/error-handler.interceptor.ts`:
- Around line 98-105: The code uses a hardcoded English message and an
unreachable fallback; replace the literal 'Invalid parameters were passed in the
request!' with a translated string and build the message so the alert fallback
is reachable: compute a baseMessage = errorMessage ||
this.translate.instant('errors.error.bad.request.message') and then set message
= parameterName ? `[${parameterName}] ${baseMessage}` : baseMessage, and pass
that message into this.alertService.alert without the extra "||
this.translate.instant(...)" fallback; update the block handling status === 400
(variables: status, parameterName, errorMessage, this.translate.instant,
this.alertService.alert) accordingly.
---
Nitpick comments:
In `@src/app/core/http/error-handler.interceptor.ts`:
- Line 137: In the catchError block where the interceptor currently does "throw
response", replace that direct throw with "return throwError(() => response)" to
match the pattern used earlier (line 113) and keep RxJS error handling
consistent; look for the ErrorHandlerInterceptor (or the intercept method /
catchError callback) and change the final throw to a return of throwError(() =>
response).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a6641a91-9313-4530-8f2f-94e3a40c1e84
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**/*
📒 Files selected for processing (3)
src/app/core/http/error-handler.interceptor.tssrc/assets/translations/en-US.jsonsrc/environments/.env.ts
💤 Files with no reviewable changes (1)
- src/environments/.env.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/assets/translations/en-US.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Error messages in the Mifos web app were always displayed in English regardless of the language selected by the user. Apache Fineract sends two localisation codes in error responses — userMessageGlobalisationCode and errors.userMessageGlobalisationCode — but the error handler interceptor was completely ignoring these and always falling back to defaultUserMessage which is in English.
This PR updates the ErrorHandlerInterceptor to:
Extract userMessageGlobalisationCode from Fineract error responses — checking nested errors first (more specific), then top-level
Pass interpolation args to translate.instant() for parameterized error messages
Use errorMessage || this.translate.instant(...) pattern so server-localized messages are never discarded
Compare invalid token check against stable raw server message instead of localized text
Fix 404 client image handling — invert condition so error flows through to service handler
Use consistent .type and .message key naming throughout
Translation keys added for all HTTP error types (401, 400, 403, 404, 500, 501) across all 14 supported language files including cs-CS, de-DE, en-US, es-CL, es-MX, fr-FR, it-IT, ko-KO, lt-LT, lv-LV, ne-NE, pt-PT, sw-SW.
Related issues and discussion
WEB-862: https://mifosforge.jira.com/browse/WEB-862
Summary by CodeRabbit
New Features
Bug Fixes
Chores