#446 - Added support for attributes to be arrays #448
Conversation
cfeckardt
force-pushed
the
feature/array_support
branch
2 times, most recently
from
68a2332 to
2e6176c
Compare
spec/support/active_record/schema.rb
Outdated
| @@ -40,6 +40,7 @@ | |||
| t.column :created_at, :datetime | |||
| t.column :updated_at, :datetime | |||
| t.column :favourite_device, :string | |||
| t.column :favourite_colours, :json | |||
There was a problem hiding this comment.
json is only supported on postgres, tests are failing for mysql/sqlite because of this.
There was a problem hiding this comment.
I ran bundle exec rspec. I see now that only runs tests on 1 db.
Will address this by only running the new tests if the db is postgres.
There was a problem hiding this comment.
@tbrisker The changes you requested have been implemented.
cfeckardt
force-pushed
the
feature/array_support
branch
from
2e6176c to
c2376a2
Compare
cfeckardt
force-pushed
the
feature/array_support
branch
from
c2376a2 to
a4e9c7c
Compare
|
@tbrisker changed as per your requests, with a green build |
| attrs | ||
| end | ||
| end | ||
|
|
||
| # Returns a hash of the changed attributes with the old values | ||
| def old_attributes | ||
| (audited_changes || {}).inject({}.with_indifferent_access) do |attrs, (attr, values)| | ||
| attrs[attr] = Array(values).first | ||
|
|
||
| attrs[attr] = self.action == 'update' ? Array(values).first : values |
There was a problem hiding this comment.
Not crucial, but now we don't need to wrap the values as an array since when the action is an update the changes are saved as an array.
| @@ -40,6 +40,10 @@ | |||
| t.column :created_at, :datetime | |||
| t.column :updated_at, :datetime | |||
| t.column :favourite_device, :string | |||
|
|
|||
| if ENV['DB'] == 'POSTGRES' | |||
| t.column :favourite_colours, :json | |||
There was a problem hiding this comment.
Is there a way to test this on other DBs as well? perhaps using serialization to save the array as a string column?
There was a problem hiding this comment.
I considered this.
However, if the field is not :json, then active record does not automatically cast, and audited_changes would contains strings.
We'd get audited_changes: ["['blue', 'red']", "['blue']"]
instead of [[blue, red],[blue]]
Therefore, it's just another string, and already covered in other tests.
If you disagree I'd be happy to come up with something a bit more explicit
There was a problem hiding this comment.
sure, this is better in any case. I think we could use serialize somehow to test other dbs, but i won't block on that.
There was a problem hiding this comment.
I haven't looked into it too deeply, but at least in Rails 5 this can be tested using audit.revision. That will raise a SerializationTypeMismatch when trying to get the revision for a create audit that contains an array serialized using Rails' serialize (because it gets a string rather than an array).
|
ping @cfeckardt - care to fix the remaining comment and add a line to the changelog? |
|
Sorry about the AWOL ill get this sorted |
|
@cfeckardt - Would love to see this finalised. Any chance on getting this over the finishing line? c @tbrisker |
|
sorry for the "me too", but I'm just trying audited out and bumped into this limitation. When I pull a revision out, it shows |
This PR should solve #446
I have tried to keep conventions similar to what you have done in the past.
The code works by checking if the action is an update when reading the new attributes.
I have not yet tested this extensively in my own application so please test it first. The tests are passing with green (however there are race conditions for me on the test suite on master too)