Fix Dirty plugin with Rails 6 (0-8-stable branch), take 2 #351
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This includes #349, which I'll merge first once specs pass there. |
shioyama
commented
Oct 24, 2019
| #expect(article.attribute_before_last_save(:title_en)).to eq('foo') | ||
| #expect(article.will_save_change_to_attribute?(:title_en)).to eq(false) | ||
| #expect(article.attribute_change_to_be_saved(:title_en)).to eq(nil) | ||
| #expect(article.attribute_in_database(:title_en)).to eq('bar') |
There was a problem hiding this comment.
@pwim These specs all currently fail on 0-8-stable, so these other attribute methods (saved_change_to_attribute, etc.) don't work with translated attributes, just saved_change_to_attribute?. I'm not exactly sure why but the fact that they worked or not was pretty much dependent on internals of AR/AM.
These other specs will now pass with the new changes (once I commit them here), and they will be independent of those AM/AR internals.
Previous to the dirty module refactor, attribute handlers like +attribute_changed?+ were supported, but with that change they became unsupported. We need to support the public ones at least. First, let's lay down good test coverage before making more changes. Also, ensure that methods which include both translated and untranslated attributes are tested against untranslated attribute changes as well, to ensure we haven't broken any basic functionality. Note: For the 0-8-stable branch, to start I've commented out specs that fail currently. These will pass once changes to the dirty modules are merged, and then these specs will be uncommented.
shioyama
force-pushed
the
rails_6_dirty_tracking_mobility_0_8
branch
from
9db8c41 to
f6fc5b3
Compare
…odules The dirty tracking methods defined by ActiveModel and ActiveRecord change from version to version, which makes it tricky to correctly support all of them for translated attributes. Rather than adding many version conditionals to Mobility code, we can instead dynamically determine the dirty method patterns by including the AR/AM dirty module into a dummy class, and extracting the attribute methods from that class. We can then use those patterns to define Mobility dirty methods. However, to support both methods like +title_changed?+ and *also* +attribute_changed?(:title)+, we need to streamline things a bit. We can do this by changing the naming of methods on the Mobility mutation tracker such that its methods correspond exactly to the attribute handler methods. Doing this allows us to dynamically define both handler method overrides and dirty methods on translated attributes with some simple metaprogramming, in a way that will be mostly robust to changes in Rails (just need to add new methods to the dirty tracker to support new dirty methods on AM/AR). The result is that the Mobility ActiveRecord plugin becomes much simpler, whereas the ActiveModel one becomes slightly more complex, although most of that added complexity is in the mutation tracker, which is easy to understand. Tests are now very important since we are defining many more overrides than before (attribute handlers were not previously being overridden, but now we have no choice but to do so.)
Currently we define attribute handler overrides once each time a dirty module is created, i.e. once per Mobility class. The handler methods are just delegators, so they can be defined once on a single module (one for AM and one for AR) and included in each module.
+changes_applied+ and +clear_changes_information+ are private in versions of ActiveModel < 5.2, so Mobility should maintain that method scope.
|
Ok everything is up to date here now. If/when specs pass, I will merge to the 0-8-stable branch and we can finally release this. In the end, this actually does quite a lot more than the dirty plugin did previously. |
|
Cool. I ran it against my application's specs and they all pass now. |
We'll fix this bug in Mobility, even though behaviour is broken in Rails 4.2.
|
Thanks for checking! I'll need to cherry-pick #353 in here as well before merging/releasing. |
|
Ok cherry-picked last commit from master. Once this one passes, we should finally be good to release. |
Previously, we were looking at the Active*Model* methods to se if they were private or public, and defining overrides accordingly. However, in some versions of Rails, the methods can be private in one module, but public in the other... so we need to ensure that the code actually looks at the correct +dirty_class+ to determine if the methods should be public or private.
shioyama
force-pushed
the
rails_6_dirty_tracking_mobility_0_8
branch
from
3c973b5 to
5f9efae
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Second attempt, following #347