Fix Dirty plugin with Rails 6 (0-8-stable branch)

[shioyama]

This is a patch to the 0-8-stable branch of #343. The branches are far apart now, so this really has to be the last major change to 0.8.x.

[pwim]

From running this branch on my application, it looks like it introduces a bug with the saved_changes method. When creating a new record, I'm only seeing the mobility attributes appearing in here, as opposed to all the attributes.

For example, when using, 0.8.8, after_save -> { puts saved_changes.keys.inspect } gave me

["id", "starts_at", "ends_at", "created_at", "updated_at", ...., "title_en"]

but on this branch it gives me

["title_en"]

Using changes in before_save appears to work as expected.

[shioyama]

Oh that's not good! I'm surprised specs don't fail on that. I'll check up on it, thanks!

[shioyama]

Ok problem is here:

mobility/lib/mobility/plugins/active_record/dirty.rb

Lines 81 to 83 in d04b821

	def saved_changes
	mutations_before_last_save_from_mobility.changes
	end

Neglected to merge super. Specs should have caught that one. I'll add more.

[pwim]

I'm having another spec failing because saved_change_to_attribute?(:address_ja) now (always?) returns false whereas it used to return true.

[shioyama]

Ah, that one I left out... but noticing now that it's public, so it should be implemented. I didn't do all the _attribute_ methods since Mobility no longer needs them (dispatches directly to the mutation tracker), and some of them are internal.

I guess I'll implement all the ones that are public. That will require more lines of code, more specs... ugh. I wish AM/AR were better designed for extension.

[shioyama]

Note that saved_change_to_address should work.

[shioyama]

Aarg... _previously_was hasn't been released yet... should I "pre-release" it? I guess not. God dirty is such a pain...

[shioyama]

@pwim So in order to support all these attribute methods, I need to override them explicitly... which is really complicated because each version of Rails supports different methods. What I've done (which I was doing previously) is to figure out the methods by including ::ActiveModel::Dirty into a class and then checking its suffixes... in this case though, I only want to define the ones that are public, which requires an extra check.

The result, though, is that at least we don't have a ton of conditionals in the code, just in the specs.

I could alternatively define the attribute_ methods and have the suffix methods call them, rather than the other way around... but in the end I don't think it really matters. Main thing is that tests cover all cases well.

[shioyama]

Actually not done yet... have to do the same for suffixes defined in ActiveRecord...

[shioyama]

Oh rats, this doesn't even work actually. This would support article.attribute_changed?(:title), but previous behaviour was article.attribute_changed?(:title_en)... need to rethink.

[shioyama]

Ok, after thinking about this for a while, I think I know how to do everything in a way that is not horribly messy/unmaintainable.

• Instead of mimicking how the mutation tracker(s) work in ActiveModel, instead define method names on the class such that they match the suffixes for dirty methods. So like mutations_from_mobility.attribute_will_change(attr_name), etc. This will make metaprogramming all the methods much easier.
• To make that metaprogramming work, though, an additional change is needed. Instead of having two mutation trackers (mutations_from_mobility/mutations_before_save_from_mobility), just make the mutation tracker itself hold both hashes internally, and delegate resetting them (in clear_changes_information, etc.) to the tracker. Then it becomes possible to define methods like attribute_previously_changed?, etc. directly on the tracker itself.
• Take the AM/AR suffixes, figure out which of the corresponding attribute handler methods are private, and then define both suffix methods (title_was, etc.) and attribute handlers (attribute_was, etc.) to map to the mutation tracker (mutations_from_mobility.attribute_was(:title)). That should be simple once methods on the mutation tracker are renamed.

I think with this approach, we can avoid most version conditionals in the code and keep it relatively clean. The complexity will be held in the mutation tracker, everything else just delegates to that.

[shioyama]

I'll make the changes to master tomorrow first, then update this one with the changes once tests are passing there.

[shioyama]

Going to close this and try with a new PR.