Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve mutation changes when updating parent_id #272

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

schovi
Copy link

@schovi schovi commented May 18, 2017

@schovi schovi changed the title Preserve mutation_changes when updating parent_id Preserve mutation changes when updating parent_id May 18, 2017
@schovi schovi force-pushed the mutation-changes-fix branch 3 times, most recently from e9ed47e to 82a2d7c Compare May 24, 2017 16:11
@mceachen
Copy link
Collaborator

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.

@schovi
Copy link
Author

schovi commented May 26, 2017

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

@mceachen
Copy link
Collaborator

Any progress on fixing the tests?

@schovi
Copy link
Author

schovi commented Jun 19, 2017

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

@n-rodriguez
Copy link
Contributor

@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!

@mceachen
Copy link
Collaborator

mceachen commented Jun 12, 2018

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

@n-rodriguez
Copy link
Contributor

n-rodriguez commented Jun 12, 2018

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

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.

3 participants