Staff fix bug#41
Conversation
|
Thank you for following naming conventions! 😻 |
Deploying ev-rental with
|
| Latest commit: |
cc836a8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f9c9562a.ev-rental.pages.dev |
| Branch Preview URL: | https://staff-fix-bug.ev-rental.pages.dev |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the staff booking workflow by extracting presentation logic into dedicated presenter classes, fixing the fulfillment route structure, and adding KYC document support for renter management. The changes improve code organization and maintainability while addressing routing bugs.
- Refactored staff-dashboard and fulfillment-page components to use presenter pattern for separation of concerns
- Fixed booking fulfillment route from
/staff/bookings/:bookingId/fulfillmentto/staff/fulfillment/:bookingId - Added comprehensive KYC document extraction and display for renter profiles
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/contract/model/bookingDetailsDto.ts | Added optional depositAmount and totalAmount fields to BookingDetailsDto |
| src/app/features/staff/staff.routes.ts | Fixed fulfillment route path and reorganized imports |
| src/app/features/staff/staff-dashboard/staff-dashboard.ts | Extracted presentation logic to StaffDashboardPresenter, delegated formatting and view model building |
| src/app/features/staff/staff-dashboard/staff-dashboard.presenter.ts | New presenter containing all view model building, formatting, and business logic |
| src/app/features/staff/staff-dashboard/staff-dashboard.models.ts | New models file containing interfaces and constants previously in component |
| src/app/features/staff/renter-management/renter-management.ts | Added KYC document extraction, prioritization, and display logic |
| src/app/features/staff/renter-management/renter-management.scss | Added styles for KYC document list display |
| src/app/features/staff/renter-management/renter-management.html | Updated template to display KYC documents and use dynamic primary KYC fields |
| src/app/features/staff/booking-fulfillment/pages/fulfillment-page/fulfillment-page.ts | Extracted presentation logic to FulfillmentPagePresenter |
| src/app/features/staff/booking-fulfillment/pages/fulfillment-page/fulfillment-page.presenter.ts | New presenter for fulfillment page logic |
| src/app/features/staff/booking-fulfillment/pages/fulfillment-page/fulfillment-page.models.ts | New models file for fulfillment page interfaces and constants |
| src/app/core-logic/renters/renters.service.ts | Added extensive KYC document extraction logic with type normalization and status classification |
| src/app/core-logic/bookings/bookings.service.ts | Added userId extraction and resolution for booking records |
| src/app/core-logic/auth/auth.interceptor.ts | Refactored to use proper token refresh with shareReplay to prevent duplicate requests |
| specs/001-staff-booking-flow/spec.md | Updated route path in documentation |
| specs/001-staff-booking-flow/quickstart.md | Updated route path in quickstart guide |
| openapi-spec.json | Added depositAmount and totalAmount fields to BookingDetailsDto schema |
| angular.json | Added 'qrcode' to allowedCommonJsDependencies |
| record.rental?.rentalId ?? undefined, | ||
| record.rental?.vehicleId ?? undefined, | ||
| record.vehicleDetails?.make ?? undefined, | ||
| record.vehicleDetails?.model ?? undefined, | ||
| record.renterProfile?.userName ?? undefined, | ||
| record.renterProfile?.address ?? undefined, | ||
| record.renterProfile?.driverLicenseNo ?? undefined, |
There was a problem hiding this comment.
The ?? undefined pattern is redundant. The nullish coalescing operator already returns undefined when the left operand is null or undefined, so explicitly specifying ?? undefined has no effect and reduces readability.
| record.rental?.rentalId ?? undefined, | |
| record.rental?.vehicleId ?? undefined, | |
| record.vehicleDetails?.make ?? undefined, | |
| record.vehicleDetails?.model ?? undefined, | |
| record.renterProfile?.userName ?? undefined, | |
| record.renterProfile?.address ?? undefined, | |
| record.renterProfile?.driverLicenseNo ?? undefined, | |
| record.rental?.rentalId, | |
| record.rental?.vehicleId, | |
| record.vehicleDetails?.make, | |
| record.vehicleDetails?.model, | |
| record.renterProfile?.userName, | |
| record.renterProfile?.address, | |
| record.renterProfile?.driverLicenseNo, |
| import { AuthService } from './auth.service'; | ||
| import { TokenService } from '../token/token.service'; | ||
|
|
||
| let refreshInFlight$: Observable<unknown> | null = null; |
There was a problem hiding this comment.
Module-level mutable state (refreshInFlight$) creates a shared singleton that persists across multiple injector contexts and can cause race conditions or stale state. Consider moving this into a service with proper lifecycle management or using a BehaviorSubject to ensure proper cleanup and testability.
| : undefined; | ||
|
|
||
| return { | ||
| id: 'kyc-' + document.type + '-' + index, |
There was a problem hiding this comment.
Using index as part of the tracking ID can cause incorrect rendering when documents are reordered or filtered. Since document.type is not unique, this can produce duplicate IDs if multiple documents of the same type exist. Consider using a combination of all unique fields: ${document.type}-${document.documentNumber ?? 'none'}-${document.status ?? 'none'}.
| id: 'kyc-' + document.type + '-' + index, | |
| id: `kyc-${document.type}-${document.documentNumber ?? 'none'}-${document.status ?? 'none'}`, |
| return this._staffService.apiStaffRentersGet().pipe( | ||
| map((response: RenterProfileDtoListApiResponse) => response.data ?? []), | ||
| map((renters) => | ||
| renters.find((renter) => this._normalizeString(renter?.renterId) === renterId), | ||
| ), | ||
| map((match) => this._extractUserId(match)), | ||
| catchError(() => of(undefined)), | ||
| ); |
There was a problem hiding this comment.
The _resolveRenterUserId$ method fetches the entire renter list on every call to find a single userId. This creates an N+1 query pattern when loading multiple bookings. Consider caching the renter list, using a dedicated API endpoint for userId lookup, or resolving all userIds in batch before processing individual bookings.
| private readonly router = inject(Router); | ||
| private readonly cdr = inject(ChangeDetectorRef); | ||
| private readonly environmentInjector = inject(EnvironmentInjector); | ||
| private readonly presenter = new StaffDashboardPresenter(); |
There was a problem hiding this comment.
Manually instantiating the presenter with new bypasses Angular's dependency injection. If the presenter later needs injected dependencies (e.g., for formatters or services), this will require refactoring. Consider making StaffDashboardPresenter an injectable service with @Injectable({ providedIn: 'root' }) or using inject() within the component constructor.
| private readonly presenter = new StaffDashboardPresenter(); | |
| private readonly presenter = inject(StaffDashboardPresenter); |
| private readonly formBuilder = inject(NonNullableFormBuilder); | ||
| private readonly cdr = inject(ChangeDetectorRef); | ||
| private readonly environmentInjector = inject(EnvironmentInjector); | ||
| private readonly presenter = new FulfillmentPagePresenter(); |
There was a problem hiding this comment.
Manually instantiating the presenter with new bypasses Angular's dependency injection. If the presenter later needs injected dependencies, this will require refactoring. Consider making FulfillmentPagePresenter an injectable service with @Injectable({ providedIn: 'root' }) or using inject() within the component constructor.
| private readonly presenter = new FulfillmentPagePresenter(); | |
| private readonly presenter = inject(FulfillmentPagePresenter); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private _resolveRenterUserId$(renterId: string | undefined): Observable<string | undefined> { | ||
| if (!renterId) { | ||
| return of(undefined); | ||
| } | ||
|
|
||
| return this._staffService.apiStaffRentersGet().pipe( | ||
| map((response: RenterProfileDtoListApiResponse) => response.data ?? []), | ||
| map((renters) => | ||
| renters.find((renter) => this._normalizeString(renter?.renterId) === renterId), | ||
| ), | ||
| map((match) => this._extractUserId(match)), | ||
| catchError(() => of(undefined)), | ||
| ); |
There was a problem hiding this comment.
When building staff booking records the new _resolveRenterUserId$ helper now calls apiStaffRentersGet() for each booking that lacks a userId. The staff dashboard can easily contain dozens of bookings, so this change issues dozens of identical requests that download the entire renter list repeatedly before any booking data resolves. This is a regression from the previous code path (which performed no additional calls) and significantly slows the dashboard and stresses the API. Consider caching the renter list or resolving all user IDs in a single request per refresh.
Useful? React with 👍 / 👎.
No description provided.