feat: add building location #142
Conversation
|
Warning Rate limit exceeded@thongdanghoang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (16)
WalkthroughThe changes add new geographical location fields ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BuildingsComponent
participant PopupService
participant BuildingDetailsComponent
User->>BuildingsComponent: Click "Add Building Location"
BuildingsComponent->>BuildingsComponent: turnOnSelectBuildingLocation()
User->>BuildingsComponent: Click on map to select location
BuildingsComponent->>PopupService: compilePopup(BuildingPopupMarkerComponent)
PopupService->>PopupService: Dynamically create popup marker
BuildingsComponent->>User: Display marker with location data
User->>BuildingsComponent: Confirm selection (click Next)
BuildingsComponent->>BuildingDetailsComponent: Navigate with query parameters (latitude, longitude)
BuildingDetailsComponent-->>User: Pre-fill location form controls
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
thongdanghoang
force-pushed
the
feature/77-userstory-enterprise
branch
from
be00a32 to
a6947e9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
sep490-frontend/src/app/modules/enterprise/services/popup.service.ts (1)
1-9: Optional modernization for Angular 14+These import statements are valid for dynamic component creation. If you're on Angular 14+ and wish to simplify, consider exploring the newer
ViewContainerRef.createComponentapproach, which can remove the need forComponentFactoryResolver.sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.ts (7)
33-36: Interface MapLocation is clearly definedIf you anticipate further attributes (e.g., address, region data), consider extending or placing it in a shared model. For now, it’s concise and sufficient.
47-48: Naming convention suggestion
addBuildingLocationandbuildingLocationare straightforward, but renaming toisSelectingBuildingLocationorselectedBuildingLocationmight better convey intent. Non-blocking.
72-78: Single marker addition logicThis code restricts the user to only one location marker. True or false, it aligns with your design. If multiple selectable locations are needed in the future, consider a more scalable approach.
90-92: Potential duplication of fetch logicTriggering
fetchBuilding()on each view mode change may be redundant if the data is already stored locally. Consider optimizing if performance becomes an issue.
94-97: Method naming clarity
turnOnSelectBuildingLocationconveys intent. If toggling becomes common, a single toggle method or external store might reduce complexity. Non-essential for now.
115-127: Marker removal approachDeleting layers by matching lat/lng works; however, if multiple markers share these coordinates, everything at that location is removed. Using a direct reference to the marker object is more robust if that scenario arises.
168-172: Validate zoom coordinatesUsing
zoomTois straightforward. If user input can be invalid, consider sanitizing or defaulting coordinates.sep490-frontend/src/app/modules/enterprise/models/enterprise.dto.ts (1)
9-10: Add JSDoc comments and consider using more specific types for coordinates.Consider adding documentation and type constraints for better code maintainability:
+ /** Latitude in degrees, must be between -90 and 90 */ latitude: number; + /** Longitude in degrees, must be between -180 and 180 */ longitude: number;You could also create a custom type for coordinates to enforce valid ranges at compile time:
type Latitude = number & { __brand: 'Latitude' }; type Longitude = number & { __brand: 'Longitude' };Also applies to: 16-17
sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.ts (1)
90-96: Enhance error handling for invalid location.The error message could be more descriptive to help users understand what went wrong.
- this.notificationService.add({ - severity: 'error', - summary: 'Error', - detail: 'Invalid location' - }); + this.notificationService.add({ + severity: 'error', + summary: this.translate.instant('enterprise.buildings.details.errors.invalidLocation'), + detail: this.translate.instant('enterprise.buildings.details.errors.locationRequired') + });sep490-frontend/src/app/modules/enterprise/components/building-popup-marker/building-popup-marker.component.html (1)
1-12: Consider enhancing the popup with additional features.The current popup shows basic information. Consider adding:
- A link to navigate to building details
- Display coordinates for admin users
- A small preview image if available
<div> <span class="font-bold"> {{ "enterprise.buildings.details.labels.name" | translate }}: </span> {{ building.name }} </div> <div> <span class="font-bold"> {{ "enterprise.buildings.details.labels.address" | translate }}: </span> {{ building.address }} </div> +<div *ngIf="isAdmin"> + <span class="font-bold"> + {{ "enterprise.buildings.details.labels.coordinates" | translate }}: + </span> + {{ building.latitude }}, {{ building.longitude }} +</div> +<div class="mt-2"> + <a [routerLink]="['/enterprise/buildings', building.id]"> + {{ "enterprise.buildings.actions.viewDetails" | translate }} + </a> +</div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
sep490-enterprise/src/main/java/enterprise/dtos/BuildingDTO.java(2 hunks)sep490-enterprise/src/main/java/enterprise/entities/BuildingEntity.java(1 hunks)sep490-enterprise/src/main/resources/db/migration/V0.0.1.7__BuildingLocations.sql(1 hunks)sep490-enterprise/src/main/resources/test-data.sql(1 hunks)sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.ts(3 hunks)sep490-frontend/src/app/modules/enterprise/components/building-popup-marker/building-popup-marker.component.html(1 hunks)sep490-frontend/src/app/modules/enterprise/components/building-popup-marker/building-popup-marker.component.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.html(3 hunks)sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.ts(6 hunks)sep490-frontend/src/app/modules/enterprise/enterprise.module.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/models/enterprise.dto.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/services/popup.service.ts(1 hunks)sep490-frontend/src/assets/i18n/en.json(1 hunks)sep490-frontend/src/assets/i18n/vi.json(1 hunks)sep490-frontend/src/assets/i18n/zh.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sep490-frontend/src/assets/i18n/zh.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Java Modules (sep490-enterprise)
🔇 Additional comments (19)
sep490-frontend/src/assets/i18n/en.json (1)
14-15: New Navigation Keys Added.
The"next"and"cancel"keys have been added in a consistent manner within the"common"section. Their inclusion supports UI navigation actions and aligns with similar updates in other localization files.sep490-frontend/src/assets/i18n/vi.json (1)
14-15: Localized Navigation Keys for Vietnamese.
The additions of"next": "Tiếp theo"and"cancel": "Hủy"are correctly implemented in the"common"section. The translations are appropriate and maintain consistency with the English version as well as other language files.sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.html (4)
1-1: Enforce Layout Consistency with Min-Height
The addition of thestyle="min-height: 42px"attribute ensures that the container maintains a consistent minimum height, which can help with alignment and spacing across different screen sizes.
6-10: Refined Building Location Button Behavior
The button now conditionally displays when!addBuildingLocationand triggers theturnOnSelectBuildingLocation()method on click. Please verify that the new method name accurately reflects its behavior and that the corresponding component logic has been updated accordingly.
15-16: Updated p-selectbutton Configuration
The modifications disable the select button whenaddBuildingLocationis true and update theonChangeevent handler to callonViewModeChanged()without parameters. Confirm that this event signature meets your requirements and that no event payload is needed for proper handling.
72-76: Enhanced Zoom Button Functionality
The map button’s click event now explicitly invokeszoomTo(building.latitude, building.longitude), leveraging the newly added geographical coordinates. Verify that all building objects reliably include validlatitudeandlongitudevalues to prevent potential runtime errors.sep490-frontend/src/app/modules/enterprise/services/popup.service.ts (2)
14-18: Constructor dependency injection looks goodInjecting
ComponentFactoryResolver,ApplicationRef, andInjectoris a standard approach for handling dynamic components. No issues found here.
20-43: Good memory management in dynamic component creationThe usage of
attachViewanddetachViewensures the component’s lifecycle is properly managed, preventing potential memory leaks. This approach is aligned with best practices for dynamic component rendering.sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.ts (8)
1-1: Importing ComponentRef is appropriateThis import is necessary for referencing component instances during popup compilation.
10-11: New dynamic popup dependencies recognizedImporting
PopupServiceandBuildingPopupMarkerComponentaligns with the new popup feature. No concerns spotted.
62-62: PopupService injection verifiedThis dependency is injected correctly and used properly in the code below.
68-68: Watch out for potential performance overheadRefetching data on view initialization is fine for smaller datasets, but for larger data sets, consider caching/building a local store.
82-84: Getter approach for mapView is cleanThis property-based approach for determinative UI states is perfectly acceptable.
86-88: Symmetrical listView getterConsistent style with the
mapViewgetter. No issues identified.
100-113: Router navigation with location queryParamsThis logic is straightforward. If the building location must be validated further (e.g., ensuring lat/lng bounds), contemplate a confirmation or data guard flow.
138-165: Subscription lacks error handlingAlthough you handle building creation of markers well, consider
.subscribe({ next:..., error:... })for better resilience. Also ensure coordinates are valid or handle out-of-range lat/lng data gracefully.sep490-frontend/src/app/modules/enterprise/components/building-popup-marker/building-popup-marker.component.ts (2)
1-2: Imports are appropriately definedAdding
Buildingvia a direct import ensures type safety for your input property.
10-11: @input({required: true}) usage is well-appliedEnsuring the parent provides
buildingat compile time is a strong type-safety measure.sep490-frontend/src/app/modules/enterprise/enterprise.module.ts (1)
15-15: LGTM!The BuildingPopupMarkerComponent is properly imported and declared in the module.
Also applies to: 24-24
sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.html
Show resolved
Hide resolved
...c/app/modules/enterprise/components/building-popup-marker/building-popup-marker.component.ts
Show resolved
Hide resolved
sep490-enterprise/src/main/java/enterprise/dtos/BuildingDTO.java
Outdated
Show resolved
Hide resolved
sep490-enterprise/src/main/java/enterprise/entities/BuildingEntity.java
Outdated
Show resolved
Hide resolved
...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
Outdated
Show resolved
Hide resolved
sep490-enterprise/src/main/resources/db/migration/V0.0.1.7__BuildingLocations.sql
Show resolved
Hide resolved
thongdanghoang
force-pushed
the
feature/77-userstory-enterprise
branch
from
a6947e9 to
3edf02b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
sep490-infrastructure/docker-compose.yml (1)
74-76: Volume Declarations and File Formatting.
The root-levelvolumesblock, declaringpostgresqlandpostgresql_data, correctly supports persistent storage for the database. Additionally, static analysis indicates that the file is missing a newline at the end. Please add a newline to conform with YAML linting standards.Suggested diff to add a newline at the end of the file:
postgresql_data: +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 76-76: no new line character at the end of file
(new-line-at-end-of-file)
sep490-frontend/src/app/modules/enterprise/models/enterprise.dto.ts (1)
9-10: LGTM! Consider adding type aliases for geographical coordinates.The addition of location fields is well-structured. Consider creating type aliases to improve type safety and reusability:
type Latitude = number; // -90 to 90 type Longitude = number; // -180 to 180 export interface Building extends BaseDTO { // ... latitude: Latitude; longitude: Longitude; }sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.ts (2)
70-79: Add cleanup for temporary markers.When selecting a building location, old temporary markers should be removed before adding a new one to prevent multiple markers from accumulating.
this.map.on('click', e => { const marker = L.marker([e.latlng.lat, e.latlng.lng]); if (this.addBuildingLocation && this.buildingLocation === null) { + // Remove any existing temporary markers + this.map.eachLayer(layer => { + if (layer instanceof L.Marker && !layer.getPopup()) { + this.map.removeLayer(layer); + } + }); marker.addTo(this.map); this.buildingLocation = { latitude: e.latlng.lat, longitude: e.latlng.lng }; } });
175-196: Consider configurable map center coordinates.The map center coordinates are hardcoded. Consider moving these to a configuration file or environment variables for better maintainability and reusability across different deployments.
+ private readonly DEFAULT_CENTER = [10.841394, 106.810052]; + private readonly DEFAULT_ZOOM = 16; + private readonly MAX_ZOOM = 16; + private readonly MIN_ZOOM = 1; private initMap(): void { if (document.getElementById('map')) { this.map = L.map('map', { - center: [10.841394, 106.810052], - zoom: 16 + center: this.DEFAULT_CENTER, + zoom: this.DEFAULT_ZOOM }); } else { throw new Error( 'Map element not found, should set id="map" to the map element' ); } const tiles = L.tileLayer( 'https://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png', { - maxZoom: 16, - minZoom: 1 + maxZoom: this.MAX_ZOOM, + minZoom: this.MIN_ZOOM } ); tiles.addTo(this.map); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
sep490-enterprise/src/main/java/enterprise/dtos/BuildingDTO.java(2 hunks)sep490-enterprise/src/main/java/enterprise/entities/BuildingEntity.java(2 hunks)sep490-enterprise/src/main/resources/db/migration/V0.0.1.7__BuildingLocations.sql(1 hunks)sep490-enterprise/src/main/resources/test-data.sql(1 hunks)sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.ts(3 hunks)sep490-frontend/src/app/modules/enterprise/components/building-popup-marker/building-popup-marker.component.html(1 hunks)sep490-frontend/src/app/modules/enterprise/components/building-popup-marker/building-popup-marker.component.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.html(3 hunks)sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.ts(6 hunks)sep490-frontend/src/app/modules/enterprise/enterprise.module.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/models/enterprise.dto.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/services/popup.service.ts(1 hunks)sep490-frontend/src/assets/i18n/en.json(1 hunks)sep490-frontend/src/assets/i18n/vi.json(1 hunks)sep490-frontend/src/assets/i18n/zh.json(1 hunks)sep490-infrastructure/docker-compose.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- sep490-frontend/src/assets/i18n/en.json
- sep490-frontend/src/app/modules/enterprise/enterprise.module.ts
- sep490-frontend/src/assets/i18n/zh.json
- sep490-frontend/src/app/modules/enterprise/components/building-popup-marker/building-popup-marker.component.html
- sep490-frontend/src/assets/i18n/vi.json
- sep490-enterprise/src/main/resources/db/migration/V0.0.1.7__BuildingLocations.sql
- sep490-frontend/src/app/modules/enterprise/components/building-popup-marker/building-popup-marker.component.ts
- sep490-enterprise/src/main/resources/test-data.sql
- sep490-enterprise/src/main/java/enterprise/dtos/BuildingDTO.java
- sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.html
🧰 Additional context used
🪛 YAMLlint (1.35.1)
sep490-infrastructure/docker-compose.yml
[error] 76-76: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Java Modules (sep490-enterprise)
🔇 Additional comments (8)
sep490-infrastructure/docker-compose.yml (2)
17-18: Confirm Volume Mounts for PostgreSQL Persistence.
The addition of the volumespostgresqlandpostgresql_datafor thesep490_databasesservice looks correct. Please verify that the paths/var/lib/postgresqland/var/lib/postgresql/dataare consistent with the container's expectations and that the host system permissions allow for proper data persistence.
72-73: Verify Kafka Partition Configuration.
A new environment variableKAFKA_NUM_PARTITIONS: 3has been introduced in the Kafka service. Please ensure this setting aligns with your Kafka deployment strategy and that it has been documented accordingly.sep490-enterprise/src/main/java/enterprise/entities/BuildingEntity.java (1)
37-45: Make location columns non-nullable and add validation constraints.The validation constraints for coordinate ranges are correct, but the columns should be non-nullable to ensure data integrity.
sep490-frontend/src/app/modules/enterprise/services/popup.service.ts (1)
27-43: LGTM! Well-implemented dynamic component creation.The implementation is clean, type-safe, and follows Angular best practices. Proper cleanup of component views prevents memory leaks.
sep490-frontend/src/app/modules/enterprise/components/building-details/building-details.component.ts (2)
92-92: Improve type safety for query params handling.The current type casting to
MapLocationis unsafe.
36-45: LGTM! Well-structured form controls with proper validation.The form controls for latitude and longitude are well-implemented with appropriate validation ranges for geographical coordinates.
sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.ts (2)
33-36: LGTM! Well-defined interface for location data.The
MapLocationinterface clearly defines the required geographical coordinates.
94-97: LGTM! Well-implemented map interaction methods.The methods handle state changes and cleanup appropriately:
turnOnSelectBuildingLocation: Ensures map view is active for location selectioncancelAddBuilding: Properly cleans up temporary markers and resets statezoomTo: Includes proper null checks and view mode handlingAlso applies to: 115-127, 168-173
sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.ts
Outdated
Show resolved
Hide resolved
sep490-frontend/src/app/modules/enterprise/components/buildings/buildings.component.ts
Show resolved
Hide resolved
thongdanghoang
force-pushed
the
feature/77-userstory-enterprise
branch
from
3edf02b to
c6cbd91
Compare
#77
Summary by CodeRabbit
New Features
UI Enhancements
Backend Updates
Internationalization