Add ticket comment functionality with access control - #19
Merged
Conversation
…ing in TicketCommentService
📝 WalkthroughWalkthroughAdded a new Spring MVC controller exposing POST and GET endpoints for ticket comments and integrated explicit role-based access checks and internal-note permission enforcement in TicketCommentService. User lookup is handled with nullable resolution for unauthenticated cases. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Controller as TicketCommentController
participant Utils as SecurityUtils
participant Service as TicketCommentService
participant Repo as Repository
rect rgba(100, 150, 200, 0.5)
Note over Client,Repo: Create Comment
Client->>Controller: POST /tickets/{ticketId}/comments (CommentCreateDTO)
Controller->>Utils: resolve current user
Utils-->>Controller: User or null
Controller->>Service: addComment(ticketId, dto, user)
Service->>Repo: find ticket by id
Service->>Service: checkAccess(ticket, user)
Service->>Service: checkInternalNotePermission(dto.internalNote, user)
alt Access allowed
Service->>Repo: persist comment
Service-->>Controller: success
else Access denied
Service-->>Controller: throws 401/403
end
Controller-->>Client: redirect to ticket view
end
sequenceDiagram
participant Client
participant Controller as TicketCommentController
participant Utils as SecurityUtils
participant Service as TicketCommentService
participant Repo as Repository
rect rgba(150, 100, 200, 0.5)
Note over Client,Repo: Retrieve Comments
Client->>Controller: GET /tickets/{ticketId}/comments
Controller->>Utils: resolve current user
Utils-->>Controller: User or null
Controller->>Service: getComments(ticketId, user)
Service->>Repo: find ticket by id
alt Ticket found
Service->>Service: checkAccess(ticket, user)
alt Access allowed
Service->>Repo: load comments
Service->>Service: filter internal notes based on role
Service-->>Controller: List<CommentViewDTO>
else Access denied
Service-->>Controller: throws 401/403
end
else Ticket not found
Service-->>Controller: throws 404
end
Controller-->>Client: JSON response or error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
🧹 Nitpick comments (1)
src/main/java/org/example/alfs/services/TicketCommentService.java (1)
46-49: Dead code:author == nullcan never be true here.After
checkAccess(ticket, author)at line 39 passes without throwing,authoris guaranteed non-null (checkAccess throws on null). Theauthor == nullcheck is unreachable and can be removed.Proposed simplification
boolean internalNote = dto.isInternalNote(); - if (internalNote && (author == null || (author.getRole() != Role.ADMIN && author.getRole() != Role.INVESTIGATOR))) { + if (internalNote && author.getRole() != Role.ADMIN && author.getRole() != Role.INVESTIGATOR) { throw new ResponseStatusException(HttpStatus.FORBIDDEN, "Only investigators/admins can create internal notes"); }🤖 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/TicketCommentService.java` around lines 46 - 49, Remove the unreachable null check from the internal-note authorization branch: since checkAccess(ticket, author) already throws on a null author, simplify the condition in the block that uses dto.isInternalNote() to only validate roles (author.getRole() != Role.ADMIN && author.getRole() != Role.INVESTIGATOR) and throw the same ResponseStatusException when the role check fails; update the if in TicketCommentService (the boolean internalNote block) to drop the "author == null" predicate so only role checks remain.
🤖 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/controllers/TicketCommentController.java`:
- Around line 48-54: The getCurrentUserOrNull method currently swallows all
Exceptions; change it to only handle expected authentication failures from
SecurityUtils.getCurrentUser() by catching RuntimeException (or the specific
exception type thrown) and inspecting the message to determine an auth failure,
returning null in that case, but rethrowing or letting other unexpected
exceptions propagate; follow the TicketService pattern used elsewhere (inspect
SecurityUtils.getCurrentUser() messages) and update getCurrentUserOrNull
accordingly so real errors (e.g., DB failures) are not masked.
- Around line 31-32: The inline comment in TicketCommentController next to
getCurrentUserOrNull() is misleading because TicketCommentService.checkAccess()
currently forbids null users; update the comment to reflect that anonymous users
are not allowed (or if anonymous commenting is intended, add a TODO noting that
the feature must be implemented and the check in
TicketCommentService.checkAccess() relaxed/handled later). Reference
getCurrentUserOrNull() and TicketCommentService.checkAccess() in the updated
comment so future readers understand the current restriction and any deferred
work.
In `@src/main/java/org/example/alfs/services/TicketCommentService.java`:
- Around line 82-87: The checkAccess method in TicketCommentService currently
throws ResponseStatusException with HttpStatus.FORBIDDEN when user is null;
change this to throw HttpStatus.UNAUTHORIZED to correctly signal unauthenticated
requests (i.e., replace the FORBIDDEN response with UNAUTHORIZED in
checkAccess), mirroring the behavior in TicketService.requireCurrentUser to keep
authentication vs authorization semantics consistent.
---
Nitpick comments:
In `@src/main/java/org/example/alfs/services/TicketCommentService.java`:
- Around line 46-49: Remove the unreachable null check from the internal-note
authorization branch: since checkAccess(ticket, author) already throws on a null
author, simplify the condition in the block that uses dto.isInternalNote() to
only validate roles (author.getRole() != Role.ADMIN && author.getRole() !=
Role.INVESTIGATOR) and throw the same ResponseStatusException when the role
check fails; update the if in TicketCommentService (the boolean internalNote
block) to drop the "author == null" predicate so only role checks remain.
🪄 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: 729e5a01-ef2a-41a9-9c75-3b037847c968
📒 Files selected for processing (2)
src/main/java/org/example/alfs/controllers/TicketCommentController.javasrc/main/java/org/example/alfs/services/TicketCommentService.java
- Improve exception handling in controller - Clarify anonymous behavior in comments - Return 401 instead of 403 for unauthenticated users - Keep role-based access rules intact
…heck into a helper method for improved readability.
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/example/alfs/services/TicketCommentService.java (1)
62-68: Push reporter filtering into the repository query.At Line 62 you load all comments, then filter internal notes in-memory at Line 64. For long threads, this does unnecessary reads/mapping. Consider querying non-internal comments directly for reporters.
💡 Suggested refactor
- List<TicketComment> all = ticketCommentRepository.findByTicketIdOrderByCreatedAtAsc(ticketId); - - if (actor.getRole() == Role.REPORTER) { - return all.stream() - .filter(comment -> !comment.isInternalNote()) - .map(ticketCommentMapper::entityToViewDTO) - .toList(); - } + List<TicketComment> all = (actor.getRole() == Role.REPORTER) + ? ticketCommentRepository.findByTicketIdAndInternalNoteFalseOrderByCreatedAtAsc(ticketId) + : ticketCommentRepository.findByTicketIdOrderByCreatedAtAsc(ticketId);Also add a repository method:
List<TicketComment> findByTicketIdAndInternalNoteFalseOrderByCreatedAtAsc(Long ticketId);🤖 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/TicketCommentService.java` around lines 62 - 68, Currently the service loads all comments via ticketCommentRepository.findByTicketIdOrderByCreatedAtAsc and then filters internal notes in-memory when actor.getRole() == Role.REPORTER; instead add a repository query List<TicketComment> findByTicketIdAndInternalNoteFalseOrderByCreatedAtAsc(Long ticketId) and, inside the branch that checks actor.getRole() == Role.REPORTER in TicketCommentService, call that new method instead of findByTicketIdOrderByCreatedAtAsc so you only map non-internal comments with ticketCommentMapper::entityToViewDTO.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/example/alfs/services/TicketCommentService.java`:
- Around line 62-68: Currently the service loads all comments via
ticketCommentRepository.findByTicketIdOrderByCreatedAtAsc and then filters
internal notes in-memory when actor.getRole() == Role.REPORTER; instead add a
repository query List<TicketComment>
findByTicketIdAndInternalNoteFalseOrderByCreatedAtAsc(Long ticketId) and, inside
the branch that checks actor.getRole() == Role.REPORTER in TicketCommentService,
call that new method instead of findByTicketIdOrderByCreatedAtAsc so you only
map non-internal comments with ticketCommentMapper::entityToViewDTO.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e8d5e9d-e6da-4297-8abf-b5dc7ea7cdf7
📒 Files selected for processing (2)
src/main/java/org/example/alfs/controllers/TicketCommentController.javasrc/main/java/org/example/alfs/services/TicketCommentService.java
…ltering of internal notes to the repository for improved clarity and performance.
This was referenced Apr 17, 2026
Merged
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
This PR adds support for ticket comments, including creation and retrieval of comments.
Features
Internal Notes
Security
Testing
Summary by CodeRabbit
New Features
Bug Fixes / Security