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

Skip any changed attributes marked to skip in the PaperTrail options #236

Merged

Conversation

joshmcarthur
Copy link
Contributor

I came across the same problem described in #224 when using paper_trail with acts-as-taggable-on, and have a (partial) solution.

The reason this happens is that acts-as-taggable-on defines some code that manually adds a non-database field to the changed_attributes collection. paper_trail then tries to assign this value directly to the record using the column accessor, resulting in the following error:

ActiveModel::MissingAttributeError (can't write unknown attribute `tag_list')

My solution to this is to exclude from changed_attributes any keys defined in the :skip options of paper_trail - identically to what happens soon after that in the process, when the object is serialised. The reason this is just a partial fix is that the tag_list attribute still needs to be specified in the "skip" options for paper_trail. I don't really see a way around this short of acts-as-taggable-on changing how it behaves, but I'm very open to any suggestions.

@batter
Copy link
Collaborator

batter commented Jul 2, 2013

Thanks for the pull request. I'm happy to pull this in if this helps as a fix for some, although, I'm wondering if we might be able to do a fix that would actually prevent this error from being raised entirely.

I'm envisioning restricting the arguments here to only attributes which have corresponding columns in the database.

Here's what I'm envisioning, instead of:

# whats currently being used now
changed_attributes.each { |attr, before| prev.send[attr] = before }

use this:

changed_attributes.select { |k,v| self.class.column_names.include?(k) }.each { |attr, before| prev[attr] = before }

This both alleviates the issue entirely as far as I can tell, and prevents future incidents from occurring in similar scenarios. Thoughts?

@joshmcarthur
Copy link
Contributor Author

@fullbridge-batkins:

  1. I'm not sure why the splat operator is for either - I had assumed that because it was used a few lines down, it was there because it was necessary - perhaps for an older version of Rails?
  2. Your approach is much, much better and exactly why I submitted a pull - I was hoping someone would be able to chip in with a more universal solution! I'll make that change on my branch now.

@joshmcarthur
Copy link
Contributor Author

I've adjusted how the changed_attributes are excluded now as per your suggestion. I also tried removing the splat operator from the object_to_string method, but found that it resulted in a test failure, so put it back - it's probably not a change that should be made in this pull anyway.

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.

2 participants