Conversation
- Implemented new endpoints in ProjectController for project requests, listing, and quote management. - Added ApiResponse DTO for standardized API responses. - Introduced ProgressUpdateDto, ProjectRequestDto, ProjectResponseDto, QuoteDto, and RejectionDto for request handling. - Enhanced ProjectService and ProjectServiceImpl with new methods for project management. - Added exception handling for project-related operations.
Backend backup
…ptionHandler for access denial handling
feat: Enhance role-based access control for project and service listings
…ying Project Service to Kubernetes
Enhance project and service management with new endpoints and DTOs
There was a problem hiding this comment.
Pull Request Overview
This PR implements a complete transformation of the Project Service from stub implementations to a fully functional microservice managing both standard services (from appointments) and custom vehicle modification projects. The implementation includes invoice generation, file uploads, service notes, progress tracking, and comprehensive role-based access control.
Key Changes:
- Implemented all business logic for service and project management (previously were stubs)
- Added complete invoice generation system with line items and tax calculation
- Implemented file storage service for progress photos with security controls
- Created comprehensive data seeder with test data for development
Reviewed Changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| StandardServiceServiceImpl.java | Implemented all 11 service methods with complete business logic including invoice generation |
| ProjectServiceImpl.java | Completed implementation of all project management methods with workflow validation |
| FileStorageServiceImpl.java | New file storage service with path traversal prevention and multi-file support |
| ServiceController.java | Enhanced from stubs to fully functional REST endpoints with proper error handling |
| ProjectController.java | Completed controller implementation with validation and access control |
| DataSeeder.java | New comprehensive data seeder with services, projects, notes, photos, and invoices |
| Multiple DTOs | Created 9 new DTOs for request/response handling |
| Multiple Entities | Created 7 new entities (ServiceNote, ProgressPhoto, Invoice, InvoiceItem, Quote, etc.) |
| Multiple Repositories | Added 4 new repositories with custom query methods |
| Exception classes | Added 5 custom exception classes for proper error handling |
| GlobalExceptionHandler.java | Comprehensive exception handler with proper HTTP status codes |
| GatewayHeaderFilter.java | Enhanced to handle SUPER_ADMIN role mapping |
| OpenApiConfig.java | New Swagger/OpenAPI configuration |
| Test configuration | Added H2 test database configuration |
| Documentation | Extensive updates to README, new QUICK_START and IMPLEMENTATION_SUMMARY |
| CI/CD | Added GitHub Actions workflows for build and deployment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private static final String CUSTOMER_1_ID = "customer"; | ||
| private static final String CUSTOMER_2_ID = "testuser"; | ||
| private static final String EMPLOYEE_1_ID = "employee"; | ||
| private static final String EMPLOYEE_2_ID = "employee"; |
There was a problem hiding this comment.
EMPLOYEE_1_ID and EMPLOYEE_2_ID both have the same value 'employee'. This will cause both employees to appear as the same user in the seeded data, which could lead to confusion during testing and development.
| private static final String EMPLOYEE_2_ID = "employee"; | |
| private static final String EMPLOYEE_2_ID = "employee2"; |
| .build(); | ||
| invoice.getItems().add(additionalItem); | ||
|
|
||
| subtotal = subtotal.add(itemDto.getAmount()); |
There was a problem hiding this comment.
The subtotal is being recalculated inside the loop but the initial value was already set from dto.getActualCost(). This means the invoice's subtotal will include the actualCost twice: once from line 291 and again when recalculated here. The subtotal should start from the actualCost before the loop, or be reset before recalculating.
|
|
||
| @Override | ||
| public String storeFile(MultipartFile file, String serviceId) { | ||
| String originalFilename = StringUtils.cleanPath(file.getOriginalFilename()); |
There was a problem hiding this comment.
Potential NullPointerException if file.getOriginalFilename() returns null. The method should check for null before calling StringUtils.cleanPath().
| Project project = projectService.getProjectDetails(projectId, userId, userRoles) | ||
| .orElseThrow(() -> new RuntimeException("Project not found or access denied")); |
There was a problem hiding this comment.
Throwing generic RuntimeException instead of the custom ProjectNotFoundException that exists in the codebase. This bypasses the GlobalExceptionHandler and returns a 500 status instead of the appropriate 404 or 403 status.
| String invoiceNumber = generateInvoiceNumber(); | ||
|
|
||
| BigDecimal subtotal = dto.getActualCost(); | ||
| BigDecimal taxRate = new BigDecimal("0.15"); // 15% tax |
| # JPA Configuration | ||
| spring.jpa.database-platform=org.hibernate.dialect.H2Dialect | ||
| spring.jpa.hibernate.ddl-auto=create-drop | ||
| spring.jpa.show-sql=false |
There was a problem hiding this comment.
[nitpick] In test configuration, spring.jpa.show-sql is set to false while logging.level.org.hibernate.SQL is set to DEBUG. These settings are contradictory. For test environments, it's typically better to set show-sql=true for debugging test failures.
| com.techtorque.project_service.entity.ServiceStatus statusEnum = | ||
| com.techtorque.project_service.entity.ServiceStatus.valueOf(status.toUpperCase()); |
There was a problem hiding this comment.
[nitpick] Fully qualified class name is unnecessarily verbose. Since ServiceStatus is not ambiguous in this context, add an import statement instead of using the fully qualified name for better readability.
| BigDecimal totalAmount = subtotal.add(taxAmount); | ||
|
|
||
| Invoice invoice = Invoice.builder() | ||
| .invoiceNumber("INV-" + System.currentTimeMillis()) |
There was a problem hiding this comment.
Using System.currentTimeMillis() for invoice number generation could lead to collisions if multiple invoices are generated in quick succession. Consider using the existing generateInvoiceNumber() method from StandardServiceServiceImpl or UUID for uniqueness.
| if (originalFilename.contains("..")) { | ||
| throw new FileStorageException("Filename contains invalid path sequence: " + originalFilename); | ||
| } |
There was a problem hiding this comment.
The path traversal check only looks for '..' but should also validate against absolute paths and other dangerous patterns. Consider using Path.normalize() and checking if the resulting path starts with the expected storage location to prevent directory traversal attacks more comprehensively.
|
|
||
| @OneToMany(mappedBy = "invoice", cascade = CascadeType.ALL, orphanRemoval = true, fetch = FetchType.EAGER) | ||
| @Builder.Default | ||
| private List<InvoiceItem> items = new ArrayList<>(); |
There was a problem hiding this comment.
getItems exposes the internal representation stored in field items. The value may be modified after this call to getItems.
getItems exposes the internal representation stored in field items. The value may be modified after this call to getItems.
getItems exposes the internal representation stored in field items. The value may be modified after this call to getItems.
getItems exposes the internal representation stored in field items. The value may be modified after this call to getItems.
getItems exposes the internal representation stored in field items. The value may be modified after this call to getItems.
getItems exposes the internal representation stored in field items. The value may be modified after this call to getItems.
No description provided.