diff --git a/Gemfile.lock b/Gemfile.lock index 1f18e05da..0fdd35cac 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -485,7 +485,7 @@ GEM railties (>= 3.2) jquery-ui-rails (8.0.0) railties (>= 3.2.16) - json (2.20.0) + json (2.21.2) json-jwt (1.17.1) activesupport (>= 4.2) aes_key_wrap diff --git a/app/admin/billing/invoice_template_playground.rb b/app/admin/billing/invoice_template_playground.rb index 8e8f57526..67a61b955 100644 --- a/app/admin/billing/invoice_template_playground.rb +++ b/app/admin/billing/invoice_template_playground.rb @@ -13,27 +13,42 @@ invoice = Billing::Invoice.find(params[:invoice_id]) return render(plain: 'Not authorized to read this invoice', status: 403) unless authorized?(:read, invoice) + # A blank filename template is invalid here (the column is NOT NULL and the + # model requires it), so it is caught locally — previewing a document that + # could never be generated for real would be misleading, and there is + # nothing to ask yeti-pdf about. + if params[:filename_template].blank? + return render(plain: 'Filename template must not be blank', status: :unprocessable_entity) + end + data = BillingInvoice::InvoiceData.call(invoice: invoice) - pdf = YetiPdf::Client.render_pdf(template: params[:template].to_s, data: data) - send_data pdf, type: 'application/pdf', disposition: 'inline' + result = YetiPdf::Client.render_pdf( + template: params[:template].to_s, + filename_template: params[:filename_template].to_s, + data: data + ) + # The name the document would be filed under, for the editor to display — + # a header, so it rides along with the inline PDF the iframe is loading. + response.set_header('X-Rendered-Filename', ERB::Util.url_encode(result.filename.to_s)) + send_data result.pdf, type: 'application/pdf', disposition: 'inline' rescue ActiveRecord::RecordNotFound render plain: 'Invoice not found', status: 404 rescue YetiPdf::Client::Error => e render plain: e.message, status: :unprocessable_entity end - # GET ?template_id= -> the saved html_template, for the "Rollback" button. + # GET ?template_id= -> the saved templates, for the "Rollback" button. page_action :template, method: :get do template = Billing::InvoiceTemplate.find(params[:template_id]) - render json: { html_template: template.html_template.to_s } + render json: { html_template: template.html_template.to_s, filename_template: template.filename_template.to_s } rescue ActiveRecord::RecordNotFound - render json: { html_template: '' }, status: 404 + render json: { html_template: '', filename_template: '' }, status: 404 end - # PATCH -> persist the edited html_template back to the template. + # PATCH -> persist the edited templates back to the template record. page_action :save, method: :patch do template = Billing::InvoiceTemplate.find(params[:template_id]) - template.update!(html_template: params[:html_template].to_s) + template.update!(html_template: params[:html_template].to_s, filename_template: params[:filename_template].to_s) render json: { ok: true } rescue ActiveRecord::RecordNotFound render json: { error: 'Template not found' }, status: 404 @@ -66,6 +81,16 @@ end end + div class: 'tp-filename-row', + style: 'display:flex; gap:8px; align-items:center; margin-bottom:8px;' do + label 'Filename:', for: 'tp-filename' + input type: 'text', id: 'tp-filename', spellcheck: 'false', style: 'flex:1;', + value: template&.filename_template.to_s + # Filled from the preview response with the name yeti-pdf actually + # rendered — the template author sees the result, not just the source. + span '', id: 'tp-filename-preview', style: 'opacity:.75; white-space:nowrap;' + end + div class: 'tp-body' do div class: 'tp-editor-pane' do textarea template&.html_template.to_s, id: 'tp-template', class: 'tp-editor', spellcheck: 'false' diff --git a/app/admin/billing/invoice_templates.rb b/app/admin/billing/invoice_templates.rb index b990fba7e..cd3da2171 100644 --- a/app/admin/billing/invoice_templates.rb +++ b/app/admin/billing/invoice_templates.rb @@ -6,7 +6,7 @@ actions :all # :index,:create, :new, :destroy, :delete, :edit, :update before_action :left_sidebar! - permit_params :name, :html_template + permit_params :name, :html_template, :filename_template acts_as_export :id, :name, :created_at @@ -39,6 +39,10 @@ def find_resource f.semantic_errors *f.object.errors.attribute_names f.inputs form_title do f.input :name + f.input :filename_template, + hint: 'pongo2 template naming the generated file, rendered against the same data as the ' \ + 'HTML template. No extension — ".pdf" is added when the document is served. ' \ + 'Example: {{ invoice.reference }}_{{ invoice.start_date|strfdate:"%Y-%m" }}' f.input :html_template, as: :text, input_html: { rows: 24 } end f.actions @@ -48,6 +52,7 @@ def find_resource attributes_table do row :id row :name + row :filename_template row :created_at end diff --git a/app/assets/javascripts/template_playground.js b/app/assets/javascripts/template_playground.js index 5afa61867..e08de934a 100644 --- a/app/assets/javascripts/template_playground.js +++ b/app/assets/javascripts/template_playground.js @@ -11,6 +11,8 @@ $(document).ready(function () { var csrf = csrfEl ? csrfEl.content : ''; var invoiceSel = document.getElementById('tp-invoice'); var editor = document.getElementById('tp-template'); + var filenameInput = document.getElementById('tp-filename'); + var filenamePreview = document.getElementById('tp-filename-preview'); var iframe = document.getElementById('tp-pdf'); var errorBox = document.getElementById('tp-error'); var body = container.querySelector('.tp-body'); @@ -84,6 +86,14 @@ $(document).ready(function () { errorBox.style.display = 'none'; } + // The name the document would be stored under, as rendered by yeti-pdf + // (URL-encoded by the preview action so it survives the header). + function showFilename(encoded) { + if (!filenamePreview) return; + var name = encoded ? decodeURIComponent(encoded) : ''; + filenamePreview.textContent = name ? '→ ' + name + '.pdf' : ''; + } + // Render the current template against the selected invoice. A spinner is // shown over the previous PDF until the response arrives. var spinner = container.querySelector('.tp-spinner'); @@ -103,13 +113,19 @@ $(document).ready(function () { 'X-CSRF-Token': csrf, 'Accept': 'application/pdf' }, - body: JSON.stringify({ invoice_id: invoiceSel.value, template: templateValue() }) + body: JSON.stringify({ + invoice_id: invoiceSel.value, + template: templateValue(), + filename_template: filenameInput ? filenameInput.value : '' + }) }).then(function (resp) { if (resp.ok) { + showFilename(resp.headers.get('X-Rendered-Filename')); return resp.blob().then(function (blob) { iframe.src = URL.createObjectURL(blob); }); } + showFilename(''); return resp.text().then(function (t) { showError('Render failed: ' + t); }); }).catch(function (e) { showError('Request failed: ' + e.message); @@ -125,6 +141,7 @@ $(document).ready(function () { } invoiceSel.addEventListener('change', render); // immediate on invoice change + if (filenameInput) { filenameInput.addEventListener('input', scheduleRender); } if (cm) { cm.on('change', scheduleRender); } else { @@ -146,7 +163,11 @@ $(document).ready(function () { hideError(); fetch(templateUrl + '?template_id=' + encodeURIComponent(templateId), { headers: { 'Accept': 'application/json' } }) .then(function (r) { return r.json(); }) - .then(function (j) { setTemplate(j.html_template || ''); render(); }) + .then(function (j) { + setTemplate(j.html_template || ''); + if (filenameInput) { filenameInput.value = j.filename_template || ''; } + render(); + }) .catch(function (e) { showError('Reload failed: ' + e.message); }); }); @@ -157,7 +178,11 @@ $(document).ready(function () { fetch(saveUrl, { method: 'PATCH', headers: { 'Content-Type': 'application/json', 'X-CSRF-Token': csrf, 'Accept': 'application/json' }, - body: JSON.stringify({ template_id: templateId, html_template: templateValue() }) + body: JSON.stringify({ + template_id: templateId, + html_template: templateValue(), + filename_template: filenameInput ? filenameInput.value : '' + }) }).then(function (r) { if (r.ok) { if (saveStatus) { diff --git a/app/controllers/api/rest/admin/invoices_controller.rb b/app/controllers/api/rest/admin/invoices_controller.rb index e68f6089b..4d9cdef87 100644 --- a/app/controllers/api/rest/admin/invoices_controller.rb +++ b/app/controllers/api/rest/admin/invoices_controller.rb @@ -5,6 +5,6 @@ def pdf doc = ::Billing::InvoiceDocument.find_by(invoice_id: params[:id]) return head 404 if doc.nil? || doc.pdf_data.blank? - send_data doc.pdf_data, filename: "invoice-#{params[:id]}.pdf" + send_data doc.pdf_data, filename: "#{doc.filename}.pdf" end end diff --git a/app/models/billing/invoice.rb b/app/models/billing/invoice.rb index e41a6a99b..b12237217 100644 --- a/app/models/billing/invoice.rb +++ b/app/models/billing/invoice.rb @@ -210,10 +210,6 @@ def regenerate_document end end - def file_name - "#{id}_#{start_date}_#{end_date}" - end - delegate :contacts_for_invoices, :invoice_period, to: :account def subject diff --git a/app/models/billing/invoice_template.rb b/app/models/billing/invoice_template.rb index 81abb6131..60e8c1c29 100644 --- a/app/models/billing/invoice_template.rb +++ b/app/models/billing/invoice_template.rb @@ -5,10 +5,11 @@ # Table name: billing.invoice_templates # Database name: primary # -# id :integer(4) not null, primary key -# html_template :text -# name :string not null -# created_at :timestamptz +# id :integer(4) not null, primary key +# filename_template :string default("invoice-{{invoice.reference}}"), not null +# html_template :text +# name :string not null +# created_at :timestamptz # # Indexes # @@ -19,6 +20,9 @@ class Billing::InvoiceTemplate < ApplicationRecord self.table_name = 'billing.invoice_templates' validates :name, presence: true, uniqueness: true validates :html_template, presence: true + # NOT NULL alone would still allow '', which yeti-pdf treats as "no filename + # template" and would leave the document with no name to store. + validates :filename_template, presence: true, length: { maximum: 255 } def display_name name diff --git a/app/resources/api/rest/admin/invoice_template_resource.rb b/app/resources/api/rest/admin/invoice_template_resource.rb index 9b93c16a5..dc81d47b3 100644 --- a/app/resources/api/rest/admin/invoice_template_resource.rb +++ b/app/resources/api/rest/admin/invoice_template_resource.rb @@ -3,7 +3,7 @@ class Api::Rest::Admin::InvoiceTemplateResource < ::BaseResource model_name 'Billing::InvoiceTemplate' - attributes :name, :html_template + attributes :name, :html_template, :filename_template paginator :paged diff --git a/app/services/billing_invoice/generate_document.rb b/app/services/billing_invoice/generate_document.rb index 57fbfeffc..fd0ec9053 100644 --- a/app/services/billing_invoice/generate_document.rb +++ b/app/services/billing_invoice/generate_document.rb @@ -1,8 +1,9 @@ # frozen_string_literal: true # Generates the invoice PDF document: builds the raw invoice data payload, sends -# it with the account's html_template to the external yeti-pdf service, and -# stores the returned PDF on the InvoiceDocument. +# it with the account's html_template and filename_template to the external +# yeti-pdf service, and stores the returned PDF and rendered name on the +# InvoiceDocument. # # Any generation failure (no template, yeti-pdf not configured, yeti-pdf error) # is recorded on invoice.pdf_error and swallowed, so a rendering problem never @@ -22,6 +23,15 @@ def initialize(invoice_id) end end + # yeti-pdf rejects a filename template that renders to something unusable, + # so reaching this means it answered without the header at all — a version + # that predates filename_template support. + class FilenameMissing < Error + def initialize(invoice_id) + super("yeti-pdf returned no filename for invoice #{invoice_id}") + end + end + parameter :invoice, required: true def call @@ -39,12 +49,17 @@ def generate_document! raise PdfApiNotConfigured, invoice.id unless YetiPdf::Client.configured? data = InvoiceData.call(invoice: invoice) - pdf_data = YetiPdf::Client.render_pdf(template: template.html_template, data: data) + result = YetiPdf::Client.render_pdf( + template: template.html_template, + filename_template: template.filename_template, + data: data + ) + raise FilenameMissing, invoice.id if result.filename.blank? Billing::InvoiceDocument.create!( invoice: invoice, - filename: invoice.file_name.to_s, - pdf_data: pdf_data + filename: result.filename, + pdf_data: result.pdf ) end diff --git a/app/services/yeti_pdf/client.rb b/app/services/yeti_pdf/client.rb index 8bd304874..12d0cd646 100644 --- a/app/services/yeti_pdf/client.rb +++ b/app/services/yeti_pdf/client.rb @@ -3,8 +3,9 @@ require 'httpx' # Client for the external yeti-pdf render service: POSTs a pongo2 template plus -# a JSON data payload and returns either the rendered PDF bytes (#render_pdf) -# or the merged HTML (#render_html — a cheap pongo2 merge with no PDF step). +# a JSON data payload and returns either the rendered PDF and the name to file +# it under (#render_pdf) or the merged HTML (#render_html — a cheap pongo2 +# merge with no PDF step). module YetiPdf class Client class Error < StandardError; end @@ -12,6 +13,12 @@ class Error < StandardError; end RENDER_PATH = '/v1/render' RENDER_HTML_PATH = '/v1/render/html' DEFAULT_TIMEOUT = 30 + PDF_EXTENSION = '.pdf' + + # What #render_pdf hands back: the document and the name to file it under, + # both produced by a single render request. #filename is nil unless a + # filename_template was sent. + Result = Struct.new(:pdf, :filename) class << self def render_pdf(...) @@ -29,33 +36,59 @@ def configured? end end - # @return [String] binary PDF bytes - def render_pdf(template:, data:, options: {}) - post(RENDER_PATH, template: template, data: data, options: options) + # Renders the document and the name to store it under — one request, one + # data payload, both templates merged by yeti-pdf. A filename template that + # renders to something unusable (empty, a path separator, a control + # character) fails the whole request there, so a bad name never reaches us. + # + # filename_template is required: yeti-web always names its documents, so + # every Content-Disposition on the response is a name we asked for. The + # endpoint itself allows the field to be omitted, but no caller here does. + # + # @return [Result] the PDF bytes and the rendered filename (nil only if + # yeti-pdf answered without one) + def render_pdf(template:, data:, filename_template:, options: {}) + response = post(RENDER_PATH, template: template, data: data, options: options, + filename_template: filename_template) + Result.new(response.body.to_s, rendered_filename(response)) end # @return [String] the merged HTML (pre-PDF); useful for debugging/preview def render_html(template:, data:, options: {}) - post(RENDER_HTML_PATH, template: template, data: data, options: options) + post(RENDER_HTML_PATH, template: template, data: data, options: options).body.to_s end private - def post(path, template:, data:, options:) + # Every keyword becomes a field of the JSON body, so each endpoint sends + # exactly the keys it was given — render_html has no filename_template to + # pass and therefore never sends one. + def post(path, **payload) cfg = YetiConfig.invoice&.pdf_api raise Error, 'invoice.pdf_api.base_url is not configured' if cfg&.base_url.blank? proxy = proxy_for(cfg) http = proxy.apply(client(cfg)) - response = proxy.run { http.post(url(cfg, path), json: { template: template, data: data, options: options }) } + response = proxy.run { http.post(url(cfg, path), json: payload) } response.raise_for_status - response.body.to_s + response rescue HTTPX::HTTPError => e raise Error, "yeti-pdf returned HTTP #{e.status}: #{safe_body(e.response)}" rescue HTTPX::Error => e raise Error, "yeti-pdf request failed: #{e.message}" end + # yeti-pdf returns the rendered name in Content-Disposition with a ".pdf" + # extension (so the response is directly saveable); we store the base name + # and re-append the extension when serving. httpx parses both the quoted + # and the RFC 2231 filename*=utf-8'' forms. + def rendered_filename(response) + name = response.body.filename + return if name.blank? + + name.delete_suffix(PDF_EXTENSION) + end + def client(cfg) t = (cfg.timeout || DEFAULT_TIMEOUT).to_i # Rendering a large invoice can take yeti-pdf minutes, so the client must diff --git a/db/custom_seeds/invoice_template_example.rb b/db/custom_seeds/invoice_template_example.rb index e26edbe7e..b31d63f5f 100644 --- a/db/custom_seeds/invoice_template_example.rb +++ b/db/custom_seeds/invoice_template_example.rb @@ -9,4 +9,7 @@ # Created once; an existing row with the same name is left untouched. Billing::InvoiceTemplate.find_or_create_by!(name: 'Example (HTML)') do |template| template.html_template = File.read(Rails.root.join('db/custom_seeds/invoice_template_example.html')) + # Rendered by yeti-pdf alongside the document itself; no extension, ".pdf" is + # appended when the document is served. + template.filename_template = 'invoice-{{invoice.reference}}-{{ invoice.end_date|strfdate:"%Y-%m" }}' end diff --git a/db/migrate/20260809120000_add_filename_template_to_invoice_templates.rb b/db/migrate/20260809120000_add_filename_template_to_invoice_templates.rb new file mode 100644 index 000000000..529594d5b --- /dev/null +++ b/db/migrate/20260809120000_add_filename_template_to_invoice_templates.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddFilenameTemplateToInvoiceTemplates < ActiveRecord::Migration[7.2] + def up + execute %q{ + ALTER TABLE billing.invoice_templates + ADD COLUMN filename_template character varying NOT NULL + DEFAULT 'invoice-{{invoice.reference}}'; + } + end + + def down + execute %q{ + ALTER TABLE billing.invoice_templates DROP COLUMN filename_template; + } + end +end diff --git a/db/structure.sql b/db/structure.sql index 684f1a31f..5d1265d8a 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -11336,7 +11336,8 @@ CREATE TABLE billing.invoice_templates ( id integer NOT NULL, name character varying NOT NULL, created_at timestamp with time zone, - html_template text + html_template text, + filename_template character varying DEFAULT 'invoice-{{invoice.reference}}'::character varying NOT NULL ); @@ -20794,6 +20795,7 @@ ALTER TABLE ONLY sys.sensors SET search_path TO gui, public, switch, billing, class4, runtime_stats, sys, logs, data_import; INSERT INTO "public"."schema_migrations" (version) VALUES +('20260809120000'), ('20260729120000'), ('20260719120000'), ('20260705140000'), diff --git a/spec/acceptance/rest/admin/api/invoice_template_spec.rb b/spec/acceptance/rest/admin/api/invoice_template_spec.rb index 3501db782..b910d5565 100644 --- a/spec/acceptance/rest/admin/api/invoice_template_spec.rb +++ b/spec/acceptance/rest/admin/api/invoice_template_spec.rb @@ -31,11 +31,14 @@ # html_template dasherizes to the "html-template" param, whose value # rspec_api_documentation looks up by that (dashed) name; point it back at # the underscored let so the value is actually sent (otherwise it arrives - # blank and the required validation returns 422). + # blank and the required validation returns 422). Same for + # filename_template. jsonapi_attribute(:html_template, required: true, method: :html_template) + jsonapi_attribute(:filename_template, required: true, method: :filename_template) let(:name) { 'Daily' } let(:html_template) { '

{{ invoice.reference }}

' } + let(:filename_template) { 'invoice-{{invoice.reference}}' } example_request 'create new entry' do expect(status).to eq(201) diff --git a/spec/factories/billing/invoice_templates.rb b/spec/factories/billing/invoice_templates.rb index f961acf0c..f010e354c 100644 --- a/spec/factories/billing/invoice_templates.rb +++ b/spec/factories/billing/invoice_templates.rb @@ -5,10 +5,11 @@ # Table name: billing.invoice_templates # Database name: primary # -# id :integer(4) not null, primary key -# html_template :text -# name :string not null -# created_at :timestamptz +# id :integer(4) not null, primary key +# filename_template :string default("invoice-{{invoice.reference}}"), not null +# html_template :text +# name :string not null +# created_at :timestamptz # # Indexes # diff --git a/spec/features/billing/invoice_templates/new_invoice_template_spec.rb b/spec/features/billing/invoice_templates/new_invoice_template_spec.rb index cf5a98f28..8d15c2b5b 100644 --- a/spec/features/billing/invoice_templates/new_invoice_template_spec.rb +++ b/spec/features/billing/invoice_templates/new_invoice_template_spec.rb @@ -9,9 +9,11 @@ include_context :fill_form, 'new_billing_invoice_template' do let(:html_template) { '{{ invoice.reference }}' } + let(:filename_template) { 'invoice-{{invoice.reference}}' } let(:attributes) do { name: 'new template', + filename_template: filename_template, html_template: html_template } end @@ -22,6 +24,7 @@ expect(Billing::InvoiceTemplate.last).to have_attributes( name: attributes[:name], + filename_template: filename_template, html_template: html_template ) end diff --git a/spec/models/billing/invoice_template_spec.rb b/spec/models/billing/invoice_template_spec.rb index 60bd9b6e2..bb10f2fa6 100644 --- a/spec/models/billing/invoice_template_spec.rb +++ b/spec/models/billing/invoice_template_spec.rb @@ -5,10 +5,11 @@ # Table name: billing.invoice_templates # Database name: primary # -# id :integer(4) not null, primary key -# html_template :text -# name :string not null -# created_at :timestamptz +# id :integer(4) not null, primary key +# filename_template :string default("invoice-{{invoice.reference}}"), not null +# html_template :text +# name :string not null +# created_at :timestamptz # # Indexes # @@ -31,4 +32,16 @@ expect(template).not_to be_valid expect(template.errors[:name]).to be_present end + + it 'defaults filename_template so a new template always names its documents' do + expect(described_class.new.filename_template).to eq('invoice-{{invoice.reference}}') + end + + # NOT NULL alone would let '' through, which yeti-pdf reads as "no filename + # template" — the document would come back nameless. + it 'is invalid with a blank filename_template' do + template = described_class.new(name: 'blank-name', html_template: '

x

', filename_template: '') + expect(template).not_to be_valid + expect(template.errors[:filename_template]).to be_present + end end diff --git a/spec/requests/api/rest/admin/invoices_spec.rb b/spec/requests/api/rest/admin/invoices_spec.rb index b49510d3c..92c40195b 100644 --- a/spec/requests/api/rest/admin/invoices_spec.rb +++ b/spec/requests/api/rest/admin/invoices_spec.rb @@ -886,14 +886,14 @@ let!(:contractor) { create(:customer) } let!(:account) { create(:account, contractor:) } let!(:invoice) { create(:invoice, account:) } - let!(:invoice_document) { create(:invoice_document, :filled, invoice:) } + let!(:invoice_document) { create(:invoice_document, :filled, invoice:, filename: 'INV-1_2020-01') } - it 'responds with pdf file' do + it 'responds with pdf file named after the stored document filename' do subject expect(response.status).to eq(200) expect(response.headers['Content-Transfer-Encoding']).to eq('binary') expect(response.headers['Content-Type']).to eq('application/pdf') - expect(response.headers['Content-Disposition']).to include("attachment; filename=\"invoice-#{invoice.id}.pdf\"") + expect(response.headers['Content-Disposition']).to include('attachment; filename="INV-1_2020-01.pdf"') expect(response.body).to match(invoice_document.pdf_data) end diff --git a/spec/requests/invoice_template_playground_spec.rb b/spec/requests/invoice_template_playground_spec.rb new file mode 100644 index 000000000..824dc3fb2 --- /dev/null +++ b/spec/requests/invoice_template_playground_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +# The playground preview action (app/admin/billing/invoice_template_playground.rb). +RSpec.describe 'Invoice template playground preview', type: :request do + include_context :login_as_admin + + let!(:contractor) { create(:vendor) } + let!(:account) { create(:account, contractor: contractor) } + let!(:invoice) { create(:invoice, account: account) } + + let(:params) do + { invoice_id: invoice.id, template: '

{{ invoice.reference }}

', filename_template: filename_template } + end + + # The test env keeps allow_forgery_protection on, so the preview POST needs a + # token from a real session — take it from the page the way the browser does + # (the editor reads the same meta tag) instead of weakening protection. + def csrf_token + get template_playground_path + Nokogiri::HTML(response.body).at('meta[name="csrf-token"]')&.[]('content') + end + + subject { post template_playground_preview_path, params: params, headers: { 'X-CSRF-Token' => csrf_token } } + + before { allow(YetiPdf::Client).to receive(:render_pdf) } + + context 'with a blank filename template' do + let(:filename_template) { '' } + + # Blank is rejected locally: the column is NOT NULL and the model requires + # it, so there is nothing worth asking yeti-pdf about. + it 'responds with a validation error and does not call yeti-pdf' do + subject + expect(response.status).to eq(422) + expect(response.body).to match(/Filename template must not be blank/) + expect(YetiPdf::Client).not_to have_received(:render_pdf) + end + end + + context 'with a filename template' do + let(:filename_template) { 'invoice-{{invoice.reference}}' } + + before do + allow(YetiPdf::Client).to receive(:render_pdf) + .and_return(YetiPdf::Client::Result.new('%PDF-bytes', 'invoice-INV-1')) + end + + it 'renders the pdf and reports the rendered name' do + subject + expect(response.status).to eq(200) + expect(response.body).to eq('%PDF-bytes') + expect(response.headers['X-Rendered-Filename']).to eq('invoice-INV-1') + expect(YetiPdf::Client).to have_received(:render_pdf) + .with(hash_including(filename_template: filename_template)) + end + end +end diff --git a/spec/services/billing_invoice/approve_spec.rb b/spec/services/billing_invoice/approve_spec.rb index ef5ce1261..5670157ef 100644 --- a/spec/services/billing_invoice/approve_spec.rb +++ b/spec/services/billing_invoice/approve_spec.rb @@ -11,7 +11,7 @@ let!(:account) { FactoryBot.create(:account, contractor:, send_invoices_to: [contact.id]) } let(:invoice_attrs) { { account:, contractor: } } let!(:invoice) { FactoryBot.create(:invoice, :pending, invoice_attrs) } - let(:invoice_document_attrs) { { invoice:, filename: "#{invoice.id}_#{invoice.start_date}_#{invoice.end_date}" } } + let(:invoice_document_attrs) { { invoice:, filename: "invoice-#{invoice.reference}" } } let!(:invoice_document) { FactoryBot.create(:invoice_document, :filled, invoice_document_attrs) } before { FactoryBot.create(:smtp_connection, global: true) } diff --git a/spec/services/billing_invoice/generate_document_spec.rb b/spec/services/billing_invoice/generate_document_spec.rb index ede85c1cf..c2977d898 100644 --- a/spec/services/billing_invoice/generate_document_spec.rb +++ b/spec/services/billing_invoice/generate_document_spec.rb @@ -21,28 +21,31 @@ context 'when yeti-pdf is configured and succeeds' do let(:pdf_bytes) { '%PDF-1.7 rendered' } + let(:rendered_filename) { 'INV-1_2020-01' } before do allow(YetiPdf::Client).to receive(:configured?).and_return(true) - allow(YetiPdf::Client).to receive(:render_pdf).and_return(pdf_bytes) + allow(YetiPdf::Client).to receive(:render_pdf) + .and_return(YetiPdf::Client::Result.new(pdf_bytes, rendered_filename)) end - it 'stores the pdf on a new invoice document' do + it 'stores the pdf and the name yeti-pdf rendered on a new invoice document' do expect { subject }.to change { Billing::InvoiceDocument.count }.by(1) doc = Billing::InvoiceDocument.last! expect(doc).to have_attributes( invoice: invoice, - filename: invoice.file_name.to_s, + filename: rendered_filename, pdf_data: pdf_bytes ) end - it 'sends the html_template and the nested raw data payload to the client' do + it 'sends both templates and the nested raw data payload in one request' do subject expect(YetiPdf::Client).to have_received(:render_pdf).with( template: '

{{ invoice.reference }}

', + filename_template: invoice_template.filename_template, data: hash_including(:account, :contractor, :invoice) - ) + ).once end it 'clears a previously recorded pdf_error' do @@ -52,6 +55,20 @@ end end + context 'when yeti-pdf answers without a filename' do + before do + allow(YetiPdf::Client).to receive(:configured?).and_return(true) + allow(YetiPdf::Client).to receive(:render_pdf) + .and_return(YetiPdf::Client::Result.new('%PDF-1.7', nil)) + end + + it 'records the error rather than storing a nameless document' do + subject + expect(invoice.reload.pdf_error).to match(/returned no filename/) + expect(Billing::InvoiceDocument.count).to eq(0) + end + end + context 'when yeti-pdf returns an error' do before do allow(YetiPdf::Client).to receive(:configured?).and_return(true) diff --git a/spec/services/yeti_pdf/client_spec.rb b/spec/services/yeti_pdf/client_spec.rb index df44b53de..f8a458c37 100644 --- a/spec/services/yeti_pdf/client_spec.rb +++ b/spec/services/yeti_pdf/client_spec.rb @@ -17,17 +17,43 @@ end describe '.render_pdf' do - subject { described_class.render_pdf(template: template, data: data) } + subject { described_class.render_pdf(template: template, data: data, filename_template: filename_template) } - it 'posts the template and data and returns the pdf bytes' do - stub = stub_request(:post, render_url) - .to_return(status: 200, headers: { 'Content-Type' => 'application/pdf' }, body: '%PDF-bytes') + let(:filename_template) { '{{ invoice.reference }}' } - expect(subject).to eq('%PDF-bytes') + it 'posts both templates and the data, and returns the pdf and the rendered name' do + stub = stub_request(:post, render_url).to_return( + status: 200, + headers: { 'Content-Type' => 'application/pdf', + 'Content-Disposition' => 'attachment; filename="INV-1.pdf"' }, + body: '%PDF-bytes' + ) + + expect(subject).to have_attributes(pdf: '%PDF-bytes', filename: 'INV-1') expect(stub).to have_been_requested expect(WebMock).to have_requested(:post, render_url) .with(headers: { 'Content-Type' => %r{application/json} }, - body: hash_including('template' => template, 'options' => {})) + body: hash_including('template' => template, + 'filename_template' => filename_template, + 'options' => {})) + end + + it 'decodes an RFC 2231 encoded name' do + stub_request(:post, render_url).to_return( + status: 200, + headers: { 'Content-Disposition' => "attachment; filename*=utf-8''Acm%C3%A9%20%26%20Co.pdf" }, + body: '%PDF-bytes' + ) + + expect(subject.filename).to eq('Acmé & Co') + end + + # Only reachable against a yeti-pdf too old to know filename_template; + # BillingInvoice::GenerateDocument turns it into a recorded pdf_error. + it 'reports no filename when the response carries none' do + stub_request(:post, render_url).to_return(status: 200, body: '%PDF-bytes') + + expect(subject.filename).to be_nil end it 'raises Error with the status on a non-2xx response' do @@ -49,13 +75,21 @@ expect(described_class.render_html(template: template, data: data)).to eq('

INV-1

') end + + it 'sends no filename_template — the endpoint renders no document to name' do + stub_request(:post, html_url).to_return(status: 200, body: '

INV-1

') + + described_class.render_html(template: template, data: data) + expect(WebMock).to have_requested(:post, html_url) + .with { |req| !JSON.parse(req.body).key?('filename_template') } + end end context 'when pdf_api is not configured' do before { allow(YetiConfig).to receive(:invoice).and_return(nil) } it 'raises a clear Error' do - expect { described_class.render_pdf(template: template, data: data) } + expect { described_class.render_pdf(template: template, data: data, filename_template: 'x') } .to raise_error(YetiPdf::Client::Error, /not configured/) end end