Conversation
…TOs for appointment operations
… status transitions
Feat/appointment service
…Config for appointment availability endpoint
… and repositories
feat: Enhance appointment filtering and role-based access in services and repositories
…ying the Appointment Service to Kubernetes
feat: Add GitHub Actions workflows for building, packaging, and deploying the Appointment Service to Kubernetes
|
Warning Rate limit exceeded@RandithaK has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 26 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 (47)
✨ 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 the Appointment Service from scratch, transforming it from 0% to 100% completion. The service handles appointment booking, scheduling, and management with comprehensive business logic including service types, service bays, business hours, and holiday management.
- Complete implementation of all 15 API endpoints for appointment and service type management
- Full business logic with validation for dates, times, business hours, holidays, and bay availability
- Comprehensive data seeding with 10 service types, 4 service bays, business hours, holidays, and sample appointments
- Role-based access control for customers, employees, and admins
Reviewed Changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| AppointmentServiceImpl.java | Complete service implementation with 546 lines of business logic for appointments |
| ServiceTypeServiceImpl.java | Service type CRUD operations with duplicate name validation |
| AppointmentController.java | 9 REST endpoints for appointment management with role-based security |
| ServiceTypeController.java | 6 REST endpoints for service type management (admin/employee only) |
| DataSeeder.java | Comprehensive seed data with 421 lines covering all entities |
| AppointmentRepository.java | 10 custom queries including native query for flexible filtering |
| ServiceTypeRepository.java | Active/inactive filtering and category-based queries |
| ServiceBayRepository.java | Active bay queries with ordering |
| BusinessHoursRepository.java | Day-of-week based operating hours lookup |
| HolidayRepository.java | Date-based holiday checking |
| GatewayHeaderFilter.java | Enhanced to handle SUPER_ADMIN role mapping to ADMIN |
| SecurityConfig.java | Added public endpoint for availability checking |
| OpenApiConfig.java | Complete Swagger/OpenAPI documentation configuration |
| Entity classes | 5 new entities (ServiceType, ServiceBay, BusinessHours, Holiday, updated Appointment) |
| DTO classes | 13 DTOs for request/response handling |
| Exception classes | 4 custom exceptions with global handler |
| Test configuration | H2 test database setup with PostgreSQL compatibility |
| Docker & CI/CD | Build, package, and deployment workflows |
| Documentation | Comprehensive README and implementation summary |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| LocalDateTime aptStart = appointment.getRequestedDateTime(); | ||
| LocalDateTime aptEnd = aptStart.plusMinutes(60); |
There was a problem hiding this comment.
Same hardcoded 60-minute duration issue when checking bay availability. This method is used in the availability checking endpoint and doesn't consider the actual duration of existing appointments, which can range from 30 to 300 minutes based on service type. Should retrieve and use the actual service type duration for each appointment.
| if (businessHours.getBreakStartTime() != null && businessHours.getBreakEndTime() != null) { | ||
| if (!time.isBefore(businessHours.getBreakStartTime()) && | ||
| time.isBefore(businessHours.getBreakEndTime())) { | ||
| throw new IllegalArgumentException("Cannot book appointment during break time"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete validation for appointments that span the break time. The current check only validates if the appointment starts during the break (lines 323-324), but doesn't prevent appointments that start before the break and would continue into it. For example, a 90-minute appointment starting at 11:30 AM would pass this validation but would overlap with the 12:00-1:00 PM lunch break.
Should add:
LocalDateTime appointmentEnd = dateTime.plusMinutes(durationMinutes);
if (time.isBefore(businessHours.getBreakEndTime()) &&
appointmentEnd.toLocalTime().isAfter(businessHours.getBreakStartTime())) {
throw new IllegalArgumentException("Appointment would overlap with break time");
}| - Customer IDs: `00000000-0000-0000-0000-000000000101`, `00000000-0000-0000-0000-000000000102` | ||
| - Employee IDs: `00000000-0000-0000-0000-000000000003-005` | ||
| - Vehicle IDs: `VEH-001` to `VEH-004` |
There was a problem hiding this comment.
Documentation inconsistency: The README states shared constants are "Customer IDs: 00000000-0000-0000-0000-000000000101, 00000000-0000-0000-0000-000000000102" and "Employee IDs: 00000000-0000-0000-0000-000000000003-005", but the actual DataSeeder.java file uses simple string values like "customer", "testuser", and "employee". The documentation should be updated to reflect the actual implementation.
| - Customer IDs: `00000000-0000-0000-0000-000000000101`, `00000000-0000-0000-0000-000000000102` | |
| - Employee IDs: `00000000-0000-0000-0000-000000000003-005` | |
| - Vehicle IDs: `VEH-001` to `VEH-004` | |
| - Customer IDs: `"customer"`, `"testuser"` | |
| - Employee IDs: `"employee"` | |
| - Vehicle IDs: `"VEH-001"`, `"VEH-002"`, `"VEH-003"`, `"VEH-004"` |
| ```java | ||
| // User IDs (from Auth Service) | ||
| CUSTOMER_1_ID = "00000000-0000-0000-0000-000000000101" | ||
| CUSTOMER_2_ID = "00000000-0000-0000-0000-000000000102" | ||
| EMPLOYEE_1_ID = "00000000-0000-0000-0000-000000000003" | ||
| EMPLOYEE_2_ID = "00000000-0000-0000-0000-000000000004" | ||
| EMPLOYEE_3_ID = "00000000-0000-0000-0000-000000000005" | ||
|
|
||
| // Vehicle IDs (for Vehicle Service) | ||
| VEHICLE_1_ID = "VEH-001" | ||
| VEHICLE_2_ID = "VEH-002" | ||
| VEHICLE_3_ID = "VEH-003" | ||
| VEHICLE_4_ID = "VEH-004" | ||
| ``` |
There was a problem hiding this comment.
Documentation inconsistency: The IMPLEMENTATION_SUMMARY.md states that the shared constants are UUID values like "00000000-0000-0000-0000-000000000101" for customers and "00000000-0000-0000-0000-000000000003" for employees, but the actual DataSeeder.java implementation uses simple string values like "customer", "testuser", and "employee". The documentation should be updated to match the actual implementation.
| if (userRoles.contains("ADMIN") || userRoles.contains("EMPLOYEE")) { | ||
| appointments = appointmentRepository.findAll(); | ||
| } else if (userRoles.contains("EMPLOYEE")) { | ||
| appointments = appointmentRepository.findByAssignedEmployeeIdAndRequestedDateTimeBetween( | ||
| userId, LocalDateTime.now().minusYears(1), LocalDateTime.now().plusYears(1)); | ||
| } else { | ||
| appointments = appointmentRepository.findByCustomerIdOrderByRequestedDateTimeDesc(userId); | ||
| } |
There was a problem hiding this comment.
Redundant and unreachable conditional logic detected. Line 77 checks if userRoles contains "ADMIN" OR "EMPLOYEE", which will match if either is present. Line 79 then checks if userRoles contains "EMPLOYEE" - but this will never be reached when the user has only the EMPLOYEE role, since line 77 would have already handled it. This means employees will incorrectly receive ALL appointments instead of just their assigned ones.
The correct logic should be:
if (userRoles.contains("ADMIN")) {
appointments = appointmentRepository.findAll();
} else if (userRoles.contains("EMPLOYEE")) {
appointments = appointmentRepository.findByAssignedEmployeeIdAndRequestedDateTimeBetween(
userId, LocalDateTime.now().minusYears(1), LocalDateTime.now().plusYears(1));
} else {
appointments = appointmentRepository.findByCustomerIdOrderByRequestedDateTimeDesc(userId);
}| public static final String EMPLOYEE_2_ID = "employee"; | ||
| public static final String EMPLOYEE_3_ID = "employee"; |
There was a problem hiding this comment.
All three EMPLOYEE constants are set to the same value "employee". This appears to be a copy-paste error. EMPLOYEE_2_ID and EMPLOYEE_3_ID should have unique values for proper employee identification across the system. This will cause issues when trying to distinguish between different employees for appointment assignments.
| public static final String EMPLOYEE_2_ID = "employee"; | |
| public static final String EMPLOYEE_3_ID = "employee"; | |
| public static final String EMPLOYEE_2_ID = "employee2"; | |
| public static final String EMPLOYEE_3_ID = "employee3"; |
| validateAppointmentDateTime(dto.getRequestedDateTime(), 60); | ||
| appointment.setRequestedDateTime(dto.getRequestedDateTime()); | ||
| String newBayId = findAvailableBay(dto.getRequestedDateTime(), 60); | ||
| appointment.setAssignedBayId(newBayId); |
There was a problem hiding this comment.
Hardcoded duration of 60 minutes is used when updating an appointment, regardless of the actual service type's estimated duration. This should retrieve the service type's duration from the appointment entity or use the original service type's estimatedDurationMinutes to ensure accurate bay availability checking and time slot validation.
| boolean isAvailable = bayAppointments.stream() | ||
| .noneMatch(apt -> { | ||
| LocalDateTime aptStart = apt.getRequestedDateTime(); | ||
| LocalDateTime aptEnd = aptStart.plusMinutes(60); |
There was a problem hiding this comment.
Hardcoded appointment duration of 60 minutes when checking for bay availability conflicts. This doesn't account for the actual duration of existing appointments, which could vary (30 min for oil changes, 180 min for full service, etc.). Should use the actual service type's estimated duration from the appointment or look it up from the service type repository.
| if (maxConfirmationOpt.isPresent() && maxConfirmationOpt.get() != null) { | ||
| String maxConfirmation = maxConfirmationOpt.get(); | ||
| String numberPart = maxConfirmation.substring(prefix.length()); | ||
| nextNumber = Integer.parseInt(numberPart) + 1; |
There was a problem hiding this comment.
Potential uncaught 'java.lang.NumberFormatException'.
No description provided.