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

store attribute metadata as it exists at creation, instead of nil #458

Merged
merged 1 commit into from
Jan 12, 2015

Conversation

rhec
Copy link
Contributor

@rhec rhec commented Jan 6, 2015

When the initial paper trail version is created any attribute metadata will be nil instead of the values that exist, because #{attribute}_was will always return nil for new models. This looks like an explicit decision, given there is a test, but I don't understand the logic behind it.
For background, in my case I'm building a multi-tenant application and I'm adding the tenant id attribute to my paper trail version metadata. It's important for us to be able to query the version history restricted by tenant, and with the current behavior it's impossible to match create events with a tenant without reifying the actual objects.
Any other use case for storing attribute metadata I can think of also would benefit from storing the metadata that exists at create, rather than storing nil, but maybe I'm missing something?

@batter
Copy link
Collaborator

batter commented Jan 7, 2015

As discussed in some previous issues (#185, #385, #387), this is the expected behavior, as all of the aspects of a model (including metadata) are meant to show what the model looked like PRIOR to the event driving the generation of the version. That being said, I'm guessing that lots of users (I've done this in the past as well) are often looking to have the metadata populated even on create events, so maybe it just makes sense to merge this.

Although it begs the question: How important are create events? I usually just skip them since they can't be reified anyways..

@batter batter added this to the 4.0.0 milestone Jan 7, 2015
@rhec
Copy link
Contributor Author

rhec commented Jan 8, 2015

For me the value of create events is to have a complete history available in paper trail without having to reify anything or check existing objects. If you don't store create events the only way to get the date a model was created is to check created_at on an existing model or reify a version for deleted models. This method also only works if your model has a created_at attribute, which isn't a given.

batter added a commit that referenced this pull request Jan 12, 2015
store attribute metadata as it exists at creation
@batter batter merged commit 692dff1 into paper-trail-gem:master Jan 12, 2015
batter added a commit that referenced this pull request Jan 12, 2015
@batter
Copy link
Collaborator

batter commented Jan 12, 2015

Cheers

@Billiam
Copy link

Billiam commented May 13, 2016

Isn't this a significant mismatch in behavior for new records vs update records?

When metadata may be nil for created records, this matches the created behavior for Version objects as well, where new versions represent the state of the previous, rather than current data.

It seems like this behavior breaks a few possible expectations:

  1. That metadata matches version behavior. With this change, this is true for updates, where both the metadata and object data refer to the state prior to the change, but no longer the case for created events.
  2. That metadata can be used for queries across versions. Per the example shown in the readme, let's say that you wanted to find all Article versions with a particular author_id, and had an Article which was created, and then had a change in author.
    A query for PaperTrail::Version.where(:author_id => author_id) will return two results back for this article (rather than one), one for the initial created version, and one for the change event, both with identical metadata.
  3. That metadata matches the current version. This is an incorrect expectation based on open issues, and documentation, but it is the apparent behavior when creating a new record.

Would a new issue be better for discussion, or has this already been hashed out?

@jaredbeck
Copy link
Member

These all seem like valid concerns, @Billiam. Ben, can address these concerns?

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.

4 participants