Using CombinePDF/Prawn/Paperclip all in one method

Question

So I'm not sure if this is a proper question for StackOverflow but with no one else to turn to I figured I'd try here.

Now, the below code works for it's intended purpose. However, I would consider myself a novice with Rails without the experience to foresee any consequences that might arise in the future as my application scales.

So the idea is that when the user clicks on a 'Generate PDF' button, Prawn will generate the custom PDF, CombinePDF will combine this and PDF's from associated sources, then the final PDF will be saved to the Rails.root directory (only because I don't know how to pass a save location to CombinePDF and have searched everywhere), then Paperclip will attach it to its appropriate model, then the original PDF generated in Rails.root will be deleted to clean up the directory.

Orders show action in Orders Controller

def show
  @orders = Order.find(params[:id])
  @properties = Property.find(@orders.property.id)
  @deeds = Deed.where(property_id: @properties, order_id: @orders).all
  @mortgages = Mortgage.where(property_id: @properties, order_id: @orders).all
  @attached_assets = AttachedAsset.where(property_id: @properties, order_id: @orders).all
  @owners = Owner.where(property_id: @properties, order_id: @orders).all
  respond_to do |format|
    format.html
    format.pdf do
      order_pdf = OrderPdf.new(@orders, @properties, @deeds, @mortgages, @attached_assets).render
      combine_order_pdf = CombinePDF.new
      combine_order_pdf << CombinePDF.parse(order_pdf)
      if @deeds.any?
        @deeds.each do |deed|
          combine_order_pdf << CombinePDF.load(deed.document.path)
        end
      end
      if @mortgages.any?
        @mortgages.each do |mtg|
          combine_order_pdf << CombinePDF.load(mtg.document.path)
        end
      end
      if @attached_assets.any?
        @attached_assets.each do |assets|
          combine_order_pdf << CombinePDF.load(assets.asset.path)
        end
      end
      combine_order_pdf.save "order_#{@orders.order_number}.pdf"
      paperclip_pdf = File.open("order_#{@orders.order_number}.pdf")
      @orders.document = paperclip_pdf
      @orders.save
      File.delete("order_#{@orders.order_number}.pdf")
      redirect_to property_order_path(@properties, @orders)
      flash[:success] = "PDF Generated Successfully. Please scroll down."
    end
  end
end

Generate PDF button in Orders show view

<%= link_to fa_icon("files-o"), order_path(@orders, format: "pdf"), class: "m-xs btn btn-lg btn-primary btn-outline" %>

To be clear: This code does work, but what I'm asking is:

Is all this logic appropriate in a controller function or is there a better place for it?
Will this process bottleneck my application?
Is there a better way to save directly to Paperclip after CombinePDF/Prawn do their thing?
Can I pass a save location to CombinePDF so its not in Rails.root?
Is storing Paperclip attachments in Rails.root/public the only way to have them able to be accessed/displayed on an intranet Rails app?
Is there anything I'm not seeing about this method that may put my application at risk for either performance, security, or stability?

Again, I understand if this isn't an appropriate question(s) but I have no one else to ask so if it is then let me know and I'll take it down. I guess to satisfy the 'Answerable' criteria for this question would be anyone who could tell me the rails way of doing this and why and/or answering the above questions. Thanks in advance for any input!

Abhinay Abhinay · Accepted Answer · 2017-07-25T06:08:44

Is all this logic appropriate in a controller function or is there a better place for it?

Yes, ideally your action should be of 7-8 lines and remaining piece can either become another action or go inside a module which you can place in concerns folder. ex: you can take out your pdf logic and write another method inside concerns folder with file name orders_related.rb

module OrdersRelated
              def self.parsing_pdf(orders, properties, deeds, mortgages, attached_assets)
                  order_pdf = OrderPdf.new(orders, properties, deeds, mortgages, attached_assets).render
                  .
                  .
                  @orders.document = paperclip_pdf
                  @orders.save
                  File.delete("order_#{@orders.order_number}.pdf")
                  redirect_to property_order_path(@properties, @orders)
        end
end

Will this process bottleneck my application?

Yes, this kind of processing should alway happen in the background. Won't go into the details of which you should be using as it depends upon your requirements.

Is storing Paperclip attachments in Rails.root/public the only way to have them able to be accessed/displayed on an intranet Rails app?

No, You should be using a storage service like s3 bucket for to keep your files. there are many advantages of doing it which again is out of scope of this question.

Is there anything I'm not seeing about this method that may put my application at risk for either performance, security, or stability?

Yes, your method clearly needs lot of refactoring I can suggest few

Remove .all from all query it is not required
add index to these columns (property, order) in all table
Never save important files to your public directory(use third party storage service)

p.s: I intentionally left two questions as I don't have much experience with CombinePdf.

Using CombinePDF/Prawn/Paperclip all in one method

1 Answers