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

Force metadata calls to attributes to use current value if value is changing for attribute #176

Merged
merged 3 commits into from
Oct 12, 2012

Conversation

batter
Copy link
Collaborator

@batter batter commented Oct 10, 2012

Currently, if an attribute is set to be called to populate metadata on the versions table, and that attribute's value is being modified in an update, it will use the new value (value it is changing to) for the metadata value. This pull request aims to fix it so it will use the current value (what the attribute's value is prior to the update to the model).

Say you have a model like such:

# == Schema Information
#
# Table name: resource_versions
#
#  id             :integer          not null, primary key
#  title          :string(255)
#  contents       :text
class Article < ActiveRecord::Base
  has_paper_trail :meta => { :title => :title }
end

Here is how the gem behaves currently:

> art = Article.create(:title => 'Lorem', :content => 'Lorem ipsum dolor sit amet')
=> #<Article id: 1, title: "Lorem", content: "Lorem ipsum dolor sit amet'">
> art.versions.last.title
=> "Lorem"
# I would expect it to actually be the value that is being stored on the object
> art.previous_version.title
=> nil
# this also happens on the update
> art.update_attribue(:title, 'Foobar')
=> true
> art.versions.last.title
=> "Foobar"
# again, I would expect it to return the value that is stored on the object
> art.previous_version.title
=> "Lorem"

This commit fixes the gem so that the metadata that gets stored uses the attribute value prior to it's modification, as I would expect it to.

Ben Atkins added 3 commits October 10, 2012 19:39
… the event that an attribute is being called upon to provide the value, and the attribute is changing, it will grab the current version of the attribute (what it was prior to the changes).
@batter batter merged commit bffbe53 into paper-trail-gem:master Oct 12, 2012
@danigb
Copy link

danigb commented Nov 23, 2012

Hi:

I've just upgraded to last version of the gem and all the test are failing because this change. I rely on the meta information to display some activity information to the user and now is broken. I think that this kind of change (big impact on user code) should be, at least, optional and well documented.

Is there any way I can use the old behaviour or I should stick to 2.6.3 ... forever? ... :-S

@batter
Copy link
Collaborator Author

batter commented Nov 26, 2012

@danigb - This change shouldn't break the metadata functionality, it should merely make it so that the metadata information stored on the version pulls the item attributes in as is in their current (pre-changed) state. This functionality seemed to make more sense to me and I would expect it to be the proper approach in most cases. Can you elaborate a little more about how exactly you are using the metadata and what exactly this broke for you?

I'm sure that there is a way to make it work the way you desire with the new version, so if you give a more detailed explanation (possibly with a code sample) then it will be easier for me to respond in an informative manner.

@danigb
Copy link

danigb commented Nov 26, 2012

Hi @fullbridge-batkins. Thanks for the response.

What I do: I store "the title" of the model as metadata in the version model so in the activity page I list the versions and display "the title". The title could be the title of the original model or the result of a method. It depends. With the new behaviour, the meta title field of the version is empty when creating a new model (an example of the broken behaviour)

I don't disuscuss the sense or unsense of the new behaviour (probably is the best approach).

Bests

@batter
Copy link
Collaborator Author

batter commented Nov 26, 2012

In the case you are referring to, doesn't it make sense for the version that is being stored on a create action for the title to be nil, seeing as the version stored in that case is also nil (version.reify should be nil).

If you want to ensure that it uses the new value on an update instead of the current value (pre-change value), you could do something like this:

has_paper_trail :meta => { :title => Proc.new { |article| article.title } }

Unless I'm missing something, your use case seems to assume that "the title" is going to be a static value that won't change throughout the course of the model's lifespan? This change was made to make it easier to store an attribute value that matches the value of the attribute on the object attributes stored in that version.

If you provide a code sample and a more specific question as to how to get the values you desire to be stored for a model, I'm happy to advise you as to what you could do to make it work to your liking?

@frenkel
Copy link

frenkel commented Dec 11, 2012

This is pretty weird for create events. Suppose I want to save an entity_id in the meta field. For create this would mean it's not set yet. Therefor you can't find the create events for that entity_id.

@batter
Copy link
Collaborator Author

batter commented Dec 11, 2012

@frenkel - Not sure what you're trying to get at? Are you questioning the behavior that resulted from this change?

If entity_id is an attribute on the model, then IMHO it doesn't make a ton of sense to store a value in the metadata column that does not (or might not) match the value stored for that attribute in the object column.

Personally, I often omit tracking on create events when using PaperTrail, as calling version.reify on that object will result in a nil object, which I don't find to be that helpful.

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

Successfully merging this pull request may close these issues.

3 participants