Skip to content

[CORE-372] Fix Highlight Clear Not-Found Handling#187

Open
dytsou wants to merge 3 commits into
mainfrom
fix/CORE-372-highlight-clear
Open

[CORE-372] Fix Highlight Clear Not-Found Handling#187
dytsou wants to merge 3 commits into
mainfrom
fix/CORE-372-highlight-clear

Conversation

@dytsou

@dytsou dytsou commented Jun 28, 2026

Copy link
Copy Markdown
Member

Type of changes

  • Fix
  • Test

Purpose

  • Return ErrHighlightNotFound when clearing a highlight that does not exist
  • Add unit tests for highlight.Service.Clear with mock dependencies
  • Resolve issue CORE-372

Additional Information

@dytsou dytsou self-assigned this Jun 28, 2026
@dytsou
dytsou requested review from Dada8825252 and yorukot June 28, 2026 07:28
@dytsou dytsou changed the title [CORE-372] Fix highlight Clear not-found handling [CORE-372] Fix Form List Deadline Filter for Null-Deadline Forms Jun 28, 2026
@dytsou dytsou changed the title [CORE-372] Fix Form List Deadline Filter for Null-Deadline Forms [CORE-372] Fix Highlight Clear Not-Found Handling Jun 28, 2026
@dytsou
dytsou force-pushed the fix/CORE-372-highlight-clear branch from e8f92c4 to 98fda1c Compare July 3, 2026 05:10
Comment thread internal/form/highlight/service.go Outdated
return internal.ErrFormNotFound
}

_, err = s.queries.GetByFormID(traceCtx, formID)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest delegating the existence check to the delete operation itself — consider changing DeleteByFormID to :execrows so the service can inspect rowsAffected directly and remove the GetByFormID pre-check entirely. This simplifies the flow from two queries down to one, and also avoids the case where a concurrent delete causes the pre-check to pass but the delete to silently no-op.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest delegating the existence check to the delete operation itself — consider changing DeleteByFormID to :execrows so the service can inspect rowsAffected directly and remove the GetByFormID pre-check entirely.

Addressed: DeleteByFormID is now :execrows and Clear checks rowsAffected == 0 for ErrHighlightNotFound, removing the extra GetByFormID round-trip.

@dytsou
dytsou requested a review from Dada8825252 July 3, 2026 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants