Refactor/consolidate assignee fields - #53
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR consolidates the ticket assignment relationship by removing the redundant Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/cyberwatch/features/staff/model/Staff.java`:
- Around line 66-67: The assignedTickets field in Staff currently uses ownership
semantics that delete tickets when staff is removed; update the mapping on the
Staff.assignedTickets Set<Ticket> to remove cascade = CascadeType.ALL and
orphanRemoval = true so Ticket lifetime is not tied to Staff, keeping
`@OneToMany`(mappedBy = "assignedTo") only; ensure Ticket.assignedTo remains the
owning side and, where tickets are unassigned or staff deleted, update the code
that manipulates assignedTickets (e.g., removal logic) to set
ticket.setAssignedTo(null) instead of relying on collection removal to delete
rows.
In
`@src/main/java/org/example/cyberwatch/features/ticket/service/TicketService.java`:
- Around line 133-138: The assignment flow in TicketService currently forces
ticket.setStatus(Status.IN_PROGRESS) and logs a status change without running
validateStatusTransition, which lets invalid transitions and false "status
change" records occur; fix by using validateStatusTransition(ticket, oldStatus,
Status.IN_PROGRESS) before changing status and only call ticket.setStatus and
activityLogService.logStatusChange when validateStatusTransition returns success
and oldStatus != Status.IN_PROGRESS; if reopening terminal states
(DRAFT/RESOLVED/CLOSED) on assignment is desired, implement an explicit reopen
transition method (e.g., reopenForAssignment) that invokes
validateStatusTransition with the intended target and documents the semantic,
otherwise keep assignment-only paths that set assignedTo and save without
touching status or logging status changes.
In `@src/main/resources/db/migration/V2_removeDuplicateAssigneeColumn.sql`:
- Around line 4-5: The migration currently drops FK_TICKETS_ON_ASSIGNEE and
assignee_id without preserving data — before removing the legacy column
(assignee_id) in V2_removeDuplicateAssigneeColumn.sql, add a backfill step that
copies non-null values from assignee_id into the canonical column assigned_to_id
and create or ensure the canonical FK exists, then drop the legacy FK/column;
additionally add a guard query that detects rows where both assigned_to_id and
assignee_id are non-null but different and aborts/fails the migration (with an
explicit error) so you don’t silently pick one value.
- Around line 1-5: Rename the migration file so Flyway can detect it by changing
the separator between version and description to a double underscore (rename
current migration to V2__remove_Duplicate_Assignee_Column.sql), and ensure the
SQL in that migration safely preserves data: before running ALTER TABLE tickets
DROP COLUMN IF EXISTS assignee_id; add a data migration step to copy any
non-null assignee_id values into assigned_to_id (e.g., UPDATE tickets SET
assigned_to_id = assignee_id WHERE assigned_to_id IS NULL AND assignee_id IS NOT
NULL), then drop the FK and column as currently written (ALTER TABLE tickets
DROP CONSTRAINT IF EXISTS FK_TICKETS_ON_ASSIGNEE; ALTER TABLE tickets DROP
COLUMN IF EXISTS assignee_id;).
🪄 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: 01277c93-a9a2-4604-9a48-98a62c0f09dc
📒 Files selected for processing (5)
src/main/java/org/example/cyberwatch/features/staff/model/Staff.javasrc/main/java/org/example/cyberwatch/features/ticket/controller/TicketController.javasrc/main/java/org/example/cyberwatch/features/ticket/model/Ticket.javasrc/main/java/org/example/cyberwatch/features/ticket/service/TicketService.javasrc/main/resources/db/migration/V2_removeDuplicateAssigneeColumn.sql
💤 Files with no reviewable changes (1)
- src/main/java/org/example/cyberwatch/features/ticket/model/Ticket.java
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/main/java/org/example/cyberwatch/features/ticket/service/TicketService.java (1)
133-136:⚠️ Potential issue | 🟠 MajorDon't log
IN_PROGRESSwhen assignment no longer changes status.Lines 134-135 only change
assignedTo, but Line 136 still recordsoldStatus → IN_PROGRESS. That leaves the activity log out of sync with the persisted ticket status returned by this method.🛠️ Minimal fix
Status oldStatus = ticket.getStatus(); ticket.setAssignedTo(staff); Ticket saved = ticketRepository.save(ticket); - activityLogService.logStatusChange(saved, assigner, oldStatus, Status.IN_PROGRESS); + if (oldStatus != saved.getStatus()) { + activityLogService.logStatusChange(saved, assigner, oldStatus, saved.getStatus()); + } return TicketResponseDTO.from(saved);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/ticket/service/TicketService.java` around lines 133 - 136, The code always logs a status change to IN_PROGRESS after only updating assignedTo; instead, capture oldStatus = ticket.getStatus(), save via ticketRepository.save(ticket), then compare the persisted status (e.g. saved.getStatus()) to oldStatus and only call activityLogService.logStatusChange(saved, assigner, oldStatus, saved.getStatus()) when they differ; do not hardcode Status.IN_PROGRESS in activityLogService.logStatusChange unless you also set the ticket status to that value.src/main/resources/db/migration/V2__remove_Duplicate_Assignee_Column.sql (1)
4-10:⚠️ Potential issue | 🔴 CriticalFail fast if the two assignee columns disagree.
V1__init.sqlcreated bothassigned_to_idandassignee_id, and this migration only backfills rows where the canonical column isNULL. If any row has both columns populated with different staff IDs, Line 10 silently drops the legacy value. Add a pre-drop guard that aborts the migration when both columns are non-null and unequal instead of discarding one side.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V2__remove_Duplicate_Assignee_Column.sql` around lines 4 - 10, Add a pre-drop guard that fails the migration if any ticket has both assigned_to_id and assignee_id populated but with different values: before running the UPDATE/ALTER TABLE statements check for existence of rows where assigned_to_id IS NOT NULL AND assignee_id IS NOT NULL AND assigned_to_id <> assignee_id and abort (raise an exception) with a clear message identifying the conflicting ticket(s). Locate the migration logic around the UPDATE to tickets SET assigned_to_id = assignee_id and the subsequent ALTER TABLE ... DROP COLUMN assignee_id and insert this existence check (and exception) so the migration fails fast instead of silently discarding differing legacy values.
🤖 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/cyberwatch/features/ticket/service/TicketService.java`:
- Around line 133-136: The code always logs a status change to IN_PROGRESS after
only updating assignedTo; instead, capture oldStatus = ticket.getStatus(), save
via ticketRepository.save(ticket), then compare the persisted status (e.g.
saved.getStatus()) to oldStatus and only call
activityLogService.logStatusChange(saved, assigner, oldStatus,
saved.getStatus()) when they differ; do not hardcode Status.IN_PROGRESS in
activityLogService.logStatusChange unless you also set the ticket status to that
value.
In `@src/main/resources/db/migration/V2__remove_Duplicate_Assignee_Column.sql`:
- Around line 4-10: Add a pre-drop guard that fails the migration if any ticket
has both assigned_to_id and assignee_id populated but with different values:
before running the UPDATE/ALTER TABLE statements check for existence of rows
where assigned_to_id IS NOT NULL AND assignee_id IS NOT NULL AND assigned_to_id
<> assignee_id and abort (raise an exception) with a clear message identifying
the conflicting ticket(s). Locate the migration logic around the UPDATE to
tickets SET assigned_to_id = assignee_id and the subsequent ALTER TABLE ... DROP
COLUMN assignee_id and insert this existence check (and exception) so the
migration fails fast instead of silently discarding differing legacy values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fdbe9050-7f51-49d5-92c1-73ee4a8cd581
📒 Files selected for processing (3)
src/main/java/org/example/cyberwatch/features/staff/model/Staff.javasrc/main/java/org/example/cyberwatch/features/ticket/service/TicketService.javasrc/main/resources/db/migration/V2__remove_Duplicate_Assignee_Column.sql
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
1 participant
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.
Fixes issue #29
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor