Make PDF generation Url based instead of htmlbased #1212
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughSwitched PDF generation from rendering HTML in-process to having Grover fetch a tokenized invoice URL; removed the PDF layout that inlined CSS, adjusted view/controller flags for PDF mode, tightened payment button visibility, and simplified Grover initializer options. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as InvoicesController
participant Grover as Grover
participant View as InvoiceView
participant Mailer as InvoiceMailer
Note over Client,Controller: Request PDF for invoice
Client->>Controller: GET /invoices/:id?pdf=true
Controller->>Controller: set `@show_navigationbar`=false, `@show_extras`=false
Controller->>Grover: Grover.fetch(invoice_url(token, pdf: true))
Grover->>Controller: HTTP GET invoice_url(token, pdf: true)
Controller->>View: render invoices/show (pdf: true) -> HTML
View-->>Grover: HTML response fetched by Grover
Grover-->>Controller: PDF bytes returned
Controller-->>Client: send PDF response
Note over Mailer,Grover: Mailer PDF generation
Mailer->>Grover: Grover.new(invoice_url(token, pdf: true))
Grover->>Mailer: fetch HTML via token URL -> PDF bytes (or error logged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request refactors PDF generation for invoices from an HTML-based approach to a URL-based approach using Grover. The change addresses staging environment issues with the previous HTML-based implementation.
Changes:
- Simplified Grover configuration by removing HTML-specific options and streamlining development vs production conditional logic
- Deleted the custom PDF layout file that handled CSS inlining for HTML-based generation
- Modified invoice mailer and controller to generate PDFs by passing URLs to Grover instead of rendered HTML strings
- Added conditional logic to hide navigation bar and extras when rendering invoices for PDF generation
- Updated payment button conditional to also check for Mollie API key presence
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| config/initializers/grover.rb | Simplified configuration by removing HTML-based options (display_url, prefer_css_page_size) and consolidating conditional logic into inline ternary operators |
| app/views/layouts/pdf.html.erb | Deleted custom PDF layout that was only needed for HTML-based generation |
| app/views/invoices/show.html.erb | Added Mollie API key check to payment button conditional |
| app/mailers/invoice_mailer.rb | Changed from rendering HTML string to using token-based URL for PDF generation |
| app/controllers/invoices_controller.rb | Added logic to hide UI elements when pdf parameter is present; updated render_invoice_pdf to use URL-based approach |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <% if !@invoice.paid? && Rails.application.config.x.mollie_api_key.present? %> | ||
| <%= link_to pay_invoice_url @invoice.token do %> | ||
| <button class="col-sm-3 offset-sm-9 btn btn-primary">Betalen</button> |
There was a problem hiding this comment.
The compound condition can be simplified for better readability. Instead of using the negation operator with unless, consider using a positive condition with if. The current change from unless @invoice.paid? to if [email protected]? && Rails.application.config.x.mollie_api_key.present? could be written more idiomatically as if @invoice.unpaid? && Rails.application.config.x.mollie_api_key.present? or keep the unless pattern consistent.
| <% if [email protected]? && Rails.application.config.x.mollie_api_key.present? %> | |
| <%= link_to pay_invoice_url @invoice.token do %> | |
| <button class="col-sm-3 offset-sm-9 btn btn-primary">Betalen</button> | |
| <% if Rails.application.config.x.mollie_api_key.present? %> | |
| <% unless @invoice.paid? %> | |
| <%= link_to pay_invoice_url @invoice.token do %> | |
| <button class="col-sm-3 offset-sm-9 btn btn-primary">Betalen</button> | |
| <% end %> |
| def render_invoice_pdf | ||
| token_based_access = !integer_id?(params[:id]) | ||
| authorize @invoice, :download? unless token_based_access | ||
|
|
||
| html = render_to_string( | ||
| template: 'invoices/show', | ||
| formats: [:html], | ||
| layout: 'pdf' | ||
| ) | ||
| pdf = Grover.new(html).to_pdf | ||
| # Use token-based URL for unauthenticated Grover access | ||
| url = invoice_url(@invoice.token, pdf: true) | ||
| pdf = Grover.new(url).to_pdf |
There was a problem hiding this comment.
The new URL-based PDF generation logic lacks test coverage. Since the codebase has comprehensive test coverage for other controller actions (as seen in spec/controllers/invoices_controller/), this change should include tests that verify the PDF generation works correctly with the new URL-based approach, including tests for successful PDF generation and error handling.
| # Use token-based URL for unauthenticated Grover access | ||
| url = url_for(controller: 'invoices', action: 'show', id: invoice.token, pdf: true, only_path: false) | ||
| pdf = Grover.new(url).to_pdf |
There was a problem hiding this comment.
The new URL-based PDF attachment generation lacks test coverage. Since the codebase has comprehensive test coverage for controllers, this change should include tests that verify the PDF attachment is correctly generated and attached to the email, including tests for error handling when PDF generation fails.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #1212 +/- ##
===========================================
+ Coverage 77.08% 77.11% +0.03%
===========================================
Files 54 54
Lines 1370 1372 +2
===========================================
+ Hits 1056 1058 +2
Misses 314 314 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/invoices_controller.rb (1)
100-116: Useonly_path: falsefor explicit full URL generation, matching the pattern in InvoiceMailer.The current code relies on implicit request context for the host, while the mailer explicitly uses
only_path: false. Sinceaction_controller.default_url_optionsis not configured (onlyaction_mailer.default_url_optionsis), make the URL generation explicit for consistency and clarity:url = invoice_url(`@invoice.token`, pdf: true, only_path: false)This matches the safer, more explicit pattern already used in InvoiceMailer (line 14) and ensures the URL works reliably regardless of request context variations.
🧹 Nitpick comments (1)
app/views/invoices/show.html.erb (1)
66-70: Consider hiding the payment button when rendering for PDF.The added
mollie_api_key.present?check is a good improvement. However, the controller sets@show_extras = falsewhenpdf: true, but this flag isn't used here. The payment button will still appear in the PDF if Mollie is configured and the invoice is unpaid, which is likely undesirable since users can't click buttons in a PDF.Suggested fix
- <% if [email protected]? && Rails.application.config.x.mollie_api_key.present? %> + <% if `@show_extras` != false && [email protected]? && Rails.application.config.x.mollie_api_key.present? %>
There where multiple issue with Html based pdf generation on staging, and this should simplify it.
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.