batter / s3_cors_fileupload

A gem for Rails to allow for uploading of files to AWS-S3 via CORS using the jQuery-File-Upload
MIT License
21 stars 8 forks source link

Adapting to Mongoid Moped::BSON needs change in id field #3

Closed exawatt closed 10 years ago

exawatt commented 10 years ago

Hi,

It was fairly straightforward to port your gem from ActiveRecord to Mongoid, except for one issue I ran into. Mongoid's Moped::BSON::ObjectId.to_json actually returns an hash rather than a string. The effect ("issue/feature") is documented here: https://groups.google.com/forum/#!topic/mongoid/MaXFVw7D_4s

The way to get around it in the s3_cors_fileupload gem is to modify class SourceFile's def to_jq_upload_orig in the source_file.rb file by adding .to_s to the id key, as follows:

  def to_jq_upload
    {
        'id' => id.to_s,     # Add .to_s to the id key
        'name' => file_name,
        'size' => file_size,
        'url' => url,
        'image' => self.is_image?,
        'delete_url' => Rails.application.routes.url_helpers.source_file_path(self, :format => :json)
    }
  end

This won't break anything in ActiveRecord but will make the gem Mongoid adaptable pretty easily.

Without the to_s call, an object gets returned, and the resulting table row HTML for the uploaded file looks like so <tr id="source_file_[object Object]" ... >, instead of the correct <tr id="source_file_38923" ... >. This makes it impossible to do any jQuery manipulations of that table row.

batter commented 10 years ago

So I tried to actually add better support for Mongoid, by way of a model template that is usable with Mongoid. Just pushed it up. Would you mind cloning it down and giving it a try with your app to see if that model template works since it's been a while since I've worked with Mongoid and I don't have a dummy app to play with at the moment?

exawatt commented 10 years ago

There's a slight problem with the new template. It looks like you are using Mongoid's dynamic fields, because you aren't specifying the fields to be saved. In this case, one has to add the include Mongoid::Attributes::Dynamic directive to the beginning of the class. Then, in order to set the fields of the model, the syntax has to be changed from (for instance) self.filename = .... to self[:filename] = ....

This is documented here: http://mongoid.org/en/mongoid/docs/documents.html#dynamic_fields

The modified source_files.rb would then look like so:

class SourceFile
  include Mongoid::Document
  include Mongoid::Attributes::Dynamic

  validates_presence_of :file_name, :file_content_type, :file_size, :key, :bucket

  before_validation do
    self[:file_name] = key.split('/').last if key
    # for some reason, the response from AWS seems to escape the slashes in the keys, this line will unescape the slash
    self[:url] = url.gsub(/%2F/, '/') if url
    self[:file_size] ||= s3_object.size rescue nil
    self[:file_content_type] ||= s3_object.content_type rescue nil
  end
  # cleanup; destroy corresponding file on S3
  after_destroy { s3_object.try(:delete) }

  def to_jq_upload
    {
        'id' => id.to_s,
        'name' => file_name,
        'size' => file_size,
        'url' => url,
        'image' => self.is_image?,
        'delete_url' => Rails.application.routes.url_helpers.source_file_path(self, :format => :json)
    }
  end

...
...

Making these two changes make the new template work out of the box. My preference is also to add a include Mongoid::Timestamp, but I guess that's a personal preference. Great job on the gem!

batter commented 10 years ago

Oh right! Like I said, it's been a while since I've used mongoid and I forgot that you need to actually declare the fields you wish to use on the model itself. And I agree, including the Mongoid::Timestamp module by default makes sense.