WEB-113 GLIM Application Fails to Submit #2662
Conversation
|
@steinwinde Please Review . |
| }) | ||
| export class CreateGlimAccountComponent { | ||
| /** Date format for interestChargedFromDate and other dates */ | ||
| _dateFormat: string = 'dd-MM-yy'; |
There was a problem hiding this comment.
why we need to use a fixed format? why we can not take this directly from the settings?
| } | ||
|
|
||
| // Enforce date format for interestChargedFromDate | ||
| const dateFormat = 'dd-MM-yy'; |
JaySoni1
force-pushed
the
WEB-113-glim-application-fails-to-submit
branch
from
0e31f16 to
e988834
Compare
|
@alberto-art3ch Thank you for pointing that out! I was initially thinking of enforcing a fixed format for consistency, but your suggestion to use the format from settings makes more sense for localization and flexibility. I have updated the code accordingly. |
JaySoni1
force-pushed
the
WEB-113-glim-application-fails-to-submit
branch
from
e988834 to
d2cbc59
Compare
|
@steinwinde I have updated the PR please review. |
There was a problem hiding this comment.
Hey @JaySoni1, you’ve only added translations for the US locale. Could you also add the required translations for the other locales
JaySoni1
force-pushed
the
WEB-113-glim-application-fails-to-submit
branch
from
d2cbc59 to
adddf1a
Compare
WalkthroughAdds client-side member-selection and per-member validations to CreateGlimAccountComponent.submit(), injects I18nService into that component, extends I18nService.translate to accept params, changes notify() to accept optional context, and adds new localized error keys across multiple locale JSON files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant C as CreateGlimAccountComponent
participant V as ValidationLogic
participant I as I18nService
participant N as Notify
U->>C: Click Submit
C->>V: Resolve selectedMembers (safe access/default)
alt no members
C->>N: notify(body, { batchSize: 0 }) and return
else members present
loop per member
C->>V: check linked savings account owner
alt owner mismatch
C->>I: translate('error.linkedSavingsAccountOwnership')
I-->>C: localized message
C->>N: notify(message, { memberId: member.id }) and abort
else owner OK
C->>V: check GSIM membership by id
alt not in GSIM
C->>I: translate('error.clientNotInGSIM', { id: member.id })
I-->>C: localized message
C->>N: notify(message, { memberId: member.id }) and abort
else member OK
C->>C: continue
end
end
end
alt all members valid
C->>V: build request & create GLIM
V-->>C: response (may include glimId)
C->>N: notify(successBody, { batchSize: N }) or notify(errorBody)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
src/assets/translations/es-MX.json (1)
5-6: Ensure i18n key path matches usage (nested vs flat).If the code calls translate('error.linkedSavingsAccountOwnership') / translate('error.clientNotInGSIM'), these should be nested under an "error" object; currently they’re flat keys with dots. Recommend nesting to avoid lookup misses.
Please confirm how I18nService is invoked in create-glim-account.component.ts. If it expects nested keys, apply:
- "error.linkedSavingsAccountOwnership": "La cuenta de ahorro vinculada no pertenece al cliente seleccionado.", - "error.clientNotInGSIM": "El cliente con id {{id}} no está presente en GSIM.", + "error": { + "linkedSavingsAccountOwnership": "La cuenta de ahorro vinculada no pertenece al cliente seleccionado.", + "clientNotInGSIM": "El cliente con ID {{id}} no está presente en GSIM." + },src/assets/translations/en-US.json (2)
5-6: Consider scoping under the existing errors namespace for consistency.Most error strings live under the "errors" object; adding top‑level "error.*" keys diverges from that pattern. If the code expects keys under "errors", move them there; otherwise, keep as‑is but be consistent across locales.
Example adjustment inside the "errors" block (and remove the top‑level duplicates):
- "error.linkedSavingsAccountOwnership": "Linked savings account does not belong to the selected client.", - "error.clientNotInGSIM": "Client with id {{id}} is not present in GSIM.", + "error.linkedSavingsAccountOwnership": "Linked savings account does not belong to the selected client.", + "error.clientNotInGSIM": "Client with ID {{id}} is not present in GSIM."
6-6: Tiny copyedit for clarity/case.Prefer “ID” over “id”; optional: “not a member of GSIM.”
- "error.clientNotInGSIM": "Client with id {{id}} is not present in GSIM.", + "error.clientNotInGSIM": "Client with ID {{id}} is not present in GSIM."Alternative:
- "Client with ID {{id}} is not a member of GSIM."
src/assets/translations/cs-CS.json (1)
5-6: i18n keys and usage verified
error.linkedSavingsAccountOwnershipanderror.clientNotInGSIMpresent in all locales and referenced increate-glim-account.component.ts.{id}placeholder is substituted viamsg.replace('{{id}}', member.id).- Optional Czech phrasing: consider “není v GSIMu” or “není členem GSIM” for a more natural tone.
src/assets/translations/ko-KO.json (1)
6-6: Optional: align terminology (“클라이언트” vs “고객”)This locale mixes both terms. If you prefer “고객” (used widely elsewhere in this file), consider:
- "error.clientNotInGSIM": "ID가 {{id}}인 클라이언트가 GSIM에 없습니다.", + "error.clientNotInGSIM": "ID가 {{id}}인 고객이 GSIM에 없습니다.",src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
247-253: Consider treating missing linkAccountOwnerId as invalid when linkAccountId is setCurrent check only fails when both IDs exist and mismatch. If linkAccountId is chosen but owner is absent, you may want to block submission.
263-263: Resolve TODO or remove itEither implement interestChargedFromDate formatting using settingsService.dateFormat, or drop the comment to avoid drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts(3 hunks)src/assets/translations/cs-CS.json(1 hunks)src/assets/translations/de-DE.json(1 hunks)src/assets/translations/en-US.json(1 hunks)src/assets/translations/es-CL.json(1 hunks)src/assets/translations/es-MX.json(1 hunks)src/assets/translations/fr-FR.json(1 hunks)src/assets/translations/it-IT.json(1 hunks)src/assets/translations/ko-KO.json(1 hunks)src/assets/translations/lt-LT.json(1 hunks)src/assets/translations/lv-LV.json(1 hunks)src/assets/translations/ne-NE.json(1 hunks)src/assets/translations/pt-PT.json(1 hunks)src/assets/translations/sw-SW.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (11)
src/assets/translations/sw-SW.json (1)
5-6: Verify translation key usage and placeholder alignment
- No occurrences of
error.linkedSavingsAccountOwnershiporerror.clientNotInGSIMfound in i18n calls; confirm these keys are actually used.- Ensure any calls to
error.clientNotInGSIMsupply{ id }to match the{{id}}placeholder.- (Optional) for a more idiomatic Swahili passive, update the phrasing:
- "error.linkedSavingsAccountOwnership": "Akaunti ya akiba iliyounganishwa haimilikiwi na mteja aliyechaguliwa.", + "error.linkedSavingsAccountOwnership": "Akaunti ya akiba iliyounganishwa haimilikiwa na mteja aliyechaguliwa.",src/assets/translations/de-DE.json (1)
5-6: Confirm key usage – I found no TS/JS or HTML references for error.linkedSavingsAccountOwnership and error.clientNotInGSIM; remove them if unused or hook them into code to avoid dead translations.src/assets/translations/it-IT.json (1)
5-6: LGTM! Translation keys properly added for GLIM loan validation.The Italian translations for the new error messages look appropriate:
error.linkedSavingsAccountOwnership: Clear message about linked savings account ownershiperror.clientNotInGSIM: Proper use of placeholder{{id}}for client ID interpolationThe translations are grammatically correct and follow the existing error message patterns in the file.
src/assets/translations/en-US.json (1)
5-6: Verify translation usage & interpolation
- Presence of both keys confirmed across all locale files.
- I didn’t find any code references to
error.clientNotInGSIM; ensure it’s invoked with an{ id }value (e.g.<FormattedMessage id="error.clientNotInGSIM" values={{ id }} />ort('error.clientNotInGSIM', { id })).- Likewise, verify
error.linkedSavingsAccountOwnershipis used correctly with any required parameters.src/assets/translations/fr-FR.json (1)
5-6: Retain top-level error. namespace*
Keys are present in every locale and components correctly reference them as top-levelerror.*; no namespace refactor needed.src/assets/translations/ne-NE.json (1)
5-6: Root placement of these error keys is correct
They’re consistently defined at the root in every locale and directly referenced viai18nService.translate('error.linkedSavingsAccountOwnership')andtranslate('error.clientNotInGSIM'). No relocation needed.Likely an incorrect or invalid review comment.
src/assets/translations/lt-LT.json (2)
5-6: Translation content LGTM.Lithuanian phrasing and {{id}} placeholder look correct. No typos.
Please ensure the same phrasing and placeholder are present in en-US and other added locales for parity.
5-6: Keep flat dotted keys for error translations at root.
Theerror.clientNotInGSIManderror.linkedSavingsAccountOwnershipentries follow the established pattern across all locales—error messages are defined as top-level keys with dotted prefixes. Introducing anerrorobject here (and especially a singular instead of the existingerrorsgrouping) would diverge from the convention and risk breaking lookups.Likely an incorrect or invalid review comment.
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
3-3: LGTM: i18n import added to support localized errors
88-90: LGTM: injected Dates and I18nService
Constructor wiring is consistent with later usage.src/assets/translations/pt-PT.json (1)
5-6: Verify flat dotted-key support or nest under “error”
We’ve added"error.linkedSavingsAccountOwnership"and"error.clientNotInGSIM"as flat keys across all locale files, but the codebase search didn’t reveal how the translation loader resolves dotted paths. Confirm that your i18n setup supports flat dotted-key lookups (e.g.translate.instant('error.clientNotInGSIM')); if not, nest these keys under an"error"object in each JSON.
JaySoni1
force-pushed
the
WEB-113-glim-application-fails-to-submit
branch
from
adddf1a to
8eb96db
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
266-279: Handle HTTP errors and avoid silent failures
subscribelacks an error handler; network/backend errors will result in no user feedback.- this.loansService.createGlimAccount(data).subscribe((response: any) => { - const body = JSON.parse(response[0].body); - if (body.glimId) { - this.router.navigate( - [ - '../', - body.glimId - ], - { relativeTo: this.route } - ); - } else { - this.notify(body, data); - } - }); + this.loansService.createGlimAccount(data).subscribe({ + next: (response: any) => { + const parsed = typeof response?.[0]?.body === 'string' ? JSON.parse(response[0].body) : response?.[0]?.body; + const body = parsed ?? {}; + if (body.glimId) { + this.router.navigate(['../', body.glimId], { relativeTo: this.route }); + } else { + this.notify(body, data); + } + }, + error: (err) => { + this.notify(err?.error ?? { defaultUserMessage: 'GLIM submission failed', errors: [] }, data); + } + });
🧹 Nitpick comments (4)
src/app/core/i18n/i18n.service.ts (1)
12-13: Tightenparamstyping for safetyPrefer
Record<string, unknown>over bareobjectto avoid structural typing pitfalls and improve DX.- public translate(key: string, params?: object): Observable<string> { + public translate(key: string, params?: Record<string, unknown>): Observable<string> { return this.translateService.get(key, params); }src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (3)
246-257: Normalize ID types to avoid false negatives inincludesIf any IDs are strings,
includescan fail due to type mismatch.- const gsimMembers = this.dataSource.map((m: any) => m.id); + const gsimMembers = this.dataSource.map((m: any) => Number(m.id)); @@ - if (!gsimMembers.includes(member.id)) { + if (!gsimMembers.includes(Number(member.id))) {
282-287: Reduce PII in logs
notifylogs raw backend errors and fulldatapayloads (member details, principals). Consider logging only minimal context in production.Example:
- message += 'Data: ' + JSON.stringify(data); - console.error(message); + const redacted = Array.isArray(data) ? `[${data.length} requests]` : `{ id: ${data?.id ?? 'n/a'} }`; + console.error(`${message} Context: ${redacted}`);
250-253: Add placeholders to linkedSavingsAccountOwnership translations before interpolation
ThelinkedSavingsAccountOwnershipkey currently has no{{id}}or{{accountId}}placeholders in any locale. Add these placeholders to the JSON entries if you intend to passmember.idandmember.linkAccountId; otherwise, keep the static message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/app/core/i18n/i18n.service.ts(1 hunks)src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts(3 hunks)src/assets/translations/cs-CS.json(1 hunks)src/assets/translations/de-DE.json(1 hunks)src/assets/translations/en-US.json(1 hunks)src/assets/translations/es-CL.json(1 hunks)src/assets/translations/es-MX.json(1 hunks)src/assets/translations/fr-FR.json(1 hunks)src/assets/translations/it-IT.json(1 hunks)src/assets/translations/ko-KO.json(1 hunks)src/assets/translations/lt-LT.json(1 hunks)src/assets/translations/lv-LV.json(1 hunks)src/assets/translations/ne-NE.json(1 hunks)src/assets/translations/pt-PT.json(1 hunks)src/assets/translations/sw-SW.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- src/assets/translations/it-IT.json
- src/assets/translations/de-DE.json
- src/assets/translations/ko-KO.json
- src/assets/translations/fr-FR.json
- src/assets/translations/en-US.json
- src/assets/translations/lv-LV.json
- src/assets/translations/cs-CS.json
- src/assets/translations/lt-LT.json
- src/assets/translations/ne-NE.json
- src/assets/translations/es-MX.json
- src/assets/translations/pt-PT.json
- src/assets/translations/sw-SW.json
- src/assets/translations/es-CL.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (2)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
3-3: LGTM: i18n service importImport aligns with new localized error handling in submit().
88-90: LGTM: constructor DIInjecting
DatesandI18nServiceis appropriate for date formatting and localized messages.
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
Outdated
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-113-glim-application-fails-to-submit
branch
from
8eb96db to
e3e6a4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
197-202: Follow-through: format interestChargedFromDate when presentThe comment notes using settings’ date format; include it in the payload if the field is set.
expectedDisbursementDate: this.dateUtils.formatDate(this.loansAccount.expectedDisbursementDate, dateFormat), submittedOnDate: this.dateUtils.formatDate(this.loansAccount.submittedOnDate, dateFormat), + ...(this.loansAccount.interestChargedFromDate + ? { interestChargedFromDate: this.dateUtils.formatDate(this.loansAccount.interestChargedFromDate, dateFormat) } + : {}), dateFormat,
282-287: Avoid logging full payloads; redact or surface via UInotify() currently console.error’s the entire payload (can expose PII). Prefer user-visible toast/snackbar and minimal context in logs.
- notify(body: any, data: any) { - let message = body.defaultUserMessage + ' '; - while (body.errors?.length > 0) message += body.errors.pop().developerMessage + ' '; - message += 'Data: ' + JSON.stringify(data); - console.error(message); - } + notify(body: any, data?: any) { + const parts = [body.defaultUserMessage]; + if (Array.isArray(body.errors)) { + for (const e of body.errors) parts.push(e.developerMessage); + } + // Log minimal context to reduce PII exposure + const ctx = data?.id ? ` (context id: ${data.id})` : ''; + console.warn(parts.join(' ') + ctx); + // TODO: optionally surface to UI via a Snackbar/NotificationService + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/app/core/i18n/i18n.service.ts(1 hunks)src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts(5 hunks)src/assets/translations/cs-CS.json(1 hunks)src/assets/translations/de-DE.json(1 hunks)src/assets/translations/en-US.json(1 hunks)src/assets/translations/es-CL.json(1 hunks)src/assets/translations/es-MX.json(1 hunks)src/assets/translations/fr-FR.json(1 hunks)src/assets/translations/it-IT.json(1 hunks)src/assets/translations/ko-KO.json(1 hunks)src/assets/translations/lt-LT.json(1 hunks)src/assets/translations/lv-LV.json(1 hunks)src/assets/translations/ne-NE.json(1 hunks)src/assets/translations/pt-PT.json(1 hunks)src/assets/translations/sw-SW.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/assets/translations/es-CL.json
- src/assets/translations/es-MX.json
- src/assets/translations/it-IT.json
- src/assets/translations/ko-KO.json
- src/assets/translations/cs-CS.json
- src/assets/translations/lv-LV.json
- src/assets/translations/pt-PT.json
- src/app/core/i18n/i18n.service.ts
- src/assets/translations/lt-LT.json
- src/assets/translations/de-DE.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (8)
src/assets/translations/en-US.json (1)
5-8: Error strings wired for GSIM validation.Thanks for adding localized messaging with the
{{id}}placeholder; this will slot straight into the parameterized translate() updates so users see the right member context.src/assets/translations/fr-FR.json (1)
5-8: Translations added correctly; keys exist in all locale files. No occurrences oferror.linkedSavingsAccountOwnershiporerror.clientNotInGSIMwere found in the codebase—please add or verify the correspondingi18nService.translate('error.linkedSavingsAccountOwnership')andi18nService.translate('error.clientNotInGSIM', { id })calls.src/assets/translations/sw-SW.json (1)
5-8: Translations look solid.The new Swahili strings read naturally and keep the
{{id}}placeholder intact, so they’ll render correctly in the UI.src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (4)
3-3: LGTM: I18nService import added where needed.
88-90: LGTM: DI additionsInjecting Dates and I18nService here is appropriate for formatting and localized errors.
217-218: Good hardening of selectedMembers accessOptional chaining + default empty array prevents runtime errors flagged earlier.
232-233: Good: total calculation guardedPrevents undefined access when computing totals.
src/assets/translations/ne-NE.json (1)
5-8: New error messages coverage confirmed across all localesBoth
linkedSavingsAccountOwnershipandclientNotInGSIMkeys are present in every file undersrc/assets/translations.
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-113-glim-application-fails-to-submit
branch
from
e3e6a4e to
6a9f570
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/app/core/i18n/i18n.service.ts (1)
12-13: Tighten param typing for better DXPrefer Record<string, unknown> (or a lib type) over bare object for params.
- public translate(key: string, params?: object): Observable<string> { + public translate(key: string, params?: Record<string, unknown>): Observable<string> { return this.translateService.get(key, params); }src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
246-246: Name nit: clarify set semanticsgsimMemberIds is a Set; consider gsimMemberIdSet for readability.
265-265: Finish the date-format TODO (interestChargedFromDate)If interestChargedFromDate can be set in loansAccount, format it with settings dateFormat before submit.
- // Use date format from settingsService for interestChargedFromDate + // Use date format from settingsService for interestChargedFromDate + if (this.loansAccount?.interestChargedFromDate) { + const df = this.settingsService.dateFormat; + this.loansAccount.interestChargedFromDate = + this.dateUtils.formatDate(this.loansAccount.interestChargedFromDate, df); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/app/core/i18n/i18n.service.ts(1 hunks)src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts(5 hunks)src/assets/translations/cs-CS.json(1 hunks)src/assets/translations/de-DE.json(1 hunks)src/assets/translations/en-US.json(1 hunks)src/assets/translations/es-CL.json(1 hunks)src/assets/translations/es-MX.json(1 hunks)src/assets/translations/fr-FR.json(1 hunks)src/assets/translations/it-IT.json(1 hunks)src/assets/translations/ko-KO.json(1 hunks)src/assets/translations/lt-LT.json(1 hunks)src/assets/translations/lv-LV.json(1 hunks)src/assets/translations/ne-NE.json(1 hunks)src/assets/translations/pt-PT.json(1 hunks)src/assets/translations/sw-SW.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- src/assets/translations/it-IT.json
- src/assets/translations/sw-SW.json
- src/assets/translations/es-CL.json
- src/assets/translations/es-MX.json
- src/assets/translations/en-US.json
- src/assets/translations/cs-CS.json
- src/assets/translations/de-DE.json
- src/assets/translations/ne-NE.json
- src/assets/translations/lv-LV.json
- src/assets/translations/ko-KO.json
- src/assets/translations/fr-FR.json
- src/assets/translations/pt-PT.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (2)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
217-217: Null-safe access for selectedMembersGood defensive defaults to empty arrays; prevents runtime errors.
Also applies to: 232-232
src/assets/translations/lt-LT.json (1)
5-8: All locales include the new error keys
Verified presence oflinkedSavingsAccountOwnershipandclientNotInGSIMacross all translation files.
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
Show resolved
Hide resolved
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
Outdated
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-113-glim-application-fails-to-submit
branch
from
6a9f570 to
dca7364
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/app/core/i18n/i18n.service.ts (1)
12-14: Tightenparamstyping for better DX and safetyCurrent
params?: objectworks, but is loose. Prefer a constrained map type for interpolation params.Apply:
@@ -import { TranslateService } from '@ngx-translate/core'; +import { TranslateService } from '@ngx-translate/core'; import { Observable } from 'rxjs'; @Injectable() export class I18nService { constructor(private translateService: TranslateService) {} - public translate(key: string, params?: object): Observable<string> { + public translate(key: string, params?: TranslateParams): Observable<string> { return this.translateService.get(key, params); } }Add this type alias near the imports:
export type TranslateParams = Record<string, string | number | boolean>;Optional: if you foresee translating multiple keys at once later, consider an overload or a dedicated
translateMany(keys: string[], params?: TranslateParams): Observable<Record<string, string>>.src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (3)
243-265: Harden ID normalization to avoid NaN edge casesCasting with Number(...) can yield NaN for unexpected inputs, breaking Set membership and equality checks. Normalize with a single helper that falls back to string when non‑numeric.
- const gsimMemberIds = new Set(this.dataSource.map((m: any) => Number(m.id))); - for (const member of memberSelected) { - const memberId = Number(member.id); + const normalizeId = (v: unknown) => { + const n = Number(v); + return Number.isFinite(n) ? n : String(v); + }; + const gsimMemberIds = new Set(this.dataSource.map((m: any) => normalizeId(m.id))); + for (const member of memberSelected) { + const memberId = normalizeId(member.id); // Validate savings account ownership - const ownerId = Number(member.linkAccountOwnerId); + const ownerId = normalizeId(member.linkAccountOwnerId); if (member.linkAccountId && member.linkAccountOwnerId && ownerId !== memberId) { this.i18nService.translate('error.linkedSavingsAccountOwnership').subscribe((msg: string) => { this.notify({ defaultUserMessage: msg, errors: [] }, { memberId }); }); return; } // Validate GSIM membership - if (!gsimMemberIds.has(memberId)) { + if (!gsimMemberIds.has(memberId)) { this.i18nService.translate('error.clientNotInGSIM', { id: memberId }).subscribe((msg: string) => { this.notify({ defaultUserMessage: msg, errors: [] }, { memberId }); }); return; } }
268-281: Add error handler for createGlimAccount subscribeSurface backend failures via notify for better UX and diagnostics.
- this.loansService.createGlimAccount(data).subscribe((response: any) => { + this.loansService.createGlimAccount(data).subscribe({ + next: (response: any) => { const body = JSON.parse(response[0].body); if (body.glimId) { this.router.navigate( [ '../', body.glimId ], { relativeTo: this.route } ); } else { this.notify(body, { batchSize: data.length }); } - }); + }, + error: (err: any) => { + const body = err?.error ?? { defaultUserMessage: 'GLIM application creation failed' }; + this.notify(body, { status: err?.status }); + } + });
284-291: Sanitized logging looks good; cap context if neededConsole output avoids dumping payloads. Consider truncating or allow‑listing context fields to prevent accidental large logs in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/app/core/i18n/i18n.service.ts(1 hunks)src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts(6 hunks)src/assets/translations/cs-CS.json(1 hunks)src/assets/translations/de-DE.json(1 hunks)src/assets/translations/en-US.json(1 hunks)src/assets/translations/es-CL.json(1 hunks)src/assets/translations/es-MX.json(1 hunks)src/assets/translations/fr-FR.json(1 hunks)src/assets/translations/it-IT.json(1 hunks)src/assets/translations/ko-KO.json(1 hunks)src/assets/translations/lt-LT.json(1 hunks)src/assets/translations/lv-LV.json(1 hunks)src/assets/translations/ne-NE.json(1 hunks)src/assets/translations/pt-PT.json(1 hunks)src/assets/translations/sw-SW.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/assets/translations/es-CL.json
- src/assets/translations/pt-PT.json
- src/assets/translations/ko-KO.json
- src/assets/translations/lv-LV.json
- src/assets/translations/es-MX.json
- src/assets/translations/de-DE.json
- src/assets/translations/it-IT.json
- src/assets/translations/ne-NE.json
- src/assets/translations/lt-LT.json
- src/assets/translations/cs-CS.json
- src/assets/translations/en-US.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (3)
src/assets/translations/sw-SW.json (1)
5-8: No issues found:error.*namespace and parameters are correct
- Code consistently uses
error.linkedSavingsAccountOwnershipanderror.clientNotInGSIM, matching the new keys.- The
{{id}}placeholder is supplied via{ id: memberId }in the.translatecall.- Both keys exist in all locale JSONs, preventing fallback-to-key errors.
- (Optional) you may consolidate
error.*anderrors.*namespaces for uniformity.src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
88-90: Good addition: i18n and date utils injectionMatches PR intent to use settings-driven formats and localized messages.
217-218: Safe guards for optional selectedMembersUsing optional chaining with sane defaults prevents runtime errors in aggregate/build paths. LGTM.
Also applies to: 232-233
| } | ||
| } | ||
|
|
||
| // Use date format from settingsService for interestChargedFromDate |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Complete TODO: format interestChargedFromDate using settings dateFormat
This is part of the PR objective; format it alongside submitted/disbursement dates to avoid server parse errors.
expectedDisbursementDate: this.dateUtils.formatDate(this.loansAccount.expectedDisbursementDate, dateFormat),
submittedOnDate: this.dateUtils.formatDate(this.loansAccount.submittedOnDate, dateFormat),
+ ...(this.loansAccount.interestChargedFromDate && {
+ interestChargedFromDate: this.dateUtils.formatDate(this.loansAccount.interestChargedFromDate, dateFormat)
+ }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Use date format from settingsService for interestChargedFromDate | |
| expectedDisbursementDate: this.dateUtils.formatDate(this.loansAccount.expectedDisbursementDate, dateFormat), | |
| submittedOnDate: this.dateUtils.formatDate(this.loansAccount.submittedOnDate, dateFormat), | |
| ...(this.loansAccount.interestChargedFromDate && { | |
| interestChargedFromDate: this.dateUtils.formatDate(this.loansAccount.interestChargedFromDate, dateFormat) | |
| }), |
🤖 Prompt for AI Agents
In
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
around line 266, interestChargedFromDate is not being formatted with the
application's date format; format interestChargedFromDate the same way
submittedDate and disbursementDate are formatted using the settingsService date
format (e.g., settingsService.dateFormat or settingsService.getDateFormat()),
convert the date to a string in that format before assigning/sending it, and
include null/undefined checks so you only format when a date exists.
src/assets/translations/fr-FR.json
Outdated
| "error": { | ||
| "linkedSavingsAccountOwnership": "Le compte d'épargne lié n'appartient pas au client sélectionné.", | ||
| "clientNotInGSIM": "Le client avec l'ID {{id}} n'est pas membre du GSIM." | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM: New error translations added; ensure locale coverage
Strings look good and match usage in code. Please verify these keys exist in all locale files referenced by the app.
Run to confirm coverage:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
missing=0
while IFS= read -r -d '' f; do
for k in linkedSavingsAccountOwnership clientNotInGSIM; do
if ! jq -e --arg k "$k" 'has("error") and .error | has($k)' "$f" >/dev/null; then
echo "Missing .error.\"$k\" in $f"
missing=1
fi
done
done < <(fd -a src/assets/translations -e json -0)
exit $missingLength of output: 455
🏁 Script executed:
#!/bin/bash
set -euo pipefail
missing=0
while IFS= read -r -d '' f; do
for k in linkedSavingsAccountOwnership clientNotInGSIM; do
if ! jq -e --arg k "$k" 'has("error") and .error | has($k)' "$f" >/dev/null; then
echo "Missing .error.\"$k\" in $f"
missing=1
fi
done
done < <(find src/assets/translations -type f -name '*.json' -print0)
exit $missingLength of output: 57
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=$(find src/assets/translations -type f -name '*.json')
for f in $files; do
for k in linkedSavingsAccountOwnership clientNotInGSIM; do
if ! jq -e --arg k "$k" 'has("error") and .error | has($k)' "$f" >/dev/null; then
echo "Missing .error.\"$k\" in $f"
exit 1
fi
done
doneLength of output: 1039
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
for f in $(find src/assets/translations -type f -name '*.json'); do
for k in linkedSavingsAccountOwnership clientNotInGSIM; do
jq -e --arg k "$k" 'has("error") and .error | has($k)' "$f" >/dev/null || { echo "Missing .error.\"$k\" in $f"; exit 1; }
done
doneLength of output: 186
Fix Czech translations and add missing error keys
The src/assets/translations/cs-CS.json file defines "error" as a non-object (boolean), so it’s missing both linkedSavingsAccountOwnership and clientNotInGSIM. Update that file (and any others) so that under "error" you have:
"error": {
"linkedSavingsAccountOwnership": "...",
"clientNotInGSIM": "..."
}Ensure all locale JSON files include these two keys under "error".
🤖 Prompt for AI Agents
In src/assets/translations/fr-FR.json around lines 5 to 8: the review notes that
cs-CS.json (and potentially other locale files) define "error" as a non-object
(boolean) and are missing the keys linkedSavingsAccountOwnership and
clientNotInGSIM; update cs-CS.json (and audit other locale JSONs) to ensure
"error" is an object, add the two keys "linkedSavingsAccountOwnership" and
"clientNotInGSIM" with appropriate translations, and validate the JSON so every
locale has an "error" object containing those keys (preserve existing
translations and nesting).
|
@steinwinde , @gkbishnoi07 I have updated the PR please review . |
There was a problem hiding this comment.
Hi @JaySoni1 , you have added the translation to the translation file under a new top level "error" (when there is already an "errors" top level category). May I ask, why do you think your addition deserves to a new top level category? If you look at the translation files, the only other translations on top level are the app name, "login as", and "Remember me". And below there are already lots of categories and sub-categories. Can't you find an existing one for your case?
|
@steinwinde Thank you for your review . I will update the PR . |
JaySoni1
force-pushed
the
WEB-113-glim-application-fails-to-submit
branch
from
dca7364 to
95ad78c
Compare
|
@steinwinde I have updated the PR . Please review |
Changes Made :-
-Added validation for savings account ownership and GSIM membership in GLIM loan.
-Enforced correct date format for loan submission.
-Enhanced error handling for invalid loan application scenarios.
WEB-113
Summary by CodeRabbit
New Features
Localization