feat: add manual admin assignment for PENDING tickets - #65
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds the capability to assign tickets to admin users. It introduces a new PATCH endpoint for ticket assignment, adds database persistence to track assigned admins, includes request/response DTOs, implements service-layer assignment logic with validation, and extends authorization for the sync notifications endpoint to admin users. Changes
Sequence DiagramsequenceDiagram
participant Client
participant TicketController
participant TicketService
participant Database
participant ProfileService
participant NotificationService
Client->>TicketController: PATCH /api/tickets/{id}/assign<br/>AssignTicketRequest(adminId)
TicketController->>TicketService: assignAdmin(ticketId, request)
TicketService->>Database: Load Ticket by ID
Database-->>TicketService: Ticket entity
TicketService->>TicketService: Validate ticket status == PENDING
TicketService->>TicketService: Determine target adminId<br/>(from request or current user)
TicketService->>ProfileService: Load admin Profile
ProfileService-->>TicketService: Profile with role
TicketService->>TicketService: Validate role == ADMIN
TicketService->>Database: Persist ticket with assignedAdminId
Database-->>TicketService: Updated Ticket
TicketService->>NotificationService: Create notification for admin
TicketService->>TicketService: Resolve admin name & build response
TicketService-->>TicketController: TicketResponse with assigned admin info
TicketController-->>Client: ApiResponse<TicketResponse><br/>"Admin assigned"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/resources/db/migration/V13__add_ticket_assigned_admin.sql (1)
1-2: Add an index forassigned_admin_idto avoid future FK lookup slowdowns.Line [2] adds a foreign key but no supporting index. This can hurt assignment-related reads and FK maintenance on larger tables.
Suggested migration addition
ALTER TABLE ticket ADD COLUMN assigned_admin_id UUID REFERENCES profile(auth_user_id); + +CREATE INDEX idx_ticket_assigned_admin_id + ON ticket(assigned_admin_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V13__add_ticket_assigned_admin.sql` around lines 1 - 2, The migration adds ticket.assigned_admin_id but omits an index; update V13__add_ticket_assigned_admin.sql to create a supporting index on the assigned_admin_id column (e.g., a non-unique index named like idx_ticket_assigned_admin_id) so FK lookups and assignment queries are fast; ensure the index creation statement is added after the ADD COLUMN/REFERENCES statement and, if running in production with minimal locking, consider using CREATE INDEX CONCURRENTLY for the index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/ucms_backend/service/TicketService.java`:
- Around line 351-380: The assignAdmin method updates and saves the Ticket but
never publishes a realtime event; after saving the ticket (Ticket saved =
ticketRepository.save(ticket)) invoke publishTicketEvent(saved) (or the existing
publishTicketEvent method used by other mutations) so the SSE/WS subscribers
receive the assignment update, and keep the existing
notificationService.createNotification and return toDetailedResponse(saved) as
before.
- Around line 75-99: The code currently calls resolveAdminName(UUID) inside
toResponse/toDetailedResponse for each ticket, causing N+1 queries; instead,
collect all assignedAdminId values for a ticket list, query
profileRepository.findAllById(...) once to build a Map<UUID,String> of
adminId->name, and change the mappers to accept that map (e.g., add overloads
toResponse(Ticket, Set<Long>, Map<UUID,String>) and toDetailedResponse(Ticket,
Map<UUID,String>) or modify resolveAdminName to accept the map) so the mapping
logic uses map.get(adminId) rather than profileRepository.findById per ticket;
ensure callers that build lists compute the map and pass it into the mapper
methods.
---
Nitpick comments:
In `@src/main/resources/db/migration/V13__add_ticket_assigned_admin.sql`:
- Around line 1-2: The migration adds ticket.assigned_admin_id but omits an
index; update V13__add_ticket_assigned_admin.sql to create a supporting index on
the assigned_admin_id column (e.g., a non-unique index named like
idx_ticket_assigned_admin_id) so FK lookups and assignment queries are fast;
ensure the index creation statement is added after the ADD COLUMN/REFERENCES
statement and, if running in production with minimal locking, consider using
CREATE INDEX CONCURRENTLY for the index.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39509c4d-41f7-4c48-bb8d-5fd4bd367ea4
📒 Files selected for processing (6)
src/main/java/com/ucms_backend/controller/TicketController.javasrc/main/java/com/ucms_backend/dto/AssignTicketRequest.javasrc/main/java/com/ucms_backend/dto/TicketResponse.javasrc/main/java/com/ucms_backend/model/entity/Ticket.javasrc/main/java/com/ucms_backend/service/TicketService.javasrc/main/resources/db/migration/V13__add_ticket_assigned_admin.sql
Comment on lines
+75
to
99
| private String resolveAdminName(UUID adminId) { | ||
| if (adminId == null) return null; | ||
| return profileRepository.findById(adminId) | ||
| .map(Profile::getName) | ||
| .orElse(null); | ||
| } | ||
|
|
||
| private TicketResponse toResponse(Ticket ticket) { | ||
| boolean hasAdminResponse = ticketResponseRepository.existsByTicketId(ticket.getId()); | ||
| return TicketResponse.from(ticket, resolveCategoryName(ticket.getCategoryId()), null, hasAdminResponse); | ||
| return TicketResponse.from(ticket, resolveCategoryName(ticket.getCategoryId()), null, hasAdminResponse, | ||
| resolveAdminName(ticket.getAssignedAdminId())); | ||
| } | ||
|
|
||
| private TicketResponse toResponse(Ticket ticket, Set<Long> ticketIdsWithResponses) { | ||
| boolean hasAdminResponse = ticketIdsWithResponses.contains(ticket.getId()); | ||
| return TicketResponse.from(ticket, resolveCategoryName(ticket.getCategoryId()), null, hasAdminResponse); | ||
| return TicketResponse.from(ticket, resolveCategoryName(ticket.getCategoryId()), null, hasAdminResponse, | ||
| resolveAdminName(ticket.getAssignedAdminId())); | ||
| } | ||
|
|
||
| private TicketResponse toDetailedResponse(Ticket ticket) { | ||
| Profile studentProfile = profileRepository.findById(ticket.getUserId()).orElse(null); | ||
| boolean hasAdminResponse = ticketResponseRepository.existsByTicketId(ticket.getId()); | ||
| return TicketResponse.from(ticket, resolveCategoryName(ticket.getCategoryId()), studentProfile, hasAdminResponse); | ||
| return TicketResponse.from(ticket, resolveCategoryName(ticket.getCategoryId()), studentProfile, hasAdminResponse, | ||
| resolveAdminName(ticket.getAssignedAdminId())); | ||
| } |
There was a problem hiding this comment.
Avoid N+1 profile queries when populating assignedAdminName.
Lines [75-79] are called from list mapping paths (Lines [84-85], [90-91], [97-98]), causing one profileRepository.findById(...) per ticket. This will degrade admin ticket-list performance.
Refactor direction (batch-resolve admin names)
+// Build once per list request
+private Map<UUID, String> resolveAdminNames(List<Ticket> tickets) {
+ Set<UUID> ids = tickets.stream()
+ .map(Ticket::getAssignedAdminId)
+ .filter(java.util.Objects::nonNull)
+ .collect(java.util.stream.Collectors.toSet());
+ if (ids.isEmpty()) return Map.of();
+ return profileRepository.findAllById(ids).stream()
+ .collect(java.util.stream.Collectors.toMap(Profile::getAuthUserId, Profile::getName));
+}Then pass the map into list mappers instead of calling findById per ticket.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/ucms_backend/service/TicketService.java` around lines 75 -
99, The code currently calls resolveAdminName(UUID) inside
toResponse/toDetailedResponse for each ticket, causing N+1 queries; instead,
collect all assignedAdminId values for a ticket list, query
profileRepository.findAllById(...) once to build a Map<UUID,String> of
adminId->name, and change the mappers to accept that map (e.g., add overloads
toResponse(Ticket, Set<Long>, Map<UUID,String>) and toDetailedResponse(Ticket,
Map<UUID,String>) or modify resolveAdminName to accept the map) so the mapping
logic uses map.get(adminId) rather than profileRepository.findById per ticket;
ensure callers that build lists compute the map and pass it into the mapper
methods.
Comment on lines
+351
to
+380
| public TicketResponse assignAdmin(Long ticketId, AssignTicketRequest request) { | ||
| Ticket ticket = ticketRepository.findById(ticketId) | ||
| .orElseThrow(() -> new AppException(404, "TICKET_NOT_FOUND", "Ticket not found")); | ||
|
|
||
| if (ticket.getStatus() != TicketStatus.PENDING) { | ||
| throw new AppException(409, "INVALID_ASSIGNMENT", "Can only assign admin to PENDING tickets"); | ||
| } | ||
|
|
||
| UUID targetAdminId = (request.getAdminId() != null) | ||
| ? request.getAdminId() | ||
| : SecurityUtils.getCurrentUserId(); | ||
|
|
||
| Profile admin = profileRepository.findById(targetAdminId) | ||
| .orElseThrow(() -> new AppException(404, "ADMIN_NOT_FOUND", "Admin not found")); | ||
|
|
||
| if (!"ADMIN".equals(admin.getRole())) { | ||
| throw new AppException(400, "INVALID_ADMIN", "Target user is not an admin"); | ||
| } | ||
|
|
||
| ticket.setAssignedAdminId(targetAdminId); | ||
| Ticket saved = ticketRepository.save(ticket); | ||
|
|
||
| notificationService.createNotification( | ||
| targetAdminId, | ||
| saved.getId(), | ||
| "You have been assigned ticket #" + saved.getTicketNumber() | ||
| ); | ||
|
|
||
| return toDetailedResponse(saved); | ||
| } |
There was a problem hiding this comment.
Publish a realtime ticket event after admin assignment.
assignAdmin(...) mutates ticket state (Line [370]) but does not call publishTicketEvent(...), unlike other ticket mutations. Clients relying on SSE may not see assignment updates until refresh.
Suggested fix
ticket.setAssignedAdminId(targetAdminId);
Ticket saved = ticketRepository.save(ticket);
+ publishTicketEvent(saved, "TICKET_ASSIGNED", "ADMIN");
notificationService.createNotification(
targetAdminId,
saved.getId(),
"You have been assigned ticket #" + saved.getTicketNumber()
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/ucms_backend/service/TicketService.java` around lines 351 -
380, The assignAdmin method updates and saves the Ticket but never publishes a
realtime event; after saving the ticket (Ticket saved =
ticketRepository.save(ticket)) invoke publishTicketEvent(saved) (or the existing
publishTicketEvent method used by other mutations) so the SSE/WS subscribers
receive the assignment update, and keep the existing
notificationService.createNotification and return toDetailedResponse(saved) as
before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of Change
Labels
p2-mediumfeatbackendWhat Changed
assigned_admin_id(nullable UUID FK) column to thetickettable via Flyway migrationV13assignedAdminIdfield toTicketentityAssignTicketRequestDTO with optionaladminIdfieldassignedAdminIdandassignedAdminNamefields toTicketResponseassignAdmin()service method inTicketServicewith validationPATCH /api/tickets/{id}/assignendpoint (ADMIN only) inTicketControllerWhy
Admins had no way to indicate ownership of a PENDING ticket. All admins received broadcast notifications but no one was formally assigned. This adds a lightweight assignment mechanism — purely informational, no access restrictions — so admins can coordinate who handles which ticket.
How to Test
assignedAdminIdisnullin responsePATCH /api/tickets/{id}/assignas ADMIN with no body ({}) → ticket is self-assigned to the authenticated admin; response showsassignedAdminIdandassignedAdminNamePATCH /api/tickets/{id}/assignas ADMIN with{ "adminId": "<uuid>" }→ ticket is assigned to the specified admin; that admin receives a notification409 INVALID_ASSIGNMENTadminIdbelonging to a STUDENT profile → expect400 INVALID_ADMINadminId→ expect404 ADMIN_NOT_FOUNDRelated Issues
Screenshots
N/A
Checklist
ApiResponse<T>wrapperapplication.yamlis NOT stagedSummary by CodeRabbit
New Features
Improvements