Repository navigation
Report a halted destroy as 422 instead of 200 - #131
Merged
Merged
Conversation
The destroy actions ignored destroy's return value and always answered success. No model halts its own destroy today, but a before_destroy guard added later would have left the record in place behind a 200. Use destroy! and rescue RecordNotDestroyed in the API base controller, answering 422 with the record's errors (or "Could not be deleted."). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L4Pma9VAqB5A22KCEEin9R
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
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.
Problem
The destroy actions (accounts, shops, item_tags, members, invitations, devices) ignored
destroy's return value and always answered 200 or 204.Nothing fails this way today:
before_action.But a
before_destroyguard added later would leave the record in place behind a success response.Fix
destroy!in all six actions.rescue_from ActiveRecord::RecordNotDestroyedinApi::V1::Shopkeeper::BaseController. It answers 422{code, error_message}with the record's errors, or "Could not be deleted." when there are none.Verification
test/integration/destroy_failure_test.rbadds a temporarybefore_destroy { throw :abort }to each model in turn, calls its endpoint, and expects 422 with the record kept. On the old code this got 200.destroyfails the test at that action's own step.rescue_fromraisesRecordNotDestroyed.{"status":200}. The probe records are gone.bin/rails test: 498 runs, 0 failures. rubocop and brakeman are clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01L4Pma9VAqB5A22KCEEin9R