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

Ensure enum changes are stored consistently #429

Merged
merged 1 commit into from
Jun 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ Changed

Fixed

- None
- Ensure enum changes are stored consistently
[#429](https://github.com/collectiveidea/audited/pull/429)

## 4.7.0 (2018-03-14)

Expand Down
35 changes: 30 additions & 5 deletions lib/audited/auditor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ def revision_at(date_or_time)

# List of attributes that are audited.
def audited_attributes
attributes.except(*self.class.non_audited_columns)
audited_attributes = attributes.except(*self.class.non_audited_columns)
normalize_enum_changes(audited_attributes)
end

# Returns a list combined of record audits and associated audits.
Expand Down Expand Up @@ -196,11 +197,35 @@ def revision_with(attributes)

def audited_changes
all_changes = respond_to?(:changes_to_save) ? changes_to_save : changes
if audited_options[:only].present?
all_changes.slice(*self.class.audited_columns)
else
all_changes.except(*self.class.non_audited_columns)
filtered_changes = \
if audited_options[:only].present?
all_changes.slice(*self.class.audited_columns)
else
all_changes.except(*self.class.non_audited_columns)
end

filtered_changes = normalize_enum_changes(filtered_changes)
filtered_changes.to_hash
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the to_hash? shouldn't this already be a hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a hash-like object (ActiveSupport::HashWithIndifferentAccess)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fatkodima a little late to the party, but this object indeed was an ActiveSupport::HashWithIndifferentAccess, but after you call to_hash on it, it's not anymore, it's simply a Hash object. The reason I'm writing this is that after upgrading from 4.8.0 to 4.9.0 I noticed that i cannot access some audited data by simbol, because they are not stored as ActiveSupport::HashWithIndifferentAccess anymore. Was this intended?

end

def normalize_enum_changes(changes)
self.class.defined_enums.each do |name, values|
if changes.has_key?(name)
changes[name] = \
if changes[name].is_a?(Array)
changes[name].map { |v| values[v] }
elsif rails_below?('5.0')
changes[name]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why for rails<5 this is a noop but only if the change isn't an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is due to the fact, that rails stores enum changes differently for rails < 5 and for rails >= 5.

else
values[changes[name]]
end
end
end
changes
end

def rails_below?(rails_version)
Gem::Version.new(Rails::VERSION::STRING) < Gem::Version.new(rails_version)
end

def audits_to(version = nil)
Expand Down
30 changes: 27 additions & 3 deletions spec/audited/auditor_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ def non_column_attr=(val)
end

describe "on create" do
let( :user ) { create_user audit_comment: "Create" }
let( :user ) { create_user status: :reliable, audit_comment: "Create" }

it "should change the audit count" do
expect {
Expand All @@ -272,6 +272,10 @@ def non_column_attr=(val)
expect(user.audits.first.audited_changes).to eq(user.audited_attributes)
end

it "should store enum value" do
expect(user.audits.first.audited_changes["status"]).to eq(1)
end

it "should store comment" do
expect(user.audits.first.comment).to eq('Create')
end
Expand All @@ -290,7 +294,7 @@ def non_column_attr=(val)

describe "on update" do
before do
@user = create_user( name: 'Brandon', audit_comment: 'Update' )
@user = create_user( name: 'Brandon', status: :active, audit_comment: 'Update' )
end

it "should save an audit" do
Expand All @@ -314,6 +318,11 @@ def non_column_attr=(val)
expect(@user.audits.last.audited_changes).to eq({ 'name' => ['Brandon', 'Changed'] })
end

it "should store changed enum values" do
@user.update_attributes status: 1
expect(@user.audits.last.audited_changes["status"]).to eq([0, 1])
end

it "should store audit comment" do
expect(@user.audits.last.comment).to eq('Update')
end
Expand Down Expand Up @@ -350,7 +359,7 @@ def non_column_attr=(val)

describe "on destroy" do
before do
@user = create_user
@user = create_user(status: :active)
end

it "should save an audit" do
Expand All @@ -375,6 +384,11 @@ def non_column_attr=(val)
expect(@user.audits.last.audited_changes).to eq(@user.audited_attributes)
end

it "should store enum value" do
@user.destroy
expect(@user.audits.last.audited_changes["status"]).to eq(0)
end

it "should be able to reconstruct a destroyed record without history" do
@user.audits.delete_all
@user.destroy
Expand Down Expand Up @@ -630,6 +644,16 @@ def stub_global_max_audits(max_audits)
expect(u.revision(1).username).to eq('brandon')
end

it "should correctly restore revision with enum" do
u = Models::ActiveRecord::User.create(status: :active)
u.update_attribute(:status, :reliable)
u.update_attribute(:status, :banned)

expect(u.revision(3)).to be_banned
expect(u.revision(2)).to be_reliable
expect(u.revision(1)).to be_active
end

it "should be able to get time for first revision" do
suspended_at = Time.zone.now
u = Models::ActiveRecord::User.create(suspended_at: suspended_at)
Expand Down
1 change: 1 addition & 0 deletions spec/support/active_record/models.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ class User < ::ActiveRecord::Base
audited except: :password
attribute :non_column_attr if Rails.version >= '5.1'
attr_protected :logins if respond_to?(:attr_protected)
enum status: { active: 0, reliable: 1, banned: 2 }

def name=(val)
write_attribute(:name, CGI.escapeHTML(val))
Expand Down
1 change: 1 addition & 0 deletions spec/support/active_record/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
t.column :username, :string
t.column :password, :string
t.column :activated, :boolean
t.column :status, :integer, default: 0
t.column :suspended_at, :datetime
t.column :logins, :integer, default: 0
t.column :created_at, :datetime
Expand Down