Fix dirty handling on update #2707
Conversation
spec/orm/activerecord_spec.rb
Outdated
| describe '#changes' do | ||
| it "should be generated" do | ||
| @event.image = stub_file('test.jpeg') | ||
| expect(@event.changes).to eq({'image' => [nil, 'test.jpeg']}) | ||
| end | ||
|
|
||
| it "shouldn't be generated when the attribute value is unchanged" do | ||
| @event.image = stub_file('test.jpeg') | ||
| @event.save! | ||
| @event.image = @event[:image] | ||
|
|
||
| expect(@event.changes).to be_blank | ||
| expect(@event).not_to be_changed | ||
| @event.remote_image_url = "" | ||
| expect(@event).not_to be_changed | ||
| end | ||
|
|
||
| it "shouldn't be generated after second save" do | ||
| @event.image = stub_file('old.jpeg') | ||
| @event.save! | ||
| @event.image = stub_file('new.jpeg') | ||
| @event.save! | ||
|
|
||
| expect(@event.changes).to be_blank | ||
| expect(@event).not_to be_changed | ||
| end |
There was a problem hiding this comment.
The third test is the issue described in #2702. Not sure yet if we can make the second one pass.
rajyan
force-pushed
the
fix-issue-2702
branch
from
1fefc15
to
f4e58cb
Compare
lib/carrierwave/mounter.rb
Outdated
| @@ -168,7 +168,7 @@ def blank? | |||
|
|
|||
| def remove=(value) | |||
| @remove = value | |||
| write_temporary_identifier | |||
| write_temporary_identifier if remove? | |||
There was a problem hiding this comment.
Doing so is not ideal, as it will prevent users from cancelling the removal by assigning false. (I guess it's better to have a spec for that...)
image.remove = true
image.remove = false # this will not call write_temporary_identifier and the attribute change won't be resetI think I found a better solution. Clearing @original_filename after store will make #write_temporary_identifier harmless, because #temporary_identifier will pick up @identifier itself. Could you try that approach?
diff --git a/lib/carrierwave/uploader/store.rb b/lib/carrierwave/uploader/store.rb
index 16a09ca..55d0c06 100644
--- a/lib/carrierwave/uploader/store.rb
+++ b/lib/carrierwave/uploader/store.rb
@@ -87,7 +87,7 @@ module CarrierWave
end
@file = new_file
@identifier = storage.identifier
- @cache_id = @deduplication_index = nil
+ @original_filename = @cache_id = @deduplication_index = nil
@staged = false
end
endThere was a problem hiding this comment.
Thank you for your review.
You are right, my fix wan't the right way to go.
Also, I wasn't aware of
from cancelling the removal by assigning false
this case.
Added some test cases related to remove image cancellation.
|
Great job, thank you always 👍 |
fixes #2702
mark_remove_#{column}_falseis called after commithttps://github.com/rajyan/carrierwave/blob/e53c9c33674289ffabd8aa6ab3160e2ce60872dd/lib/carrierwave/mount.rb#L387-L389
which was the cause of this issue.
Changed to write a temporary identifier only if remove is changed to a truthy value.