bugfix: missed address => geocoding #158
Conversation
WalkthroughThis update integrates a new geocoding feature across backend and frontend systems. On the backend, a geocoding adapter and supporting DTOs and mappers are introduced, along with a new REST endpoint. The building and enterprise user entities now include address-related fields. The frontend adds corresponding service methods, UI changes, and localization entries to handle geocoding results and updated user details. Additionally, configuration files and a SQL migration have been updated to support these data model changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GeocodingResource
participant GeocodingAdapter
participant RestTemplate
participant GeocodingMapper
Client->>GeocodingResource: GET /geocoding/reverse?latitude=&longitude=
GeocodingResource->>GeocodingAdapter: reverse(latitude, longitude)
GeocodingAdapter->>RestTemplate: GET constructed URL with key, lat, lon
RestTemplate-->>GeocodingAdapter: GeocodingReverseResponse
GeocodingAdapter->>GeocodingMapper: toAddressSuggestion(response)
GeocodingMapper-->>GeocodingAdapter: AddressSuggestionDTO
GeocodingAdapter-->>GeocodingResource: AddressSuggestionDTO
GeocodingResource-->>Client: ResponseEntity<AddressSuggestionDTO>
sequenceDiagram
participant User
participant BuildingDetailsComponent
participant GeocodingService
participant ExternalGeoAPI
User->>BuildingDetailsComponent: Update location (lat, lon)
BuildingDetailsComponent->>GeocodingService: reverse(latitude, longitude)
GeocodingService->>ExternalGeoAPI: GET request to geocode URL
ExternalGeoAPI-->>GeocodingService: AddressSuggestion response
GeocodingService-->>BuildingDetailsComponent: Return AddressSuggestion
BuildingDetailsComponent->>BuildingDetailsComponent: Update address field in form
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ 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 (
|
bugfix: null pointer bugfix: missed email verified and createdDate in edit enterprise user bugfix: missed i18n sidebar bugfix: no buildings found
thongdanghoang
force-pushed
the
bugfix/80-code-review-clean-todo-technical-debts
branch
from
85ae143 to
6757a58
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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. If you are seeing this consistently, please check "Code review limits" under "Moderation" settings.
Actionable comments posted: 3
🧹 Nitpick comments (7)
sep490-enterprise/src/main/resources/application.yml (1)
80-82: Add newline at end of fileThe file is missing a newline character at the end, which is flagged by YAMLlint. While this doesn't affect functionality, it's a best practice to include a newline at the end of files.
geocoding: key: secret - url: https://geocode.maps.co/reverse + url: https://geocode.maps.co/reverse +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 82-82: no new line character at the end of file
(new-line-at-end-of-file)
sep490-enterprise/src/main/resources/application-prod.yml (1)
80-82: Add newline at end of fileThe file is missing a newline character at the end, which is flagged by YAMLlint. While this doesn't affect functionality, it's a best practice to include a newline at the end of files.
geocoding: key: ${GEOCODING_API_KEY} - url: https://geocode.maps.co/reverse + url: https://geocode.maps.co/reverse +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 82-82: no new line character at the end of file
(new-line-at-end-of-file)
sep490-enterprise/src/main/java/greenbuildings/enterprise/adapters/geocoding/GeocodingMapper.java (1)
9-20: Well-structured MapStruct mapper for geocoding.The mapper properly maps address fields from the geocoding response to the AddressSuggestionDTO, contributing to the geocoding functionality mentioned in the PR title.
Consider removing the unnecessary blank line after the interface declaration for better code cleanliness:
@Mapper(unmappedTargetPolicy = ReportingPolicy.IGNORE, componentModel = MappingConstants.ComponentModel.SPRING) public interface GeocodingMapper { - @Mapping(target = "city", source = "address.city") @Mapping(target = "road", source = "address.road")sep490-enterprise/src/main/java/greenbuildings/enterprise/rest/GeocodingResource.java (1)
19-23: Consider adding error handling for the reverse geocoding endpoint.While the endpoint correctly validates input parameters with
@NotNullto prevent null pointer exceptions, there's no explicit error handling for cases where the geocoding service might fail or return unexpected results.Consider adding try-catch blocks or using Spring's exception handling mechanisms to gracefully handle potential errors:
@GetMapping("/reverse") public ResponseEntity<AddressSuggestionDTO> reverse(@RequestParam @NotNull Double latitude, @RequestParam @NotNull Double longitude) { - return ResponseEntity.ok(geocodingAdapter.reverse(latitude, longitude)); + try { + AddressSuggestionDTO result = geocodingAdapter.reverse(latitude, longitude); + return ResponseEntity.ok(result); + } catch (Exception e) { + // Log the error + log.error("Error during reverse geocoding", e); + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).build(); + } }Don't forget to add the lombok
@Slf4jannotation to the class if you implement this change.sep490-enterprise/src/main/java/greenbuildings/enterprise/adapters/geocoding/GeocodingAdapter.java (1)
25-34: Consider validating latitude and longitude parameters.The current implementation doesn't validate the latitude and longitude values, which could lead to invalid API requests if extreme or incorrect values are provided.
Add validation for the coordinates:
public AddressSuggestionDTO reverse(double latitude, double longitude) { + // Validate latitude (-90 to 90) and longitude (-180 to 180) + if (latitude < -90 || latitude > 90 || longitude < -180 || longitude > 180) { + throw new IllegalArgumentException("Invalid latitude or longitude values"); + } + var requestUrl = StringSubstitutor .replace("${url}?lat=${lat}&lon=${lon}&api_key=${key}", Map.of("url", this.url, "lat", latitude, "lon", longitude, "key", key)); var response = restTemplate.getForObject(requestUrl, GeocodingReverseResponse.class); return mapper.toAddressSuggestion(response); }sep490-frontend/src/app/components/sidebar/sidebar.component.ts (1)
51-53: Commented code should be removed.There's commented-out code related to settings menu items. This should either be removed completely or restored if needed.
- // this.settings = [ - // {label: 'Settings', icon: 'pi pi-cog', route: '/settings'} - // ];sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.ts (1)
115-146: Improved location handling with geocoding.The handleLocationChange method now properly validates location coordinates and fetches address information via the geocoding service, addressing the issue where "no buildings were found" as mentioned in the PR objectives.
Consider using internationalization for the error message "Invalid location" to maintain consistency with the rest of the application:
this.notificationService.add({ severity: 'error', summary: 'Error', - detail: 'Invalid location' + detail: this.translate.instant('error.invalidLocation') });
🛑 Comments failed to post (3)
sep490-frontend/src/app/modules/shared/components/form/abstract-form-component.ts (1)
194-194: 🛠️ Refactor suggestion
Enhanced form data retrieval to include disabled controls.
The change from
this.formGroup.valuetothis.formGroup.getRawValue()ensures that values from disabled form controls are also included in the submitted data. This is an important improvement that prevents data loss when form controls are disabled but their values are still needed for submission.sep490-enterprise/src/main/java/greenbuildings/enterprise/adapters/geocoding/GeocodingAdapter.java (1)
25-34:
⚠️ Potential issueMissing error handling for geocoding service.
The
reversemethod doesn't handle potential errors that might occur during the API call, such as network issues, invalid coordinates, or API service unavailability. This could lead to unexpected crashes.Consider adding proper exception handling:
public AddressSuggestionDTO reverse(double latitude, double longitude) { var requestUrl = StringSubstitutor .replace("${url}?lat=${lat}&lon=${lon}&api_key=${key}", Map.of("url", this.url, "lat", latitude, "lon", longitude, "key", key)); - var response = restTemplate.getForObject(requestUrl, GeocodingReverseResponse.class); - return mapper.toAddressSuggestion(response); + try { + var response = restTemplate.getForObject(requestUrl, GeocodingReverseResponse.class); + if (response == null) { + return null; + } + return mapper.toAddressSuggestion(response); + } catch (Exception e) { + // Log the error + // Consider implementing fallback behavior or returning a default response + return null; + } }📝 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.public AddressSuggestionDTO reverse(double latitude, double longitude) { var requestUrl = StringSubstitutor .replace("${url}?lat=${lat}&lon=${lon}&api_key=${key}", Map.of("url", this.url, "lat", latitude, "lon", longitude, "key", key)); try { var response = restTemplate.getForObject(requestUrl, GeocodingReverseResponse.class); if (response == null) { return null; } return mapper.toAddressSuggestion(response); } catch (Exception e) { // Log the error // Consider implementing fallback behavior or returning a default response return null; } }sep490-frontend/src/app/components/sidebar/sidebar.component.ts (1)
25-27:
⚠️ Potential issueTranslateService dependency removed but still using translation keys.
The TranslateService has been removed from the constructor, but the component still uses translation keys in the menu items. This might cause translation functionality to fail.
Either:
- Re-add the TranslateService dependency:
constructor( private readonly applicationService: ApplicationService, - private readonly router: Router + private readonly router: Router, + private readonly translate: TranslateService ) { super(); }
- Or use a different approach to handle translations if the service is no longer needed.
📝 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.constructor( private readonly applicationService: ApplicationService, private readonly router: Router, private readonly translate: TranslateService ) { super(); }
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
sep490-frontend/src/app/components/sidebar/sidebar.component.ts (1)
51-53: Remove commented-out code instead of keeping it.Commented-out code should be removed entirely rather than left in the codebase, as it can lead to confusion and maintenance challenges in the future.
- // this.settings = [ - // {label: 'Settings', icon: 'pi pi-cog', route: '/settings'} - // ];sep490-enterprise/src/main/resources/application.yml (2)
80-82: Confirm New Geocoding ConfigurationA new "geocoding" configuration block has been added with a default key ("secret") and the URL ("https://geocode.maps.co/reverse"). Ensure these values meet the requirements for your environment. Also, consider externalizing sensitive configuration values (such as the API key) via environment variables or a secure vault if applicable.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 82-82: no new line character at the end of file
(new-line-at-end-of-file)
82-82: Add New Line at End of FileYAMLlint has flagged that there is no newline character at the end of this file. Please add a newline to ensure compliance with linting standards.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 82-82: no new line character at the end of file
(new-line-at-end-of-file)
sep490-enterprise/src/test/java/greenbuildings/enterprise/rest/BuildingControllerTest.java (1)
36-51: Consider enhancing test to verify geocoding functionality.The test currently only verifies the HTTP status code. Consider extending it to also verify that the address was properly geocoded and stored by checking the response body or making a subsequent GET request.
sep490-enterprise/src/main/resources/application-prod.yml (1)
82-82: Add newline at end of file.The file is missing a newline at the end, which is flagged by YAMLlint.
url: https://geocode.maps.co/reverse +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 82-82: no new line character at the end of file
(new-line-at-end-of-file)
sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.ts (2)
100-113: Add error handling to fetchBuildingDetails method.The method doesn't handle errors that might occur when fetching building details.
private fetchBuildingDetails(): void { this.activatedRoute.paramMap .pipe( takeUntil(this.destroy$), map(params => params.get('id')), filter((idParam): idParam is string => !!idParam), filter(id => validate(id)), tap(id => this.buildingDetailsStructure.id.setValue(id)), switchMap(id => this.buildingService.getBuildingDetails(id as UUID)) ) .subscribe({ - subscribe(building => { + next: (building) => { this.formGroup.patchValue(building); + }, + error: (error) => { + this.notificationService.add({ + severity: 'error', + summary: this.translate.instant('common.error'), + detail: this.translate.instant('building.details.fetch.error') + }); + console.error('Error fetching building details:', error); } }); }
115-147: Improve error message for invalid location and refactor empty array returns.The error message for invalid location is generic, and the empty array returns could be more explicit.
private handleLocationChange(): void { this.activatedRoute.queryParams .pipe( takeUntil(this.destroy$), filter((params): params is MapLocation => !!params), switchMap(location => { if (!!location.latitude && !!location.longitude) { this.buildingDetailsStructure.latitude.setValue(location.latitude); this.buildingDetailsStructure.longitude.setValue( location.longitude ); return this.geocodingService.reverse( location.latitude, location.longitude ); } else if (!this.isEdit) { // only check when creating a new building this.notificationService.add({ severity: 'error', summary: 'Error', - detail: 'Invalid location' + summary: this.translate.instant('common.error'), + detail: this.translate.instant('building.details.location.invalid') }); - return []; + return []; // Observable that emits nothing } - return []; + return []; // Observable that emits nothing }) ) .subscribe({ - subscribe(address => { + next: (address) => { if (address) { this.buildingDetailsStructure.address.setValue(address.displayName); } + }, + error: (error) => { + this.notificationService.add({ + severity: 'error', + summary: this.translate.instant('common.error'), + detail: this.translate.instant('building.details.geocoding.error') + }); + console.error('Error getting address from coordinates:', error); } }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
sep490-frontend/src/assets/images/avatar.jpegis excluded by!**/*.jpeg
📒 Files selected for processing (34)
sep490-enterprise/src/main/java/greenbuildings/enterprise/adapters/geocoding/GeocodingAdapter.java(1 hunks)sep490-enterprise/src/main/java/greenbuildings/enterprise/adapters/geocoding/GeocodingAddress.java(1 hunks)sep490-enterprise/src/main/java/greenbuildings/enterprise/adapters/geocoding/GeocodingMapper.java(1 hunks)sep490-enterprise/src/main/java/greenbuildings/enterprise/adapters/geocoding/GeocodingReverseResponse.java(1 hunks)sep490-enterprise/src/main/java/greenbuildings/enterprise/dtos/AddressSuggestionDTO.java(1 hunks)sep490-enterprise/src/main/java/greenbuildings/enterprise/dtos/BuildingDTO.java(1 hunks)sep490-enterprise/src/main/java/greenbuildings/enterprise/entities/BuildingEntity.java(1 hunks)sep490-enterprise/src/main/java/greenbuildings/enterprise/rest/BuildingController.java(2 hunks)sep490-enterprise/src/main/java/greenbuildings/enterprise/rest/GeocodingResource.java(1 hunks)sep490-enterprise/src/main/resources/application-prod.yml(1 hunks)sep490-enterprise/src/main/resources/application.yml(1 hunks)sep490-enterprise/src/main/resources/db/migration/V0.0.1.10__BuildingAddress.sql(1 hunks)sep490-enterprise/src/test/java/greenbuildings/enterprise/rest/BuildingControllerTest.java(2 hunks)sep490-frontend/src/app/app-routing.constant.ts(1 hunks)sep490-frontend/src/app/components/header/header.component.html(1 hunks)sep490-frontend/src/app/components/sidebar/sidebar.component.html(1 hunks)sep490-frontend/src/app/components/sidebar/sidebar.component.ts(4 hunks)sep490-frontend/src/app/modules/authorization/components/create-user/enterprise-user-details.component.html(10 hunks)sep490-frontend/src/app/modules/authorization/components/create-user/enterprise-user-details.component.ts(3 hunks)sep490-frontend/src/app/modules/authorization/models/enterprise-user.ts(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(5 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(2 hunks)sep490-frontend/src/app/modules/enterprise/models/enterprise.dto.ts(1 hunks)sep490-frontend/src/app/modules/shared/components/form/abstract-form-component.ts(1 hunks)sep490-frontend/src/app/modules/shared/shared.module.ts(4 hunks)sep490-frontend/src/app/services/building.service.ts(2 hunks)sep490-frontend/src/app/services/geocoding.service.ts(1 hunks)sep490-frontend/src/assets/i18n/en.json(2 hunks)sep490-frontend/src/assets/i18n/vi.json(3 hunks)sep490-frontend/src/assets/i18n/zh.json(3 hunks)sep490-idp/src/main/java/greenbuildings/idp/dto/EnterpriseUserDetailsDTO.java(1 hunks)sep490-idp/src/main/java/greenbuildings/idp/mapper/EnterpriseUserMapper.java(0 hunks)
💤 Files with no reviewable changes (1)
- sep490-idp/src/main/java/greenbuildings/idp/mapper/EnterpriseUserMapper.java
🚧 Files skipped from review as they are similar to previous changes (26)
- sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.html
- sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.html
- sep490-enterprise/src/main/java/greenbuildings/enterprise/adapters/geocoding/GeocodingAddress.java
- sep490-frontend/src/app/app-routing.constant.ts
- sep490-enterprise/src/main/java/greenbuildings/enterprise/dtos/AddressSuggestionDTO.java
- sep490-enterprise/src/main/java/greenbuildings/enterprise/adapters/geocoding/GeocodingMapper.java
- sep490-enterprise/src/main/java/greenbuildings/enterprise/entities/BuildingEntity.java
- sep490-frontend/src/app/modules/authorization/models/enterprise-user.ts
- sep490-enterprise/src/main/java/greenbuildings/enterprise/adapters/geocoding/GeocodingReverseResponse.java
- sep490-enterprise/src/main/java/greenbuildings/enterprise/dtos/BuildingDTO.java
- sep490-enterprise/src/main/resources/db/migration/V0.0.1.10__BuildingAddress.sql
- sep490-enterprise/src/main/java/greenbuildings/enterprise/rest/GeocodingResource.java
- sep490-frontend/src/app/components/header/header.component.html
- sep490-frontend/src/app/components/sidebar/sidebar.component.html
- sep490-idp/src/main/java/greenbuildings/idp/dto/EnterpriseUserDetailsDTO.java
- sep490-enterprise/src/main/java/greenbuildings/enterprise/rest/BuildingController.java
- sep490-frontend/src/app/modules/shared/components/form/abstract-form-component.ts
- sep490-frontend/src/app/modules/enterprise/models/enterprise.dto.ts
- sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.ts
- sep490-frontend/src/app/services/geocoding.service.ts
- sep490-frontend/src/assets/i18n/zh.json
- sep490-frontend/src/assets/i18n/vi.json
- sep490-frontend/src/app/modules/shared/shared.module.ts
- sep490-frontend/src/assets/i18n/en.json
- sep490-enterprise/src/main/java/greenbuildings/enterprise/adapters/geocoding/GeocodingAdapter.java
- sep490-frontend/src/app/modules/authorization/components/create-user/enterprise-user-details.component.ts
🧰 Additional context used
🪛 YAMLlint (1.35.1)
sep490-enterprise/src/main/resources/application-prod.yml
[error] 82-82: no new line character at the end of file
(new-line-at-end-of-file)
sep490-enterprise/src/main/resources/application.yml
[error] 82-82: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (16)
sep490-frontend/src/app/services/building.service.ts (1)
31-38: Improved method implementation by removing unnecessary transformation logic.The simplification of the
searchBuildingsmethod aligns with the PR objective of fixing the "no buildings were found" issue. Removing the transformation logic is appropriate since the backend now includes the address field in the BuildingDTO.This change makes the code more maintainable and reduces potential for frontend-specific transformation bugs.
sep490-frontend/src/app/components/sidebar/sidebar.component.ts (2)
26-26: Constructor signature updated to remove TranslateService dependency.The removal of the TranslateService dependency from the constructor is appropriate since translation is now likely handled through pipes in the template rather than in the component class.
67-68: Menu labels properly internationalized.The static text labels have been correctly replaced with translation keys following a consistent naming convention (sidebar.[role].[feature]), which is good for maintainability and aligns with the PR objective of adding i18n support to the sidebar.
Also applies to: 72-73, 77-78, 84-85, 87-88, 92-93, 97-98, 109-110, 112-113
sep490-enterprise/src/main/resources/application.yml (1)
76-79: Review the Exchangerate-API URL ConfigurationThe "exchangerate-api" section now specifies a new URL ("https://v6.exchangerate-api.com/v6/"). Please verify that this change is intentional and that the updated endpoint is consistent with the external API documentation and any related integration tests.
sep490-enterprise/src/test/java/greenbuildings/enterprise/rest/BuildingControllerTest.java (3)
3-3: Import statement relocation looks good.The SearchCriteriaDTO import has been moved but maintains the same functionality.
41-41: Address field properly added to test.Good addition of the address field with a detailed, valid real-world address which supports the new geocoding functionality and matches the updated BuildingDTO requirements.
54-63:✅ Verification successful
Verify incomplete building test still works with new required address field.
The
createBuilding_withMissingFields_returns400test usesBuildingDTO.builder().build()without providing an address. Since the address field is now required (with@NotBlankannotation according to the summary), verify that this test still correctly validates the 400 response.
🏁 Script executed:
#!/bin/bash # Check if BuildingDTO has @NotBlank on address field rg -A 5 -B 5 "@NotBlank.*address|address.*@NotBlank" --type javaLength of output: 1282
Action: Confirm that the missing "address" field in the BuildingDTO still triggers a 400 error.
The verification output confirms that the
addressfield inBuildingDTOis annotated with@NotBlank, ensuring that it’s required. ThecreateBuilding_withMissingFields_returns400test, which creates a building without setting any fields (BuildingDTO.builder().build()), is still valid as it correctly expects a 400 response due to the missing mandatory address field.sep490-enterprise/src/main/resources/application-prod.yml (2)
80-82: New geocoding configuration added correctly.The geocoding configuration is structured well and follows the same pattern as other API configurations in the file.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 82-82: no new line character at the end of file
(new-line-at-end-of-file)
81-81:❓ Verification inconclusive
Ensure the GEOCODING_API_KEY environment variable is configured.
Make sure this environment variable is properly set in your production environment.
🏁 Script executed:
#!/bin/bash # Check if the environment variable is mentioned in deployment documentation grep -r "GEOCODING_API_KEY" --include="*.md" --include="*.txt" .Length of output: 64
Action: Please verify that the GEOCODING_API_KEY is documented and configured in your deployment process.
The reference in
sep490-enterprise/src/main/resources/application-prod.yml:key: ${GEOCODING_API_KEY}indicates that the environment variable must be provided at runtime. However, our search in markdown and text files did not return any documentation references for GEOCODING_API_KEY. Please manually check or update your deployment documentation to ensure that:
- The GEOCODING_API_KEY is clearly defined in your production environment configuration.
- Any automated deployment or CI/CD pipeline includes a mechanism to set this variable.
sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.ts (2)
47-47: Form control for address added correctly.The address field is properly added to the form structure with the required validator.
79-82: Consider potential race condition in initialization methods.Both
fetchBuildingDetails()andhandleLocationChange()are called simultaneously and might overwrite each other's values if they both update the form.Consider refactoring to ensure values are not unexpectedly overwritten:
protected initializeData(): void { - this.fetchBuildingDetails(); - this.handleLocationChange(); + // First fetch building details, then handle location change if needed + this.fetchBuildingDetails(); + // Only handle location change if we're not in edit mode + if (!this.isEdit) { + this.handleLocationChange(); + } }sep490-frontend/src/app/modules/authorization/components/create-user/enterprise-user-details.component.html (5)
1-1: Responsive Container UpdateThe outer
<div>now usessm:px-5, which improves the layout on smaller screens.
15-15: Enhanced Form PaddingThe
<form>element now includes the responsive classessm:mt-5 sm:px-5 py-5, which should help maintain a consistent and mobile-friendly layout.
42-49: Email Field Layout ConsistencyThe email field block now uses updated responsive grid classes and includes a correctly paired label and input (with
for="email"andid="email"). This change is consistent with the overall responsive design improvements.
179-188: Role Section ReviewThe role section’s updated layout (including the conditional rendering and grid configuration) appears consistent. The label’s
forattribute ("buildingPermissions") corresponds correctly with the associated control.
289-289: Button Alignment ImprovementThe button container now uses
flex justify-end sm:justify-start gap-4 *:min-w-24, which should enhance alignment on different screen sizes.
...rontend/src/app/modules/enterprise/components/building-details/building-details.component.ts
Show resolved
Hide resolved
.../src/app/modules/authorization/components/create-user/enterprise-user-details.component.html
Show resolved
Hide resolved
.../src/app/modules/authorization/components/create-user/enterprise-user-details.component.html
Show resolved
Hide resolved
.../src/app/modules/authorization/components/create-user/enterprise-user-details.component.html
Show resolved
Hide resolved
thongdanghoang
deleted the
bugfix/80-code-review-clean-todo-technical-debts
branch
bugfix: null pointer
bugfix: missed email verified and createdDate in edit enterprise user
bugfix: missed i18n sidebar
bugfix: no buildings found
Summary by CodeRabbit
New Features
UI/UX Enhancements
Data Enhancements