[Bugfix] #7892 change city name display #3522
Conversation
WalkthroughThe pull request introduces modifications across multiple files in the UBS (Unified Booking System) application, focusing on location handling, user interface styling, and localization. The changes primarily involve refining how location names are displayed, adjusting responsive design parameters, and updating support-related text in both English and Ukrainian languages. The modifications aim to improve user experience by providing more precise location representations and more engaging support-related messaging. Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.ts (1)
303-307: Consider reusing location name formatting logic.The current implementation duplicates the logic for handling standalone cities that exists in
UbsMainPageComponent. Consider extracting this logic into a shared service to maintain consistency and reduce code duplication.Example implementation:
// location.service.ts @Injectable({ providedIn: 'root' }) export class LocationService { private readonly standaloneCities = ['Kyiv', 'Київ']; getLocationName(location: string, region: string): string { return this.standaloneCities.includes(location) ? location : `${location}, ${region}`; } }Then inject and use this service in both components:
- this.currentLocation = - location.nameEn === 'Kyiv' - ? this.langService.getLangValue(location.nameUk, location.nameEn) - : this.langService.getLangValue(location.nameUk, location.nameEn) + - ', ' + - this.langService.getLangValue(region.nameUk, region.nameEn); + const locationName = this.locationService.getLocationName( + this.langService.getLangValue(location.nameUk, location.nameEn), + this.langService.getLangValue(region.nameUk, region.nameEn) + ); + this.currentLocation = locationName;src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.spec.ts (1)
296-296: Remove duplicate test case.There are two identical test cases for
initLocationmethod that verify the same behavior. Consider removing one of them to maintain a clean test suite.Also applies to: 317-317
src/app/ubs/ubs/components/ubs-order-details/ubs-order-location-popup/ubs-order-location-popup.component.ts (1)
111-115: Consider making the city name comparison more maintainable.The hard-coded string comparison with 'Kyiv' might be fragile. Consider:
- Moving special city names to a configuration constant
- Adding null/undefined checks for the location and region parameters
+ private readonly STANDALONE_CITIES = ['Kyiv']; + private getLocationName(location: { nameUk: string; nameEn: string }, region: { nameUk: string; nameEn: string }): string { + if (!location || !region) { + return ''; + } - return location.nameEn === 'Kyiv' + return this.STANDALONE_CITIES.includes(location.nameEn) ? this.getLangValue(location.nameUk, location.nameEn) : this.getLangValue(location.nameUk, location.nameEn) + ', ' + this.getLangValue(region.nameUk, region.nameEn); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/app/ubs/ubs/components/ubs-confirm-page/ubs-confirm-page.component.html(1 hunks)src/app/ubs/ubs/components/ubs-main-page/ubs-main-page.component.scss(2 hunks)src/app/ubs/ubs/components/ubs-main-page/ubs-main-page.component.ts(3 hunks)src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.spec.ts(2 hunks)src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.ts(1 hunks)src/app/ubs/ubs/components/ubs-order-details/ubs-order-location-popup/ubs-order-location-popup.component.ts(4 hunks)src/assets/i18n/ubs/en.json(1 hunks)src/assets/i18n/ubs/ua.json(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/app/ubs/ubs/components/ubs-confirm-page/ubs-confirm-page.component.html
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (18.x)
🔇 Additional comments (8)
src/app/ubs/ubs/components/ubs-main-page/ubs-main-page.component.scss (2)
103-106: Verify layout on small screens.The increased minimum width (320px) for form fields could potentially cause overflow or layout issues on very small screens.
Please test the layout on various small screen sizes (320px-375px) to ensure:
- No horizontal scrolling is introduced
- Form fields remain properly aligned
- Text remains readable without truncation
122-122: LGTM! Font size adjustment improves readability.The reduction in font size for location names on medium screens (17px) maintains good readability while better utilizing the available space.
src/app/ubs/ubs/components/ubs-main-page/ubs-main-page.component.ts (2)
53-53: LGTM! Good approach to define standalone cities.The array of standalone cities is well-defined and makes it easy to maintain the list of cities that should be displayed without region names.
272-273: LGTM! Clean implementation of location name formatting.The
getLocationNamemethod provides a clean and consistent way to format location names. The implementation correctly handles both standalone cities and cities that need region names.Also applies to: 282-284
src/assets/i18n/ubs/ua.json (1)
Line range hint
1-1000: Verify city name display changes.The PR objective mentions changing city name display (#7892), but the changes are about support chat messaging. Let's verify if there are any city-related strings that need attention.
src/app/ubs/ubs/components/ubs-order-details/ubs-order-location-popup/ubs-order-location-popup.component.ts (3)
12-12: LGTM! Clean import of LanguageService.The import follows Angular's best practices for dependency injection.
35-39: LGTM! Good use of readonly for dependency injection.Making dependencies readonly is a TypeScript best practice that prevents accidental reassignment and improves code safety.
95-97: Verify location name parsing in changeLocation method.The
changeLocationmethod splits the location name by comma to extract the city name, which might break with the new location name format fromgetLocationName. Consider using a more robust approach:changeLocation(id: number, locationName: string): number { this.selectedLocationId = id; - this.currentLocation = locationName.split(',')[0]; + this.currentLocation = locationName.split(',')[0].trim(); return id; }Run this script to verify the impact:
Also applies to: 166-170
✅ Verification successful
Current location name parsing is correct and safe
The
getLocationNamemethod consistently formats locations as either a standalone city name (for Kyiv) or "city, region" with a comma separator. The currentsplit(',')[0]operation inchangeLocationsafely extracts the city name in both cases. No changes are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of location name parsing rg -l 'split.*,.*\[0\]' | xargs rg -A 5 'split.*,.*\[0\]'Length of output: 2263
Script:
#!/bin/bash # Search for getLocationName implementation ast-grep --pattern 'getLocationName($$$) { $$$ }'Length of output: 53
Script:
#!/bin/bash # Search for getLocationName method with context rg -A 5 "getLocationName"Length of output: 3220
| } | ||
|
|
||
| private getLocationName(location: { nameUk: string; nameEn: string }, region: { nameUk: string; nameEn: string }): string { | ||
| return location.nameEn === 'Kyiv' |
There was a problem hiding this comment.
Avoid repeating the this.getLangValue function calls and it can be made more concise and clear:
const locationName = this.getLangValue(location.nameUk, location.nameEn);
const regionName = this.getLangValue(region.nameUk, region.nameEn);
return location.nameEn === 'Kyiv' ? locationName : `${locationName}, ${regionName}`;
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/app/ubs/ubs/services/order.service.ts (1)
34-35: Consider making standaloneCities more maintainable.The hardcoded array of standalone cities could be moved to a configuration file or enum to make it more maintainable and reusable across the application.
src/app/main/component/shared/components/form-base/form-base.component.spec.ts (1)
24-27: Consider enhancing the mock TranslateService implementation.While the current mock implementation is functional, consider these improvements:
- Use different mock values for different translation keys to better simulate real scenarios
- Add error cases to test error handling
Here's a suggested enhancement:
const mockTranslateService = { - get: jasmine.createSpy('get').and.returnValue(of('mockTranslation')), - instant: jasmine.createSpy('instant').and.returnValue('mockTranslation') + get: jasmine.createSpy('get').and.callFake((key: string) => + of(key === 'ERROR.KEY' ? null : `translated.${key}`)), + instant: jasmine.createSpy('instant').and.callFake((key: string) => + key === 'ERROR.KEY' ? null : `translated.${key}`) };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/app/chat/component/chat-message/chat-message.component.spec.ts(1 hunks)src/app/chat/service/socket/socket.service.spec.ts(2 hunks)src/app/main/component/shared/components/form-base/form-base.component.spec.ts(3 hunks)src/app/ubs/ubs/components/ubs-confirm-page/ubs-confirm-page.component.spec.ts(1 hunks)src/app/ubs/ubs/components/ubs-main-page/ubs-main-page.component.ts(3 hunks)src/app/ubs/ubs/components/ubs-order-details/ubs-order-certificate/ubs-order-certificate.component.spec.ts(3 hunks)src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.spec.ts(1 hunks)src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.ts(3 hunks)src/app/ubs/ubs/components/ubs-order-details/ubs-order-location-popup/ubs-order-location-popup.component.ts(2 hunks)src/app/ubs/ubs/services/order.service.spec.ts(4 hunks)src/app/ubs/ubs/services/order.service.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.spec.ts
- src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.ts
- src/app/ubs/ubs/components/ubs-order-details/ubs-order-location-popup/ubs-order-location-popup.component.ts
- src/app/ubs/ubs/components/ubs-main-page/ubs-main-page.component.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (18.x)
🔇 Additional comments (7)
src/app/ubs/ubs/components/ubs-order-details/ubs-order-certificate/ubs-order-certificate.component.spec.ts (2)
8-8: LGTM! TranslateStore import added for localization supportThe addition of TranslateStore aligns with the PR's objective of improving city name display.
57-57: LGTM! TranslateStore provider addedThe provider configuration ensures proper translation support for the component.
src/app/chat/component/chat-message/chat-message.component.spec.ts (1)
8-9: LGTM! Well-structured test setup for translations.The mock implementation properly covers both synchronous and asynchronous translation methods, following testing best practices.
Also applies to: 14-17, 23-23
src/app/chat/service/socket/socket.service.spec.ts (1)
10-11: Verify the removal of UserService provider.While the translation service setup looks good, please verify that the UserService provider was intentionally removed and isn't needed by the SocketService or its tests.
Also applies to: 21-24, 32-35
src/app/main/component/shared/components/form-base/form-base.component.spec.ts (3)
11-11: LGTM: Import statement is correctly placed.The TranslateService import is appropriately added and necessary for the translation functionality being tested.
38-39: LGTM: Provider configuration is correct.The TranslateService provider is properly configured in the TestBed setup, following Angular testing best practices.
Line range hint
52-66: Verify translation functionality test coverage.While TranslateService has been added to the test setup, there don't appear to be any tests specifically verifying its usage. Consider adding test cases that verify the component's interaction with translations.
Let's check for translation usage in the component:
| const localStorageService = jasmine.createSpyObj('localStorageService', [ | ||
| 'getCurrentLanguage', | ||
| 'languageSubject', | ||
| 'getUbsOrderData', | ||
| 'getUserId' | ||
| ]); |
There was a problem hiding this comment.
💡 Codebase verification
Remove unused mock method getUserId
The getUserId mock method is defined but never used in either the component implementation or test cases. Consider removing it from the mock definition to maintain clean code.
🔗 Analysis chain
Verify test coverage for getUserId usage
The mock has been extended with getUserId, but there are no visible test cases verifying its usage.
Let's verify the component's usage of getUserId:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for getUserId usage in the component
rg "getUserId" "src/app/ubs/ubs/components/ubs-order-details/ubs-order-certificate/ubs-order-certificate.component.ts"
Length of output: 116
Script:
#!/bin/bash
# Search for getUserId usage in the spec file
rg "getUserId" "src/app/ubs/ubs/components/ubs-order-details/ubs-order-certificate/ubs-order-certificate.component.spec.ts" -A 3
# Search for test descriptions that might relate to user ID
rg "describe|it\(" "src/app/ubs/ubs/components/ubs-order-details/ubs-order-certificate/ubs-order-certificate.component.spec.ts"
Length of output: 431
| 'removeUBSExistingOrderId', | ||
| 'getUserId' |
There was a problem hiding this comment.
💡 Codebase verification
Remove unused getUserId mock from LocalStorageService
The getUserId mock method is not used by the UbsConfirmPageComponent. It should be removed to maintain clean and relevant test specifications.
🔗 Analysis chain
Add test coverage for getUserId functionality
The mock service has been extended with getUserId, but there are no test cases verifying its usage in the component.
Let's verify the component's usage of getUserId:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for getUserId usage in the component
rg "getUserId" "src/app/ubs/ubs/components/ubs-confirm-page/ubs-confirm-page.component.ts"
# If found, we should add test cases to verify the functionality
Length of output: 88
Script:
#!/bin/bash
# Check component class definition and inheritance
rg "export class UbsConfirmPageComponent" -A 2 "src/app/ubs/ubs/components/ubs-confirm-page/ubs-confirm-page.component.ts"
# Search for getUserId usage in related files
rg "getUserId" --type ts "src/app/ubs/ubs/"
Length of output: 838
| private getName(input: { nameUk: string; nameEn: string } | string): string { | ||
| return typeof input === 'string' ? input : this.langService.getLangValue(input.nameUk, input.nameEn); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation to getName method.
The method should handle undefined/null inputs and validate object properties.
Consider this implementation:
private getName(input: { nameUk: string; nameEn: string } | string): string {
+ if (!input) {
+ return '';
+ }
+ if (typeof input === 'object' && (!input.nameUk || !input.nameEn)) {
+ return '';
+ }
return typeof input === 'string' ? input : this.langService.getLangValue(input.nameUk, input.nameEn);
}📝 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.
| private getName(input: { nameUk: string; nameEn: string } | string): string { | |
| return typeof input === 'string' ? input : this.langService.getLangValue(input.nameUk, input.nameEn); | |
| } | |
| private getName(input: { nameUk: string; nameEn: string } | string): string { | |
| if (!input) { | |
| return ''; | |
| } | |
| if (typeof input === 'object' && (!input.nameUk || !input.nameEn)) { | |
| return ''; | |
| } | |
| return typeof input === 'string' ? input : this.langService.getLangValue(input.nameUk, input.nameEn); | |
| } |
| getLocationName(location: { nameUk: string; nameEn: string } | string, region: { nameUk: string; nameEn: string } | string): string { | ||
| const locationName = this.getName(location); | ||
| const regionName = this.getName(region); | ||
|
|
||
| return this.standaloneCities.includes(locationName) ? locationName : `${locationName}, ${regionName}`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation to getLocationName method.
The method should handle undefined/null inputs to prevent runtime errors.
Consider this implementation:
getLocationName(location: { nameUk: string; nameEn: string } | string, region: { nameUk: string; nameEn: string } | string): string {
+ if (!location || !region) {
+ return '';
+ }
const locationName = this.getName(location);
const regionName = this.getName(region);
return this.standaloneCities.includes(locationName) ? locationName : `${locationName}, ${regionName}`;
}📝 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.
| getLocationName(location: { nameUk: string; nameEn: string } | string, region: { nameUk: string; nameEn: string } | string): string { | |
| const locationName = this.getName(location); | |
| const regionName = this.getName(region); | |
| return this.standaloneCities.includes(locationName) ? locationName : `${locationName}, ${regionName}`; | |
| } | |
| getLocationName(location: { nameUk: string; nameEn: string } | string, region: { nameUk: string; nameEn: string } | string): string { | |
| if (!location || !region) { | |
| return ''; | |
| } | |
| const locationName = this.getName(location); | |
| const regionName = this.getName(region); | |
| return this.standaloneCities.includes(locationName) ? locationName : `${locationName}, ${regionName}`; | |
| } |
| it('should return locationName when it is in standaloneCities', () => { | ||
| const location = 'Kyiv'; | ||
| const region = 'Ukraine'; | ||
| const result = service.getLocationName(location, region); | ||
| expect(result).toBe('Kyiv'); | ||
| }); | ||
|
|
||
| it('should return "locationName, regionName" when locationName is not in standaloneCities', () => { | ||
| const location = 'Lviv'; | ||
| const region = 'Ukraine'; | ||
| const result = service.getLocationName(location, region); | ||
| expect(result).toBe('Lviv, Ukraine'); | ||
| }); | ||
|
|
||
| it('should handle object input for location and region', () => { | ||
| const location = { nameUk: 'Львів', nameEn: 'Lviv' }; | ||
| const region = { nameUk: 'Україна', nameEn: 'Ukraine' }; | ||
| const result = service.getLocationName(location, region); | ||
| expect(result).toBe('Lviv, Ukraine'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for edge cases in getLocationName.
The current test coverage should be expanded to include:
- Null/undefined inputs
- Invalid object inputs (missing properties)
- Empty string inputs
Consider adding these test cases:
it('should handle null/undefined inputs', () => {
expect(service.getLocationName(null, null)).toBe('');
expect(service.getLocationName(undefined, undefined)).toBe('');
});
it('should handle invalid object inputs', () => {
const invalidLocation = { nameUk: null, nameEn: null };
const invalidRegion = { nameUk: '', nameEn: '' };
expect(service.getLocationName(invalidLocation, invalidRegion)).toBe('');
});
it('should handle empty string inputs', () => {
expect(service.getLocationName('', '')).toBe('');
});
|
#7892
Summary by CodeRabbit
Release Notes
UI/UX Improvements
Localization Updates
Performance
Bug Fixes