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

Fix Mounter.blank? method #1746

Merged
merged 1 commit into from
Nov 27, 2015

Conversation

Bonias
Copy link
Contributor

@Bonias Bonias commented Sep 24, 2015

No description provided.

@bensie
Copy link
Member

bensie commented Oct 13, 2015

@Bonias Can you explain this change?

@Bonias
Copy link
Contributor Author

Bonias commented Oct 15, 2015

I just wanted to point out that blank? method is currently broken and tests don't cover it, but I'm not sure that my changes are proper way to fix it or test it.

How it is working now:

@instance.image = nil
@instance.image? # => true

What I expect:

@instance.image = nil
@instance.image? # => false

Looks like problem was introduced here e38789c#diff-bff15cc47155b179e2ad618ed40ebbe2L92. Earlier uploader.blank? was called, which was defined as file.blank?, so we were checking if uploader had file assigned. At current implementation uploaders.empty? (Array#empty?) method is used. This mean that true (wrongly) would be returned when uploaders includes only one uploader without a file.

Why current tests does not cover it?

When expect(@instance).to receive(:read_uploader).with(:image).and_return('') is used
read_identifiers (https://github.com/carrierwaveuploader/carrierwave/blob/master/lib/carrierwave/mounter.rb#L27) returns empty array and thus uploaders (https://github.com/carrierwaveuploader/carrierwave/blob/master/lib/carrierwave/mounter.rb#L31) returns empty array.

When we use @instance.image= then cache method (https://github.com/carrierwaveuploader/carrierwave/blob/master/lib/carrierwave/mounter.rb#L39) is used which sets @uploaders and now @uploaders consist of one uploader without a file which makes whole that truble.

@thomasfedb
Copy link
Contributor

Is the return value of blank? defined anywhere in documentation or comments?

@thomasfedb
Copy link
Contributor

@bensie This appears to be a reasonable change with good justification provided by @Bonias, can it be merged?

@thomasfedb thomasfedb added the bug label Nov 26, 2015
bensie added a commit that referenced this pull request Nov 27, 2015
@bensie bensie merged commit a8ee3f3 into carrierwaveuploader:master Nov 27, 2015
@bensie
Copy link
Member

bensie commented Nov 27, 2015

Thanks @Bonias!

@Bonias Bonias deleted the fix-mounter-blank-method branch November 25, 2019 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants