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

Rails 5.1 saved_changes is broken when updating parent_id on existing model #271

Closed
schovi opened this issue May 18, 2017 · 10 comments
Closed

Comments

@schovi
Copy link

schovi commented May 18, 2017

Having simple model

class Category < ApplicationModel
  has_closure_tree order: :position
  before_save :show_changes
  after_save :show_saved_changes

  def show_changes
    p "changes #{changes}"
  end

  def show_saved_changes
    p "saved_changes #{saved_changes}"
  end
end

When updating position attribute or other model attributes it behave as expected. But when I change parent_id it will print

changes {"parent_id": [1, 2]}
saved_changes {}

The problem seems to be in closure tree after_save callback in if branch for not a new record.

if public_send(changes_method)[_ct.parent_column_name] && !@was_new_record
  # Resetting the ancestral collections addresses
  # https://github.com/mceachen/closure_tree/issues/68
  ancestor_hierarchies.reload
  self_and_ancestors.reload
end

which will obviously reload and clear model.

Quick solution is to put my callbacks before initializing closure tree in model

class Category < ApplicationModel
  before_save :show_changes
  after_save :show_saved_changes

  has_closure_tree order: :position
end

But it is not that pretty and you can run to other issues.

PS: think if there is more things which can break due to closure tree saving

@schovi
Copy link
Author

schovi commented May 18, 2017

Naive fix #272

@fmluizao
Copy link

This is generating a lot of warnings with Rails 5.1 too:

DEPRECATION WARNING: The behavior of `changes` inside of after callbacks will be changing in the next version of Rails. The new return value will reflect the behavior of calling the method after `save` returned (e.g. the opposite of what it returns now). To maintain the current behavior, use `saved_changes` instead.
DEPRECATION WARNING: The behavior of `changed` inside of after callbacks will be changing in the next version of Rails. The new return value will reflect the behavior of calling the method after `save` returned (e.g. the opposite of what it returns now). To maintain the current behavior, use `saved_changes.keys` instead.
DEPRECATION WARNING: The behavior of `attribute_change` inside of after callbacks will be changing in the next version of Rails. The new return value will reflect the behavior of calling the method after `save` returned (e.g. the opposite of what it returns now). To maintain the current behavior, use `saved_change_to_attribute` instead.
DEPRECATION WARNING: The behavior of `attribute_changed?` inside of after callbacks will be changing in the next version of Rails. The new return value will reflect the behavior of calling the method after `save` returned (e.g. the opposite of what it returns now). To maintain the current behavior, use `saved_change_to_attribute?` instead.

However, I did not experience any problems so far.

@schovi
Copy link
Author

schovi commented May 24, 2017

These warnings are with or without my patch. See #269

When you have not experienced any problem, have you tried exactly what I wrote in the description?
The problem is obviously visible from source code, where closure tree reloads model which reset it to initial state.

I just need to validate my fix if there is something more which is needed to persist. And add test ofc :D

@mceachen
Copy link
Collaborator

I'd love to merge that PR, but the tests need to pass. I don't have time to help with that right now, I'm afraid.

@Exelord
Copy link

Exelord commented Sep 21, 2017

Hey, any progress on this?

@mceachen
Copy link
Collaborator

@Exelord feel free to fix the PR!

@schovi
Copy link
Author

schovi commented Sep 22, 2017

If anybody can help with tests it would be awesome.

@n-rodriguez
Copy link
Contributor

@schovi this should have been fixed : https://github.com/ClosureTree/closure_tree/blob/master/lib/closure_tree/hierarchy_maintenance.rb#L38
Can you please test with master branch?

@schovi
Copy link
Author

schovi commented Jun 25, 2018

@n-rodriguez seems fine 👍

@TBadyl
Copy link

TBadyl commented Jul 3, 2020

Hey.

I just stumbled into the same issue. In the after_save callbacks the saved_changes hash is emptified by closure_tree callbacks and I have no way to determine what attributes has changed.

Any plans to address this issue?

@seuros seuros closed this as completed Jul 31, 2022
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

No branches or pull requests

7 participants