Move to grover for PDF generation & add a dedicated download button #1194
Conversation
|
Warning Rate limit exceeded@lodewiges has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 6 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces WickedPdf/wkhtmltopdf with Grover/Chromium, updates Dockerfile and Gemfile, adds Grover initializer and controller/mailer PDF generation via Grover, adds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Rails as InvoicesController
participant Grover
participant ServerResp as Server Response
Note over User,Browser: User clicks "Download" (GET /invoices/:id.pdf)
User->>Browser: click download link
Browser->>Rails: GET /invoices/:id.pdf
Rails->>Rails: integer_id? -> token_based_access?
alt integer id (authenticated)
Rails->>Rails: authorize :show?
end
Rails->>Rails: render invoice HTML (layout: 'pdf')
Rails->>Grover: Grover.from_html(rendered_html, options)
Grover-->>Rails: PDF binary
Rails->>ServerResp: send_data (attachment, filename: Factuur-<human_id>.pdf)
ServerResp-->>Browser: HTTP response (PDF)
Browser-->>User: prompt/save PDF
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 PR migrates the PDF generation system from WickedPDF (using wkhtmltopdf) to Grover (using Chrome/Chromium) and adds a dedicated download button for invoices in the invoice list view.
- Replaced WickedPDF with Grover for PDF generation throughout the application
- Added a dedicated "Download" button column to the invoice index table with proper authorization
- Updated the Dockerfile to use Chromium instead of wkhtmltopdf
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| config/initializers/wicked_pdf.rb | Removed WickedPDF configuration (entire file deleted) |
| config/initializers/grover.rb | Added Grover configuration with A4 format, background printing, and display URL settings |
| app/views/invoices/index.html.erb | Added download button column with authorization check using the new download policy |
| app/policies/invoice_policy.rb | Added download? policy method restricting downloads to treasurers |
| app/mailers/invoice_mailer.rb | Updated to use Grover for PDF attachment generation instead of WickedPDF |
| app/controllers/invoices_controller.rb | Refactored PDF rendering to use Grover with new render_invoice_pdf and integer_id? helper methods |
| app/assets/stylesheets/pdf.scss | Removed Bootstrap table header workaround styles (previously needed for wkhtmltopdf) |
| Gemfile.lock | Replaced wicked_pdf and wkhtmltopdf-binary dependencies with grover and its dependencies |
| Gemfile | Updated gem dependencies: removed wicked_pdf/wkhtmltopdf-binary, added grover |
| Dockerfile | Replaced wkhtmltopdf with chromium and chromium-driver system packages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| font-weight: bold; | ||
| } | ||
| } | ||
| // PDF-specific styles |
There was a problem hiding this comment.
The removed CSS styles addressed a specific issue where <td> elements inside <thead> needed to be bold due to a Bootstrap tables bug in PDF generation. The invoice show template (lines 97, 127) still uses <td> inside <thead>. While this might work correctly with Grover/Chromium (unlike wkhtmltopdf), please verify that the PDF output still displays table headers with appropriate styling. If the headers appear non-bold, this CSS may need to be restored or the template markup should be updated to use <th> instead of <td>.
| // PDF-specific styles | |
| // PDF-specific styles | |
| // Ensure table headers render bold in PDFs even when using <td> inside <thead>, | |
| // which can be affected by Bootstrap/PDF generator quirks. | |
| thead td { | |
| font-weight: bold; | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #1194 +/- ##
===========================================
- Coverage 77.62% 77.20% -0.43%
===========================================
Files 54 54
Lines 1341 1360 +19
===========================================
+ Hits 1041 1050 +9
- Misses 300 310 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/controllers/invoices_controller.rb (1)
93-105: DRY violation: PDF generation duplicated in mailer.As noted in past reviews, PDF generation logic (rendering template with Grover) is duplicated between this controller method and
InvoiceMailer. Consider extracting to a shared service or helper to ensure consistency and maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/invoices_controller.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (4)
app/controllers/invoices_controller.rb (4)
5-5: LGTM! Exception list correctly includesshow.The
verify_authorizedexception list now includesshow, which correctly allows token-based access in the show action without triggering authorization errors.
18-21: LGTM! Token-based access detection and conditional authorization implemented correctly.The logic properly distinguishes between token-based access (which skips authorization) and integer ID access (which requires authorization). The implementation correctly handles both access patterns.
25-25: LGTM! Parameter removal addressed past review feedback.The call to
render_invoice_pdfcorrectly omits the unused parameter, as addressed in the previous review.
76-81: LGTM! Clean implementation of integer ID detection.The helper method uses idiomatic Ruby with
Integer()and exception handling to distinguish between integer IDs and token-based access. The implementation is correct and readable.
| def render_invoice_pdf # rubocop:disable Metrics/MethodLength | ||
| html = render_to_string( | ||
| template: 'invoices/show', | ||
| formats: [:html], | ||
| layout: 'pdf' | ||
| ) | ||
| pdf = Grover.new(html).to_pdf | ||
| send_data pdf, filename: "Factuur-#{@invoice.human_id}.pdf", type: 'application/pdf', disposition: 'attachment' | ||
| rescue StandardError => e | ||
| Rails.logger.error "Failed to generate PDF for invoice #{@invoice.id}: #{e.message}" | ||
| flash[:error] = 'Er is een fout opgetreden bij het genereren van de PDF. Probeer het later opnieuw.' | ||
| redirect_to invoice_path(@invoice) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the InvoicePolicy file
fd -t f "invoice_policy" app/Repository: csvalpha/sofia
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Read the InvoicePolicy file to understand download? and show? implementations
cat app/policies/invoice_policy.rbRepository: csvalpha/sofia
Length of output: 287
🏁 Script executed:
#!/bin/bash
# Search for token_based_access usage in the controller
rg -n "token_based_access|render_invoice_pdf" app/controllers/invoices_controller.rb -B 5 -A 5Repository: csvalpha/sofia
Length of output: 921
🏁 Script executed:
#!/bin/bash
# Verify the parent ApplicationPolicy to understand default show? behavior
rg -n "class ApplicationPolicy|def show\?" app/policies/application_policy.rb -A 3Repository: csvalpha/sofia
Length of output: 227
🏁 Script executed:
#!/bin/bash
# Check if there are any other calls to render_invoice_pdf outside the show action
rg -n "render_invoice_pdf" app/Repository: csvalpha/sofia
Length of output: 245
Add authorization check for download permission in render_invoice_pdf.
The InvoicePolicy defines separate show? and download? policies: show? authorizes treasurers and renting managers, while download? restricts access to treasurers only. Currently, render_invoice_pdf is called within the show action's response handler without verifying the download? permission. This allows renting managers to download invoices they shouldn't have access to.
Add the authorization check, respecting token-based access the same way the show action does (line 21):
def render_invoice_pdf
token_based_access = !integer_id?(@invoice.id)
authorize @invoice, :download? unless token_based_access
# ... rest of method
end🤖 Prompt for AI Agents
In app/controllers/invoices_controller.rb around lines 93 to 105, add the
missing authorization check so only users allowed to download can generate the
PDF: determine token-based access the same way as the show action (e.g.,
token_based_access = !integer_id?(@invoice.id)) and call authorize @invoice,
:download? unless token_based_access before rendering/generating the PDF; leave
the rest of the method (render_to_string, Grover, send_data, and the rescue)
unchanged.
| rescue StandardError => e | ||
| Rails.logger.error "Failed to generate PDF for invoice #{@invoice.id}: #{e.message}" | ||
| flash[:error] = 'Er is een fout opgetreden bij het genereren van de PDF. Probeer het later opnieuw.' | ||
| redirect_to invoice_path(@invoice) |
There was a problem hiding this comment.
Error handling may not work well for PDF requests.
The rescue block sets flash[:error] and redirects, but this occurs within a PDF format response. When a user requests a PDF download, the browser expects a PDF response, and a redirect may not display the flash message or provide clear feedback to the user. Consider rendering an error page or returning a 500 status with an error message instead.
🔎 Alternative error handling approach
rescue StandardError => e
Rails.logger.error "Failed to generate PDF for invoice #{@invoice.id}: #{e.message}"
- flash[:error] = 'Er is een fout opgetreden bij het genereren van de PDF. Probeer het later opnieuw.'
- redirect_to invoice_path(@invoice)
+ render plain: 'Er is een fout opgetreden bij het genereren van de PDF. Probeer het later opnieuw.', status: :internal_server_error📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rescue StandardError => e | |
| Rails.logger.error "Failed to generate PDF for invoice #{@invoice.id}: #{e.message}" | |
| flash[:error] = 'Er is een fout opgetreden bij het genereren van de PDF. Probeer het later opnieuw.' | |
| redirect_to invoice_path(@invoice) | |
| rescue StandardError => e | |
| Rails.logger.error "Failed to generate PDF for invoice #{@invoice.id}: #{e.message}" | |
| render plain: 'Er is een fout opgetreden bij het genereren van de PDF. Probeer het later opnieuw.', status: :internal_server_error |
🤖 Prompt for AI Agents
In app/controllers/invoices_controller.rb around lines 101-104, the rescue block
always sets a flash and redirects which fails for PDF requests; change the
rescue to branch on request.format.pdf? — for non-PDF keep the flash+redirect,
but for PDF return an appropriate error response (e.g. render an error template
with status 500, or send a simple error PDF or plain text body with content-type
and status 500) so the browser/client receives a valid response instead of a
redirect; preserve the Rails.logger.error call and ensure the response
content-type and HTTP status reflect the failure.
Moves to grover for pdf generation because wkhtmltopdf is EOL and doesnot support ruby 3.4.8
Fixes #843
Fix authorisation issues and download button not working
Summary by CodeRabbit
New Features
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.