Skip to content

fix: preserve dirty tracking when reordering siblings with numeric_order - #498

Open
ihamzatarar wants to merge 1 commit into
ClosureTree:masterfrom
ihamzatarar:preserve-dirty-tracking-on-numeric-reorder
Open

ihamzatarar wants to merge 1 commit into
ClosureTree:masterfrom
ihamzatarar:preserve-dirty-tracking-on-numeric-reorder

Conversation

@ihamzatarar

Copy link
Copy Markdown

Fixes #414.

The problem

With numeric_order: true, a node that changes parent comes out of the save with no
dirty tracking at all:

category.update!(parent: other_category, name: "Foo")
category.saved_change_to_name? # => false

_ct_after_save calls rebuild!, which calls _ct_reorder_siblings, which ended with
reload unless destroyed?. ActiveRecord::AttributeMethods::Dirty#reload sets both
@mutations_before_last_save and @mutations_from_database to nil, so every
saved_change_to_* predicate returns false from that point on.

The callbacks that read those predicates run after the reload, so they see nothing.
after_update_commit :reindex, if: :saved_change_to_name? and the same shape used for
audit logs and cache busting silently skip moved nodes. Nothing raises, which is why
this is easy to miss.

The fix

_ct_reorder_siblings only needs one value back from the database: the sort order the
reorder statement just wrote. It now reads that single column with pick and clears the
resulting attribute change:

return if destroyed?

self.order_value = self.class.where(self.class.primary_key => _ct_id).pick(_ct.order_column_sym)
clear_attribute_change(_ct.order_column_sym)

clear_attribute_change calls mutations_from_database.forget_change. It does not touch
mutations_before_last_save, so the post-save dirty state survives while the record is
still reported as unchanged, which is what reload used to give callers. Without the
clear_attribute_change the record would be left changed? with a pending write to the
order column that is in fact already in the database.

Query count is unchanged: one SELECT before, one SELECT now.

I checked what else the reload was providing. The three reorder_with_parent_id
adapters in numeric_order_support.rb write the order column and nothing else, so no
other attribute could be stale. The association cache does not depend on it either: the
two associations that matter after a reparent are reloaded explicitly a few lines later
in _ct_after_save (the ancestor_hierarchies.reload / self_and_ancestors.reload pair
added for #68), and rebuild!'s recursive calls pass called_by_rebuild = true, which
skips _ct_reorder_siblings entirely and therefore never had the reload to begin with.
prepend_child and add_sibling call reload themselves where they need it.

The after_destroy :_ct_reorder_siblings path behaves as before: reload unless destroyed? became return if destroyed?.

Sibling methods

_ct_reorder_children and _ct_reorder_prior_siblings_if_parent_changed do not need the
same treatment and are left alone. Neither calls reload, and neither can change the
receiver's own order column. _ct_reorder_children renumbers rows whose parent is the
receiver, and _ct_reorder_prior_siblings_if_parent_changed renumbers rows under the
previous parent, which by then no longer include the receiver.

Test

test/closure_tree/label_order_value_test.rb gains one test next to the existing
"resets order_value when a node is moved" case. It moves a node to a new parent while
also changing its name, and asserts both halves of the behaviour: that
saved_change_to_name? and saved_change_to_name survive and the record is not left
dirty, and that the sort order is still right afterwards, both for the moved node and
for the siblings it left behind.

Without the lib change it fails on the first assertion:

LabelOrderValueTest#test_should_keep_dirty_tracking_when_a_node_is_moved_to_another_location_for_Label
[test/closure_tree/label_order_value_test.rb:50]:
Expected false to be truthy.

Full suite against PostgreSQL, MySQL and SQLite: 369 runs / 1350 assertions / 0 failures
on master, 370 runs / 1356 assertions / 0 failures with this change. I also confirmed by
hand that the fix holds on all three adapters, including the generic SQLite path where
reorder_with_parent_id goes through ActiveRecord objects rather than raw SQL.

Credit

@ryanb reported the issue and posted a working monkey-patch in
#414 (comment). This is
that patch, rebased onto the current shape of _ct_reorder_siblings, which now passes
scope_conditions through to reorder_with_parent_id.

_ct_reorder_siblings called reload to pick up the sort order the reorder
had just written. reload also clears @mutations_before_last_save, so every
saved_change_to_* predicate returned false for a node that had moved, and
after_update_commit callbacks guarded by them stopped firing.

Read the order column back with pick instead, then clear_attribute_change
so the assignment is not reported as a pending change. That touches only
mutations_from_database, leaving the post-save dirty state intact, and
costs the same single SELECT that reload did.

Fixes ClosureTree#414.

This branch has not been deployed

No deployments
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.

Dirty tracking lost when setting parent with numeric_order option

1 participant