Bugfix/96 language management by parameter lang from idp to angular and vice versa #151
Conversation
bugfix: interceptor not work when redirect user to login
WalkthroughThis update integrates language and theme configuration across the system. On the backend, a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant H as HeaderComponent
participant TS as TranslateService
participant US as UserService
participant UR as UserResource
U->>H: Selects new language/theme
H->>TS: Update UI language (translate.use)
H->>US: Invoke changeLanguage/changeTheme
US->>UR: HTTP PUT /api/user/{language|theme}/{value}
UR-->>US: 204 No Content
H->>U: UI reflects new settings
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 8
🧹 Nitpick comments (10)
sep490-frontend/src/app/modules/core/services/theme.service.ts (2)
76-94: Add error handling for localStorage operations.While the Theme enum usage improves type safety, the localStorage operations should be wrapped in try-catch blocks to handle potential errors (e.g., when localStorage is disabled or full).
initTheme(): void { if (this.isThemeConfigured()) { this.config.theme.set(this.userPreferredColorTheme); - if (localStorage.getItem(this.LOCAL_STORAGE_KEY) === Theme[Theme.DARK]) { - document.querySelector('html')?.classList.add(this.TOKEN); + try { + if (localStorage.getItem(this.LOCAL_STORAGE_KEY) === Theme[Theme.DARK]) { + document.querySelector('html')?.classList.add(this.TOKEN); + } + } catch (error) { + console.warn('Failed to access localStorage:', error); + // Fallback to system theme + this.config.theme.set(this.systemPreferredColorTheme); + } return; } // ... rest of the code } isDarkMode(): Observable<boolean> { if (this.isThemeConfigured()) { - return of( - localStorage.getItem(this.LOCAL_STORAGE_KEY) === Theme[Theme.DARK] - ); + try { + return of( + localStorage.getItem(this.LOCAL_STORAGE_KEY) === Theme[Theme.DARK] + ); + } catch (error) { + console.warn('Failed to access localStorage:', error); + return this.systemPreferredColorThemeChanged; + } } return this.systemPreferredColorThemeChanged; }
99-109: Improve error handling and simplify theme toggle logic.The theme persistence is correctly implemented, but there are opportunities for improvement:
- Add error handling for UserService operations
- Simplify the theme toggle logic by extracting the new theme determination
toggleLightDark(): void { this.config.theme.set(this.userPreferredColorTheme); - if (document.querySelector('html')?.classList.contains(this.TOKEN)) { - localStorage.setItem(this.LOCAL_STORAGE_KEY, Theme[Theme.LIGHT]); - this.userService.changeTheme(Theme[Theme.LIGHT]).subscribe(); - } else { - localStorage.setItem(this.LOCAL_STORAGE_KEY, Theme[Theme.DARK]); - this.userService.changeTheme(Theme[Theme.DARK]).subscribe(); - } - document.querySelector('html')?.classList.toggle(this.TOKEN); + const isDark = document.querySelector('html')?.classList.contains(this.TOKEN); + const newTheme = isDark ? Theme[Theme.LIGHT] : Theme[Theme.DARK]; + + try { + localStorage.setItem(this.LOCAL_STORAGE_KEY, newTheme); + this.userService.changeTheme(newTheme).subscribe({ + error: (error) => { + console.error('Failed to update theme preference:', error); + // Optionally revert the local change + localStorage.setItem(this.LOCAL_STORAGE_KEY, isDark ? Theme[Theme.DARK] : Theme[Theme.LIGHT]); + } + }); + document.querySelector('html')?.classList.toggle(this.TOKEN); + } catch (error) { + console.warn('Failed to access localStorage:', error); + } }sep490-commons/api/src/main/java/green_buildings/commons/api/enums/UserLocale.java (1)
19-24: Improve error handling in fromCode method.The
fromCodemethod throwsNoSuchElementExceptionwithout a clear error message when an invalid code is provided. Consider adding a descriptive message to help with debugging.Apply this diff to improve error handling:
public static UserLocale fromCode(String code) { return Arrays.stream(values()) .filter(userLanguage -> userLanguage.getCode().equals(code)) .findFirst() - .orElseThrow(); + .orElseThrow(() -> + new NoSuchElementException("Invalid locale code: " + code)); }sep490-frontend/src/app/services/user.service.ts (1)
20-32: Optimize PUT requests by removing empty body objects.The empty object bodies in PUT requests are unnecessary and can be omitted.
changeLanguage(language: UserLanguage): Observable<void> { - return this.httpClient.put<void>( - `${AppRoutingConstants.IDP_API_URL}/user/language/${language}`, - {} - ); + return this.httpClient.put<void>( + `${AppRoutingConstants.IDP_API_URL}/user/language/${language}`, + null + ); } changeTheme(theme: string): Observable<void> { - return this.httpClient.put<void>( - `${AppRoutingConstants.IDP_API_URL}/user/theme/${theme}`, - {} - ); + return this.httpClient.put<void>( + `${AppRoutingConstants.IDP_API_URL}/user/theme/${theme}`, + null + ); }sep490-frontend/src/app/components/header/header.component.ts (1)
46-55: Simplify language change subscription.The subscription logic can be simplified using RxJS operators.
Consider this optimization:
- this.translate.onLangChange - .pipe( - takeUntil(this.destroy$), - map(event => event.lang) - ) - .subscribe(lang => { - this.selectedLanguage = this.languages?.find( - language => language.key.split('-')[0] === lang - ); - }); + this.translate.onLangChange.pipe( + takeUntil(this.destroy$), + map(event => this.languages?.find( + language => language.key.split('-')[0] === event.lang + )) + ).subscribe(language => { + this.selectedLanguage = language; + });sep490-idp/src/main/java/green_buildings/idp/rest/UserResource.java (1)
53-69: Extract common user update logic.The theme and language update endpoints share similar patterns.
Consider extracting the common logic:
+ private ResponseEntity<Void> updateUserPreference( + UserContextData userContextData, + Consumer<UserEntity> updateFunction + ) { + var userEntity = userService.findByEmail(userContextData.getUsername()) + .orElseThrow(() -> new UserNotFoundException("User not found: " + userContextData.getUsername())); + updateFunction.accept(userEntity); + userService.update(userEntity); + return ResponseEntity.status(HttpStatus.NO_CONTENT).build(); + } + @PutMapping("/language/{language}") public ResponseEntity<Void> changeLanguage(@PathVariable("language") String locale, @AuthenticationPrincipal UserContextData userContextData) { if (Arrays.stream(UserLocale.values()) .map(UserLocale::getCode) .noneMatch(code -> code.equals(locale))) { return ResponseEntity.badRequest().build(); } - var userEntity = userService.findByEmail(userContextData.getUsername()).orElseThrow(); - userEntity.setLocale(locale); - userService.update(userEntity); - return ResponseEntity.status(HttpStatus.NO_CONTENT).build(); + return updateUserPreference(userContextData, user -> user.setLocale(locale)); }sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/JwtAuthenticationConverter.java (1)
71-72: Consider using a default language value instead of empty string.Using
StringUtils.EMPTYfor language initialization might lead to undefined behavior. Consider using a default language value (e.g., 'en' or system default) to ensure consistent language handling.- StringUtils.EMPTY, // can't initialize language from JWT + UserLocale.VI.name(), // Default to Vietnamese as per database migrationsep490-frontend/src/app/modules/authorization/components/users/users.component.ts (1)
152-162: Enhance error handling in deleteUsers method.The error handler could provide more specific error information to help users understand and resolve issues.
- error: () => { + error: (error) => { + const errorDetail = error?.error?.message || + this.translate.instant('enterprise.Users.message.error.detail'); this.messageService.add({ severity: 'error', summary: this.translate.instant( 'enterprise.Users.message.error.summary' ), - detail: this.translate.instant( - 'enterprise.Users.message.error.detail' - ) + detail: errorDetail }); }sep490-frontend/src/app/modules/authorization/components/create-user/enterprise-user-details.component.ts (1)
109-109: Address the TODO comment.There's a pending TODO comment about enterprise use case integration. This might be related to the mock buildings data being used.
Would you like me to help create a new issue to track the enterprise use case integration task?
sep490-idp/src/main/resources/db/migration/V0.0.1.8__UserConfigs.sql (1)
1-3: Verify the locale format and theme values.The migration script looks good, but please ensure:
- The default locale 'vi-VN' matches the supported locales in the
UserLocaleenum.- The default theme 'system' is a valid value in the frontend's
Themeenum.Also, consider adding a CHECK constraint to validate the locale format and theme values.
Here's a suggested enhancement:
ALTER TABLE users ADD COLUMN locale VARCHAR(5) NOT NULL DEFAULT 'vi-VN', - ADD COLUMN theme VARCHAR(16) NOT NULL DEFAULT 'system'; + ADD COLUMN theme VARCHAR(16) NOT NULL DEFAULT 'system', + ADD CONSTRAINT chk_locale CHECK (locale IN ('vi-VN', 'en-US', 'zh-CN')), + ADD CONSTRAINT chk_theme CHECK (theme IN ('system', 'light', 'dark'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
sep490-commons/api/src/main/java/green_buildings/commons/api/enums/UserLocale.java(1 hunks)sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/JwtAuthenticationConverter.java(1 hunks)sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/UserContextData.java(1 hunks)sep490-frontend/src/app/app.component.ts(2 hunks)sep490-frontend/src/app/app.module.ts(0 hunks)sep490-frontend/src/app/components/header/header.component.ts(3 hunks)sep490-frontend/src/app/modules/authorization/authorization.module.ts(2 hunks)sep490-frontend/src/app/modules/authorization/components/create-user/enterprise-user-details.component.ts(2 hunks)sep490-frontend/src/app/modules/authorization/components/users/users.component.ts(2 hunks)sep490-frontend/src/app/modules/authorization/services/enterprise-user.service.ts(1 hunks)sep490-frontend/src/app/modules/core/services/theme.service.ts(5 hunks)sep490-frontend/src/app/modules/shared/enums/user-language.enum.ts(1 hunks)sep490-frontend/src/app/modules/shared/interceptor/language.interceptor.ts(0 hunks)sep490-frontend/src/app/modules/shared/models/user-configs.ts(1 hunks)sep490-frontend/src/app/services/user.service.ts(1 hunks)sep490-idp/src/main/java/green_buildings/idp/dto/UserConfigs.java(1 hunks)sep490-idp/src/main/java/green_buildings/idp/entity/UserEntity.java(1 hunks)sep490-idp/src/main/java/green_buildings/idp/rest/UserResource.java(1 hunks)sep490-idp/src/main/java/green_buildings/idp/security/MvcUserContextData.java(1 hunks)sep490-idp/src/main/java/green_buildings/idp/service/UserService.java(2 hunks)sep490-idp/src/main/java/green_buildings/idp/service/impl/LoginService.java(1 hunks)sep490-idp/src/main/java/green_buildings/idp/service/impl/UserInfoService.java(2 hunks)sep490-idp/src/main/java/green_buildings/idp/service/impl/UserServiceImpl.java(2 hunks)sep490-idp/src/main/resources/db/migration/V0.0.1.8__UserConfigs.sql(1 hunks)
💤 Files with no reviewable changes (2)
- sep490-frontend/src/app/modules/shared/interceptor/language.interceptor.ts
- sep490-frontend/src/app/app.module.ts
✅ Files skipped from review due to trivial changes (3)
- sep490-frontend/src/app/modules/shared/enums/user-language.enum.ts
- sep490-frontend/src/app/modules/authorization/services/enterprise-user.service.ts
- sep490-idp/src/main/java/green_buildings/idp/service/impl/UserInfoService.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Java Modules (sep490-enterprise)
🔇 Additional comments (12)
sep490-frontend/src/app/modules/core/services/theme.service.ts (2)
6-7: LGTM! Imports are correctly added.The new imports for UserService and Theme model are properly added and necessary for the enhanced theme management functionality.
51-54: LGTM! Constructor injection follows best practices.The UserService is properly injected following Angular's dependency injection pattern, and the readonly modifier is correctly used.
sep490-frontend/src/app/modules/authorization/authorization.module.ts (1)
8-8: LGTM! Service replacement is properly configured.The module correctly imports and provides the new
EnterpriseUserServiceas a replacement forUserService.Also applies to: 17-17
sep490-idp/src/main/java/green_buildings/idp/security/MvcUserContextData.java (1)
22-22: LGTM! Locale support properly integrated.The locale from UserEntity is correctly passed to the superclass constructor, enabling language management support.
sep490-frontend/src/app/services/user.service.ts (1)
1-19: LGTM! Well-structured service implementation.The service is properly decorated, injected, and implements user configuration management with proper typing and observables.
sep490-idp/src/main/java/green_buildings/idp/service/UserService.java (1)
31-33: LGTM! Well-defined service methods.The new methods are properly typed and follow Java conventions:
findByEmailappropriately uses Optional for nullable resultsupdatemethod signature is clear and concisesep490-idp/src/main/java/green_buildings/idp/service/impl/LoginService.java (1)
29-30: LGTM! Code readability improvement.Extracting
MvcUserContextDatacreation to a separate variable improves code readability and maintainability.sep490-frontend/src/app/app.component.ts (2)
23-32: Verify language code handling.While the subscription logic is well-implemented with proper lifecycle management, the language code splitting might need validation to handle malformed language codes.
Consider adding a fallback:
- this.translate.use(userConfigs.language.split('-')[0]); + const langCode = userConfigs.language?.split('-')?.[0] || 'en'; + this.translate.use(langCode);
43-45: LGTM! Proper cleanup implementation.The component correctly cleans up subscriptions using the destroy$ subject.
sep490-frontend/src/app/components/header/header.component.ts (1)
10-14: LGTM! Type safety improvement.Using the
UserLanguageenum instead of string type improves type safety and maintainability.sep490-frontend/src/app/modules/authorization/components/create-user/enterprise-user-details.component.ts (2)
28-28: LGTM! Service import updated for better separation of concerns.The change from
UserServicetoEnterpriseUserServicealigns with the component's enterprise-specific functionality.
128-128: LGTM! Constructor dependency updated to match the service change.The constructor parameter type has been correctly updated to match the imported service.
...90-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/UserContextData.java
Show resolved
Hide resolved
sep490-idp/src/main/java/green_buildings/idp/service/impl/UserServiceImpl.java
Show resolved
Hide resolved
thongdanghoang
deleted the
bugfix/96-language-management-by-parameter-lang-from-idp-to-angular-and-vice-versa
branch
Summary by CodeRabbit