Feature/77 userstory enterprise #135
Conversation
WalkthroughThis pull request revises the building-related data model and functionality across both backend and frontend codebases. The changes remove the existing Changes
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: 10
🧹 Nitpick comments (10)
sep490-frontend/src/app/modules/core/services/http-error-handler.interceptor.ts (4)
26-31: Useelse iffor exclusive error handling
Currently, the code checks both the'TECHNICAL'and'BUSINESS'error types with separateifblocks. If an error were to inadvertently contain both types, both handlers would be invoked. Usingelse iffor the second condition can help ensure only one handler is triggered.if (error.error && error.error.errorType === 'TECHNICAL') { this.handleTechnicalErrorException(error); } - if (error.error && error.error.errorType === 'BUSINESS') { + else if (error.error && error.error.errorType === 'BUSINESS') { this.handleBusinessErrorException(error); }
37-49: Avoid mutating the error object
By storingerrorNotifieddirectly inerror.error, you risk altering the original error payload. This can create issues if upstream or other interceptors rely on the unchanged error object. Consider creating a clone or using auxiliary state to track notification.private handleTechnicalErrorException(error: any): void { ... - error.error.errorNotified = true; + const clonedError = { ...error, error: { ...error.error, errorNotified: true } }; + // Use clonedError for any further processing if needed }
51-64: Handle missing i18n keys gracefully
Whenerror.error.i18nKeyis undefined, thetranslateService.instant(...)might not produce a meaningful message. Consider a fallback mechanism to avoid displaying confusing or missing translations.if (!error.error.field) { this.messageService.add({ severity: 'error', summary: 'Error', - detail: this.translateService.instant( - `validation.${error.error.i18nKey}`, - this.convertErrorParams(error.error.args) - ), + detail: error.error.i18nKey + ? this.translateService.instant( + `validation.${error.error.i18nKey}`, + this.convertErrorParams(error.error.args) + ) + : 'An unexpected business error occurred.', sticky: true }); }
66-76: Validateparamscontent
WhileconvertErrorParamsgracefully handlesnullorundefinedparamsby returning{}, it does not guard against invalid array content (e.g., missingkeyorvalue). If you expect well-formed data, at least log or handle unexpected param entries.private convertErrorParams(params: BusinessErrorParam[]): { [key: string]: string; } { - return params?.reduce( + return (params ?? []).reduce( (result, arg: BusinessErrorParam) => { // Optionally validate arg.key and arg.value here result[arg.key] = arg.value; return result; }, {} as {[key: string]: string} ); }sep490-frontend/src/app/modules/enterprise/models/building-details.dto.ts (1)
3-6: Add docstring to clarify property usage
Providing short doc comments on these properties would help future contributors understand their purposes, constraints, and possible edge cases (e.g., whethernameis guaranteed to be unique).export interface BuildingDetails extends BaseDTO { /** * The unique name of the building. */ name: string; /** * The number of devices installed in the building. */ numberOfDevices: number; }sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.ts (2)
77-89: Configure pagination size through environment variables.The page size is hardcoded to 100 which might not be optimal for all scenarios.
Consider moving the pagination configuration to environment:
- pageSize: 100 + pageSize: environment.defaultPageSize
117-128: Extract map configuration to environment variables.The map center coordinates are hardcoded which makes it difficult to configure for different regions.
Consider this approach:
- center: [10.841394, 106.810052], - zoom: 20 + center: [environment.mapDefaultLat, environment.mapDefaultLng], + zoom: environment.mapDefaultZoomsep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.html (1)
68-73: Floating Label Input TemplateThe
<ng-template>for a floating label input (using<p-floatlabel>) is a neat addition. However, it appears to replicate the "name" input already defined earlier. Confirm whether this is intentional (e.g., for handling a different UI state) or if the template should be refactored or removed.sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.html (1)
24-30: Map Container SetupThe map container is conditionally shown based on
listViewand set to a fixed height of 600px. Consider the responsiveness of this container on different devices and whether a fixed height is optimal.sep490-frontend/src/app/modules/dev/components/toolbox/toolbox.component.html (1)
81-89: Rounded, Text, and Raised Icon Buttons Group
This section combines multiple stylistic attributes ([rounded],[text], and[raised]) to create a layered button appearance. While the markup is clear, consider refactoring repetitive button definitions into a dynamic construct (e.g., using an*ngFordirective) to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
sep490-enterprise/src/main/java/enterprise/dtos/BuildingDTO.java(1 hunks)sep490-enterprise/src/main/java/enterprise/entities/BuildingEntity.java(1 hunks)sep490-enterprise/src/main/java/enterprise/rest/BuildingController.java(2 hunks)sep490-enterprise/src/main/java/enterprise/services/BuildingService.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/impl/BuildingServiceImpl.java(2 hunks)sep490-enterprise/src/main/resources/db/migration/V0.0.1.5__CorrectBuilding.sql(1 hunks)sep490-enterprise/src/test/java/enterprise/rest/BuildingControllerTest.java(1 hunks)sep490-frontend/src/app/app.component.html(1 hunks)sep490-frontend/src/app/app.component.scss(1 hunks)sep490-frontend/src/app/app.module.ts(2 hunks)sep490-frontend/src/app/components/sidebar/sidebar.component.css(1 hunks)sep490-frontend/src/app/modules/authorization/authorization.component.css(1 hunks)sep490-frontend/src/app/modules/authorization/authorization.module.ts(1 hunks)sep490-frontend/src/app/modules/authorization/components/users/users.component.html(2 hunks)sep490-frontend/src/app/modules/core/services/http-error-handler.interceptor.ts(1 hunks)sep490-frontend/src/app/modules/dev/components/toolbox/toolbox.component.html(1 hunks)sep490-frontend/src/app/modules/emissions/emissions.component.css(1 hunks)sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.css(1 hunks)sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.html(1 hunks)sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.css(1 hunks)sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.html(1 hunks)sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.ts(4 hunks)sep490-frontend/src/app/modules/enterprise/components/subscription/subscription.component.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/enterprise-routing.module.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/enterprise.component.css(1 hunks)sep490-frontend/src/app/modules/enterprise/enterprise.module.ts(2 hunks)sep490-frontend/src/app/modules/enterprise/models/building-details.dto.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/models/building.dto.ts(1 hunks)sep490-frontend/src/app/modules/shared/components/table-template/table-template.component.html(1 hunks)sep490-frontend/src/app/modules/shared/shared.module.ts(3 hunks)sep490-frontend/src/app/services/building.service.ts(1 hunks)sep490-frontend/src/assets/i18n/en.json(6 hunks)sep490-frontend/src/assets/i18n/vi.json(5 hunks)sep490-frontend/src/assets/i18n/zh.json(5 hunks)sep490-idp/src/main/java/green_buildings/idp/entity/UserEntity.java(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- sep490-frontend/src/app/modules/authorization/authorization.component.css
- sep490-frontend/src/app/components/sidebar/sidebar.component.css
- sep490-frontend/src/app/modules/emissions/emissions.component.css
- sep490-frontend/src/app/modules/enterprise/enterprise.component.css
- sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.css
- sep490-frontend/src/app/app.component.html
- sep490-idp/src/main/java/green_buildings/idp/entity/UserEntity.java
- sep490-frontend/src/app/modules/enterprise/components/subscription/subscription.component.ts
🧰 Additional context used
🪛 Biome (1.9.4)
sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.ts
[error] 60-60: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Java Modules (sep490-enterprise)
- GitHub Check: Build Java Modules (sep490-idp)
🔇 Additional comments (43)
sep490-frontend/src/app/app.component.scss (1)
2-2: Responsive Layout Improvement: Use ofmin-h-dvhThe update from a fixed height (
h-dvh) to a minimum height (min-h-dvh) is a good move to allow the host element to expand as necessary, improving responsiveness. Please verify that this change aligns with the overall design and behavior expectations across different screens.sep490-frontend/src/app/modules/authorization/components/users/users.component.html (2)
1-3: Header Enhancement: Addedtext-primaryClassThe
<h1>element now includes thetext-primaryclass along with the other utility classes. This enhancement should help enforce theme consistency by applying the primary color styling without altering the displayed title text.
31-33: Search Button Icon UpdateThe search button’s icon has been updated to
pi pi-search, which more clearly conveys its functionality as a search trigger compared to the previous icon. Please confirm that this change aligns with the design language across the application.sep490-frontend/src/app/modules/shared/shared.module.ts (1)
14-14:❓ Verification inconclusive
LGTM! Verify the usage of new PrimeNG modules.
The addition of DividerModule, MessageModule, and MessagesModule from PrimeNG is well-structured and follows the module's existing patterns. The modules are correctly imported and added to the shared module's configuration.
Let's verify that these modules are being used in the codebase:
Also applies to: 24-25, 57-57, 63-64
🏁 Script executed:
#!/bin/bash # Description: Search for usage of the newly added PrimeNG components # Expected: Find component usage in templates echo "Searching for p-divider usage..." rg -t html "p-divider" echo "Searching for p-message usage..." rg -t html "p-message" echo "Searching for p-messages usage..." rg -t html "p-messages"Length of output: 589
Review Approved – Please Verify Module Usage
The update in the shared module is well structured and follows the existing patterns. We confirmed that
<p-message>inenterprise-user-details.component.htmlreflects usage of the new MessageModule. However, our automated search did not return any matches for DividerModule (i.e.p-divider) or MessagesModule (i.e.p-messages).
- Verify whether these two modules are utilized in other parts of the codebase (e.g., inline templates or upcoming features).
- If these modules are not used, consider removing them to keep the shared module lean.
sep490-frontend/src/app/modules/authorization/authorization.module.ts (1)
16-16:❓ Verification inconclusive
LGTM! Removal of unused Message component.
The simplified imports array improves module cleanliness by removing the unused Message component.
Let's verify that the Message component is not used in any of the module's components:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining usage of Message component in authorization module components # Search for Message usage in component files rg -i "message" "sep490-frontend/src/app/modules/authorization/"Length of output: 3072
LGTM – Change Approved with a Caveat
The simplified imports array in
authorization.module.tsimproves clarity by removing the unused Message component. Note that while the Message component is no longer imported in the module, there are still usages of<p-message>in the HTML templates andMessageServicein the component TypeScript files (e.g., inenterprise-user-details.component.tsandusers.component.ts). Please ensure that theSharedModulecorrectly imports and re-exports the necessary PrimeNG Message-related modules so that these usages remain fully supported.sep490-frontend/src/app/modules/enterprise/models/building.dto.ts (1)
3-9:❓ Verification inconclusive
Ensure property compatibility with back-end expectations
If the backend expects certain constraints or formats (e.g., date formats, string length limits, etc.), document or enforce them here for clarity. This helps keep front-end and back-end models in sync.Would you like to verify these property constraints in the backend DTOs to confirm alignment?
🏁 Script executed:
#!/bin/bash # Searching for BuildingDTO definitions in the backend to compare constraints fd -t f "BuildingDTO.java" --exec cat {} | rg -A 5 'class BuildingDTO'Length of output: 72
Manual Verification Required: Backend DTO Property Constraints
It appears that the search for a
BuildingDTO.javafile did not return any results. Because we couldn’t retrieve the backend constraints automatically, please manually verify that the backend's definitions for the building model enforce or document similar property constraints (e.g., date formats, string length limits, etc.) as defined in the frontend'sBuildinginterface. Confirm that properties such asvalidFromInclusiveandvalidToInclusivefollow the expected formats and any additional requirements expected by the backend.
- Action items:
- Manually confirm the backend DTO properties and constraints.
- Ensure that the frontend model remains in sync with backend expectations.
sep490-enterprise/src/main/java/enterprise/services/BuildingService.java (1)
14-14: LGTM! Good use of Optional for nullable return.The new
findByIdmethod follows best practices by using Optional to handle potential null cases.sep490-frontend/src/app/modules/enterprise/enterprise.module.ts (1)
16-21: LGTM! Component declarations are properly organized.The module declarations are correctly updated with the new components while maintaining proper organization.
sep490-enterprise/src/main/java/enterprise/services/impl/BuildingServiceImpl.java (1)
27-30: LGTM!The implementation is clean and follows best practices by using Optional for nullable results and properly delegating to the repository.
sep490-enterprise/src/main/java/enterprise/rest/BuildingController.java (1)
47-47:❓ Verification inconclusive
Consider API versioning for the renamed endpoint.
The method has been renamed from
getEnterpriseBuildingstosearchEnterpriseBuildingswhich could break existing API consumers.Consider:
- Adding API versioning (e.g.,
/v2/api/buildings/search)- Temporarily keeping both endpoints for backward compatibility
Let's verify existing API consumers:
🏁 Script executed:
#!/bin/bash # Search for API calls to the old endpoint name rg -l "getEnterpriseBuildings" --type tsLength of output: 40
API Versioning and Backward Compatibility Check for Renamed Endpoint
The method was renamed from
getEnterpriseBuildingstosearchEnterpriseBuildings, which might break external API consumers. Our initial search in TypeScript files did not return any references togetEnterpriseBuildings, but this result is based solely on TS files and may not cover all potential consumers.
- Action Items:
- Verify Usage: Perform an extensive search across the entire repository (including non-TypeScript files) or manually confirm that no external consumers rely on the old endpoint.
- Backward Compatibility: If any usage is found or external dependencies are anticipated, consider implementing API versioning (e.g.,
/v2/api/buildings/search) or temporarily maintaining both endpoints.sep490-frontend/src/app/app.module.ts (1)
113-119: Review the order of HTTP interceptors.The order of interceptors is crucial as they are executed in the order they are provided. The current order is:
- Language
- Auth
- Error Handler
This is correct as the error handler should be last to catch errors from other interceptors.
sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.ts (1)
40-42: LGTM! Good use of SubscriptionAwareComponent.The component properly extends SubscriptionAwareComponent and uses takeUntil for subscription management, which helps prevent memory leaks.
sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.css (2)
1-3: :host Styling AdjustmentsThe updated
:hoststyling now applies a flexbox layout with column direction and a gap, without the previous padding utility. Ensure that this change aligns with the overall layout expectations.
5-7: Added.box-shadowClassThe new
.box-shadowclass applies a subtle box-shadow effect. Verify that the shadow parameters (offsets, blur, and color) meet the design specifications and are used consistently across the application.sep490-enterprise/src/main/resources/db/migration/V0.0.1.5__CorrectBuilding.sql (3)
1-2: DroppingfloorsColumnRemoving the
floorscolumn is in line with the new data model. Ensure that any existing queries or application logic referencing this column have been updated.
3-4: Droppingsquare_metersColumnThe removal of the
square_meterscolumn further reinforces the updated schema. Confirm that dependent components have been refactored accordingly and that a data backup strategy is in place for production.
6-7: Addingnumber_of_devicesColumnThe addition of the new
number_of_devicescolumn—with a BIGINT type, non-null constraint, and a default value of 0—appropriately reflects the new business requirements. Verify that downstream code (DTOs, entities, and services) properly handles this field.sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.html (6)
1-10: Back Button ImplementationThe back button is well implemented using
<p-button>with the appropriate event binding(onClick)="back()"and visual properties. Consider adding accessibility attributes (e.g.,aria-label) for enhanced usability.
12-19: Dynamic Title RenderingThe dynamic title, which toggles between "edit" and "create" based on the
isEditflag and is translated via thetranslatepipe, is clear and concise. Ensure that corresponding translation keys are correctly defined.
21-31: Building Name Input FieldThe building name field is correctly set up with reactive form binding (
formControlName="name") and paired with an error display component<form-field-error>. Confirm that the form control is properly initialized in the component.
34-45: Number of Devices Input FieldThe implementation for the
numberOfDevicesinput is straightforward and mirrors the setup of the name field, including label translation and error display. Validate that the form control exists and that the numeric input behaves as expected.
46-52: Informational Icon for Device LimitIntroducing an informational icon with a message (via translation) to indicate the maximum number of devices is a helpful UX enhancement. Verify that the translation key (
enterprise.buildings.details.labels.maxNumberOfDevices) is properly set.
54-66: Form Action ButtonsThe reset and submit buttons are correctly bound to their respective methods (
reset()andsubmit()) and display dynamic labels based on the form mode. Ensure that the overall form validation and submission handling cover all edge cases.sep490-frontend/src/app/modules/shared/components/table-template/table-template.component.html (1)
68-75: Empty Table Message TemplateThe newly added
<ng-template pTemplate="emptymessage">provides a clean user message when no table rows exist. The dynamic colspan calculation (columns.length + (checkbox ? 1 : 0)) ensures proper cell spanning. Confirm that the translation key (common.noRow) is available.sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.html (4)
1-11: Header Section with Title and Add Building ButtonThis header section clearly presents the title (translated) and includes a primary action button to add a building. Double-check that the translation keys for both the title and button label are correctly defined.
13-23: View Mode Selection via p-selectbuttonThe
<p-selectbutton>implementation for toggling view modes uses two-way binding and a custom template for icons, which is neatly integrated. Ensure thatjustifyOptionsincludes all necessary view options.
32-38: Conditional Rendering of Building CardsThe template uses server-side style directives (
@ifand@for) to render building cards when in list view. Ensure that these constructs are properly processed in your Angular setup or confirm if a framework-specific templating mechanism is used.
40-110: Building Card TemplateThe card template efficiently displays detailed information about a building—its name, number of devices, validity dates, and address—with a clean layout and action buttons. The use of the
.box-shadowclass adds visual depth. Confirm that date formatting and translation keys are accurate and that all data fields (such asnumberOfDevices) are correctly populated.sep490-frontend/src/assets/i18n/zh.json (2)
12-17: Common Translations Update
New keys for"revert","back", and the nested"error"object (with its"title"and"technicalServerError") have been added to the common section. These entries provide clearer user guidance in error scenarios and offer improved navigational context.
109-129: Enterprise Buildings Localization Enhancements
The addition of the"buildings"object under the"enterprise"section—with its"overview"(including"title"and button labels) and"details"(with"edit","create", and detailed"labels"such as"numberOfDevices")—aligns perfectly with the updated data model. The Chinese translations are clear and consistent with UI expectations.sep490-frontend/src/app/modules/dev/components/toolbox/toolbox.component.html (9)
1-5: Introduction of Card Template Component
A new<card-template>component is introduced to encapsulate header, body, and footer sections. The structure is clear; however, if these static texts ("header", "body", "footer") are intended for end-user display, consider linking them to localization keys for better internationalization support.
7-16: Primary Raised Buttons Group
This block creates a set of raised buttons with labels such as "Primary", "Secondary", "Success", etc. The usage of the[raised]="true"attribute is consistent and visually portrays the intended emphasis.
18-27: Text Variant Buttons Group
Buttons withvariant="text"are implemented here, offering a lighter visual style. The markup is straightforward and fits well within the design language.
29-38: Text Variant with Raised Buttons Group
This block combines the text variant with the raised effect ([raised]="true"), providing an elevated appearance while keeping the minimalistic text style. The implementation is clear and maintains consistency in styling.
40-48: Outlined Buttons Group
Outlined buttons are showcased here viavariant="outlined". The consistent application of the outlined style across various button labels reinforces a versatile yet unified UI design.
51-59: Icon-Only Buttons Group
A group of buttons using only icons is implemented. The selection of icons (e.g.,"pi pi-check","pi pi-bell") combined with varying severity settings serves well for compact or symbolic UI actions.
61-69: Rounded Icon Buttons Group
Buttons here add the[rounded]="true"attribute to icon-only buttons, softening their appearance. The change is executed neatly and is consistent with modern design trends.
71-79: Rounded Outlined Icon Buttons Group
The integration of both[rounded]="true"and[outlined]="true"attributes in this block delivers a distinct visual style. The consistent treatment across different buttons enhances the interface’s cohesiveness.
91-99: Rounded and Text Icon Buttons Group
The final group applies[rounded]="true"alongside[text]="true", offering a sleek, minimalist presentation for icon buttons. The consistency in design is well-maintained across the component.sep490-frontend/src/assets/i18n/en.json (2)
12-17: Updated Common Translations
Newly added entries for"revert","back", and the error handling object (with"title"and"technicalServerError") enhance user communication during error states. The placeholder{{correlationId}}is correctly preserved, ensuring consistency with error reporting.
109-128: Enterprise Buildings Localization Enhancements
The introduction of the"buildings"object—with its"overview"and"details"sections, including labels like"numberOfDevices"—reflects the recent data model changes. The English translations are clear and align with the backend adjustments, ensuring a smooth UI transition.sep490-frontend/src/assets/i18n/vi.json (2)
12-17: Cập nhật Bản Dịch Chung
Các khóa mới cho"revert","back"và đối tượng"error"đã được bổ sung, giúp truyền đạt thông điệp lỗi một cách rõ ràng cho người dùng. Placeholder{{correlationId}}được duy trì một cách chính xác.
110-127: Cải Tiến Bản Dịch cho Phần Tòa Nhà
Phần"buildings"mới, bao gồm các mục"overview"và"details", đã được bổ sung đầy đủ. Các nhãn như"numberOfDevices"được cập nhật để phản ánh chính xác thay đổi trong mô hình dữ liệu. Các bản dịch tiếng Việt phù hợp và rõ ràng với yêu cầu của giao diện người dùng.
...rontend/src/app/modules/enterprise/components/building-details/building-details.component.ts
Show resolved
Hide resolved
...rontend/src/app/modules/enterprise/components/building-details/building-details.component.ts
Show resolved
Hide resolved
sep490-enterprise/src/test/java/enterprise/rest/BuildingControllerTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/Configs.java (1)
14-17: LGTM! Consider configuring BCrypt strength.The implementation is secure using BCryptPasswordEncoder. For additional security, you could explicitly configure the strength parameter.
- public PasswordEncoder passwordEncoder() { - return new BCryptPasswordEncoder(); + public PasswordEncoder passwordEncoder() { + return new BCryptPasswordEncoder(12); // Increased strength for better securitysep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/JwtAuthenticationConverter.java (1)
10-10: LGTM! Good use of Spring's dependency injection.The addition of
@Componentannotation is correct and aligns with Spring's dependency injection pattern.Consider extracting the JWT claims processing logic into separate private methods to improve readability and maintainability. For example:
+ private Set<SimpleGrantedAuthority> extractAuthorities(Object authoritiesClaim) { + if (!(authoritiesClaim instanceof List<?> authoritiesLists)) { + return Collections.emptySet(); + } + return authoritiesLists + .stream() + .filter(String.class::isInstance) + .map(authority -> new SimpleGrantedAuthority((String) authority)) + .collect(Collectors.toUnmodifiableSet()); + } + + private List<BuildingPermissionDTO> extractBuildingPermissions(Object buildingPermissionsClaim) { + if (!(buildingPermissionsClaim instanceof List<?> buildingPermissionsList)) { + return Collections.emptyList(); + } + return buildingPermissionsList + .stream() + .filter(LinkedTreeMap.class::isInstance) + .map(this::mapToBuildingPermission) + .toList(); + } + + private BuildingPermissionDTO mapToBuildingPermission(Object permission) { + var permissionMap = (LinkedTreeMap) permission; + UUID uuid = Optional.ofNullable(permissionMap.get(BuildingPermissionDTO.Fields.buildingId)) + .map(String.class::cast) + .map(UUID::fromString) + .orElse(null); + return new BuildingPermissionDTO( + uuid, + BuildingPermissionRole.valueOf((String) permissionMap.get(BuildingPermissionDTO.Fields.role)) + ); + }Also applies to: 20-20
sep490-frontend/src/app/components/sidebar/sidebar.component.ts (2)
33-46: Consider adding a fallback forthis.itemswhen the user is not an ENTERPRISE_OWNER.Currently, the
itemsproperty is only set ifincludeRole()returns true for theENTERPRISE_OWNERrole. If not,itemsremains undefined. For better clarity and user experience, you might want to handle other roles or set an empty menu array as a fallback to avoid potential runtime checks for undefined.this.applicationService.UserData.pipe(takeUntil(this.destroy$)).subscribe( (userData: UserData): void => { if (this.applicationService.includeRole(userData.authorities, UserRole.ENTERPRISE_OWNER)) { this.items = this.buildEnterpriseOwnerMenu(); } else { + this.items = []; } } );
47-49: AlignSettingslabel with existing localization approach.While other labels (like
'enterprise.title') seem to use translation keys,'Settings'is a plain string. Consider using a translation key to maintain consistency in localization.sep490-frontend/src/app/components/sidebar/sidebar.component.html (1)
3-6: Clickable logo span may need accessibility enhancements.Wrapping a
<span>with a click listener is fine, but it might not be keyboard-accessible by default. Consider using an<a>or<button>element, adding atabindexor ARIA attributes to improve accessibility for keyboard-only users.-<span - class="inline-flex items-center pt-8 px-4 cursor-pointer" - (click)="homepage()" -> +<a + href="#" + class="inline-flex items-center pt-8 px-4 cursor-pointer" + (click)="homepage()" + tabindex="0" + aria-label="Go to homepage" +>sep490-enterprise/src/main/java/enterprise/rest/DevController.java (1)
1-51: Add API documentation.Consider adding OpenAPI/Swagger documentation to describe the purpose and usage of these development endpoints.
Add these annotations:
+@Tag(name = "Development", description = "Endpoints for development and testing purposes only") @RestController @PostMapping + @Operation(summary = "Log and return request body") + @ApiResponse(responseCode = "200", description = "Request body returned successfully") public String save(@RequestBody String body) { @PostMapping("/save-business-error") + @Operation(summary = "Simulate a business error") + @ApiResponse(responseCode = "417", description = "Demo business error thrown") public String saveBusinessError(@RequestBody String body) { @PostMapping("/save-technical-error") + @Operation(summary = "Simulate a technical error") + @ApiResponse(responseCode = "500", description = "Demo technical error thrown") public String saveTechnicalError(@RequestBody String body) {sep490-frontend/src/app/app-routing.module.ts (1)
76-80: DEV_PATH route might need a guard for production.
Consider restricting the dev module to authorized users or removing it from production builds.sep490-frontend/src/app/modules/core/services/application.service.ts (2)
69-70:private readonly userInfoSubjectanduserDataSubjectintroduced.
Consider explicitly typing these subjects to improve code clarity (e.g.,private readonly userDataSubject: BehaviorSubject<UserData | null>). Inferred types are acceptable, but explicit types can reduce confusion.
114-127:postLoginmethod updates user data.
Functionality is appropriate, but lacks explicit error handling. Consider adding.catchError(...)or.subscribe({ next: ..., error: ... })for robust error management.sep490-idp/src/main/java/green_buildings/idp/configs/AuthorizationServerConfig.java (1)
50-50: Address TODO comment regarding deprecation.There's a TODO comment about solving deprecated code. This should be addressed to ensure the application uses current, supported APIs.
Would you like me to help identify the deprecated APIs and suggest current alternatives?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/Configs.java(1 hunks)sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/audit/AuditorAwareImpl.java(1 hunks)sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/JwtAuthenticationConverter.java(2 hunks)sep490-enterprise/src/main/java/enterprise/EnterpriseApplication.java(1 hunks)sep490-enterprise/src/main/java/enterprise/rest/DevController.java(1 hunks)sep490-enterprise/src/main/java/enterprise/securities/ResourceServerConfig.java(1 hunks)sep490-enterprise/src/test/java/enterprise/rest/DevControllerTest.java(1 hunks)sep490-frontend/src/app/app-routing.module.ts(2 hunks)sep490-frontend/src/app/components/sidebar/sidebar.component.html(2 hunks)sep490-frontend/src/app/components/sidebar/sidebar.component.ts(1 hunks)sep490-frontend/src/app/modules/core/services/application.service.ts(3 hunks)sep490-idp/src/main/java/green_buildings/idp/IdentityProvider.java(1 hunks)sep490-idp/src/main/java/green_buildings/idp/configs/AuthorizationServerConfig.java(3 hunks)sep490-idp/src/main/java/green_buildings/idp/configs/GenericSecurityConfig.java(0 hunks)sep490-idp/src/main/java/green_buildings/idp/validators/UserValidator.java(0 hunks)
💤 Files with no reviewable changes (2)
- sep490-idp/src/main/java/green_buildings/idp/validators/UserValidator.java
- sep490-idp/src/main/java/green_buildings/idp/configs/GenericSecurityConfig.java
🔇 Additional comments (29)
sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/audit/AuditorAwareImpl.java (1)
6-6: LGTM! Good use of Spring's dependency injection.The addition of
@Componentannotation is correct and aligns with Spring's dependency injection pattern.Also applies to: 10-10
sep490-enterprise/src/main/java/enterprise/EnterpriseApplication.java (1)
5-5:❓ Verification inconclusive
Verify if explicit component scanning is necessary.
@SpringBootApplicationalready includes@ComponentScanby default. The explicit@ComponentScanmight be redundant unless you have a specific reason to override the default scanning behavior.Run this script to check if there are any components that require explicit scanning:
Also applies to: 9-9
🏁 Script executed:
#!/bin/bash # Description: Check for components in the commons.springfw package that might require explicit scanning # Search for Spring components in the commons.springfw package echo "Spring components in commons.springfw package:" rg -l "@(Component|Service|Repository|Controller|Configuration)" "commons/springfw" # Check if commons.springfw is already included in any other configuration echo -e "\nExisting component scan configurations:" rg -A 2 "@ComponentScan" .Length of output: 1051
Review Verification: Confirming the Need for Explicit Component Scanning
Based on the executed script, we see that the application explicitly scans for the packages "enterprise" and "commons.springfw". While the default behavior of
@SpringBootApplicationwould scan only the package of the main class (here, "enterprise"), the explicit inclusion of "commons.springfw" suggests that components from that package are intended to be picked up—likely because they are external to the main package hierarchy.Key observations:
- The search for Spring components in a folder corresponding to "commons.springfw" returned an error, implying that either the source isn’t in the repository or it's being provided via an external dependency.
- A similar scanning configuration appears in another module, consistently including "commons.springfw".
Action Items:
- Confirm if the "commons.springfw" package represents an external dependency or if it’s located outside the default scan path. If it isn’t actively contributing Spring components (or they are already auto-configured), you might consider removing the explicit
@ComponentScan.- Otherwise, if components from "commons.springfw" are required and are not automatically detected by Spring Boot, retaining the explicit scan is justified.
sep490-frontend/src/app/components/sidebar/sidebar.component.ts (5)
2-11: All imports appear consistent and appropriate.The imports for
Router,takeUntil,UserRole,ApplicationService,UserData, andSubscriptionAwareComponentalign well with the usage in this component, ensuring proper subscription handling and route navigation.
18-21: ExtendingSubscriptionAwareComponentis a good practice for managing subscriptions.By extending
SubscriptionAwareComponent, you ensure that all subscriptions are properly terminated when the component is destroyed, reducing the risk of memory leaks.
26-31: Constructor injection andsuper()call look correct.Injecting
ApplicationServiceandRouterwhile callingsuper()ensures the base class (SubscriptionAwareComponent) is fully initialized. No issues here.
51-52:homepage()implementation is concise and effective.Using
void this.router.navigate(['/'])is a quick way to ignore the resulting promise. This approach is acceptable, and the method name clearly indicates its intent.
55-57: Building a specialized enterprise menu is clear and organized.The
buildEnterpriseOwnerMenu()method neatly encapsulates the items for enterprise users. If other roles gain distinct menus, consider a similar approach to keep the code modular and maintainable.sep490-frontend/src/app/components/sidebar/sidebar.component.html (1)
35-35: Removal of extra space in the closing tag is harmless.No functional impact. This minor HTML cleanup is fine.
sep490-frontend/src/app/app-routing.module.ts (8)
1-1: Import forinjectlooks good.
No issues found. This change is consistent with the newer Angular dependency injection approach and requires Angular 14+.
3-8: Imports fromangular-auth-oidc-clientandrxjsappear consistent.
These imports match the usage below and there are no obvious issues.
15-15: ImportingApplicationServiceis appropriate.
No problems or code smells detected here.
36-37: Guard activation on Home route is valid.
AddingauthGuardalongsideAutoLoginPartialRoutesGuardenforces stricter access control.
41-42: Dashboard route guard updated successfully.
Ensuring the user is authenticated before accessing dashboard functionality is a good practice.
50-50: Authentication guard for AUTH_PATH.
Applying the guard here is consistent with the rest of the routes.
58-58: Authentication guard for ENTERPRISE_PATH.
No issues found, this aligns with enterprise module protection.
66-66: Guard applied to EMISSIONS_PATH.
Good practice to ensure that only authenticated users can access emissions functionality.sep490-frontend/src/app/modules/core/services/application.service.ts (7)
7-14: New imports for reactivity and data handling.
The additional RxJS imports (BehaviorSubject, filter, map, etc.) appear properly utilized below.
15-16: UUID andUserRoleimports.
These imports provide type safety for enterprise IDs and roles. No issues found.
29-31:UserInfoDatainterface extension.
MergingUserInfoEmailScopeandUserInfoPhoneScopewithsubis a clean approach.
33-37:UserDatainterface introduced.
ExtendingJwtPayloadwithauthorities,permissions, andenterpriseIdis a clear way to unify user data.
74-74:userDataSubjectinitialization.
InitializingBehaviorSubject<UserData | null>(null)is correct. No issues.
93-97: GetterUserDatareturning a filtered observable.
Straightforward approach to expose non-nullUserData. This is good practice for typed reactive data flows.
105-108:includeRolechecks for presence of a role.
Implementation looks correct. Be mindful of any potential case sensitivity or future role naming conventions, but for now, this is fine.sep490-idp/src/main/java/green_buildings/idp/IdentityProvider.java (2)
10-10:❓ Verification inconclusive
Verify component scanning configuration.
The component scanning configuration now includes both
commons.springfwandgreen_buildings.idppackages. Ensure that:
- All required components are being picked up
- No unintended components are being scanned
🏁 Script executed:
#!/bin/bash # Description: Check for Spring components in the specified packages # Look for @Component, @Service, @Repository, @Controller, etc. echo "Checking for Spring components in commons.springfw:" rg -l "@(Component|Service|Repository|Controller|Configuration)" -g "commons/springfw/**" echo -e "\nChecking for Spring components in green_buildings.idp:" rg -l "@(Component|Service|Repository|Controller|Configuration)" -g "green_buildings/idp/**"Length of output: 722
Component Scanning Verification: Manual Validation Required
The component scanning configuration now targets the "commons.springfw" and "green_buildings.idp" packages. However, our initial automated search did not yield any Spring component files—likely due to the file filters applied by ripgrep. Please:
- Manually verify or adjust the search criteria to ensure the repository contains the expected Spring components (i.e., classes annotated with @component, @service, @repository, etc.) under these package paths.
- Double-check that the directory structure matches the expected patterns for the packages specified.
12-12:✅ Verification successful
Verify method security configuration impact.
The addition of
@EnableMethodSecurity(jsr250Enabled = true)enables JSR-250 security annotations. This is a good practice for method-level security but requires verification of:
- Existing
@RolesAllowed,@PermitAll, and@DenyAllannotations- Potential security gaps during the transition
🏁 Script executed:
#!/bin/bash # Description: Check for JSR-250 security annotations usage echo "Checking for JSR-250 security annotations:" rg -A 2 "@(RolesAllowed|PermitAll|DenyAll)"Length of output: 2165
JSR-250 security configuration verified – no gaps detected.
The verification confirms that the codebase already leverages JSR-250 annotations (primarily
@RolesAllowed) in multiple locations, including:
sep490-idp/src/main/java/green_buildings/idp/rest/DevResource.javasep490-idp/src/main/java/green_buildings/idp/rest/EnterpriseUserRestController.javasep490-enterprise/src/main/java/enterprise/rest/BuildingController.javasep490-enterprise/src/main/java/enterprise/rest/DevController.javasep490-enterprise/src/main/java/enterprise/rest/EnterpriseController.javaSince no instances of
@PermitAllor@DenyAllwere encountered and the usage aligns with the expected behavior for enabling method-level security, no further changes are required at this point. Please continue to monitor the runtime behavior to ensure that no security gaps emerge during the transition.sep490-enterprise/src/main/java/enterprise/securities/ResourceServerConfig.java (2)
18-21: Good use of constructor injection pattern.The switch to constructor injection using
@RequiredArgsConstructorforJwtAuthenticationConverterfollows Spring best practices for dependency management.
29-29:✅ Verification successful
Verify CORS configuration after bean removal.
The CORS configuration is now using defaults (
Customizer.withDefaults()), but theCorsConfigurationSourcebean has been removed. This might affect cross-origin requests.Also applies to: 34-34
🏁 Script executed:
#!/bin/bash # Description: Check for CORS-related configurations echo "Checking for CORS configurations:" rg -A 5 "CorsConfiguration|@CrossOrigin" echo -e "\nChecking for existing CORS endpoints:" rg -l "Origin.*" -g "**/*.java"Length of output: 5330
Action: Confirm reliance on centralized CORS configuration
The removal of the local
CorsConfigurationSourcebean inResourceServerConfig.javaappears intentional. Our search shows that CORS is configured in shared modules (notably insep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/CorsConfig.javaandConfigs.java). Please ensure that these global settings (allowed headers, methods, origins, and credentials) fully cover your use case for cross-origin requests.
- Verify that endpoints relying on CORS are correctly wired to the centralized configuration.
- Confirm that the default CORS configuration meets your security and application requirements in the absence of a locally defined bean.
sep490-idp/src/main/java/green_buildings/idp/configs/AuthorizationServerConfig.java (2)
74-74: Verify JWT converter configuration.The JWT authentication converter is now injected and configured consistently across both resource and authorization servers. This is a good practice for maintaining consistent JWT handling.
82-82:❓ Verification inconclusive
Verify exception handling for JWK source.
The
NoSuchAlgorithmExceptionhas been removed from the method signature. Ensure that:
- The exception is properly handled internally
- No cryptographic algorithm issues can occur at runtime
🏁 Script executed:
#!/bin/bash # Description: Check for cryptographic algorithm usage and exception handling echo "Checking for cryptographic algorithm configurations:" rg -A 5 "KeyPair|KeyStore|RSAKey" echo -e "\nChecking for exception handling patterns:" rg -A 5 "try|catch|throw" -g "**/AuthorizationServerConfig.java"Length of output: 7306
Review: Confirm Exception Handling in
jwkSource()After reviewing the code and verifying the cryptographic configurations, note that the
jwkSource()method no longer declares aNoSuchAlgorithmExceptionand does not include an explicit try–catch block. It relies on the underlying Spring Security and Nimbus JOSE libraries to handle any cryptographic or key generation issues as runtime exceptions. Please ensure that:
- The keystore resource (
keystore.jks) is reliably available and correctly configured.- Any cryptographic misconfiguration or algorithm issue results in a controlled startup failure or is otherwise managed through integration/unit tests.
- You’ve verified via tests that no unexpected runtime failures occur due to missing handling for potential cryptographic errors.
sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/Configs.java
Show resolved
Hide resolved
sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/Configs.java
Show resolved
Hide resolved
thongdanghoang
force-pushed
the
feature/77-userstory-enterprise
branch
2 times, most recently
from
8b1fafc to
bdde695
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.ts (1)
67-70: 🛠️ Refactor suggestionImprove map marker handling.
The current implementation adds markers on click without any validation or clear purpose. Consider:
- Adding validation before placing markers
- Implementing a clear use case for marker placement
- Adding the ability to remove markers
sep490-idp/src/main/java/green_buildings/idp/configs/AuthorizationServerConfig.java (2)
82-93:⚠️ Potential issueMove sensitive credentials to configuration.
The keystore password is hard-coded in the source code. This poses a security risk and violates security best practices.
Move the password to a secure configuration source (e.g., environment variables or a secure configuration service). Example:
-var keyStoreKeyFactory = new KeyStoreKeyFactory(new ClassPathResource("keystore.jks"), "P@ssW0rd".toCharArray()); +@Value("${security.keystore.password}") +private String keystorePassword; + +// In the method +var keyStoreKeyFactory = new KeyStoreKeyFactory(new ClassPathResource("keystore.jks"), keystorePassword.toCharArray());
50-51: 💡 Verification agent🧩 Analysis chain
Address the TODO comment regarding deprecated code.
The comment indicates that
OAuth2AuthorizationServerConfiguration.applyDefaultSecurity(http)is deprecated. This should be addressed to ensure future compatibility.
🌐 Web query:
What is the recommended replacement for OAuth2AuthorizationServerConfiguration.applyDefaultSecurity in Spring Authorization Server?💡 Result:
The recommended replacement for the deprecated
OAuth2AuthorizationServerConfiguration.applyDefaultSecurity()in Spring Authorization Server (version 1.4+) is to useOAuth2AuthorizationServerConfigurerdirectly withHttpSecurity. Here's how to implement it:Updated Configuration Approach
@Bean public SecurityFilterChain authorizationServerSecurityFilterChain(HttpSecurity http) throws Exception { http .securityMatcher( new OrRequestMatcher( new AntPathRequestMatcher("/oauth2/**"), new AntPathRequestMatcher("/connect/**") ) ) .with( OAuth2AuthorizationServerConfigurer.authorizationServer(), Customizer.withDefaults() ); return http.build(); }Key Changes:
Replace Deprecated Method
- Old (deprecated):
OAuth2AuthorizationServerConfiguration.applyDefaultSecurity(http);- New:
http.with(OAuth2AuthorizationServerConfigurer.authorizationServer(), Customizer.withDefaults());Required Imports
import org.springframework.security.config.Customizer; import org.springframework.security.oauth2.server.authorization.config.annotation.web.configurers.OAuth2AuthorizationServerConfigurer;Why This Works:
- The
OAuth2AuthorizationServerConfigurernow handles protocol endpoints (e.g.,/oauth2/authorize,/oauth2/token) by default[1][5][9].Customizer.withDefaults()applies standard security settings equivalent to the deprecated method[2][9].Additional Considerations:
- OpenID Connect Support: To enable OpenID Connect 1.0, explicitly initialize
OidcConfigurer:http.getConfigurer(OAuth2AuthorizationServerConfigurer.class) .oidc(Customizer.withDefaults());- Endpoint Customization: Override default endpoints via
AuthorizationServerSettingsif needed[1][3].This update resolves deprecation warnings and ensures compatibility with Spring Authorization Server versions ≥1.4[2][5][9].
Citations:
- 1: https://docs.spring.io/spring-authorization-server/reference/configuration-model.html
- 2: https://www.codejava.net/frameworks/spring/oauth2authorizationserverconfiguration-applydefaultsecurity-deprecated
- 3: https://docs.spring.io/spring-authorization-server/docs/0.3.0/reference/html/configuration-model.html
- 4: https://www.javaadvent.com/2022/12/a-new-spring-security-authorization-server.html
- 5: https://docs.spring.io/spring-authorization-server/docs/current/api/org/springframework/security/oauth2/server/authorization/config/annotation/web/configuration/OAuth2AuthorizationServerConfiguration.html
- 6: https://www.baeldung.com/spring-dynamic-client-registration
- 7: https://www.lydtechconsulting.com/blog-oauth-oidc-part4.html
- 8: https://docs.spring.io/spring-authorization-server/docs/current/api/deprecated-list.html
- 9: https://www.youtube.com/watch?v=jYnTfkDwvVo
- 10: @Import(OAuth2AuthorizationServerConfiguration.class) cause error. spring-projects/spring-authorization-server#183
Update Deprecated Security Configuration
Please replace the deprecated call with the new recommended approach. Specifically, change:
// TODO: solve deprecated OAuth2AuthorizationServerConfiguration.applyDefaultSecurity(http);to
http.with(OAuth2AuthorizationServerConfigurer.authorizationServer(), Customizer.withDefaults());Make sure to update your imports accordingly:
import org.springframework.security.config.Customizer; import org.springframework.security.oauth2.server.authorization.config.annotation.web.configurers.OAuth2AuthorizationServerConfigurer;This change adheres to Spring Authorization Server (≥1.4) recommendations and resolves the deprecation warning.
🧹 Nitpick comments (12)
sep490-frontend/src/app/modules/enterprise/components/subscription/subscription.component.ts (2)
11-13: Align selector name with component name.The selector 'app-subscriptions' (plural) doesn't match the component name 'subscription' (singular). Consider updating the selector to maintain consistency:
- selector: 'app-subscriptions', + selector: 'app-subscription',
43-47: Implement credit purchase logic.The
onBuyCreditmethod contains a TODO comment but lacks implementation. This could block users from purchasing credits.Would you like me to help implement the credit purchase logic or create an issue to track this task?
sep490-frontend/src/app/components/sidebar/sidebar.component.ts (3)
33-49: Consider adding error handling and localization.While the subscription management is good, consider these improvements:
- Add error handling to the subscription
- Use translation keys for the settings menu label
Apply this diff:
ngOnInit(): void { this.applicationService.UserData.pipe(takeUntil(this.destroy$)).subscribe( (userData: UserData): void => { if ( this.applicationService.includeRole( userData.authorities, UserRole.ENTERPRISE_OWNER ) ) { this.items = this.buildEnterpriseOwnerMenu(); } - } + }, + (error: Error) => { + console.error('Failed to load user data:', error); + } ); this.settings = [ - {label: 'Settings', icon: 'pi pi-cog', route: '/settings'} + {label: 'settings.title', icon: 'pi pi-cog', route: '/settings'} ]; }
51-53: Simplify the homepage navigation.The void operator is unnecessary here as router.navigate already returns a Promise.
Apply this diff:
homepage(): void { - return void this.router.navigate(['/']); + this.router.navigate(['/']); }
55-99: Improve menu structure and consistency.
- The method is too long, consider breaking it into smaller methods
- Menu labels should consistently use translation keys
Apply this diff:
- // eslint-disable-next-line max-lines-per-function - private buildEnterpriseOwnerMenu(): MenuItem[] { + private buildEnterpriseOwnerMenu(): MenuItem[] { + return [ + this.buildEnterpriseSection(), + this.buildManageSection() + ]; + } + + private buildEnterpriseSection(): MenuItem { return [ { - label: 'enterprise.title', + label: 'menu.enterprise.title', items: [ { - label: 'Dashboard', + label: 'menu.dashboard.title', icon: 'pi pi-chart-line', route: `/${AppRoutingConstants.DASHBOARD_PATH}` }, // ... rest of the enterprise items ] - }, + } + ]; + } + + private buildManageSection(): MenuItem { + return [ { - label: 'Manage', + label: 'menu.manage.title', items: [ { - label: 'Users', + label: 'menu.users.title', icon: 'pi pi-users', route: `/${AppRoutingConstants.AUTH_PATH}/${AppRoutingConstants.USERS_PATH}` }, // ... rest of the manage items ] } ]; }sep490-frontend/src/app/app-routing.module.ts (1)
29-29: Simplify the operator chain.The
switchMap(loggedIn => of(loggedIn))is unnecessary as it's just passing through the value without transformation.- switchMap(loggedIn => of(loggedIn)) + map(loggedIn => loggedIn)sep490-frontend/src/app/modules/core/services/application.service.ts (2)
114-127: Optimize postLogin subscriptions.The method has two separate subscriptions that could be combined using
forkJoinfor better efficiency.postLogin(): void { - this.oidcSecurityService - .getPayloadFromAccessToken() - .pipe(takeUntil(this.destroy$)) - .subscribe((data: UserData): void => { - this.userDataSubject.next(data); - }); - this.oidcSecurityService - .getUserData() - .pipe(takeUntil(this.destroy$)) - .subscribe(userInfo => { - this.userInfoSubject.next(userInfo); - }); + forkJoin({ + userData: this.oidcSecurityService.getPayloadFromAccessToken(), + userInfo: this.oidcSecurityService.getUserData() + }) + .pipe(takeUntil(this.destroy$)) + .subscribe(({ userData, userInfo }) => { + this.userDataSubject.next(userData); + this.userInfoSubject.next(userInfo); + }); }
93-93: Follow Angular naming conventions for getters.The getter name should be camelCase according to Angular naming conventions.
- get UserData(): Observable<UserData> { + get userData(): Observable<UserData> {sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.ts (1)
77-89: Consider implementing proper pagination.The current implementation uses a hardcoded page size of 100, which may not scale well with large datasets. Consider:
- Implementing server-side pagination
- Adding load more functionality
- Using a reasonable initial page size (e.g., 20-25 items)
sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.html (2)
40-109: Enhance accessibility of the building card template.Consider adding the following accessibility improvements:
- ARIA labels for buttons and interactive elements
- Role attributes for semantic HTML
- Keyboard navigation support
<div class="rounded box-shadow border-t-2 border-t-primary *:w-full"> - <div class="p-4 rounded-t bg-primary-50 flex justify-between"> + <div class="p-4 rounded-t bg-primary-50 flex justify-between" role="heading" aria-level="2"> <div class="text-xl font-bold">{{ building.name }}</div> <div class=""> <p-button (onClick)="viewBuildingDetails(building.id)" icon="pi pi-info" text + [attr.aria-label]="'View details for ' + building.name" /> <p-button icon="pi pi-map" text + [attr.aria-label]="'View map for ' + building.name" /> </div>
24-30: Improve map container responsiveness.The fixed height of 600px may not be optimal for all screen sizes. Consider:
- Using viewport-relative units (vh)
- Adding media queries for different screen sizes
- Implementing a responsive container system
-<div [hidden]="listView" class="border-black border-2 border-solid" style="height: 600px"> +<div [hidden]="listView" class="border-black border-2 border-solid" style="height: 80vh; min-height: 400px; max-height: 800px">sep490-frontend/src/app/modules/dev/components/toolbox/toolbox.component.html (1)
1-99: Add documentation for the toolbox component.Consider adding:
- Comments explaining different button variations
- Usage examples in code comments
- Documentation for the card-template component
+<!-- Card template example --> <card-template> <div header>header</div> <div body>body</div> <div footer>footer</div> </card-template> +<!-- Primary button variations --> <div class="flex justify-center flex-wrap gap-4 mb-6"> <p-button label="Primary" [raised]="true"/> <!-- ... --> </div>
🛑 Comments failed to post (1)
sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.ts (1)
31-34:
⚠️ Potential issueFix inconsistent validation for numberOfDevices.
The field is initialized to 0 but has a minimum validation of 1, which creates an invalid initial state.
- numberOfDevices: new FormControl(0, { + numberOfDevices: new FormControl(1, { nonNullable: true, validators: [Validators.min(1), Validators.required] })📝 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.numberOfDevices: new FormControl(1, { nonNullable: true, validators: [Validators.min(1), Validators.required] })
thongdanghoang
force-pushed
the
feature/77-userstory-enterprise
branch
from
bdde695 to
2469597
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
sep490-frontend/src/app/app-routing.module.ts (1)
17-31: 🛠️ Refactor suggestionOptimize the auth guard implementation.
The current implementation has a few areas for improvement:
- The
switchMap(loggedIn => of(loggedIn))is unnecessary and can be removed as it doesn't transform the data.- The return type
Observable<LoginResponse>might not match the actual type fromcheckAuth().- Consider implementing
canActivateinterface for better type safety.Here's the suggested implementation:
-const authGuard: () => Observable<LoginResponse> = () => { +const authGuard = () => { const authService = inject(OidcSecurityService); const applicationService = inject(ApplicationService); return authService.checkAuth().pipe( tap(authData => { if (!authData.isAuthenticated) { applicationService.login(); } else { applicationService.postLogin(); } }) - switchMap(loggedIn => of(loggedIn)) ); };
🧹 Nitpick comments (6)
sep490-enterprise/src/test/java/enterprise/TestcontainersConfigs.java (1)
43-56: Consider removing commented-out code.The old static lifecycle methods are now commented out and can be safely removed since they've been replaced with the static initialization block.
-// @BeforeAll -// static void beforeAll() { -// postgres.start(); -// idP.withExposedPorts(8180) -// .withCommand("start-dev --http-port 8180") -// .waitingFor(Wait.forHttp("/realms/greenbuildings/.well-known/openid-configuration")) -// .start(); -// } - -// @AfterAll -// static void afterAll() { -// postgres.stop(); -// idP.stop(); -// }sep490-frontend/src/app/components/sidebar/sidebar.component.ts (3)
33-49: Consider improving i18n and configuration management.While the implementation is functionally correct, consider these improvements:
- Move the settings menu items to a configuration file for better maintainability.
- Use a translation key for the 'Settings' label to maintain i18n consistency, similar to how 'enterprise.title' is used in the main menu.
- this.settings = [ - {label: 'Settings', icon: 'pi pi-cog', route: '/settings'} - ]; + this.settings = [ + {label: 'settings.title', icon: 'pi pi-cog', route: '/settings'} + ];
51-53: Consider using routing constants.For consistency with other routes in the component, consider using
AppRoutingConstantsfor the root route navigation.- return void this.router.navigate(['/']); + return void this.router.navigate([AppRoutingConstants.ROOT_PATH]);
55-99: Consider refactoring menu configuration.The menu structure is well-organized, but consider these improvements:
- Move menu items to a configuration file to improve maintainability and avoid the eslint warning.
- Split the method into smaller functions, each handling a specific menu section (enterprise/manage).
Example refactor:
private buildEnterpriseOwnerMenu(): MenuItem[] { return [ this.buildEnterpriseSection(), this.buildManageSection() ]; } private buildEnterpriseSection(): MenuItem { return { label: 'enterprise.title', items: [ /* enterprise menu items */ ] }; } private buildManageSection(): MenuItem { return { label: 'Manage', items: [ /* manage menu items */ ] }; }sep490-frontend/src/app/modules/core/services/application.service.ts (2)
76-85: Consider adding error handling to the token payload subscription.While the subscription logic is correct, it would be beneficial to handle potential errors when retrieving the token payload.
Consider adding error handling:
this.registerSubscription( this.oidcSecurityService.isAuthenticated$ .pipe( filter((auth: AuthenticatedResult): boolean => auth.isAuthenticated), switchMap(() => this.oidcSecurityService.getPayloadFromAccessToken()) ) - .subscribe((userData: UserData): void => - this.userDataSubject.next(userData) - ) + .subscribe({ + next: (userData: UserData): void => this.userDataSubject.next(userData), + error: (error): void => { + console.error('Failed to retrieve token payload:', error); + this.userDataSubject.next(null); + } + }) );
105-108: Consider extracting the JWT role prefix as a constant.The role prefix should be defined as a class constant for better maintainability.
+private static readonly JWT_ROLE_PREFIX = 'ROLE_'; includeRole(authorities: string[], role: UserRole): boolean { - const jwtRolePrefix = 'ROLE_'; - return authorities.includes(`${jwtRolePrefix}${UserRole[role]}`); + return authorities.includes(`${ApplicationService.JWT_ROLE_PREFIX}${UserRole[role]}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/Configs.java(1 hunks)sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/audit/AuditorAwareImpl.java(1 hunks)sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/JwtAuthenticationConverter.java(2 hunks)sep490-enterprise/src/main/java/enterprise/EnterpriseApplication.java(1 hunks)sep490-enterprise/src/main/java/enterprise/rest/DevController.java(1 hunks)sep490-enterprise/src/main/java/enterprise/securities/ResourceServerConfig.java(1 hunks)sep490-enterprise/src/test/java/enterprise/TestcontainersConfigs.java(1 hunks)sep490-enterprise/src/test/java/enterprise/adapters/exchangerate_api/ExchangeRateApiCurrencyConverterTest.java(0 hunks)sep490-enterprise/src/test/java/enterprise/rest/BuildingControllerTest.java(1 hunks)sep490-enterprise/src/test/java/enterprise/rest/DevControllerTest.java(1 hunks)sep490-frontend/src/app/app-routing.module.ts(2 hunks)sep490-frontend/src/app/components/sidebar/sidebar.component.html(2 hunks)sep490-frontend/src/app/components/sidebar/sidebar.component.ts(1 hunks)sep490-frontend/src/app/modules/core/services/application.service.ts(3 hunks)sep490-frontend/src/app/modules/enterprise/components/subscription/subscription.component.ts(1 hunks)sep490-idp/src/main/java/green_buildings/idp/IdentityProvider.java(1 hunks)sep490-idp/src/main/java/green_buildings/idp/configs/AuthorizationServerConfig.java(3 hunks)sep490-idp/src/main/java/green_buildings/idp/configs/GenericSecurityConfig.java(0 hunks)sep490-idp/src/main/java/green_buildings/idp/validators/UserValidator.java(0 hunks)
💤 Files with no reviewable changes (3)
- sep490-enterprise/src/test/java/enterprise/adapters/exchangerate_api/ExchangeRateApiCurrencyConverterTest.java
- sep490-idp/src/main/java/green_buildings/idp/validators/UserValidator.java
- sep490-idp/src/main/java/green_buildings/idp/configs/GenericSecurityConfig.java
🚧 Files skipped from review as they are similar to previous changes (11)
- sep490-frontend/src/app/components/sidebar/sidebar.component.html
- sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/JwtAuthenticationConverter.java
- sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/audit/AuditorAwareImpl.java
- sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/Configs.java
- sep490-enterprise/src/test/java/enterprise/rest/DevControllerTest.java
- sep490-enterprise/src/main/java/enterprise/rest/DevController.java
- sep490-enterprise/src/main/java/enterprise/securities/ResourceServerConfig.java
- sep490-enterprise/src/main/java/enterprise/EnterpriseApplication.java
- sep490-frontend/src/app/modules/enterprise/components/subscription/subscription.component.ts
- sep490-idp/src/main/java/green_buildings/idp/IdentityProvider.java
- sep490-idp/src/main/java/green_buildings/idp/configs/AuthorizationServerConfig.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Java Modules (sep490-enterprise)
🔇 Additional comments (10)
sep490-enterprise/src/test/java/enterprise/rest/BuildingControllerTest.java (2)
40-40: LGTM!The test has been updated to use the new
numberOfDevicesfield instead of the removedfloorsfield.
37-41: Add test cases for invalid numberOfDevices values.The test coverage for the new
numberOfDevicesfield should be expanded to include edge cases.Consider adding these test cases:
@Test void createBuilding_withNegativeNumberOfDevices_returns400() { var building = BuildingDTO.builder() .name("Building 1") .numberOfDevices(-1) .build(); RestAssured.given() .auth().oauth2(getToken("enterprise.owner@greenbuildings.com", "enterprise.owner")) .contentType(ContentType.JSON) .body(building) .when() .post("/api/buildings") .then() .statusCode(400); }sep490-enterprise/src/test/java/enterprise/TestcontainersConfigs.java (3)
7-9: LGTM!The test configuration has been properly updated to use Spring Boot's test framework with Testcontainers support.
Also applies to: 15-15, 20-21
27-33: LGTM!The container initialization has been moved to a static block, which is a good practice for ensuring containers are started only once before any tests run.
35-41: LGTM!The instance-level setup with
@BeforeEachensures that each test starts with a freshbaseURIconfiguration.sep490-frontend/src/app/components/sidebar/sidebar.component.ts (2)
1-21: LGTM! Good use of subscription management.The component now properly extends
SubscriptionAwareComponentwhich is a good practice for managing RxJS subscriptions and preventing memory leaks.
26-31: LGTM! Clean dependency injection.The constructor follows Angular best practices with proper dependency injection and base class initialization.
sep490-frontend/src/app/modules/core/services/application.service.ts (3)
7-17: LGTM! Well-structured interfaces and imports.The new interfaces
UserInfoDataandUserDataare well-designed, properly extending their base interfaces and including all necessary fields for role-based access control.Also applies to: 29-37
69-70: LGTM! Good encapsulation and immutability.The changes improve encapsulation by making subjects private and readonly, preventing unintended modifications from outside the service.
Also applies to: 74-75
93-97: LGTM! Well-implemented UserData getter.The getter properly filters out null values and provides type-safe access to user data.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.ts (1)
53-56: Consider using type-safe field names for sorting.The sort field name should be type-safe to prevent runtime errors if the field name changes.
Consider using an enum or const for sort field names:
enum PaymentSortField { CreatedDate = 'createdDate', Amount = 'amount' } this.sort = { field: PaymentSortField.CreatedDate, direction: 'DESC' };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.html(1 hunks)sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Java Modules (sep490-enterprise)
🔇 Additional comments (2)
sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.ts (1)
62-84: LGTM! Sort configuration aligns with table columns.The sort configuration correctly aligns with the sortable columns defined in the table. Both 'createdDate' and 'amount' fields are marked as sortable, which matches the sort implementation.
sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.html (1)
35-42: LGTM! Sort binding is correctly implemented.The sort binding is properly implemented on the table-template component, maintaining consistency with other input bindings.
Summary by CodeRabbit
New Features
Style
Refactor