1

This is the default code I'm using in an associated model to download images from Paperclip as a save_to.

months_controller 

def download
  @wallpaper = Wallpaper.find(params[:wallpaper_id])
  @month = @wallpaper.months.find(params[:id])

  send_file @month.wallpaper_picture.path,
              :filename => @month.wallpaper_picture_file_name,
              :type => @month.wallpaper_picture_content_type,
              :disposition => 'attachment'
end

routes 

resources :wallpapers do  
  resources :months  
end  
match 'wallpaper/:wallpaper_id/download/:id' => 'months#download', :as => :download

view/months/index 

- @months.each do |month|  
  = link_to 'default', download_path(month.wallpaper_id, month.id)

But my application has near 6 Paperclip different styles declared in my model and each must be downloadable. To do that I did this (I'll show just 2 of the 6 code blocks):

months_controller 

  def download_iphone4
    @wallpaper = Wallpaper.find(params[:wallpaper_id])
    @month = @wallpaper.months.find(params[:id])

    @month = 'public/system/wallpaper_pictures/' + @month.id.to_s + '/iphone4/' + @month.wallpaper_picture_file_name
    send_file @month,
              :disposition => 'attachment'
  end

  def download_iphone5
    @wallpaper = Wallpaper.find(params[:wallpaper_id])
    @month = @wallpaper.months.find(params[:id])

    @month = 'public/system/wallpaper_pictures/' + @month.id.to_s + '/iphone5/' + @month.wallpaper_picture_file_name
    send_file @month,
              :disposition => 'attachment'
  end 

  def download_ipad ...
  def download_1440 ...
  def download_1680 ...
  def download_1920 ...
  etc ...

routes 

match 'wallpaper_pictures/:wallpaper_id/iphone4/:id' => 'months#download_iphone4', :as => :download_iphone4  
match 'wallpaper_pictures/:wallpaper_id/iphone5/:id' => 'months#download_iphone5', :as => :download_iphone5  
match 'wallpaper_pictures/:wallpaper_id/ipad4/:id' => 'months#download_ipad', :as => :download_ipad  
match 'wallpaper_pictures/:wallpaper_id/1440/:id' => 'months#download_1440', :as => :download_1440  
match 'wallpaper_pictures/:wallpaper_id/1680/:id' => 'months#download_1680', :as => :download_1680  
match 'wallpaper_pictures/:wallpaper_id/1920/:id' => 'months#download_1920', :as => :download_1920

views/months/index

- @months.each do |month|  
     = link_to 'iphone4', download_iphone4_path(month.wallpaper_id, month.id) 
     = link_to 'iphone5', download_iphone5_path(month.wallpaper_id, month.id) 
     = link_to 'ipad', download_ipad_path(month.wallpaper_id, month.id) 
     = link_to '1440', download_1440_path(month.wallpaper_id, month.id) 
     = link_to '1680', download_1680_path(month.wallpaper_id, month.id) 
     = link_to '1920', download_1920_path(month.wallpaper_id, month.id)

Here come my questions:
1) Can I do it in a cleaner/better way?
2) Must I move the blocks from my controller to the model or a new controller?
3) In the first and default method in the code there are some hashes like: 

:filename => @month.wallpaper_picture_file_name,  
:type => @month.wallpaper_picture_content_type

But in the other method I realized I didn't need to use them. Are those hashes necessary?
4) I call them 'hashers'. Is it correct? Any other correction?

PD: if send_file fails in Production, change it to send_data or
comment out this line in config/production.rb 

config.action_dispatch.x_sendfile_header = "X-Sendfile"

send_file just sends an empty file

Community
  • 1
  • 1
Ricardo Castañeda
  • 5,746
  • 6
  • 28
  • 41
  • 1
    There are many ways this could be done; the controller code should be *trivial* to refactor as-is, though. You could also just use more parameters in the routing instead of hard-coding them. – Dave Newton Jan 02 '13 at 18:09

1 Answers1

3

Refactor your controller code as:

def download_iphone4
  send_file (myfile params, :iphone4), :disposition => 'attachment'
end

def download_iphone5
  send_file (myfile params, :iphone5), :disposition => 'attachment'
end 

def download_ipad ...
def download_1440 ...
def download_1680 ...
def download_1920 ...


private 

def myfile params, style
  @wallpaper = Wallpaper.find(params[:wallpaper_id])
  @month = @wallpaper.months.find(params[:id])
  'public/system/wallpaper_pictures/' + @month.id.to_s + "/#{style}/" +@month.wallpaper_picture_file_name
end

or, You can further DRY this with your routes modifications and controller as:

routes.rb

match 'wallpaper/:wallpaper_id/download/:id/:style' => 'months#download', :as => :download

controller

def download
  send_file (myfile params), :disposition => 'attachment'
end 


private 

def myfile params
  @wallpaper = Wallpaper.find(params[:wallpaper_id])
  @month = @wallpaper.months.find(params[:id])
  'public/system/wallpaper_pictures/' + @month.id.to_s + "/#{params[:style]}/" +@month.wallpaper_picture_file_name
end

and generate path from your views something like:

 = link_to 'iphone4', download_path(month.wallpaper_id, month.id,'iphone4') 
 = link_to 'iphone5', download_path(month.wallpaper_id, month.id,'iphone5')

In both above cases you can move your private method to the wallpaper/month model accordingly.

3) In the first and default method in the code there are some hashes like:

:filename => @month.wallpaper_picture_file_name, :type => @month.wallpaper_picture_content_type

But in the other method I realized I didn't need to use them. Are those hashes necessary?

:filename gives you control over what will be the filename of the file you are sending as attachment, you can modify it and give different file name than of actual file. By default it will return attachment with its actual file name.

:type again works similarly, you can force it to some other content type; however it won't affect much since you are rendering the file already as an attachment.

They are not hashes, they are the key,values to the hashes or can be considered simply as attributes here..

Rahul garg
  • 9,202
  • 5
  • 34
  • 67