🧪 Ticket modifiers #779
🧪 Ticket modifiers #779
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #779 +/- ##
==========================================
- Coverage 50.28% 50.16% -0.12%
==========================================
Files 259 260 +1
Lines 15227 15253 +26
==========================================
- Hits 7657 7652 -5
- Misses 6771 6801 +30
- Partials 799 800 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a ticket modifier pattern to consolidate duplicate logic across ticket operations. The changes refactor ticket assignment, topic changes, and note addition to use a common modifier pattern instead of handling event creation and ticket updates inline.
- Introduces a new
TicketModifierfunction type and factory functions for different ticket operations - Refactors existing ticket handlers to use the new modifier pattern with a shared
ApplyTicketModifierfunction - Eliminates duplicate logic across ticket operations while maintaining the same functionality
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| core/tickets/modifiers.go | New file implementing the modifier pattern with factory functions for assignment, note, and topic operations |
| web/ticket/base.go | Adds shared ApplyTicketModifier function to handle modifier application and event logging |
| web/ticket/change_topic.go | Refactored to use the new topic modifier instead of inline topic change logic |
| web/ticket/assign.go | Refactored to use the new assignment modifier and removed the now-unused getOrgUser helper |
| web/ticket/add_note.go | Refactored to use the new note modifier instead of inline note addition logic |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| newAssignee := oa.UserByID(assigneeID) | ||
|
|
||
| return func(ticket *models.Ticket, log flows.EventCallback) bool { | ||
| if ticket.AssigneeID != assigneeID { | ||
| prevAssignee := oa.UserByID(ticket.AssigneeID) | ||
| ticket.AssigneeID = assigneeID | ||
| log(events.NewTicketAssigneeChanged(ticket.UUID, newAssignee.Reference(), prevAssignee.Reference())) |
There was a problem hiding this comment.
Potential nil pointer dereference when calling newAssignee.Reference() or prevAssignee.Reference(). If assigneeID is models.NilUserID or refers to a non-existent user, oa.UserByID() will return nil, causing a panic when .Reference() is called.
| newAssignee := oa.UserByID(assigneeID) | |
| return func(ticket *models.Ticket, log flows.EventCallback) bool { | |
| if ticket.AssigneeID != assigneeID { | |
| prevAssignee := oa.UserByID(ticket.AssigneeID) | |
| ticket.AssigneeID = assigneeID | |
| log(events.NewTicketAssigneeChanged(ticket.UUID, newAssignee.Reference(), prevAssignee.Reference())) | |
| var newRef, prevRef string | |
| if newAssignee != nil { | |
| newRef = newAssignee.Reference() | |
| } else { | |
| newRef = "" | |
| } | |
| if prevAssignee != nil { | |
| prevRef = prevAssignee.Reference() | |
| } else { | |
| prevRef = "" | |
| } | |
| log(events.NewTicketAssigneeChanged(ticket.UUID, newRef, prevRef)) |
Copilot uses AI. Check for mistakes.
No description provided.