Refactor invoice PDF generation, download and delivery

- Remove `Que::Mailer` (#895)
- Extract controllers
- Extract translations
- Convert HAML to ERB
- Add mailer preview
- Improve UI
- Remove unused routes
- Add tests

app/controllers/admin/invoices/delivery_controller.rb

@@ -0,0 +1,13 @@
+module Admin
+  module Invoices
+    class DeliveryController < BaseController
+      include Deliverable
+
+      private
+
+      def redirect_url
+        admin_invoice_path(@invoice)
+      end
+    end
+  end
+end

app/controllers/admin/invoices_controller.rb

@@ -2,8 +2,6 @@ module Admin
   class InvoicesController < BaseController
     load_and_authorize_resource
 
-    before_action :set_invoice, only: [:forward, :download_pdf]
-
     def new
       @deposit = Deposit.new
     end
@@ -28,33 +26,16 @@ module Admin
       @invoices = @q.result.page(params[:page])
     end
 
-    def show
-      @invoice = Invoice.find(params[:id])
-    end
+    def show; end
 
     def cancel
       @invoice.cancel
       redirect_to [:admin, @invoice], notice: t('.cancelled')
     end
 
-    def forward
-      @invoice.billing_email = @invoice.buyer.billing_email
-
-      return unless request.post?
-
-      @invoice.billing_email = params[:invoice][:billing_email]
-
-      if @invoice.forward(render_to_string('registrar/invoices/pdf', layout: false))
-        flash[:notice] = t(:invoice_forwared)
-        redirect_to([:admin, @invoice])
-      else
-        flash.now[:alert] = t(:failed_to_forward_invoice)
-      end
-    end
-
-    def download_pdf
-      pdf = @invoice.pdf(render_to_string('registrar/invoices/pdf', layout: false))
-      send_data pdf, filename: @invoice.pdf_name
+    def download
+      filename = "invoice-#{@invoice.number}.pdf"
+      send_data @invoice.as_pdf, filename: filename
     end
 
     private
@@ -62,9 +43,5 @@ module Admin
     def deposit_params
       params.require(:deposit).permit(:amount, :description, :registrar_id)
     end
-
-    def set_invoice
-      @invoice = Invoice.find(params[:invoice_id])
-    end
   end
 end

app/controllers/concerns/deliverable.rb

@@ -0,0 +1,26 @@
+module Deliverable
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :find_invoice
+  end
+
+  def new
+    authorize! :manage, @invoice
+    @recipient = @invoice.buyer.billing_email
+  end
+
+  def create
+    authorize! :manage, @invoice
+
+    InvoiceMailer.invoice_email(invoice: @invoice, recipient: params[:recipient]).deliver_now
+
+    redirect_to redirect_url, notice: t('.delivered')
+  end
+
+  private
+
+  def find_invoice
+    @invoice = Invoice.find(params[:invoice_id])
+  end
+end

app/controllers/registrar/invoices/delivery_controller.rb

@@ -0,0 +1,13 @@
+class Registrar
+  module Invoices
+    class DeliveryController < BaseController
+      include Deliverable
+
+      private
+
+      def redirect_url
+        registrar_invoice_path(@invoice)
+      end
+    end
+  end
+end

app/controllers/registrar/invoices_controller.rb

@@ -2,8 +2,6 @@ class Registrar
   class InvoicesController < BaseController
     load_and_authorize_resource
 
-    before_action :set_invoice, only: [:show, :forward, :download_pdf]
-
     def index
       params[:q] ||= {}
       invoices = current_registrar_user.registrar.invoices.includes(:items, :account_activity)
@@ -15,40 +13,20 @@ class Registrar
       end
     end
 
-    def show;
-    end
-
-    def forward
-      @invoice.billing_email = @invoice.buyer.billing_email
-
-      return unless request.post?
-
-      @invoice.billing_email = params[:invoice][:billing_email]
-
-      if @invoice.forward(render_to_string('pdf', layout: false))
-        flash[:notice] = t(:invoice_forwared)
-        redirect_to([:registrar, @invoice])
-      else
-        flash.now[:alert] = t(:failed_to_forward_invoice)
-      end
-    end
+    def show; end
 
     def cancel
       @invoice.cancel
       redirect_to [:registrar, @invoice], notice: t('.cancelled')
     end
 
-    def download_pdf
-      pdf = @invoice.pdf(render_to_string('pdf', layout: false))
-      send_data pdf, filename: @invoice.pdf_name
+    def download
+      filename = "invoice-#{@invoice.number}.pdf"
+      send_data @invoice.as_pdf, filename: filename
     end
 
     private
 
-    def set_invoice
-      @invoice = Invoice.find(params[:id])
-    end
-
     def normalize_search_parameters
       params[:q][:total_gteq].gsub!(',', '.') if params[:q][:total_gteq]
       params[:q][:total_lteq].gsub!(',', '.') if params[:q][:total_lteq]

app/mailers/invoice_mailer.rb

@@ -1,16 +1,9 @@
 class InvoiceMailer < ApplicationMailer
-  include Que::Mailer
+  def invoice_email(invoice:, recipient:)
+    @invoice = invoice
 
-  def invoice_email(invoice_id, html, billing_email)
-    @invoice = Invoice.find_by(id: invoice_id)
-    billing_email ||= @invoice.billing_email
-    return unless @invoice
-
-    kit = PDFKit.new(html)
-    pdf = kit.to_pdf
-    invoice = @invoice
-
-    attachments[invoice.pdf_name] = pdf
-    mail(to: format(billing_email), subject: invoice)
+    subject = default_i18n_subject(invoice_number: invoice.number)
+    attachments["invoice-#{invoice.number}.pdf"] = invoice.as_pdf
+    mail(to: recipient, subject: subject)
   end
-end
+end

app/models/invoice.rb

@@ -26,9 +26,6 @@ class Invoice < ActiveRecord::Base
 
   scope :overdue, -> { unpaid.non_cancelled.where('due_date < ?', Time.zone.today) }
 
-  attr_accessor :billing_email
-  validates :billing_email, email_format: { message: :invalid }, allow_blank: true
-
   validates :issue_date, presence: true
   validates :due_date, :currency, :seller_name,
             :seller_iban, :buyer_name, :items, presence: true
@@ -84,23 +81,6 @@ class Invoice < ActiveRecord::Base
     "Order nr. #{number}"
   end
 
-  def pdf(html)
-    kit = PDFKit.new(html)
-    kit.to_pdf
-  end
-
-  def pdf_name
-    "invoice-#{number}.pdf"
-  end
-
-  def forward(html)
-    return false unless valid?
-    return false unless billing_email.present?
-
-    InvoiceMailer.invoice_email(id, html, billing_email).deliver
-    true
-  end
-
   def subtotal
     items.map(&:item_sum_without_vat).reduce(:+)
   end
@@ -119,6 +99,11 @@ class Invoice < ActiveRecord::Base
     items.each { |item| yield item }
   end
 
+  def as_pdf
+    generator = PdfGenerator.new(self)
+    generator.as_pdf
+  end
+
   private
 
   def apply_default_vat_rate
@@ -132,4 +117,4 @@ class Invoice < ActiveRecord::Base
   def calculate_total
     self.total = subtotal + vat_amount
   end
-end
+end

app/models/invoice/pdf_generator.rb

@@ -0,0 +1,22 @@
+class Invoice
+  class PdfGenerator
+    attr_reader :invoice
+
+    def initialize(invoice)
+      @invoice = invoice
+    end
+
+    def as_pdf
+      generator = PDFKit.new(invoice_html)
+      generator.to_pdf
+    end
+
+    private
+
+    def invoice_html
+      view = ActionView::Base.new(ActionController::Base.view_paths, invoice: invoice)
+      view.class_eval { include ApplicationHelper }
+      view.render(file: 'invoice/pdf', layout: false)
+    end
+  end
+end

app/views/admin/invoices/delivery/new.html.erb

@@ -0,0 +1,25 @@
+<ol class="breadcrumb">
+    <li><%= link_to t('admin.invoices.index.header'), admin_invoices_path %></li>
+    <li><%= link_to @invoice, admin_invoice_path(@invoice) %></li>
+</ol>
+
+<div class="page-header">
+    <h1><%= t '.header' %></h1>
+</div>
+
+<%= form_tag(admin_invoice_delivery_path(@invoice)) do %>
+    <div class="row">
+        <div class="col-md-4 col-md-offset-4">
+            <div class="form-group">
+                <%= label_tag :recipient %>
+                <%= email_field_tag :recipient, @recipient, required: true, autofocus: true,
+                                    class: 'form-control' %>
+            </div>
+            <div class="row">
+                <div class="col-md-12 text-right">
+                    <%= submit_tag t('.submit_btn'), class: 'btn btn-warning' %>
+                </div>
+            </div>
+        </div>
+    </div>
+<% end %>

app/views/admin/invoices/forward.haml

@@ -1,15 +0,0 @@
-- content_for :actions do
-  = link_to(t(:back_to_invoice), admin_invoice_path(@invoice), class: 'btn btn-default')
-= render 'shared/title', name: t(:forward_invoice)
-
-= form_for([:admin, @invoice], url: { action: :forward }, method: :post) do |f|
-  .row
-    .col-md-4.col-md-offset-4
-      = render 'shared/full_errors', object: @invoice
-      .form-group
-        = f.label :billing_email
-        = f.text_field :billing_email, class: 'form-control', autocomplete: 'off'
-
-      .row
-        .col-md-12.text-right
-          = button_tag(t(:forward), class: 'btn btn-warning')

app/views/admin/invoices/show.haml

@@ -6,8 +6,8 @@
     %h1.text-right.text-center-xs
       - if @invoice.unpaid?
         = link_to(t(:payment_received), new_admin_bank_statement_path(invoice_id: @invoice.id), class: 'btn btn-default')
-      = link_to(t(:download), admin_invoice_download_pdf_path(@invoice), class: 'btn btn-default')
-      = link_to(t(:forward), admin_invoice_forward_path(@invoice), class: 'btn btn-default')
+      = link_to(t('.download_btn'), download_admin_invoice_path(@invoice), class: 'btn btn-default')
+      = link_to(t('.deliver_btn'), new_admin_invoice_delivery_path(@invoice), class: 'btn btn-default')
       - if @invoice.cancellable?
         = link_to(t(:cancel), cancel_admin_invoice_path(@invoice), method: :patch, class: 'btn btn-warning')
       = link_to(t(:back), admin_invoices_path, class: 'btn btn-default')

app/views/registrar/invoices/delivery/new.html.erb

@@ -0,0 +1,25 @@
+<ol class="breadcrumb">
+    <li><%= link_to t('registrar.invoices.index.header'), registrar_invoices_path %></li>
+    <li><%= link_to @invoice, registrar_invoice_path(@invoice) %></li>
+</ol>
+
+<div class="page-header">
+    <h1><%= t '.header' %></h1>
+</div>
+
+<%= form_tag(registrar_invoice_delivery_path(@invoice)) do %>
+    <div class="row">
+        <div class="col-md-4 col-md-offset-4">
+            <div class="form-group">
+                <%= label_tag :recipient %>
+                <%= email_field_tag :recipient, @recipient, required: true, autofocus: true,
+                                    class: 'form-control' %>
+            </div>
+            <div class="row">
+                <div class="col-md-12 text-right">
+                    <%= submit_tag t('.submit_btn'), class: 'btn btn-warning' %>
+                </div>
+            </div>
+        </div>
+    </div>
+<% end %>

app/views/registrar/invoices/forward.haml

@@ -1,15 +0,0 @@
-- content_for :actions do
-  = link_to(t(:back_to_invoice), registrar_invoice_path(@invoice), class: 'btn btn-default')
-= render 'shared/title', name: t(:forward_invoice)
-
-= form_for([:registrar, @invoice], url: { action: :forward }, method: :post) do |f|
-  .row
-    .col-md-4.col-md-offset-4
-      = render 'shared/full_errors', object: @invoice
-      .form-group
-        = f.label :billing_email
-        = f.text_field :billing_email, class: 'form-control', autocomplete: 'off'
-
-      .row
-        .col-md-12.text-right
-          = button_tag(t(:forward), class: 'btn btn-warning')

app/views/registrar/invoices/show.haml

@@ -1,6 +1,6 @@
 - content_for :actions do
-  = link_to(t(:download), download_pdf_registrar_invoice_path(@invoice), class: 'btn btn-default')
-  = link_to(t(:forward), forward_registrar_invoice_path(@invoice), class: 'btn btn-default')
+  = link_to(t('.download_btn'), download_registrar_invoice_path(@invoice), class: 'btn btn-default')
+  = link_to(t('.deliver_btn'), new_registrar_invoice_delivery_path(@invoice), class: 'btn btn-default')
   - if @invoice.cancellable?
     = link_to(t(:cancel), cancel_registrar_invoice_path(@invoice), method: :patch, class: 'btn btn-warning')
   = link_to(t(:back), registrar_invoices_path, class: 'btn btn-default')

config/locales/admin/invoices.en.yml

@@ -1,5 +1,12 @@
 en:
   admin:
     invoices:
+      index:
+        header: Invoices
+
+      show:
+        download_btn: Download
+        deliver_btn: Send
+
       cancel:
         cancelled: Invoice has been cancelled

config/locales/admin/invoices/delivery.en.yml

@@ -0,0 +1,10 @@
+en:
+  admin:
+    invoices:
+      delivery:
+        new:
+          header: Send invoice
+          submit_btn: Send
+
+        create:
+          delivered: Invoice has been sent

config/locales/en.yml

@@ -558,11 +558,6 @@ en:
   registrant_head_title: 'EIS Registrant'
   registrant_head_title_sufix: ' - EIS Registrant'
   bind_manually: 'Bind manually'
-  forward_invoice: 'Forward invoice'
-  forward: 'Forward'
-  back_to_invoice: 'Back to invoice'
-  invoice_forwared: 'Invoice forwarded'
-  failed_to_forward_invoice: 'Failed to forward invoice'
   client: 'Client'
   you_have_a_new_invoice: 'You have a new invoice.'
   sincerely: 'Sincerely'

config/locales/mailers/invoice.en.yml

@@ -0,0 +1,4 @@
+en:
+  invoice_mailer:
+    invoice_email:
+      subject: Invoice no. %{invoice_number}

config/locales/registrar/invoices.en.yml

@@ -7,8 +7,14 @@ en:
       go_to_intermediary: 'Go to intermediary'
       pay_by_credit_card: Pay by credit card
       payment_complete: Credit Card payment Complete
+
       index:
+        header: Invoices
         reset_btn: Reset
 
+      show:
+        download_btn: Download
+        deliver_btn: Send
+
       cancel:
         cancelled: Invoice has been cancelled

config/locales/registrar/invoices/delivery.en.yml

@@ -0,0 +1,10 @@
+en:
+  registrar:
+    invoices:
+      delivery:
+        new:
+          header: Send invoice
+          submit_btn: Send
+
+        create:
+          delivered: Invoice has been sent

config/routes.rb

@@ -53,10 +53,11 @@ Rails.application.routes.draw do
       post 'mid' => 'sessions#mid'
     end
 
-    resources :invoices do
+    resources :invoices, except: %i[new create edit update destroy] do
+      resource :delivery, controller: 'invoices/delivery', only: %i[new create]
+
       member do
-        get 'download_pdf'
-        match 'forward', via: [:post, :get]
+        get 'download'
         patch 'cancel'
       end
     end
@@ -199,10 +200,13 @@ Rails.application.routes.draw do
       patch 'bind', on: :member
     end
 
-    resources :invoices do
-      get 'download_pdf'
-      patch 'cancel', on: :member
-      match 'forward', via: [:post, :get]
+    resources :invoices, except: %i[edit update destroy] do
+      resource :delivery, controller: 'invoices/delivery', only: %i[new create]
+
+      member do
+        get 'download'
+        patch 'cancel'
+      end
     end
 
     resources :domains, except: %i[new create destroy] do

test/fixtures/invoices.yml

@@ -9,6 +9,7 @@ one:
   vat_rate: 0.1
   total: 16.50
   reference_no: 13
+  number: 1
 
 for_payments_test:
   number: 1

test/fixtures/registrars.yml

@@ -10,7 +10,7 @@ bestnames:
   country_code: US
   accounting_customer_code: bestnames
   language: en
-  billing_email: billing@example.com
+  billing_email: billing@bestnames.test
   website:  https://bestnames.test
   reference_no: 13
 

test/integration/admin_area/invoices_test.rb

@@ -6,8 +6,14 @@ class AdminAreaInvoicesIntegrationTest < ApplicationIntegrationTest
     sign_in users(:admin)
   end
 
-  def test_download_invoice_pdf
-    get admin_invoice_download_pdf_path(@invoice)
+  def test_downloads_invoice
+    assert_equal 1, @invoice.number
+
+    get download_admin_invoice_path(@invoice)
+
     assert_response :ok
+    assert_equal 'application/pdf', response.headers['Content-Type']
+    assert_equal 'attachment; filename="invoice-1.pdf"', response.headers['Content-Disposition']
+    assert_not_empty response.body
   end
 end

test/integration/registrar_area/invoices_test.rb

@@ -6,8 +6,14 @@ class RegistrarAreaInvoicesIntegrationTest < ApplicationIntegrationTest
     sign_in users(:api_bestnames)
   end
 
-  def test_download_invoice_pdf
-    get download_pdf_registrar_invoice_path(@invoice)
+  def test_downloads_invoice
+    assert_equal 1, @invoice.number
+
+    get download_registrar_invoice_path(@invoice)
+
     assert_response :ok
+    assert_equal 'application/pdf', response.headers['Content-Type']
+    assert_equal 'attachment; filename="invoice-1.pdf"', response.headers['Content-Disposition']
+    assert_not_empty response.body
   end
 end

test/mailers/invoice_mailer_test.rb

@@ -0,0 +1,22 @@
+require 'test_helper'
+
+class InvoiceMailerTest < ActiveSupport::TestCase
+  include ActionMailer::TestHelper
+
+  setup do
+    @invoice = invoices(:one)
+    ActionMailer::Base.deliveries.clear
+  end
+
+  def test_delivers_invoice_email
+    assert_equal 1, @invoice.number
+
+    email = InvoiceMailer.invoice_email(invoice: @invoice, recipient: 'billing@bestnames.test')
+              .deliver_now
+
+    assert_emails 1
+    assert_equal ['billing@bestnames.test'], email.to
+    assert_equal 'Invoice no. 1', email.subject
+    assert email.attachments['invoice-1.pdf']
+  end
+end

test/mailers/previews/invoice_mailer_preview.rb

@@ -0,0 +1,6 @@
+class InvoiceMailerPreview < ActionMailer::Preview
+  def invoice_email
+    invoice = Invoice.first
+    InvoiceMailer.invoice_email(invoice: invoice, recipient: 'billing@registrar.test')
+  end
+end

test/system/admin_area/invoices_test.rb

@@ -1,9 +1,13 @@
 require 'test_helper'
 
 class AdminAreaInvoicesTest < ApplicationSystemTestCase
+  include ActionMailer::TestHelper
+
   setup do
     sign_in users(:admin)
     @invoice = invoices(:one)
+
+    ActionMailer::Base.deliveries.clear
   end
 
   def test_cancels_an_invoice
@@ -17,4 +21,23 @@ class AdminAreaInvoicesTest < ApplicationSystemTestCase
     assert @invoice.cancelled?
     assert_text 'Invoice has been cancelled'
   end
+
+  def test_invoice_delivery_form_is_pre_populated_with_billing_email_of_a_registrar
+    assert_equal 'billing@bestnames.test', @invoice.buyer.billing_email
+    visit new_admin_invoice_delivery_url(@invoice)
+    assert_field 'Recipient', with: 'billing@bestnames.test'
+  end
+
+  def test_delivers_an_invoice
+    visit admin_invoice_url(@invoice)
+    click_on 'Send'
+    fill_in 'Recipient', with: 'billing@registrar.test'
+    click_on 'Send'
+
+    assert_emails 1
+    email = ActionMailer::Base.deliveries.first
+    assert_equal ['billing@registrar.test'], email.to
+    assert_current_path admin_invoice_path(@invoice)
+    assert_text 'Invoice has been sent'
+  end
 end

test/system/registrar_area/invoices_test.rb

@@ -1,9 +1,13 @@
 require 'test_helper'
 
 class RegistrarAreaInvoicesTest < ApplicationSystemTestCase
+  include ActionMailer::TestHelper
+
   setup do
     sign_in users(:api_bestnames)
     @invoice = invoices(:one)
+
+    ActionMailer::Base.deliveries.clear
   end
 
   def test_cancels_an_invoice
@@ -17,4 +21,23 @@ class RegistrarAreaInvoicesTest < ApplicationSystemTestCase
     assert @invoice.cancelled?
     assert_text 'Invoice has been cancelled'
   end
+
+  def test_invoice_delivery_form_is_pre_populated_with_billing_email_of_a_registrar
+    assert_equal 'billing@bestnames.test', @invoice.buyer.billing_email
+    visit new_registrar_invoice_delivery_url(@invoice)
+    assert_field 'Recipient', with: 'billing@bestnames.test'
+  end
+
+  def test_delivers_an_invoice
+    visit registrar_invoice_url(@invoice)
+    click_on 'Send'
+    fill_in 'Recipient', with: 'billing@registrar.test'
+    click_on 'Send'
+
+    assert_emails 1
+    email = ActionMailer::Base.deliveries.first
+    assert_equal ['billing@registrar.test'], email.to
+    assert_current_path registrar_invoice_path(@invoice)
+    assert_text 'Invoice has been sent'
+  end
 end