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

Bug with enum / enums #798

Closed
WaKeMaTTa opened this issue May 9, 2016 · 13 comments
Closed

Bug with enum / enums #798

WaKeMaTTa opened this issue May 9, 2016 · 13 comments

Comments

@WaKeMaTTa
Copy link

WaKeMaTTa commented May 9, 2016

Hi,
I have a bug with enums and paper_trail. I will explain the bug with a dummy example.

# We have this model `models/post.rb`
class Post < ActiveRecord::Base
  has_paper_trail
  enum kind: { document: 0, newspaper: 1 }
end
# We create a post
@post = Post.create(title: "Hello world", kind: :document)

# We update the post for set the enum `kind` to `newspaper`
@post.update_attribute(kind: :newspaper)

# We print the versions we have (only for more informatión)
@post.versions
# return: 
[

    [0] <PaperTrail::Version:0x00558ca746a800> {
                    :id => 3,
             :item_type => "Post",
               :item_id => 916,
                 :event => "create",
             :whodunnit => "684",
                :object => "---\nid: 916\nuser_id: 739\ntitle: 'Hello world'\nkind: \n...",
            :created_at => Mon, 09 May 2016 16:15:55 UTC +00:00,
        :transaction_id => 3
    },
    [1] <PaperTrail::Version:0x00558ca746a6c0> {
                    :id => 4,
             :item_type => "Post",
               :item_id => 916,
                 :event => "update",
             :whodunnit => "684",
                :object => "---\nid: 916\nuser_id: 739\ntitle: 'Hello world'\nkind: \n...",
            :created_at => Mon, 09 May 2016 16:16:15 UTC +00:00,
        :transaction_id => 4
    }
]

# We check previous version
@old_version = @post.previous_version
# return:
<Post:0x00558ca743fad8> {
                    :id => 916,
               :user_id => 739
                 :title => 'Hello world',
                  :kind => nil, # this needs to be "newspaper"
            :created_at => Mon, 09 May 2016 16:15:55 UTC +00:00,
            :updated_at => Mon, 09 May 2016 16:16:05 UTC +00:00
}

Some extra info

  • Ruby 2.2.5
  • Rails 4.2.6
  • PostgreSQL 9.5.2
  • gem PaperTrail 5.0.1 with Associations (--with-associations)
@jaredbeck
Copy link
Member

Hi Mohamed, Sounds like you're on the right track! Thanks for the issue report.

Can you convert what you've got into a script, using our bug report template? Thanks!

@WaKeMaTTa
Copy link
Author

WaKeMaTTa commented May 11, 2016

Here it is bug report.

Without PaperTrail.config.track_associations = true, it fails too.

@jaredbeck
Copy link
Member

jaredbeck commented May 11, 2016

Thanks Mohamed! I will mark this as a confirmed bug.

Your test passes in PT 4.1.0, so this appears to be a regression between 4.1.0 and 5.0.1.

jaredbeck added a commit that referenced this issue May 15, 2016
jaredbeck added a commit that referenced this issue May 16, 2016
@whatjakecodes
Copy link

This still appears to be an issue even in 5.1.1. Both update and destroy events still store the enum columns in the object data as nil.

@jaredbeck
Copy link
Member

This still appears to be an issue even in 5.1.1.

That is correct. I played around with a fix in #805 but wasn't happy with my implementation. If you have any suggestions of how to fix it, I'd welcome them.

@jaredbeck
Copy link
Member

Have either of you (Mohamed, barnone27) had any luck fixing this? I don't use enums, so this is a low priority for me, but if either of you can take the lead on this, I'd be happy to help.

@key88sf
Copy link

key88sf commented Jul 21, 2016

Enums are very common now - I would hope this becomes a higher priority bug to fix if possible.

@jaredbeck
Copy link
Member

jaredbeck commented Jul 21, 2016

I would hope this becomes a higher priority bug to fix if possible.

I'm actually not sure what the right way to fix this is. See #805 for something you can build on. Contributions are welcome, thanks!

@jaredbeck
Copy link
Member

It's worth pointing out that this is not an issue with rails 5, only with rails 4.2.

jaredbeck added a commit that referenced this issue Aug 14, 2016
Fixes #798

Our test suite has one model that tests enums, PostWithStatus. Its
spec did not cover `touch_with_version`. Somehow, probably during
our upgrade to rails 5, we broke `touch_with_version` for enums.
Unlike other methods, when an enum arrives at
`CastAttributeSerializer#serialize` during `touch_with_version`, it
is already a number. We assumed it was always a string.

This is sort of a hack, just handling both strings and numbers in
`#serialize`. I'd rather figure out how a number got to `#serialize`
in the first place. However, a hack is acceptable, as it's only
for rails 4.2, and the hack will eventually be deleted when we
drop support for 4.2.
jaredbeck added a commit that referenced this issue Aug 14, 2016
Fixes #798

Our test suite has one model that tests enums, PostWithStatus. Its
spec did not cover `touch_with_version`. Somehow, probably during
our upgrade to rails 5, we broke `touch_with_version` for enums.
Unlike other methods, when an enum arrives at
`CastAttributeSerializer#serialize` during `touch_with_version`, it
is already a number. We assumed it was always a string.

This is sort of a hack, just handling both strings and numbers in
`#serialize`. I'd rather figure out how a number got to `#serialize`
in the first place. However, a hack is acceptable, as it's only
for rails 4.2, and the hack will eventually be deleted when we
drop support for 4.2.
jaredbeck added a commit that referenced this issue Aug 14, 2016
Fixes #798

Our test suite has one model that tests enums, PostWithStatus. Its
spec did not cover `touch_with_version`. Somehow, probably during
our upgrade to rails 5, we broke `touch_with_version` for enums.
Unlike other methods, when an enum arrives at
`CastAttributeSerializer#serialize` during `touch_with_version`, it
is already a number. We assumed it was always a string.

This is sort of a hack, just handling both strings and numbers in
`#serialize`. I'd rather figure out how a number got to `#serialize`
in the first place. However, a hack is acceptable, as it's only
for rails 4.2, and the hack will eventually be deleted when we
drop support for 4.2.
@jaredbeck
Copy link
Member

jaredbeck commented Aug 15, 2016

@WaKeMaTTa, @barnone27, and @key88sf please try master and confirm that it fixes the issue, thanks!

@jaredbeck
Copy link
Member

Released in 5.2.1. Please test, thanks!

@WaKeMaTTa
Copy link
Author

WaKeMaTTa commented Sep 5, 2016

It works perfectly. Good job. 🎉


I'm so sorry because when you posted this:

@WaKeMaTTa, @barnone27, and @key88sf please try master and confirm that it fixes the issue, thanks!

I did try master, but i forgot to give you the feedback (It didn't work). Sorry.

@jaredbeck
Copy link
Member

jaredbeck commented Sep 6, 2016

It works perfectly. Good job. 🎉

Thanks Mohamed. That code (CastedAttributeSerializer) is sooo hard to understand 😭 I wish I understood it better so I could improve it instead of just adding band-aids. But, at least we have a few more tests around enums now. ("few" being the key word)

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

No branches or pull requests

4 participants