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

Changelog: Enum-related breaking change in v4.1.0 #926

Merged
merged 1 commit into from Feb 10, 2017

Conversation

olliebennett
Copy link
Contributor

@olliebennett olliebennett commented Feb 8, 2017

A breaking change from v4.0.2 to v4.1.0 is not documented in the changelog.

Arguably this is the introduction of a bug, rather than a deliberate change, so perhaps it is not worth adding to the changelog.

The behaviour change is around retrieval of enum values from a version's changeset. After the update, the display behaviour seems to have changed from string-based to integer-based.

To reproduce this, I used the following code:

# my_model.rb
class MyModel < ActiveRecord::Base
  enum my_enum: { foo: 4, bar: 5 }
  has_paper_trail
end

# Create the version
sample = MyModel.create(my_enum: :foo)
sample.update!(my_enum: :bar)

# displaying human-readable changeset
changeset = MyModel.last.versions.last.changeset
from = changeset['my_enum'][0]
to = changeset['my_enum'][1]
puts "Enum value changed from '#{from}' to '#{to}'."

Output under paper_trail from 4.0.2:

Enum value changed from 'foo' to 'bar'.

Output under paper_trail from 4.1.0.

Enum value changed from '0' to '0'.

I've not checked the behaviour of the latest paper_trail version; this proposal is simply to fix the changelog (even though it is for quite an outdated version).

@olliebennett
Copy link
Contributor Author

olliebennett commented Feb 8, 2017

The bug appears to have been resolved by #731 (released in version 5.0.0.

Also related: #798.

Regardless, the problem itself seems to be resolved in more recent versions of paper_trail; this is just about raising awareness for others upgrading between these versions.

@batter
Copy link
Collaborator

batter commented Feb 8, 2017

Also related #857. I understand the issue, would be nice to understand what triggered the change between the two versions, but this seems like a reasonable warning for now since we we're now aware it's broken.

@jaredbeck
Copy link
Member

Hi Ollie. I'm fine with having this information in the changelog. Listing "known issues" in the changelog is quite uncommon in the ruby community, but common in other communities.

I'd prefer the heading "Known Issues" to "Breaking Changes", to separate the unintentional from the intentional, respectively. If that's acceptable to you, please make that change.

There have been many issues with enums over the past few versions (some due to changes in activerecord, some just bugs in PT) and I need to review the issues to confirm that this happened in v4.1.0.

@olliebennett
Copy link
Contributor Author

olliebennett commented Feb 8, 2017

Thanks; I have moved it to a "Known Issues" section - good idea. Let me know if I can give better reproduction steps for confirming this bug arrived in 4.1.0, or I can bisect to find the culprit commit if needed.

@jaredbeck
Copy link
Member

jaredbeck commented Feb 9, 2017

Ollie, I've tested PT 4.0.2 and 4.1.0 with your example. (https://gist.github.com/jaredbeck/e8a2b572c806bfff782ed6df12f48715) With rails 4, the output differs as you describe. However, with rails 5, they produce the same output. So, this is only an issue for rails 4. Please update your patch to reflect this.

@olliebennett
Copy link
Contributor Author

Good observation; I should have mentioned I am using Rails 4. I've modified the commit. Thanks for the help.

@jaredbeck jaredbeck merged commit add57b4 into paper-trail-gem:master Feb 10, 2017
@jaredbeck
Copy link
Member

Merged. Thanks, Ollie!

@olliebennett olliebennett deleted the patch-1 branch September 3, 2018 16:12
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.

None yet

3 participants