Conversation
notification function created
- Update AssignEmployeesRequestDto to accept username instead of employee ID - Add employee username validation in assignment flow - Improve employee lookup by username in DataSeeder - Update AppointmentController to handle username-based assignment
…erviceImpl to use it
… ServiceTypeService for improved service type management
…integrating with Time Logging Service. Added TimeSession entity, repository, and response DTO. Updated AppointmentServiceImpl to handle clock in/out logic and notify customers. Enhanced TimeLoggingClient for JWT authentication. Created integration tests for time tracking features.
… status transitions
…tatus and updating cancellation logic
Randitha superbranch
|
Warning Rate limit exceeded@RandithaK has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 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 (26)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR implements an intelligent time tracking system that integrates with the Time Logging Service, adds multi-employee assignment capability, and refactors service type management to use the Admin Service instead of local persistence.
Key changes:
- Automated clock in/out functionality with live timer support for employee time tracking
- Multi-employee assignment support for appointments (changed from single to multiple employees)
- Service type management delegated to Admin Service via WebClient integration
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| test-time-tracking.sh | Integration test script for time tracking workflow |
| application.properties | Added Admin Service URL configuration |
| ServiceTypeServiceImpl.java | Refactored to fetch service types from Admin Service instead of local DB |
| AppointmentServiceImpl.java | Added time tracking methods, multi-employee support, and enhanced notifications |
| NotificationClient.java | New client for sending notifications to Notification Service |
| AppointmentStateTransitionValidator.java | New component for validating appointment status transitions |
| AppointmentService.java | Added interface methods for time tracking and employee assignment |
| TimeSessionRepository.java | New repository for time session tracking |
| AppointmentRepository.java | Updated query to support multi-employee assignments |
| InvalidStatusTransitionException.java | Added constructor with custom reason message |
| TimeSession.java | New entity for tracking clock in/out sessions |
| AppointmentStatus.java | Added CUSTOMER_CONFIRMED status with documentation |
| Appointment.java | Changed to multi-employee support and added vehicle arrival tracking |
| TimeSessionResponse.java | New DTO for time session API responses |
| AppointmentResponseDto.java | Updated to support multi-employee IDs and vehicle tracking |
| AssignEmployeesRequestDto.java | New DTO for employee assignment requests |
| NotificationRequest.java | New DTO for notification service requests |
| ServiceTypeController.java | Updated authorization to allow customers access |
| AppointmentController.java | Added endpoints for time tracking and employee workflows |
| WebClientConfig.java | New configuration for Admin Service WebClient with JWT propagation |
| AppConfig.java | Added RestTemplate bean configuration |
| DataSeeder.java | Updated seed data to use multi-employee sets |
| TimeLoggingClient.java | New client for Time Logging Service integration |
| pom.xml | Added OAuth2 resource server and WebFlux dependencies |
| TIME_TRACKING_IMPLEMENTATION.md | Comprehensive documentation for time tracking feature |
| buildtest.yaml | Updated workflow triggers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| try { | ||
| // Call Admin Service public endpoint to get active service types | ||
| // Note: We ignore includeInactive parameter because public endpoint only returns active types |
There was a problem hiding this comment.
The includeInactive parameter is accepted but ignored, which creates an inconsistent API. Either remove the parameter from the method signature or implement proper handling by calling a different Admin Service endpoint that supports filtering inactive types.
| com.techtorque.appointment_service.service.NotificationClient notificationClient, | ||
| com.techtorque.appointment_service.client.TimeLoggingClient timeLoggingClient, | ||
| AppointmentStateTransitionValidator stateTransitionValidator) { | ||
| this.appointmentRepository = appointmentRepository; // assign required repository |
There was a problem hiding this comment.
[nitpick] Unnecessary inline comment. The assignment is self-explanatory and the comment adds no value.
| this.appointmentRepository = appointmentRepository; // assign required repository | |
| this.appointmentRepository = appointmentRepository; |
| if ((newStatus == AppointmentStatus.CONFIRMED || newStatus == AppointmentStatus.IN_PROGRESS) && | ||
| appointment.getAssignedEmployeeId() == null) { | ||
| appointment.setAssignedEmployeeId(employeeId); | ||
| (appointment.getAssignedEmployeeIds() == null || appointment.getAssignedEmployeeIds().isEmpty())) { |
There was a problem hiding this comment.
Potential NullPointerException. Line 269 checks if assignedEmployeeIds is null, but line 270 directly calls add() on it. If the collection is null, this will throw a NullPointerException. Initialize the set before adding: if (appointment.getAssignedEmployeeIds() == null) { appointment.setAssignedEmployeeIds(new HashSet<>()); } appointment.getAssignedEmployeeIds().add(employeeId);
| (appointment.getAssignedEmployeeIds() == null || appointment.getAssignedEmployeeIds().isEmpty())) { | |
| (appointment.getAssignedEmployeeIds() == null || appointment.getAssignedEmployeeIds().isEmpty())) { | |
| if (appointment.getAssignedEmployeeIds() == null) { | |
| appointment.setAssignedEmployeeIds(new HashSet<>()); | |
| } |
| .serviceType("AC Service") | ||
| .requestedDateTime(today.plusDays(1).atTime(10, 0)) | ||
| .status(AppointmentStatus.CONFIRMED) | ||
| .specialInstructions("AC not cooling properly") // <-- Modified comment |
There was a problem hiding this comment.
Remove the inline comment '// <-- Modified comment' which appears to be a development note accidentally left in the code.
| .specialInstructions("AC not cooling properly") // <-- Modified comment | |
| .specialInstructions("AC not cooling properly") |
| .build()); | ||
| .customerId(CUSTOMER_1_ID) | ||
| .vehicleId(VEHICLE_1_ID) | ||
| .assignedEmployeeIds(Set.of(EMPLOYEE_ID)) // <-- CHANGED |
There was a problem hiding this comment.
Remove the inline comment '// <-- CHANGED' which appears to be a development note accidentally left in the code.
| // Tomorrow's appointment - CONFIRMED | ||
| .customerId(CUSTOMER_2_ID) | ||
| .vehicleId(VEHICLE_4_ID) | ||
| .assignedEmployeeIds(Set.of(EMPLOYEE_ID)) // <-- CHANGED |
There was a problem hiding this comment.
Remove the inline comment '// <-- CHANGED' which appears to be a development note accidentally left in the code.
| .build()); | ||
| .customerId(CUSTOMER_1_ID) | ||
| .vehicleId(VEHICLE_1_ID) | ||
| .assignedEmployeeIds(Set.of(EMPLOYEE_ID)) // <-- CHANGED (This was the line that failed) |
There was a problem hiding this comment.
Remove the inline comment '// <-- CHANGED (This was the line that failed)' which appears to be a development note accidentally left in the code.
| .assignedEmployeeIds(Set.of(EMPLOYEE_ID)) // <-- CHANGED (This was the line that failed) | |
| .assignedEmployeeIds(Set.of(EMPLOYEE_ID)) |
| Appointment savedAppointment = appointmentRepository.save(appointment); | ||
|
|
||
| // Use the new Clock In functionality to start time tracking | ||
| clockIn(appointmentId, employeeId); |
There was a problem hiding this comment.
The acceptVehicleArrival method calls clockIn on line 749, but then returns the result of convertToDto(savedAppointment) on line 763 where savedAppointment was saved before the clockIn call. This means the returned appointment status will still be CONFIRMED instead of IN_PROGRESS (which is set by clockIn). Consider reloading the appointment after clockIn to return the updated status.
| request.put("date", LocalDate.now().toString()); | ||
| request.put("hours", hours); | ||
| request.put("description", description); | ||
| request.put("workType", "APPOINTMENT"); |
There was a problem hiding this comment.
[nitpick] The workType value is hardcoded as 'APPOINTMENT', but the parameter name in the method is description. Consider adding workType as a parameter to make the method more flexible, or document why it's always 'APPOINTMENT' for this use case.
| @CollectionTable(name = "appointment_assigned_employees", joinColumns = @JoinColumn(name = "appointment_id")) | ||
| @Column(name = "employee_id") | ||
| @Builder.Default | ||
| private Set<String> assignedEmployeeIds = new HashSet<>(); // Multiple employees can be assigned |
There was a problem hiding this comment.
getAssignedEmployeeIds exposes the internal representation stored in field assignedEmployeeIds. The value may be modified after this call to getAssignedEmployeeIds.
No description provided.