Feature/ticket lifecycle update - #12
Conversation
…assignment and status update logic
…r removal and status update logic
… in `TicketService`
…us comparison readability in `TicketService`
…lement `TicketCommentMapper` for DTO transformations
📝 WalkthroughWalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant TicketCommentService
participant TicketRepository
participant TicketCommentRepository
participant TicketCommentMapper
participant Database
rect rgba(100, 150, 255, 0.5)
Note over Client,Database: Add Comment Flow
Client->>TicketCommentService: addComment(ticketId, CommentCreateDTO, author)
TicketCommentService->>TicketRepository: findById(ticketId)
TicketRepository->>Database: SELECT ticket
Database-->>TicketRepository: ticket
TicketRepository-->>TicketCommentService: Ticket
TicketCommentService->>TicketCommentRepository: save(TicketComment)
TicketCommentRepository->>Database: INSERT comment
Database-->>TicketCommentRepository: saved entity
TicketCommentRepository-->>TicketCommentService: TicketComment
TicketCommentService->>TicketCommentMapper: entityToViewDTO(comment)
TicketCommentMapper-->>TicketCommentService: CommentViewDTO
TicketCommentService-->>Client: CommentViewDTO
end
rect rgba(150, 200, 100, 0.5)
Note over Client,Database: Get Comments Flow (with Role-Based Filtering)
Client->>TicketCommentService: getComments(ticketId, actor)
TicketCommentService->>TicketCommentRepository: findByTicketIdOrderByCreatedAtAsc(ticketId)
TicketCommentRepository->>Database: SELECT comments WHERE ticket_id = ? ORDER BY created_at ASC
Database-->>TicketCommentRepository: comments
TicketCommentRepository-->>TicketCommentService: List<TicketComment>
alt actor is REPORTER or actor == null
TicketCommentService->>TicketCommentService: filter out comments where internalNote == true
end
TicketCommentService->>TicketCommentMapper: entityToViewDTO(comment) for each
TicketCommentMapper-->>TicketCommentService: List<CommentViewDTO>
TicketCommentService-->>Client: List<CommentViewDTO>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/TicketCommentService.java`:
- Around line 34-35: Replace the thrown RuntimeException when a ticket is
missing with a ResponseStatusException(HttpStatus.NOT_FOUND, ...) so the service
returns a 404; specifically update the ticketRepository.findById(...) calls in
TicketCommentService (the occurrence using ticketId and the other occurrence
around lines 52-54) to throw new ResponseStatusException(HttpStatus.NOT_FOUND,
"Ticket not found") and add the necessary imports
(org.springframework.web.server.ResponseStatusException and
org.springframework.http.HttpStatus).
- Around line 48-66: The getComments method dereferences lazy
TicketComment.author inside ticketCommentMapper.entityToViewDTO outside a
transaction, which can cause LazyInitializationException; fix by making the
operation transactional (annotate getComments with `@Transactional`(readOnly =
true)) or change ticketCommentRepository.findByTicketIdOrderByCreatedAtAsc to
fetch authors eagerly (use a JPQL fetch join or an EntityGraph that includes
author) so mapper has author available without relying on OSIV.
- Around line 33-45: The code copies dto.isInternalNote() into the entity
without server-side role enforcement in TicketCommentService.addComment; change
the logic so internalNote is only set to dto.isInternalNote() when the invoking
author has the investigator/admin privilege, otherwise force internalNote to
false (do not trust the client). Locate the addComment method and replace the
direct call to comment.setInternalNote(dto.isInternalNote()) with a check
against the author's roles/flags (e.g., author.isAdmin() or
author.hasRole("INVESTIGATOR")) and set comment.setInternalNote(true) only if
that check passes, otherwise set comment.setInternalNote(false). Ensure the rest
of the save flow (ticketCommentRepository.save and
ticketCommentMapper.entityToViewDTO) remains unchanged.
In `@src/main/java/org/example/alfs/services/TicketService.java`:
- Around line 107-108: The code currently throws RuntimeException when a ticket
lookup fails (e.g., the expression using ticketRepository.findById(id)), which
should be replaced with the project's 404 pattern: throw new
ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found"); update every
similar occurrence in TicketService where findById(...) currently
.orElseThrow(() -> new RuntimeException(...)) (including the other noted
occurrences) to throw ResponseStatusException with HttpStatus.NOT_FOUND and an
appropriate message so missing records return 404 instead of 500.
- Around line 99-124: The updateTicketStatus method allows OPEN -> IN_PROGRESS
without an investigator; add a guard before setting status that if oldStatus ==
TicketStatus.OPEN && newStatus == TicketStatus.IN_PROGRESS then require
ticket.getInvestigator() != null and throw an IllegalStateException (with a
clear message instructing to call assignInvestigator or use the assignment
workflow) if null; keep this check near the allowed-transitions logic in
updateTicketStatus and reference existing symbols ticket.getInvestigator(),
assignInvestigator(), unassignInvestigator(), ALLOWED_TRANSITIONS,
ticketRepository, and ticketMapper.
- Around line 164-168: The current assignment retrieves a User via
userRepository.findById(investigatorId) and sets it as the ticket investigator
without checking their role; change the logic in the TicketService method that
does this so after fetching the User you verify they have the Investigator role
(e.g., user.getRole() or user.hasRole(Role.INVESTIGATOR)), and if not, throw a
clear exception (e.g., IllegalArgumentException or a domain-specific exception)
instead of assigning; keep the calls to ticket.setInvestigator(investigator) and
ticket.setStatus(TicketStatus.IN_PROGRESS) only when the role check passes.
- Around line 143-206: assignInvestigator and unassignInvestigator bypass the
ticket state machine by directly setting TicketStatus.IN_PROGRESS and
TicketStatus.OPEN; instead, verify the transition against the existing
ALLOWED_TRANSITIONS (or reuse updateTicketStatus) before changing status: in
assignInvestigator(Long id, Long investigatorId) check that
ALLOWED_TRANSITIONS.get(ticket.getStatus()) contains TicketStatus.IN_PROGRESS
(or call updateTicketStatus(id, TicketStatus.IN_PROGRESS) after assigning the
investigator) and throw an IllegalStateException if not allowed; similarly in
unassignInvestigator(Long id) verify that transitioning to TicketStatus.OPEN is
allowed (or delegate to updateTicketStatus) before nulling investigator and
saving the ticket so transitions cannot be bypassed.
🪄 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: d9c8b46f-7202-402a-ad65-fa095edbeab0
📒 Files selected for processing (4)
src/main/java/org/example/alfs/dto/comment/CommentCreateDTO.javasrc/main/java/org/example/alfs/mapper/TicketCommentMapper.javasrc/main/java/org/example/alfs/services/TicketCommentService.javasrc/main/java/org/example/alfs/services/TicketService.java
…ommentService` and throw `FORBIDDEN` exception for unauthorized roles.
…ent HTTP error handling in `TicketCommentService`.
…tService` for consistency
…ent HTTP error handling in `TicketService`.
…to `IN_PROGRESS` in `TicketService`.
…assignment in `TicketService`.
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/TicketCommentService.java`:
- Around line 64-68: The code in getComments() dereferences actor without
null-check; change the guard to treat a null actor as a reporter by checking
(actor == null || actor.getRole() == Role.REPORTER) before filtering so
anonymous/token access won't NPE and will hide internal notes; update the
conditional that currently uses actor.getRole() and ensure the same branch still
maps via ticketCommentMapper::entityToViewDTO after filtering.
In `@src/main/java/org/example/alfs/services/TicketService.java`:
- Around line 108-129: The three mutation flows (updateTicketStatus,
assignInvestigator, unassignInvestigator) perform read-check-write on the same
Ticket and are race-prone; fix by adding concurrency control: either add an
optimistic lock field to the Ticket entity (a long/int field annotated with
`@Version`) and let JPA detect concurrent updates on ticketRepository.save, or
change the repository access in those methods to fetch the row with a
pessimistic lock (e.g., a new repository method or using EntityManager.find with
LockModeType.PESSIMISTIC_WRITE) so the precondition checks and the subsequent
save happen under a lock; update the code paths in updateTicketStatus,
assignInvestigator, and unassignInvestigator to use the chosen locking strategy
and handle OptimisticLockException / lock-related exceptions appropriately.
- Around line 120-125: Replace thrown
IllegalStateException/IllegalArgumentException in TicketService validation
branches with ResponseStatusException to produce proper 4xx HTTP responses: for
the invalid status transition check in the method handling status changes (the
branch that currently throws "Invalid ticket status transition...") throw new
ResponseStatusException(HttpStatus.BAD_REQUEST, ...); for investigator
assignment violations (the branches that currently throw when assigning an
investigator or when an investigator is already assigned around the
investigator-assignment logic) throw
ResponseStatusException(HttpStatus.BAD_REQUEST) (or HttpStatus.CONFLICT for the
"already assigned" case); and for investigator removal violations (the branch
that currently throws when preventing removal) throw
ResponseStatusException(HttpStatus.BAD_REQUEST). Use descriptive messages and
follow the same pattern already used for 404 ResponseStatusException in this
class.
🪄 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: ec21238a-aadd-42e1-a321-bb3ae936d85e
📒 Files selected for processing (2)
src/main/java/org/example/alfs/services/TicketCommentService.javasrc/main/java/org/example/alfs/services/TicketService.java
| Ticket ticket = ticketRepository.findById(id) | ||
| .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found")); | ||
|
|
||
| TicketStatus oldStatus = ticket.getStatus(); | ||
|
|
||
| // No-op check | ||
| if (oldStatus == newStatus) { | ||
| return ticketMapper.entityToViewDTO(ticket); | ||
| } | ||
|
|
||
| Set<TicketStatus> allowedTransitions = ALLOWED_TRANSITIONS.getOrDefault(ticket.getStatus(), Set.of()); | ||
|
|
||
| if (!allowedTransitions.contains(newStatus)) { | ||
| throw new IllegalStateException("Invalid ticket status transition: Cannot transition from " + ticket.getStatus() + " to " + newStatus); | ||
| } | ||
|
|
||
| if (newStatus == TicketStatus.IN_PROGRESS && ticket.getInvestigator() == null) { | ||
| throw new IllegalStateException("Cannot move ticket to IN_PROGRESS without an assigned investigator"); | ||
| } | ||
|
|
||
| ticket.setStatus(newStatus); | ||
| Ticket savedTicket = ticketRepository.save(ticket); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Locking/version support ==="
rg -n -C2 '@Version|@Lock|LockModeType|version\b' \
src/main/java/org/example/alfs/entities \
src/main/java/org/example/alfs/repositories
echo
echo "=== Ticket mutation methods ==="
rg -n -C3 'updateTicketStatus|assignInvestigator|unassignInvestigator' \
src/main/java/org/example/alfs/services/TicketService.javaRepository: ithsjava25/project-backend-alfs
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Ticket entity structure ==="
fd 'Ticket\.java$' src/main/java/org/example/alfs/entities --type f | head -5 | xargs -I {} sh -c 'echo "File: {}"; cat -n "{}"'
echo
echo "=== TicketRepository ==="
fd 'TicketRepository\.java$' src/main/java/org/example/alfs/repositories --type f | head -5 | xargs -I {} sh -c 'echo "File: {}"; cat -n "{}"'
echo
echo "=== Full TicketService.updateTicketStatus method ==="
rg -n -A 20 'public.*updateTicketStatus' src/main/java/org/example/alfs/services/TicketService.java
echo
echo "=== Full TicketService.assignInvestigator method ==="
rg -n -A 20 'public.*assignInvestigator' src/main/java/org/example/alfs/services/TicketService.java
echo
echo "=== Full TicketService.unassignInvestigator method ==="
rg -n -A 20 'public.*unassignInvestigator' src/main/java/org/example/alfs/services/TicketService.javaRepository: ithsjava25/project-backend-alfs
Length of output: 8064
These ticket mutations are race-prone and need concurrency control.
All three mutation methods (updateTicketStatus, assignInvestigator, unassignInvestigator) perform read-check-write sequences on the same Ticket row without concurrency guards. Two concurrent requests can both pass precondition checks and then overwrite each other—last-write-wins. For example, two calls to assignInvestigator() can both observe ticket.getInvestigator() == null, both pass the guard at line 159, and then both attempt to save, with the second request overwriting the first.
The Ticket entity has no @Version field for optimistic locking, and TicketRepository exposes no locked query methods, so there is currently no mechanism to prevent these collisions.
Add optimistic locking (@Version on Ticket) or use pessimistic locking when fetching the row in these mutation flows.
🤖 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 108
- 129, The three mutation flows (updateTicketStatus, assignInvestigator,
unassignInvestigator) perform read-check-write on the same Ticket and are
race-prone; fix by adding concurrency control: either add an optimistic lock
field to the Ticket entity (a long/int field annotated with `@Version`) and let
JPA detect concurrent updates on ticketRepository.save, or change the repository
access in those methods to fetch the row with a pessimistic lock (e.g., a new
repository method or using EntityManager.find with
LockModeType.PESSIMISTIC_WRITE) so the precondition checks and the subsequent
save happen under a lock; update the code paths in updateTicketStatus,
assignInvestigator, and unassignInvestigator to use the chosen locking strategy
and handle OptimisticLockException / lock-related exceptions appropriately.
… (anonymous reporter) when filtering comments.
…ResponseStatusException` for consistent HTTP error handling in `TicketService`.
…t in `TicketService` to include current status in error responses.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/main/java/org/example/alfs/services/TicketService.java (2)
99-100:⚠️ Potential issue | 🔴 CriticalThese state mutations still need concurrency control.
All three methods do a read-check-write on the same
Ticketrow. Two concurrent requests can both pass the guards and then overwrite each other, e.g. dual assignments or a status change racing with unassignment.@Transactionaldoes not prevent that by itself; this needs optimistic locking (@VersiononTicket) or a locked fetch for these mutation paths.Also applies to: 147-148, 185-186
🤖 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 99 - 100, The three read-check-write methods (including updateTicketStatus and the other two mutation methods that perform guard-then-write) need concurrency control to avoid lost updates; either add optimistic locking to the Ticket entity by adding a `@Version` field (e.g., private Long version) and handle OptimisticLockingFailureException/retry in the service methods, or change the service to fetch the Ticket with a database lock (e.g., repository methods annotated with `@Lock`(PESSIMISTIC_WRITE) or using entityManager.find(..., LockModeType.PESSIMISTIC_WRITE)) before performing the guard checks and persisting changes; apply the chosen approach consistently to updateTicketStatus and the other two mutation methods and ensure failures are surfaced or retried appropriately.
124-125:⚠️ Potential issue | 🟠 MajorReturn a 4xx here instead of
IllegalStateException.This branch is still outside the service's
ResponseStatusExceptionpattern, so a missing investigator duringOPEN -> IN_PROGRESSwill likely bubble up as a 500 instead of a client error.Suggested fix
- if (newStatus == TicketStatus.IN_PROGRESS && ticket.getInvestigator() == null) { - throw new IllegalStateException("Cannot move ticket to IN_PROGRESS without an assigned investigator"); - } + if (newStatus == TicketStatus.IN_PROGRESS && ticket.getInvestigator() == null) { + throw new ResponseStatusException( + HttpStatus.BAD_REQUEST, + "Cannot move ticket to IN_PROGRESS without an assigned investigator"); + }🤖 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 124 - 125, In TicketService replace the IllegalStateException thrown when newStatus == TicketStatus.IN_PROGRESS and ticket.getInvestigator() == null with throwing a client HTTP exception (e.g., new ResponseStatusException(HttpStatus.BAD_REQUEST, "...")) so the controller returns a 4xx rather than a 500; update imports if needed to reference org.springframework.web.server.ResponseStatusException and keep the same check using newStatus and ticket.getInvestigator() to locate the spot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/example/alfs/services/TicketService.java`:
- Around line 99-100: The three read-check-write methods (including
updateTicketStatus and the other two mutation methods that perform
guard-then-write) need concurrency control to avoid lost updates; either add
optimistic locking to the Ticket entity by adding a `@Version` field (e.g.,
private Long version) and handle OptimisticLockingFailureException/retry in the
service methods, or change the service to fetch the Ticket with a database lock
(e.g., repository methods annotated with `@Lock`(PESSIMISTIC_WRITE) or using
entityManager.find(..., LockModeType.PESSIMISTIC_WRITE)) before performing the
guard checks and persisting changes; apply the chosen approach consistently to
updateTicketStatus and the other two mutation methods and ensure failures are
surfaced or retried appropriately.
- Around line 124-125: In TicketService replace the IllegalStateException thrown
when newStatus == TicketStatus.IN_PROGRESS and ticket.getInvestigator() == null
with throwing a client HTTP exception (e.g., new
ResponseStatusException(HttpStatus.BAD_REQUEST, "...")) so the controller
returns a 4xx rather than a 500; update imports if needed to reference
org.springframework.web.server.ResponseStatusException and keep the same check
using newStatus and ticket.getInvestigator() to locate the spot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77936399-54b6-429c-b80d-9f54aee7e279
📒 Files selected for processing (2)
src/main/java/org/example/alfs/services/TicketCommentService.javasrc/main/java/org/example/alfs/services/TicketService.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/example/alfs/services/TicketCommentService.java
…nsistent HTTP error handling in `TicketService`.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/example/alfs/services/TicketService.java (1)
108-129:⚠️ Potential issue | 🔴 CriticalThese lifecycle mutations are still last-write-wins.
@Transactionaldoes not serialize these read-check-write paths. Two concurrentassignInvestigator()calls can both pass the null-assignee guard, and the later save overwrites the first; the same applies to status updates racing with assignment or unassignment. Please add optimistic locking onTicket(@Version) or fetch the row with a write lock before relying on these guards.Verification script
#!/bin/bash set -e ticket_file="$(fd 'Ticket\.java$' src/main/java --type f | head -n 1)" repo_file="$(fd 'TicketRepository\.java$' src/main/java --type f | head -n 1)" echo "=== Ticket entity locking markers ===" rg -n -C2 '@Version|class Ticket|status|investigator' "$ticket_file" echo echo "=== TicketRepository locking APIs ===" rg -n -C2 '@Lock|PESSIMISTIC|OPTIMISTIC|findById' "$repo_file" echo echo "=== TicketService lifecycle mutations ===" sed -n '99,214p' src/main/java/org/example/alfs/services/TicketService.java | cat -nExpected result: either a
@Versionfield onTicketor a locked fetch path inTicketRepository. If neither exists, these guards remain race-prone.Also applies to: 156-176, 194-207
🤖 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 108 - 129, The guards in TicketService (e.g., the read-check-write paths around TicketRepository.findById(...) used in status updates and assignInvestigator/unassign flows) are vulnerable to races; add optimistic locking to the Ticket entity by introducing a `@Version` field on class Ticket or, alternatively, change the repository fetches used by methods like assignInvestigator() and the status update path to acquire a write lock (e.g., use a `@Lock`(PESSIMISTIC_WRITE) or a repository method that fetches with a FOR UPDATE-style lock) and handle OptimisticLockException/lock timeouts by retrying or returning an appropriate error; ensure the checks that inspect ticket.getStatus() and ticket.getInvestigator() are done after the locked/versioned fetch and before ticketRepository.save(ticket) so concurrent mutations cannot bypass the guards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/example/alfs/services/TicketService.java`:
- Around line 108-129: The guards in TicketService (e.g., the read-check-write
paths around TicketRepository.findById(...) used in status updates and
assignInvestigator/unassign flows) are vulnerable to races; add optimistic
locking to the Ticket entity by introducing a `@Version` field on class Ticket or,
alternatively, change the repository fetches used by methods like
assignInvestigator() and the status update path to acquire a write lock (e.g.,
use a `@Lock`(PESSIMISTIC_WRITE) or a repository method that fetches with a FOR
UPDATE-style lock) and handle OptimisticLockException/lock timeouts by retrying
or returning an appropriate error; ensure the checks that inspect
ticket.getStatus() and ticket.getInvestigator() are done after the
locked/versioned fetch and before ticketRepository.save(ticket) so concurrent
mutations cannot bypass the guards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e81137c-7b42-4fb9-991c-d9c9afe13562
📒 Files selected for processing (1)
src/main/java/org/example/alfs/services/TicketService.java
addee1
left a comment
There was a problem hiding this comment.
Looks good! Ready to merge
Adds ticket lifecycle service methods:
ALLOWED_TRANSITIONSmapTicketCommentServicewith internal note filtering by roleAudit log calls and role/security checks are noted as TODOs pending
AuditLogServiceand security context.Summary by CodeRabbit