Change order of operations in persist_changes#781
Conversation
…o save the project before components are removed
Test coverage89.61% line coverage reported by SimpleCov. |
There was a problem hiding this comment.
Pull request overview
Reorders persistence steps in Project::Update.persist_changes to delete components marked for removal before saving the project, aiming to prevent ActiveRecord::RecordInvalid when soon-to-be-deleted components fail validation.
Changes:
- Move component deletions (
destroy_all) ahead ofproject.save!inside the transaction.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ActiveRecord::Base.transaction do | ||
| response[:project].save! | ||
| response[:project].components.where(id: response[:component_ids_to_delete]).destroy_all |
There was a problem hiding this comment.
Because update_component_attributes enumerates response[:project].components, the association is loaded before persist_changes. Calling components.where(...).destroy_all may not reliably remove those destroyed records from the already-loaded association cache, so project.save! can still validate the destroyed components and fail (potentially keeping the Sentry error). Prefer deleting via the association proxy (e.g., response[:project].components.destroy(response[:component_ids_to_delete])) or otherwise ensuring the in-memory collection no longer contains the records being deleted before save! runs validations.
| response[:project].components.where(id: response[:component_ids_to_delete]).destroy_all | |
| response[:project].components.destroy(response[:component_ids_to_delete]) |
| def persist_changes(response) | ||
| return if response.failure? | ||
|
|
||
| ActiveRecord::Base.transaction do | ||
| response[:project].save! | ||
| response[:project].components.where(id: response[:component_ids_to_delete]).destroy_all | ||
| response[:project].save! | ||
| end |
There was a problem hiding this comment.
This change is intended to fix a validation failure when an invalid component is being deleted. There are existing unit specs for Project::Update under spec/concepts/project/; please add a regression test that reproduces the failure (invalid component present but omitted from update_hash[:components]) and asserts the update succeeds and the invalid component is removed.
Description
Attempted fix for Sentry errors saying
Validation failed: Components extension can't be blank (ActiveRecord::RecordInvalid)Example here
Status
What's changed?
Update the order of operations in persist_changes. Currently we validate and save before deleting components marked for deletion. If one of the components is invalid and going to be deleted this will cause save to fail.