filename_valid not compatible with multiple uploader per model changes?

[lawso017]

When reviewing the code during the effort to disable default validations, noted that we are creating a method filename_valid?

Reviewing the code raised two questions:

1) We are only creating one method on the class, and naming it based upon the attribute being mounted; therefore multiple uploaders mounted on a model will not work. Believe we should refactor this method to be {column}_filename_valid? so it will be compatible with multiple uploaders per model.

2) This method currently requires that filename validations be enabled either globally or for the uploader attribute being tested, since it simply calls valid? on the underlying class. That may be OK... but wondering if it might be better to actually run the validation regexp instead of delegating to valid? so the method would give a consistent answer regardless of whether validations are enabled or not?

[p8]

I think we should revert the multicolumn implementation for now. There still are some things we have to workout like {column}_filename_valid? but there might be others. We can create a new release and work on the multicolumn implementation in a branch. It probably should be 1.0.0 since we break some backwards compatibility (dropping ruby 1.9 and using AWS V4 POST authentication).

[lawso017]

@p8 thanks for the clarification, you're right about the validations. Now that I'm actually looking at the diff I see what's happening.

My unit tests started failing after your last round of PR acceptances. They were failing because of the UniqueFilenameValidator and FilenameFormatValidator. The unit tests were using FactoryGirl to assign an attachment in as simple a manner as possible:

  factory :equipment_import do
    document { File.open('spec/models/data/equipment_imports/complete.csv') }
  end

I was not using sample_key or doing any explicit key assignment, and validations were not being triggered. I've since confirmed that the following PR was responsible for the new test failures:

https://github.com/dwilkie/carrierwave_direct/pull/198/commits/6c022cb154b18f9c8461af9e588b3230d8ca0daa

     def key
        return @key if @key.present?
        if present?
 -        self.key = decoded_key # explicitly set key
 +        identifier = model.send("#{mounted_as}_identifier")
 +        self.key = "#{store_dir}/#{identifier}"
        else
          @key = "#{store_dir}/#{guid}/#{FILENAME_WILDCARD}"
        end

As it turns out, assigning the key to decoded_key in the situation where only a document was mounted in a unit test resulted in the library creating a valid key with a UUID embedded in the key string for uniqueness:

DECODED_KEY: uploads/tmp/1465742965-18306-0002-5665/complete.csv

However, the new key logic results in:

KEY FROM MODEL: uploads/equipment_import/document/

... in other words, no key is created by default in the testing scenario.

It seems like creating a valid key by default in this simple use case is desirable. I'm also not sure of the rationale behind this PR.

So at this point I do not see a problem with the multicolumn implementation and think it can remain. Would be happy to add a PR for {column}_filename_valid?, that would be straightforward.

It looks like the unexpected test failures are limited to this change in PR #198. Appreciate your thoughts on the best way to resolve.

Having said that, I agree the new version should be 1.0 based upon the v4 POSTS and dropping 1.9 support.

[p8]

@lawso017 sorry for the late reply. You can probably fix this by setting the key in your factory. Which is probably more consistent with how things work with a real uploads (the key is never empty when an upload is present)

factory :equipment_import do
    document { File.open('spec/models/data/equipment_imports/complete.csv') }
    key { sample_key(DocumentUploader.new) }
end

[kreintjes]

I can confirm @lawso017's issue and @p8's fix (with some adjustments). However, when directly providing a file, shouldn't Carrierwave Direct set the key for you automatically? This also allows to create EquipmentImports in console, giving it a file which is then automatically uploaded with the correct key being stored in the database. This worked nicely in previous versions.

The whole fix that worked for me:

include CarrierWaveDirect::Test::Helpers

factory :equipment_import do
    document { File.open('spec/models/data/equipment_imports/complete.csv') }
    document_key { sample_key(DocumentUploader.new, filename: 'complete.csv') }
end