richardkmichael / carrierwave-activerecord Goto Github PK

• Move all TODOs to GH issues.
• Remove all unnecessary comments.

• Re-factor to use implicit subject and review expectations.
• We use mocks with .and_call_original because the methods being
  tested must execute the rest of the method to decorate the returned
  File. Alternately, we could use .and_return(@active_record_file_mock)
  to inject the correct type of file [build it from a Factory, because
  otherwise, the mocked file and the real File will eventually be out of
  sync.
• Remove deep "implementation" testing, e.g. "calls #create!".
• Instead of specing the return behavior of "store!" and
  "retrieve!", should we spec the return behaviour of only "retrieve!"
  and then specifiy that "store!" and "retrieve!" should return the same
  thing?
• DRY URL spec with "shared_examples" / "it_behaves_like" / helper method module?
• Use a named subject to fix the initialization of the StorageProvider under test.
• Remove indirect and deep testing of File via uploader and should_receive (write it as a stub):
  

  carrierwave-activerecord/spec/lib/carrierwave-activerecord/storage/storage_provider_spec.rb

  Line 89 in 0b48c23

  uploader.should_receive(:filename).with(no_args)

• Can we use its with a tag to call a before block?
  

  carrierwave-activerecord/spec/lib/carrierwave-activerecord/storage/storage_provider_spec.rb

  Line 124 in 0b48c23

  # TODO: Can we use "its" with a tag for a before block?

Instead of writing a new engine using ActiveRecord, perhaps I should have just used the uploader module and extended it?

class ArticleUploader
  include CarrierWave::Mount

  def read_uploader ; end
  def write_uploader ; end

http://rubydoc.info/gems/carrierwave/CarrierWave/Mount

Return class

Should it return CarrierWave::SanitizedFile (like the file store)? The fog store returns a Fog::File instance.

Testing if a file is in the storage engine

What to do about .exists?? It can't be on the AR::Base instance. How to use the AR::Base.exists??

File metadata

What to do about .attributes?

If we want http://app.com/files/my_file.txt to serve my_file.txt, we need a controller [to retrieve the file from the database and serve the content].

How do other non-filesystem storage engines present URLs to their files?

Although the data in the database is stored correctly and files can be stored and retrieved, there is confusion in the Rails log.

Binary data inserted for `string` type on column `file`
...
Binary data inserted for `string` type on column `identifier`

• Migrate rspec specification to minitest/spec.
• Update .gemspec gem.test_files to execute the minitest suite.

Why do uploader.rb and store.rb both include these modules [and, they are included in a different order]?

store.rb:

      include CarrierWave::Uploader::Callbacks
      include CarrierWave::Uploader::Configuration
      include CarrierWave::Uploader::Cache

https://github.com/richardkmichael/carrierwave/blob/master/lib/carrierwave/uploader.rb#L28

lib/carrierwave/storage/active_record.rb

Add the engine to the engines variable in config.

• Remove :path => ... from the Gemfile
• Relax ActiveRecord dependency, only need 3.2.x? Follow the version specified by CarrierWave.
• Add explicit dependencies to .gemspec.

Nested resources need a URL through their parent resource(s), and it must honour the retrieving request's path (in case they belong_to multiple models), ex. /path/via/gallery1/photo/1/file, /path/via/gallery/10/photo/1/file, /path/via/person/5/photo/1/file.

Approaches

1. How can we obtain the request or URL from the controller when inside the model?
   Use Thread.current to store the request. Handled easily by the request_store gem. Then use the Rails router to re-parse the request.url? Or, put only the requisite parts of the parsed request into the Thread local instead of the entire request?
2. Could we decorate the model using ActionView (like FormBuilder) on the way out of the controller?

Can @article.url call a Proc, which is executed with the binding of the view [controller?] to access the request?

def url
  Proc.new do
    # Access the request ... ?  We can capture the request using a Thread local.
  end.call
end

CarrierWave overwrites an existing file when a new file with the same name is uploaded (even if handled by different uploaders), because store_dir defaults to /public/uploads.

The storage path may be configured per uploader by defining 'store_dir'. However, because we store in the database, this is not particularly helpful.

Required setup:

• Database table: we should generate a migration.
• Rails route convention: /<model>/<model_id>/<mounted_attribute>, e.g. /person/1/avatar.

If we can't store (DB unavailable, etc.) or if we can't find, raise a CarrierWave error. What type of error?

See: https://github.com/jnicklas/carrierwave/blob/master/lib/carrierwave/error.rb

There appears to be a bug in CarrierWave::Uploader::Store#filename -> @filename is nil, e.g. @filename is not being set by the cache code. Neither cache! nor retrieve_from_cache! are being called.

• Review CarrierWave's Uploader spec re: Store#filename()

Workaround by adding a filename() method to your uploader:

class FooUploader
  def filename
    self.file.filename
  end
end

CarrierWave::Uploader should proxy the #filename() message to the file, as it does for other convenience methods. E.g. https://github.com/jnicklas/carrierwave/blob/master/lib/carrierwave/uploader/store.rb#L27 could do:

  @filename ||= file.filename if file.respond_to?(:filename)

Probably not, since it would mean needing all the images (to precompile - can't compile on the fly without a filesystem? What about compiling to memcache?) ; and these are uploaded assets.

• table_name => default to 'carrier_wave_files'
• by-pass the local cache (note that the cache sets @filename in the Uploader, should be fixed in CW)

We don't use implicit subject everywhere, that's a smell.

• Some spec is at the wrong level e.g. some StorageProvider spec is talking to Uploader, then File.
• Move some let's to helpers?
• Remove its, it will be gone in RSpec 3: https://gist.github.com/myronmarston/4503509
• Remove "one-expection-per-test" style repetitive testing: betterspecs/betterspecs#5 (comment)
• Review the remove integration spec: https://github.com/richardkmichael/carrierwave-activerecord/blob/0b48c2345d669db8e9af3610bf6552fedb2e472d/spec/integration/activerecord_spec.rb

References

Implicit subject with expect: http://stackoverflow.com/questions/12260534/using-implicit-subject-with-expect-in-rspec-2-11

Implicit subject as lambda: https://gist.github.com/timocratic/816049

Explicit subject as a test smell: http://blog.davidchelimsky.net/2012/05/13/spec-smell-explicit-use-of-subject/

Dynamic context with let: http://perevodik.net/en/posts/33/

Better RSpec techniques: http://betterspecs.org/

Discussion on command query separation, should_receive with and_return, and mocking on test doubles vs. partial mocking: rspec/rspec-expectations#190

The Transfer example, but should be atomic, and using a context seems strange:
https://gist.github.com/RStankov/1510673
http://blog.firsthand.ca/2011/12/example-using-rspec-double-mock-and.html

Re-factor move_to and copy_to, the cut and paste is bad.
https://github.com/jnicklas/carrierwave/blob/master/lib/carrierwave/sanitized_file.rb#L173

Perhaps test inside the file operation, instead of before?
https://github.com/jnicklas/carrierwave/blob/master/lib/carrierwave/uploader/cache.rb#L128

Need spec covering: if uploader.filename == nil, then identifier == nil.

We compute a SHA1 as the identifier because we can't use a filename (.find_by_filename!()), because the filename won't necessarily be unique.

CarrierWave uses a before_save callback to invoke write_identifier: https://github.com/jnicklas/carrierwave/blob/master/lib/carrierwave/orm/activerecord.rb#L29

Because it's a before_save, the file has not been saved and we can't use the carrier_wave_files table ID as the identifier.

(What about giving the file to the storage engine, and receiving an identifier in return? E.g. Call store! and write_identifier in the before_save callback; or call write_identifier in an after_save callback?)

Or, use the filename as the identifier and change retrieval to use the associated model as well (e.g. identifier is effectively filename + model). However, this requires storing the model name and ID in the carrier_wave_files table (otherwise, we won't have any model information to join on).

• Write create specs to cover the case when a same-named file already
  exists; e.g. push into the two contexts.
• Set to a SHA1; it will matter once validations are on
  the ARFile and constraints are in the database.
• In the description of an "it ... do", we can't use a let. In
  the block of an "it ... do", we can't use an instance variable.
  This makes it hard to DRY out the spec: @provider_file_class.
• Should we return anything for CW::B::Proxy#path() and
  CW::SanitizedFile#rewind() ?
• Test content_type= when a file was not found in the database?
• Remove indirect testing of ActiveRecordFile by should_receive:
  https://github.com/richardkmichael/carrierwave-activerecord/blob/master/spec/lib/carrierwave-activerecord/storage/file_spec.rb#L46
  https://github.com/richardkmichael/carrierwave-activerecord/blob/master/spec/lib/carrierwave-activerecord/storage/file_spec.rb#L70
• expect with implicit subject (also see #23) : https://github.com/richardkmichael/carrierwave-activerecord/blob/master/spec/lib/carrierwave-activerecord/storage/file_spec.rb#L108

• Setup rcov, then improve testing coverage.
• Tests are needed for the private ActiveRecordFile class: attributes and table name.
• Test improvements are needed, ensuring duck-typing of the CW::SanitizedFile methods.
• Rspec implicit subject improvements

Add this to CarrierWave directly?

Seems that @article.image_filename would be helpful in views. Currently, we must do @article.image.filename. Not much difference .. but other convenience methods ._url`, etc. are provided.

It appears to be a string, but Rails complains it's binary data.

  SQL (3.2ms)  INSERT INTO "articles" ("created_at", "file", "title", "updated_at") VALUES (?, ?, ?, ?)  [["created_at", Sun, 06 May 2012 10:49:26 UTC +00:00], ["file
", "twitter-setup-800-600.png"], ["title", "test 14"], ["updated_at", Sun, 06 May 2012 10:49:26 UTC +00:00]]
Binary data inserted for `string` type on column `content_type`
  SQL (0.6ms)  INSERT INTO "carrier_wave_files" ("content_type", "created_at", "data", "extension", "filename", "original_filename", "size", "updated_at") VALUES (?, 
?, ?, ?, ?, ?, ?, ?)  [["content_type", "image/png"], ["created_at", Sun, 06 May 2012 10:49:26 UTC +00:00], ["data", "\x89PNG\r\n\x1A\n\x00\x00\x00\rIHDR\x00\x00\x00\
xC8\x00\x00\x00\x96\b\x06\x00\x00\x00\x9B\xDC\xC7\x19\x00\x00\x00\x04gAMA\x00\x00\xB1\x8F\v\xFCa\x05\x00\x00\x00\x01sRGB\x00\xAE\xCE\x1C\xE9\x00\x00\x00 cHRM\x00\x00z
&\x00\x00\x80\x84\x00\x00\xFA\x00\x00\x00\x80\xE8\x00\x00u0\x00\x00\xEA`\x00\x00:\x98\x00\x00\x17p\x9C\xBAQ<\x00\x00\x00\x06bKGD\x00\xFF\x00\xFF\x00\xFF\xA0\xBD\xA7\x
93\x00\x00\x00\tpHYs\x00\x00\v\x13\x00\x00\v\x13\x01\x00\x9A\x9C\x18\x00\x000\x17IDATx\xDA\xED\xBD\xDB\x8F$\xC9\x95\xE6\xF73\xBF{\xDC\xF3\x9Eu\xED.\x16\xBB\xC9n\xB2\x
C9\x19\r\xC9\xE1\x0E\xB1\x02\xB8\x02\xA1\x05\bi\x06|\x9A\x9D}\x1C\f\xF4$\xFD-\x12\xF6A\x8F\x1A\xBD\rV\x80\xB0\xC0\xCCR;Z\x8C0+\xED\x10\x04\xD9\x9C\xAEi\xB2\x97dW_\xAB
*\xAB2+#3\"#\xC2\xEF\xEEfz\x884+\xCF\xA8\xAC\xCC\xA8\xEEjfU\xB7\x7F@vW

No releases in years and years. Forks ahead with PR's that haven't been touched. Whats up?

Feedback wanted on the implementation of:

1. The "decorator" idea to set the url properties of a file: https://github.com/richardkmichael/carrierwave-activerecord/blob/master/lib/carrierwave-activerecord/storage/storage_provider.rb#L31
2. The reflection on Rails to determine the URL: https://github.com/richardkmichael/carrierwave-activerecord/blob/master/lib/carrierwave-activerecord/storage/storage_provider.rb#L66
3. The mocking of Rails in the storage provider spec: https://github.com/richardkmichael/carrierwave-activerecord/blob/master/spec/lib/carrierwave-activerecord/storage/storage_provider_spec.rb#L44

Version 0.1.0rc6 fails to bundle with rails 5 projects because of dependency conflicts. With those resolved, the tests fail to pass because of use of attr_accessible in lib/carrierwave-activerecord/storage/active_record_file.rb

Pull Request incoming, with a version bump so that separate branches and ruby gems can be maintained if you wish going forward. :)

• Use in a non-Rails environment
• Wiki and example project links
• Update cut-and-paste migration: remove content-type and other unused columns
• Update file URL conventions and discussion

Hello

How can I serve files with versions, to get image thumbnail for example?