Feature/comment on ticket#139
Merged
Merged
Conversation
…improve UI for ticket details
…amic elements, and refine styling for comment and activity sections
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds comment creation and display to tickets, passes actor identity through services for audit logging, maps audit user IDs to usernames for UI display, expands close-ticket authorization to handlers/assigned users, updates templates and styling, and adds a display-name helper on the AuditAction enum. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Controller as CaseController
participant CommentSvc as CommentService
participant CaseSvc as CaseService
participant UserSvc as UserService
participant AuditSvc as AuditLogService
Note over Browser,Controller: View ticket details
Browser->>Controller: GET /tickets/{id}
Controller->>CaseSvc: fetch ticket, activities
Controller->>UserSvc: findAllByIds(activityUserIds)
Controller-->>Browser: render ticket.html (with auditUserMap, canComment, canClose)
Note over Browser,Controller: Submit comment
Browser->>Controller: POST /tickets/{id}/comments (comment, user)
Controller->>CaseSvc: fetch ticket
Controller->>CommentSvc: createComment(comment, ticket, username)
CommentSvc->>UserSvc: findByUsername(username)
CommentSvc->>AuditSvc: log COMMENT_CREATED (with authorId)
CommentSvc-->>Controller: success
Controller-->>Browser: redirect to /tickets/{id}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Closed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/main/resources/static/style.css (1)
436-454: Unused CSS classes.comment-boxand.comment-author.The template
ticket.htmlrenders comments using.ticket-info(instead of.comment-box) for the wrapper and.ticket-title/.assigned(instead of.comment-author) for the author and timestamp. Either drop these rules or update the template to use them so the styling is intentional and consistent with the new "Comments" UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/style.css` around lines 436 - 454, The CSS defines unused classes .comment-box and .comment-author while the template ticket.html now uses .ticket-info, .ticket-title and .assigned; either remove the dead rules or align the template with the CSS—update ticket.html to replace .ticket-info/.ticket-title/.assigned usages with .comment-box/.comment-author (and .comment-text if needed) to apply the styling, or delete/merge the .comment-box and .comment-author rules from src/main/resources/static/style.css to avoid unused styles.src/main/java/org/example/untitled/usercase/controller/CaseController.java (1)
147-159:getTicketByID(id)is fetched twice incloseTicket.Line 147 already loads the ticket into
ticket; line 159 re-fetches it for the model. Reuse the already-loaded value to save a DB round trip and keep the two views consistent.Proposed fix
- model.addAttribute("ticket", caseService.getTicketByID(id)); + model.addAttribute("ticket", ticket); model.addAttribute("comment", new CreateCommentRequest()); return "close_ticket";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/untitled/usercase/controller/CaseController.java` around lines 147 - 159, The method re-fetches the same ticket twice: CaseController calls caseService.getTicketByID(id) into local variable ticket, then calls caseService.getTicketByID(id) again when adding to the model; change the model.addAttribute call to reuse the already-loaded ticket variable (ticket) instead of calling getTicketByID(id) again so you avoid an extra DB round trip and keep the same object used for authorization and view rendering.src/main/java/org/example/untitled/usercase/AuditAction.java (1)
12-15: Use a fixed locale fortoLowerCase().
String.toLowerCase()is locale-sensitive (e.g. Turkish locale lowercasesItoı), which can produce unexpected display names depending on the JVM's default locale. PassLocale.ROOTfor predictable, technical-string casing.Proposed fix
public String getDisplayName() { - String name = this.name().toLowerCase().replace('_', ' '); + String name = this.name().toLowerCase(java.util.Locale.ROOT).replace('_', ' '); return Character.toUpperCase(name.charAt(0)) + name.substring(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/untitled/usercase/AuditAction.java` around lines 12 - 15, AuditAction.getDisplayName() uses locale-sensitive this.name().toLowerCase(); change it to use a fixed locale by calling toLowerCase(Locale.ROOT) so casing is deterministic across JVM locales, and add the necessary import for java.util.Locale; update the method signature in AuditAction.getDisplayName() to call this.name().toLowerCase(Locale.ROOT) before replacing '_' and capitalizing the first character.
🤖 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/untitled/usercase/controller/CaseController.java`:
- Around line 273-288: When handling validation failure inside CaseController
(the bindingResult.hasErrors() branch), you forgot to populate the model
attribute "canClose", so the ticket view hides the comment/close UI; compute
canClose once (extract or call the existing helper method that determines close
permission, e.g., a method like canCloseCase(caseEntityDto, currentUser) or
computeCanClose(...)) and add model.addAttribute("canClose", canClose) in that
branch before returning "ticket"; ensure you reuse the same helper used
elsewhere in this controller so the logic is identical.
- Around line 289-297: Replace the broad silent catch in CaseController around
the comment creation block with specific handling and logging: in the try/catch
that calls comment.setCaseId(id), caseService.getTicketByID(id) and
commentService.createComment(comment, ticket) catch specific exceptions (e.g.,
IllegalArgumentException, DataAccessException, ResponseStatusException) and log
the exception e with an appropriate logger at the same level and message style
used in assignTicketForm/updateStatusForm, then set a user-facing flash error
that includes a concise reason when available; ensure you still fall back to a
generic error flash for unknown exceptions but log the full stack trace before
returning the redirect to "redirect:/tickets/" + id.
- Line 292: The addComment call is missing the authenticated user: change the
controller's addComment to pass the `@AuthenticationPrincipal` UserDetails (or
username/User) into commentService.createComment(...) and update the
CommentService.createComment(...) signature to accept that user parameter;
inside createComment, stop using caseEntity.getOwner() to set the comment author
and instead set the author to the passed authenticated user (or resolve the User
entity from the username if necessary) so the comment author reflects who
submitted it rather than the ticket owner.
- Around line 91-97: The current auditUserMap construction uses
userService.findById per AuditLog and will throw
ResponseStatusException(NOT_FOUND) if any user is deleted (causing
showTicketDetails and addComment to 404) and causes N+1 queries; add a
UserRepository/UserService method findAllByIds(Collection<Long>) and implement a
private helper buildAuditUserMap(List<AuditLog> auditLogs) that collects
non-null userIds into a Set, batch-fetches users via
userService.findAllByIds(ids), and returns a Map<Long,String> of id→username
(resolve duplicates with (a,b)->a) so missing users are simply absent; update
both places that build auditUserMap (the code block using userService.findById
and the duplicate in addComment) to call buildAuditUserMap, and ensure the
template falls back to a placeholder like "Unknown user" when auditUserMap lacks
an id.
- Around line 264-298: The addComment endpoint currently allows any
authenticated user to post to any ticket; replicate the same ownership/role
authorization used in showTicketDetails and processCloseTicket by checking
whether the current user is the owner/handler/assigned (or using the same helper
if one exists) before calling commentService.createComment: fetch the current
user via the same mechanism used elsewhere (authentication principal /
userService), check the same predicates (isOwner/isHandler/isAssigned) against
CaseEntityDto from caseService.getTicketByID(id), and if the check fails, stop
processing and return an access-denied response/redirect (or add a flash error)
instead of creating the comment; place this check just before
comment.setCaseId(id)/commentService.createComment and reuse the exact logic or
helper used in showTicketDetails and processCloseTicket to keep behavior
consistent.
In `@src/main/resources/templates/ticket.html`:
- Around line 48-60: The template currently gates the comment form on the
canClose flag which mixes closing and commenting permissions; update the view to
use a separate canComment boolean (instead of canClose) so commenting visibility
is independent, and ensure the controller that renders this template sets
canComment appropriately; also verify and enforce the same permission in
CaseController.addComment (the POST /tickets/{id}/comments handler) so
server-side checks match the new canComment UI flag.
- Around line 33-46: The template lacks any field-level error display for the
comment form, so when addComment re-renders the "ticket" view due to
bindingResult.hasErrors() users see no validation feedback; update the comment
form to bind to the comment model attribute with th:object="${comment}", change
the textarea to use th:field="*{text}" to preserve input, and add validation
output using th:errors="*{text}" (or a conditional block with
`#fields.hasErrors`('text')) adjacent to the textarea so field errors are shown
when bindingResult.hasErrors() triggers.
---
Nitpick comments:
In `@src/main/java/org/example/untitled/usercase/AuditAction.java`:
- Around line 12-15: AuditAction.getDisplayName() uses locale-sensitive
this.name().toLowerCase(); change it to use a fixed locale by calling
toLowerCase(Locale.ROOT) so casing is deterministic across JVM locales, and add
the necessary import for java.util.Locale; update the method signature in
AuditAction.getDisplayName() to call this.name().toLowerCase(Locale.ROOT) before
replacing '_' and capitalizing the first character.
In `@src/main/java/org/example/untitled/usercase/controller/CaseController.java`:
- Around line 147-159: The method re-fetches the same ticket twice:
CaseController calls caseService.getTicketByID(id) into local variable ticket,
then calls caseService.getTicketByID(id) again when adding to the model; change
the model.addAttribute call to reuse the already-loaded ticket variable (ticket)
instead of calling getTicketByID(id) again so you avoid an extra DB round trip
and keep the same object used for authorization and view rendering.
In `@src/main/resources/static/style.css`:
- Around line 436-454: The CSS defines unused classes .comment-box and
.comment-author while the template ticket.html now uses .ticket-info,
.ticket-title and .assigned; either remove the dead rules or align the template
with the CSS—update ticket.html to replace .ticket-info/.ticket-title/.assigned
usages with .comment-box/.comment-author (and .comment-text if needed) to apply
the styling, or delete/merge the .comment-box and .comment-author rules from
src/main/resources/static/style.css to avoid unused styles.
🪄 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: 6eafb455-49eb-4e30-890c-2a3c59c2eb24
📒 Files selected for processing (4)
src/main/java/org/example/untitled/usercase/AuditAction.javasrc/main/java/org/example/untitled/usercase/controller/CaseController.javasrc/main/resources/static/style.csssrc/main/resources/templates/ticket.html
…amic elements, and refine styling for comment and activity sections
…t case conversion.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/example/untitled/usercase/controller/CaseController.java (1)
76-86: Recommended: extract the repeatedisHandler/isAssigned/isOwnerblock into a helper.The same trio of role/relationship checks is duplicated four times (
showTicketDetails,closeTicketGET,processCloseTicket,addComment). The next time the auth model changes (e.g., addingcanCommentticket-state gating, or a new role) it has to be updated in all four places — easy to miss. Consider a smallTicketPermissionsrecord/helper that returns{isOwner, isAssigned, isHandler, canClose, canComment}for(ticket, userDetails).Also applies to: 143-146, 163-166, 262-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/untitled/usercase/controller/CaseController.java` around lines 76 - 86, Extract the repeated role/relationship logic in CaseController into a small helper record/class (e.g., TicketPermissions) that takes (Case ticket, UserDetails userDetails) and exposes boolean fields/methods isOwner, isAssigned, isHandler, canClose, canComment (compute canClose using ticket.status and the three role booleans). Replace the duplicated blocks in the methods showTicketDetails, closeTicket (GET), processCloseTicket, and addComment to construct a TicketPermissions instance and consult its fields instead of duplicating the checks; keep existing semantics (use caseService.isNotOwner(...) for handler check where appropriate) so behavior remains identical.
🤖 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/untitled/usercase/controller/CaseController.java`:
- Around line 146-147: The ResponseStatusException message in CaseController (in
the branch checking isHandler, isAssigned, and caseService.isNotOwner in the
close/authorization paths) is misleading ("You do not own this ticket"); change
the exception message to a neutral, accurate one such as "You are not allowed to
close this ticket" in both places (the check around the
isHandler/isAssigned/isNotOwner logic and the similar check later at 166-167) so
handlers/assignees who lose access see the correct text.
---
Nitpick comments:
In `@src/main/java/org/example/untitled/usercase/controller/CaseController.java`:
- Around line 76-86: Extract the repeated role/relationship logic in
CaseController into a small helper record/class (e.g., TicketPermissions) that
takes (Case ticket, UserDetails userDetails) and exposes boolean fields/methods
isOwner, isAssigned, isHandler, canClose, canComment (compute canClose using
ticket.status and the three role booleans). Replace the duplicated blocks in the
methods showTicketDetails, closeTicket (GET), processCloseTicket, and addComment
to construct a TicketPermissions instance and consult its fields instead of
duplicating the checks; keep existing semantics (use caseService.isNotOwner(...)
for handler check where appropriate) so behavior remains identical.
🪄 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: 6b63d1b6-44e8-4f45-b7f9-76faf63ac346
📒 Files selected for processing (6)
src/main/java/org/example/untitled/user/service/UserService.javasrc/main/java/org/example/untitled/usercase/AuditAction.javasrc/main/java/org/example/untitled/usercase/controller/CaseController.javasrc/main/java/org/example/untitled/usercase/service/CaseService.javasrc/main/java/org/example/untitled/usercase/service/CommentService.javasrc/main/resources/templates/ticket.html
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/example/untitled/usercase/AuditAction.java
- src/main/resources/templates/ticket.html
Comment on lines
146
to
147
| if (!isHandler && !isAssigned && caseService.isNotOwner(ticket, userDetails.getUsername())) | ||
| throw new ResponseStatusException(HttpStatus.FORBIDDEN, "You do not own this ticket"); |
There was a problem hiding this comment.
Misleading error message on close-auth failure.
The branch is now reached when the user is neither handler-level nor assignee nor owner, but the message still says "You do not own this ticket" — handlers/assignees who lose access (e.g., reassignment) will see incorrect text. Consider "You are not allowed to close this ticket".
Suggested change
- if (!isHandler && !isAssigned && caseService.isNotOwner(ticket, userDetails.getUsername()))
- throw new ResponseStatusException(HttpStatus.FORBIDDEN, "You do not own this ticket");
+ if (!isHandler && !isAssigned && caseService.isNotOwner(ticket, userDetails.getUsername()))
+ throw new ResponseStatusException(HttpStatus.FORBIDDEN, "You are not allowed to close this ticket");Also applies to: 166-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/untitled/usercase/controller/CaseController.java`
around lines 146 - 147, The ResponseStatusException message in CaseController
(in the branch checking isHandler, isAssigned, and caseService.isNotOwner in the
close/authorization paths) is misleading ("You do not own this ticket"); change
the exception message to a neutral, accurate one such as "You are not allowed to
close this ticket" in both places (the check around the
isHandler/isAssigned/isNotOwner logic and the similar check later at 166-167) so
handlers/assignees who lose access see the correct text.
FeFFe1996
approved these changes
Apr 27, 2026
…roved reusability and streamlined controller logic
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 by CodeRabbit
New Features
UI & Style