WEB-457 Tenant aware feature #2854
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Home component src/app/home/home.component.ts, src/app/home/home.component.html |
Injected SettingsService, added public tenant: string and helper tenantIdentifier(). Template updated: multi-line mat-card-title with translations and username, and card image source changed to assets/images/{{ tenant }}_home.png. |
Footer component src/app/shared/footer/footer.component.ts, src/app/shared/footer/footer.component.html |
Added public tenant: string and tenantIdentifier(); initialized on component init. Template gains a new table row displaying tenant name with titlecase. |
Translations (13 locales) src/assets/translations/{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}.json |
Added two translation keys in each file: "Tenant" and "To" with locale-specific values. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10–15 minutes
- Check SettingsService injection and
tenantIdentifier()defaulting behavior in both components. - Verify template bindings: translation keys exist,
titlecaseusage, and the dynamic image path resolves as expected. - Confirm all 13 locale files include both
"Tenant"and"To"keys with correct JSON syntax.
Suggested reviewers
- alberto-art3ch
- IOhacker
- gkbishnoi07
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Title check | ✅ Passed | The title 'WEB-457 Tenant aware feature' clearly relates to the main changes in the PR, which add tenant-specific functionality (tenant-aware home and footer components, tenant image paths, and multi-language translations for tenant-related text). |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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.
![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
♻️ Duplicate comments (1)
src/app/home/home.component.ts (1)
203-208: Code duplication: Extract toSettingsService.This method is identical to the one in
footer.component.ts(lines 103-108). Please see the review comment on the footer component for the suggested refactoring to extract this logic toSettingsService.getTenantIdentifier().
🧹 Nitpick comments (3)
src/assets/translations/cs-CS.json (1)
3341-3342: Verify translation formatting for "Tenant" entry.The new translation entries are syntactically valid and the Czech translations appear semantically correct. However, two minor observations:
Trailing space in "Tenant" translation (Line 3341): The value
"Nájemce: "includes a trailing space and colon. Confirm this formatting is intentional for UI display purposes.Key naming consistency (Line 3342): The key
"TO"uses all uppercase, which differs from the CamelCase/descriptive naming convention used throughout the rest of the file. Consider whether this should be"To"for consistency.Ensure these same translations and formatting are consistently applied across all 13 locale files mentioned in the PR summary.
src/app/shared/footer/footer.component.html (1)
5-10: Use translation pipe for "Tenant" label to maintain consistency with other footer labels.The new tenant row hardcodes the "Tenant" label, while all other labels in this footer component (e.g., line 18, 24, 30) use the
| translatepipe. Update line 6 to use{{ 'labels.inputs.Tenant' | translate }}or the appropriate translation key to maintain i18n consistency and align with the footer pattern.- <td class="footer-content">Tenant</td> + <td class="footer-content">{{ 'labels.inputs.Tenant' | translate }}</td>src/app/home/home.component.html (1)
13-16: Consider using single-line interpolation for better readability.The multi-line title binding may introduce unexpected whitespace. Consider refactoring to a single line or using a computed property in the component:
- <mat-card-title class="title"> - {{ 'labels.text.Welcome' | translate }} {{ username }} {{ 'labels.text.To' | translate }} - {{ tenant | titlecase }}! - </mat-card-title> + <mat-card-title class="title"> + {{ 'labels.text.Welcome' | translate }} {{ username }} {{ 'labels.text.To' | translate }} {{ tenant | titlecase }}! + </mat-card-title>Or in the component:
get welcomeMessage(): string { return `${this.translate.instant('labels.text.Welcome')} ${this.username} ${this.translate.instant('labels.text.To')} ${this.tenant}!`; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/assets/images/default_home.pngis excluded by!**/*.pngand included by**/*
📒 Files selected for processing (17)
src/app/home/home.component.html(2 hunks)src/app/home/home.component.ts(5 hunks)src/app/shared/footer/footer.component.html(1 hunks)src/app/shared/footer/footer.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(2 hunks)src/assets/translations/es-CL.json(2 hunks)src/assets/translations/es-MX.json(2 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)
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/home/home.component.htmlsrc/app/shared/footer/footer.component.tssrc/app/home/home.component.tssrc/app/shared/footer/footer.component.html
🪛 Biome (2.1.2)
src/assets/translations/en-US.json
[error] 3343-3343: The key To was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (16)
src/assets/translations/it-IT.json (1)
3341-3342: Translation entries correctly added to Italian locale file.The two new translation keys for the tenant feature are syntactically valid and properly integrated into the Italian translation file. "Inquilino" and "a" are appropriate Italian translations for "Tenant" and "To" respectively.
src/assets/translations/ne-NE.json (1)
3341-3342: Translation entries added for tenant-aware display ✓The two new translation entries are syntactically valid and follow the established pattern in this file:
- Line 3341:
"Tenant": "किरायेदार"(tenant/renter in Nepali)- Line 3342:
"To": "लाई"(postposition "to" in Nepali)Both entries are correctly placed within the
"text"section with proper comma termination and maintain JSON validity.Please confirm that:
- These same translation keys have been added to all 13 locale files as mentioned in the PR objectives
- The Nepali translations are accurate and contextually appropriate for the tenant-aware UI components (e.g., footer display: "Tenant To [Name]")
src/assets/translations/sw-SW.json (1)
3340-3341: Translations added appropriately for tenant-aware feature.The two new translation keys ("Tenant" → "Mpangaji" and "To" → "Kwa") are correctly added to the Swahili locale file in the "text" section with accurate translations. The placement is consistent with the alphabetical organization of the file, and the additions align with the PR's objective to introduce tenant-aware display capabilities across the application.
src/assets/translations/es-CL.json (1)
3342-3343: Verify translation accuracy for tenant-aware UI strings.The translations "Tenant" → "Inquilino" and "To" → "a" appear semantically correct for Spanish (Chile). However, ensure these match the intended context in the HomeComponent and FooterComponent where they'll be displayed, and verify consistency with related locale files (es-MX, de-DE, cs-CS, etc.) to maintain translation quality across the application.
src/assets/translations/fr-FR.json (1)
3340-3341: Verify generic "À" translation covers all intended uses in tenant components.The French translation "À" for "To" is quite generic and its suitability depends heavily on context within the tenant components. In French, "To" can translate to "à," "vers," "pour," or other prepositions depending on usage (e.g., "Tenant from Date A to Date B" vs. "Transfer to Account").
Please verify that this translation works correctly in all contexts where "To" is used in the tenant-aware display features (HomeComponent, FooterComponent, and any tenant-related UI elements). If there are multiple distinct uses with different semantic meanings, consider whether locale-specific keys would be more appropriate.
src/assets/translations/lt-LT.json (2)
3341-3342: Translation entries added correctly with valid JSON syntax.The two new Lithuanian translation entries are properly formatted:
- Line 3341:
"Tenant": "Nuomininkas"- Line 3342:
"To": "Kam"Both are correctly placed in the
textsection of the translations object with appropriate comma separators. The JSON structure remains valid.
1-3342: Both "Tenant" and "To" translation keys already exist in src/assets/translations/lt-LT.json with proper Lithuanian translations ("Nuomininkas" and "Kam" respectively). The review comment's claim that these keys need to be added is inaccurate based on the provided file content.Likely an incorrect or invalid review comment.
src/assets/translations/pt-PT.json (1)
3341-3342: Good: Translation entries are correctly added with proper Portuguese language equivalents.The two new translations for tenant-aware display are accurate and well-placed:
- "Inquilino" is the appropriate Portuguese translation for "Tenant"
- "Para" correctly translates "To"
The entries are properly integrated into the text section of the JSON structure, maintaining consistency with the existing translation pattern.
src/assets/translations/de-DE.json (1)
3340-3341: Duplicate translation keys across sections with inconsistent capitalization.Line 2346 already defines
"Tenant": "Mieter"in theinputssection, and line 2356 already defines"To": "Zu"in theinputssection. These new entries in thetextsection (lines 3340–3341) duplicate those keys.Additionally, the
"To"translation uses lowercase"zu"here but capitalized"Zu"at line 2356, which introduces an inconsistency—both refer to the same semantic concept.Options:
- Consolidate: Remove one set of duplicates and verify the remaining key is in the appropriate section for UI component consumption.
- Clarify intent: If section-specific translations are needed (e.g., different rendering contexts), document this pattern and ensure consistent capitalization.
src/assets/translations/es-MX.json (1)
3344-3344: Verify "Tenant" translation is used by component with proper i18n binding.A translation entry for "Tenant": "Inquilino" has been added. Confirm that the corresponding component template (footer.component.html) uses this translation key with the
| translatepipe rather than hardcoding the label to ensure consistency with other footer labels and proper localization support. As per coding guidelines for Angular code insrc/app/**, this should follow i18n best practices.src/app/shared/footer/footer.component.html (2)
5-10: Verify component initialization and type safety for tenant property.The footer now displays a
tenantproperty (line 8:{{ tenant | titlecase }}). Confirm that:
- FooterComponent.ts properly initializes the
tenantproperty with strict type safety (notany)- The
tenantIdentifier()method mentioned in the AI summary safely handles the SettingsService call with proper error handling- The tenant value defaults appropriately if SettingsService is unavailable
As per coding guidelines for
src/app/**, ensure clean observable patterns and strict type safety are maintained.
5-10: Consider adding display condition for tenant row.Unlike other dynamic rows in this footer (e.g., line 23:
*ngIf="displayBackEndInfo"and line 29:*ngIf="isBusinessDateDefined"), the new tenant row has no visibility condition. Determine whether this row should always display or be conditional (e.g., only in development mode or when a valid tenant is available).src/assets/translations/lv-LV.json (1)
3341-3342: Straightforward translation additions for tenant-aware UI. ✓The two new Latvian translation entries are correctly formatted and semantically appropriate. "Īrnieks" (tenant) and "Līdz" (to) align with the PR's goal of adding tenant display to home and footer components.
src/assets/translations/ko-KO.json (1)
3341-3342: LGTM! Translation keys added for tenant feature.The new translation keys "Tenant" and "To" support the tenant-aware display functionality introduced in this PR. The Korean translations appear appropriate.
src/app/home/home.component.ts (1)
19-19: LGTM! SettingsService integration and tenant initialization.The addition of
SettingsServicedependency and thetenantproperty are correctly implemented:
- Import statement added
- Constructor parameter properly injected
- JSDoc updated
- Property initialized in
ngOnInitHowever, note that the
tenantIdentifier()method (lines 203-208) duplicates logic infooter.component.tsand should be extracted to a shared location as mentioned in the footer component review.Also applies to: 50-51, 77-77, 85-87, 96-96
src/app/home/home.component.html (1)
34-34: Add error handling and validation for tenant-specific image.The dynamic image path
assets/images/{{ tenant }}_home.pnghas several concerns:
- Missing asset validation: No check if the tenant-specific image exists
- No error handling: Missing
(error)handler for failed image loads- Security risk: The
tenantvalue should be validated to prevent path traversal attacksConsider adding:
- <img mat-card-image src="assets/images/{{ tenant }}_home.png" alt="Mifos X" /> + <img mat-card-image + [src]="'assets/images/' + tenant + '_home.png'" + (error)="onImageError($event)" + alt="Mifos X" />And in the component:
onImageError(event: Event): void { // Fallback to default image (event.target as HTMLImageElement).src = 'assets/images/default_home.png'; }Additionally, validate the
tenantvalue to ensure it only contains safe characters:tenantIdentifier(): string { const identifier = this.settingsService.tenantIdentifier || 'default'; // Only allow alphanumeric, dash, and underscore return identifier.replace(/[^a-zA-Z0-9_-]/g, ''); }Verify that tenant-specific images exist for all configured tenants and that the component implementation follows these best practices.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/home/home.component.ts (1)
203-208: Safe fallback implementation with defensive checks.The
tenantIdentifier()method provides a reliable string return value with a 'default' fallback. The null/empty checks ensure the component never breaks due to missing configuration.Optional refinement: The condition on line 204 checks both falsy (
!this.settingsService.tenantIdentifier) and empty string explicitly. Since falsy checks cover empty strings, you could simplify to:tenantIdentifier() { - if (!this.settingsService.tenantIdentifier || this.settingsService.tenantIdentifier === '') { + if (!this.settingsService.tenantIdentifier) { return 'default'; } return this.settingsService.tenantIdentifier; }However, the explicit empty string check adds clarity and is perfectly acceptable defensive programming.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/assets/images/default_home.pngis excluded by!**/*.pngand included by**/*
📒 Files selected for processing (17)
src/app/home/home.component.html(2 hunks)src/app/home/home.component.ts(5 hunks)src/app/shared/footer/footer.component.html(1 hunks)src/app/shared/footer/footer.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(2 hunks)src/assets/translations/es-CL.json(2 hunks)src/assets/translations/es-MX.json(2 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/sw-SW.json
- src/assets/translations/it-IT.json
- src/app/shared/footer/footer.component.ts
- src/assets/translations/es-CL.json
- src/assets/translations/fr-FR.json
- src/assets/translations/es-MX.json
- src/app/home/home.component.html
- src/assets/translations/de-DE.json
- src/assets/translations/lt-LT.json
- src/app/shared/footer/footer.component.html
- src/assets/translations/pt-PT.json
- src/assets/translations/ne-NE.json
- src/assets/translations/lv-LV.json
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/home/home.component.ts
🪛 Biome (2.1.2)
src/assets/translations/en-US.json
[error] 3343-3343: The key To was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
⏰ 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 (7)
src/assets/translations/cs-CS.json (1)
3341-3342: Translations are accurate and appropriately formatted. ✓The Czech translations for the two new tenant-related keys look correct:
"Tenant": "Nájemce: "— Accurately translates to Czech with label-style formatting (colon + space for UI display)"TO": "do"— Correct and idiomatic Czech for "from X to Y" use casesBoth entries fit naturally into the existing translation structure and align with the PR's tenant-aware UI feature. Consider verifying that these keys have been consistently added across all other supported locale files (de-DE, en-US, es-CL, es-MX, fr-FR, it-IT, ko-KO, lt-LT, lv-LV, ne-NE, pt-PT, sw-SW) to ensure a uniform rollout of the tenant feature.
src/assets/translations/en-US.json (1)
3448-3448: Addition of "Tenant" translation is correct.The new translation key
"Tenant": "Tenant"at line 3448 is appropriately added to support tenant-aware UI components (Home and Footer) as described in the PR objectives. Once the duplicate "To" key is resolved, this entry will function correctly.src/assets/translations/ko-KO.json (1)
3341-3342: LGTM - Translation additions look good.The two new translation entries for "Tenant" (임차인) and "To" (에게) are correctly formatted and semantically appropriate for the tenant-aware UI feature described in the PR objectives. The translations align with the pattern of similar additions across other locale files.
Note: If you have native Korean speakers available, consider having them verify these translations match your intended UI context.
src/app/home/home.component.ts (4)
19-19: LGTM - SettingsService import added correctly.The import follows Angular conventions and is consistent with other service imports in the file.
50-51: Property declaration is correctly typed.The
tenantproperty is appropriately declared as a public string for template binding. The initialization inngOnInitwith thetenantIdentifier()method ensures it always has a value.
85-86: Service injection follows Angular best practices.The SettingsService is correctly injected as a private dependency, consistent with the other services in the constructor. The JSDoc is properly updated to document the new parameter.
96-96: Tenant initialization placed appropriately.The tenant property is correctly initialized in
ngOnInit, ensuring the SettingsService is available and the value is set before the template renders.
|
@JaySoni1 there are some items to resolve |
|
@IOhacker Ok I will resolve this and will update the PR soon |
|
@JaySoni1 please let me know once it is ready to review it |
|
@IOhacker Ok |
|
@JaySoni1 squash and commit please :) |
|
@IOhacker OK |
2 participants
Changes Made :-
-Logged user must be aware which is the Tenant that he has signed in.
WEB-457
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.