closure_tree icon indicating copy to clipboard operation
closure_tree copied to clipboard

Preserve mutation changes when updating parent_id

Open schovi opened this issue 7 years ago • 10 comments

https://github.com/mceachen/closure_tree/issues/271

schovi avatar May 18 '17 13:05 schovi

Thanks for your PR, but I won't be able to merge until the tests pass. I don't have time to help with that right now.

mceachen avatar May 24 '17 16:05 mceachen

@mceachen Having a hard time to run all tests. I will try to handle that.

schovi avatar May 26 '17 07:05 schovi

Any progress on fixing the tests?

mceachen avatar Jun 19 '17 17:06 mceachen

@mceachen no. It would be awesome to have tests prepared in Docker

schovi avatar Jun 19 '17 18:06 schovi

@mceachen no. It would be awesome to have tests prepared in Docker

Hi there! Now the master branch is stable can you please rebase to see if it works? Thank you!

n-rodriguez avatar Jun 11 '18 21:06 n-rodriguez

I merged master with this PR and kicked off another build here: https://travis-ci.org/ClosureTree/closure_tree/builds/391065251. Checks failed.

mceachen avatar Jun 12 '18 02:06 mceachen

Checks failed.

If (you) don't change the if logic and keep some redundant code test pass :)

changed = public_send(changes_method)[_ct.parent_column_name]

if changed || @was_new_record
  _ct_persist_activerecord_state do
    rebuild!
  end
end

if changed && !@was_new_record
  # Resetting the ancestral collections addresses
  # https://github.com/mceachen/closure_tree/issues/68
  _ct_persist_activerecord_state do
    ancestor_hierarchies.reload
    self_and_ancestors.reload
  end
end

n-rodriguez avatar Jun 12 '18 04:06 n-rodriguez

This :

if changed || @was_new_record
  ...
end

if changed && !@was_new_record
  ...
end

has nothing to do with this :

if changed
  if @was_new_record
    ...
  else
    ...
  end
end

n-rodriguez avatar Jun 12 '18 04:06 n-rodriguez

The change should not break tests as we only save and restore some instance variables but it would be great to have tests to cover this case.

n-rodriguez avatar Jun 12 '18 04:06 n-rodriguez

@schovi can you please rebase on master branch? thank you!

n-rodriguez avatar Jan 10 '21 18:01 n-rodriguez