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.

Sign up for GitHub

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

Jump to bottom

Fix dup mounter cache behavior #2706

Merged
merged 3 commits into from
Sep 17, 2023
Merged

Fix dup mounter cache behavior #2706

merged 3 commits into from
Sep 17, 2023
+28 −1

Conversation

rajyan
Copy link
Contributor

@rajyan rajyan commented Sep 12, 2023 •
edited
Loading

dup should not prevent uploading

fixes #2700

@rajyan
Copy link
Contributor Author

rajyan commented Sep 12, 2023

I'm very confused with the results, but these test fails inconsistently...

@rajyan rajyan marked this pull request as draft September 13, 2023 00:16
@rajyan rajyan force-pushed the dup-file-upload-failure-failing-test branch 2 times, most recently from 1199008 to 6c75e31 Compare September 13, 2023 00:26
@rajyan rajyan changed the title add failing test case for #2700 Fix dup mounter cache behavior Sep 13, 2023
@rajyan
Copy link
Contributor Author

rajyan commented Sep 13, 2023 •
edited
Loading

I'm not sure yet with

  • If this test is expressing the problem in the issue
  • Why the tests are failing randomly

Also, testing locally with counter_culture, I still could not upload an image like explained in #2700.

@rajyan
Copy link
Contributor Author

rajyan commented Sep 13, 2023 •
edited
Loading

I'm very confused with the results, but these test fails inconsistently...

This was simply cause by test fixture setup...
The first commit passes, because 'test.jpg' is uploaded by another test and the uploaded files isn't cleaned.

I would fix up them.

@rajyan
add failing test cases for carrierwaveuploader#2700
0f4b300 
dup should not prevent uploading
@rajyan rajyan force-pushed the dup-file-upload-failure-failing-test branch from 781f6ae to 0f4b300 Compare September 13, 2023 09:52
@rajyan
Copy link
Contributor Author

rajyan commented Sep 13, 2023

All problems are resolved.
I believe this pull request now fixes #2700

@rajyan rajyan marked this pull request as ready for review September 13, 2023 10:47
@rajyan
Copy link
Contributor Author

rajyan commented Sep 13, 2023

@mshibuya

The last commit in
#2690
broke the @_mounters cache, because the dup is called after nil is set.
Sorry that I couldn't catch it.

@rajyan rajyan mentioned this pull request Sep 13, 2023
Closed
@mshibuya
Copy link
Member

Looks great except for the tiny issue I commented!
This is my mistake as I didn't understand the invocation order of CarrierWave::Mount::Extension#initialize_dup and CarrierWave::ActiveRecord 's #initialize_dup correctly... 😓

@rajyan @mshibuya
Update spec/orm/activerecord_spec.rb
54c97f2 
Co-authored-by: Mitsuhiro Shibuya <mit.shibuya@gmail.com>
@mshibuya mshibuya merged commit fac358c into carrierwaveuploader:master Sep 17, 2023
@mshibuya
Copy link
Member

Thank you so much!

@rajyan rajyan deleted the dup-file-upload-failure-failing-test branch September 17, 2023 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mshibuya mshibuya mshibuya left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upload fails when calling ActiveRecord model 'dup' with after_create or after_update.
2 participants
@rajyan @mshibuya