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

RFC - model save hooks and mobility #574

Open
mrbrdo opened this issue May 24, 2022 · 3 comments
Open

RFC - model save hooks and mobility #574

mrbrdo opened this issue May 24, 2022 · 3 comments

Comments

@mrbrdo
Copy link
Contributor

mrbrdo commented May 24, 2022

There seems to be an issue with attr readers in some configuration of model after_create/save hooks. See also original issue mrbrdo/spree_mobility#6

Code triggering the issue (in Spree project if you are familiar):

    after_create :set_root
    after_save :set_root_taxon_name

    private

    def set_root
      self.root ||= Taxon.create!(taxonomy_id: id, name: name)
    end

    def set_root_taxon_name
      return unless saved_change_to_name?
      return if name.to_s == root.name.to_s

      root.update(name: name)
    end

At some point here, Taxon's name (for translation) is set to nil, although a non-nil value was given to base model. It has something to do with the save hooks, it seems. Since I have a validation on the translation model's name presence, the validation fails. Do you have any idea what could be the issue here?

@mrbrdo
Copy link
Contributor Author

mrbrdo commented May 26, 2022

@shioyama I managed to isolate the behavior into a failing mobility spec:
https://github.com/mrbrdo/mobility/blob/b87ac2f14e29c40a4d1aa7de15e7acd0a65abc59/spec/integration/active_record_compatibility_spec.rb#L163
(if you move the translates calls above the defintion of the hooks, the spec will pass)

Basically, if translates is not done before declaring model after_* hooks, it is possible to not get the translated attribute inside a hook (on an associated model that is). It seems that ActiveRecord will run the hooks before persisting the associations, leading to this issue, as in this case the associated model will fetch the main model from DB and it will not find the translations.
Although it might look like a contrived example, it is exactly what happened with above code from Spree, so it is a concern (although admittedly the code is kinda bad). Do you have a suggestion for a fix?

@shioyama
Copy link
Owner

shioyama commented Jun 6, 2022

Just glancing at this I doubt this is something that can be fixed very easily.

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Jun 11, 2022

Yeah it seems so :(

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

2 participants