Implement ticket access control and status workflow - #14
Merged
Conversation
…-control # Conflicts: # src/main/java/org/example/alfs/services/TicketService.java
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMakes controller endpoints role-restricted via Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/org/example/alfs/services/TicketService.java`:
- Around line 191-198: The current transition check still allows RESOLVED ->
IN_PROGRESS because ALLOWED_TRANSITIONS contains that mapping; update the
transition rules so a TicketStatus.RESOLVED cannot move back to IN_PROGRESS
(either by removing RESOLVED -> IN_PROGRESS from the ALLOWED_TRANSITIONS map or
by adding an explicit guard in TicketService around ticket.getStatus()/newStatus
to reject RESOLVED -> IN_PROGRESS), ensuring the logic that throws the
ResponseStatusException continues to run for that case.
- Around line 130-155: The access checks in checkAccess and requireAdmin call
securityUtils.getCurrentUser() directly, which can throw raw RuntimeExceptions
and produce 500s; replace those direct calls with the existing auth-aware helper
(the helper that maps missing principal or missing user to 401/403) everywhere
in checkAccess and requireAdmin so lookup failures produce proper
ResponseStatusException 401/403 responses rather than raw RuntimeExceptions, and
don't catch/convert the helper's exceptions — let them propagate as intended.
- Around line 222-239: In assignInvestigator, validate the incoming
investigatorId parameter before calling userRepository.findById: if
investigatorId is null (or otherwise invalid), throw a
ResponseStatusException(HttpStatus.BAD_REQUEST, "Investigator id is required")
so a 400 is returned instead of letting userRepository.findById produce a server
error; place this check near the start of assignInvestigator (before the
userRepository.findById call) referencing investigatorId and
userRepository.findById.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6edd8c76-2fa7-4345-85c2-6433204d6711
📒 Files selected for processing (2)
src/main/java/org/example/alfs/controllers/TicketController.javasrc/main/java/org/example/alfs/services/TicketService.java
…gn investigator, minor cleanup
- Return 401 instead of 500 when authentication fails - Disallow transition from RESOLVED to IN_PROGRESS
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/example/alfs/services/TicketService.java (1)
239-306: Centralize status transitions for assignment flows.Line 279 and Line 304 mutate ticket status directly instead of reusing transition validation. This can drift from
ALLOWED_TRANSITIONSover time and create inconsistent workflow behavior.If reopening to
OPENis intentional for unassignment, make that transition explicit in one shared transition helper (or map), and routeupdateTicketStatus,assignInvestigator, andunassignInvestigatorthrough the same validator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/services/TicketService.java` around lines 239 - 306, assignInvestigator and unassignInvestigator directly set ticket status (lines where ticket.setStatus is used) which bypasses the shared transition rules; centralize status transitions by routing these flows through the same validator/transition helper used by updateTicketStatus (or introduce a new method like validateAndApplyTransition/updateStatusWithValidation that consults ALLOWED_TRANSITIONS), replace direct ticket.setStatus(TicketStatus.IN_PROGRESS) and ticket.setStatus(TicketStatus.OPEN) with calls to that helper, and ensure the helper throws the same ResponseStatusException on illegal transitions so all status changes use a single source of truth.
🤖 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/org/example/alfs/services/TicketService.java`:
- Around line 158-167: The method requireCurrentUser currently catches all
RuntimeException from securityUtils.getCurrentUser() and maps them to 401;
change this to only catch authentication-related exceptions (e.g.,
AuthenticationCredentialsNotFoundException, BadCredentialsException,
UsernameNotFoundException or your project's specific auth exception) and wrap
those in a ResponseStatusException(HttpStatus.UNAUTHORIZED, ...), while allowing
all other RuntimeExceptions from securityUtils.getCurrentUser() to propagate
unchanged; update the catch clause in requireCurrentUser to list the specific
exception types (or check the exception instance and rethrow if not
auth-related) so only genuine auth failures become 401.
---
Nitpick comments:
In `@src/main/java/org/example/alfs/services/TicketService.java`:
- Around line 239-306: assignInvestigator and unassignInvestigator directly set
ticket status (lines where ticket.setStatus is used) which bypasses the shared
transition rules; centralize status transitions by routing these flows through
the same validator/transition helper used by updateTicketStatus (or introduce a
new method like validateAndApplyTransition/updateStatusWithValidation that
consults ALLOWED_TRANSITIONS), replace direct
ticket.setStatus(TicketStatus.IN_PROGRESS) and
ticket.setStatus(TicketStatus.OPEN) with calls to that helper, and ensure the
helper throws the same ResponseStatusException on illegal transitions so all
status changes use a single source of truth.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7db2f343-b37f-4b17-9ca2-f1e0584ab6b9
📒 Files selected for processing (1)
src/main/java/org/example/alfs/services/TicketService.java
| } | ||
|
|
||
| ticket.setInvestigator(null); | ||
| ticket.setStatus(TicketStatus.OPEN); |
Contributor
There was a problem hiding this comment.
Nitpick! Maybe revert to
Ticket savedTicket = ticketRepository.save(ticket);
return ticketMapper.entityToViewDTO(savedTicket);
to make it consistent with updateTicketStatus and assignInvestigator?
This was referenced Apr 13, 2026
Merged
This was referenced Apr 21, 2026
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
2 participants
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.
Summary
Implemented access control and status update logic for tickets.
Changes
Added role-based access control for viewing tickets
Reporter can only view their own tickets
Investigator can only view and update assigned tickets
Admin has full access
Implemented status update restrictions:
Added service logic for:
Notes
Summary by CodeRabbit
New Features
Security
Behavior