Fix retrieving :has_one association

[kiela]

Some weird situation came up when I was working with Paper Trail. I have:

• created an instance of Category and Post models
• added several different categories to DB
• created new instance of Post, associated it with one Category and save to the DB
• edited several times the post without editing the associated category
• added controller which shows different versions of posts and associated categories
• tried to show choosen version of the post - here where the problem occurred

In my controller I have tried to retrieve proper version of the post by executing Version.find(params[:id]).reify(has_one: true). Although this command found proper version of Post it wasn't be able to find the proper version of Category. The problem is here: https://github.com/airblade/paper_trail/blob/ffd7bc233a18f1910588bd0c51a572a235da3c98/lib/paper_trail/version_concern.rb#L201 PaperTrail tried to fetch version of Category from max 3 seconds (by default) before chosen version of Post was created.

I think the better idea is to drop lookback option and fetch proper version of :has_one association by using .version_at.

I know that I need to update README.md and comments in the code as well, but firstly I want to get some feedback from the maintainers.

[batter]

This looks good to me at a glance. I like some of the cleanup you've done and your logic makes sense as far as I can tell. I haven't done a ton of work on the associations portion of the gem, much of that was in place before I started working with it and hasn't been touched much for a while now, so it's probably in need of some updating.

I'm not sure that this will impact this directly, but just so you know, I'm working on a feature that is going to alter the way the scope methods (subsequent, preceding, between) work so that if the primary key is an integer for the versions table, it relies on the auto-incrementing primary key for sorting instead of the timestamp_field alone, revering some of the changes from 6a4aba2. See #314 for more details. Hoping to get a feature branch up for this by EOD today.

[kiela]

Using integer value which will be auto-incrementing instead of timestamp_field sounds as a good idea. I can go back to this bug and test it again once this feature is done and merged into master.

As for now, are there any contraindications/objections not to merge fix from this PR?

[kiela]

As for now, are there any contraindications/objections not to merge fix from this PR?

/ping @batter

[batter]

@kiela - I released 3.0.2 last week, which included the switch to using the integer for sorting instead of the timestamp_field (when possible). However, I think there is still a blocker to merging this:

I agree that it makes sense to drop the lookback argument, however, the callbacks that build the versions for create and update events need to be modified to create them using after_create and after_update, which will then allow for the Version timestamps to be manually assigned to match the exact same time the event is processed on the base model.

The aforementioned invalid behavior has been documented in #131 and #354. So I guess that is the next priority for me to work on. Until I can come up with a solution for that, I don't think we can drop the lookback argument due to the fact that the timestamps on the version instances are actually a few milli/microseconds behind the updated_at timestamp on the base model. Does this make sense?

[batter]

@kiela - After reading your description more closely just now and the proposed solution you have submitted, I want to pose to you the question as to whether you have tested your proposed solution to ensure it works as expected?

As in, have you taken your branch here, employed it in a scenario like the one you described, and checked to see if it works? I still think we have an issue here:

version_at when receiving a timestamp arg, grabs the first version where the created_at (or whatever your timestamp_field is) is subsequent to the value that it receives. So perhaps this method shouldn't be using version_at to do the selection. What we really want is to grab the preceding version to the timestamp, right (since at the state an obj is persisted, it's associations are already persisted with a pre-existing timestamp). Because of this I wonder if the logic should be changed to something like this:

def reify_has_ones(model)
  model.class.reflect_on_all_associations(:has_one).each do |assoc|
    child = model.send assoc.name
    if child.respond_to? :version_at
      # we want to grab the associated child as it was immediately PRIOR
      # to the timestamp field being passed in here
      if child_as_it_was = child.version_at(send(PaperTrail.timestamp_field)).previous_version
        child_as_it_was.attributes.each do |k,v|
          model.send(assoc.name).send :write_attribute, k.to_sym, v rescue nil
        end
      else
        model.send "#{assoc.name}=", nil
      end
    end
  end
end

Or we could even clean this up (and optimize it) by adding some sort of a method to the PaperTrail::Model::InstanceMethods like this:

# works like `version_at` method but grabs previous version (if it exists)
def version_prior_to(timestamp, reify_options={})
  v = send(self.class.versions_association_name).preceding(timestamp, true).first
  return v.reify(reify_options) if v
end

Then we can change the setter line for child_as_it_was to look like this:

if child_as_it_was = child.version_prior_to(send(PaperTrail.timestamp_field))

Am I totally offbase here? Can we add some better tests surrounding this usecase?

[kiela]

@batter I'm a little busy lately so please give me some time and I will get back to this issue as soon as I will be able to.

[batter]

@kiela - I just released v4.0.0.beta1 to Rubygems, which contains a bunch of modifications to how the versioning and reification of associations work (including supporting of many-to-many associations). The majority of this work was brought in via PR #439. Part of the work that was brought in was a re-write of sorts of the has_one association handling, dropping the lookback option (as you suggested) was part of that. Can you give it a whirl and see if the changes made for that satisfy the issues you were trying to address with this PR? I'm thinking/hoping that they will!