Move to grover for PDF generation & add a dedicated download button #1194
Conversation
|
Warning Rate limit exceeded
⌛ 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 (2)
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. 📝 WalkthroughWalkthroughReplaces WickedPdf/wkhtmltopdf with Grover/Chromium, updates Dockerfile and package.json, centralizes PDF rendering in controller and mailer using 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 `@invoice` (show?/pay?)
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 | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 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.
![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.08% -0.55%
===========================================
Files 54 54
Lines 1341 1370 +29
===========================================
+ Hits 1041 1056 +15
- Misses 300 314 +14 ☔ 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.
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
TO DO
Summary by CodeRabbit
New Features
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.