feat: add optional PDF Button for Sales and Purchase Doctypes#75
feat: add optional PDF Button for Sales and Purchase Doctypes#75HenningWendtland wants to merge 7 commits intoalyf-de:version-16from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an optional PDF download button for sales and purchase document types in ERPNext. The feature respects PDF on Submit Settings configurations and falls back to default ERPNext print formats when appropriate.
Changes:
- Added a new whitelisted Python utility function to retrieve print format and letterhead settings for documents
- Implemented a JavaScript utility that adds a PDF button to common sales/purchase doctypes (Quotation, Sales Order, Invoice, etc.) when enabled via settings
- Added a new checkbox field "Show PDF Button in Sales and Purchase DocTypes" to PDF on Submit Settings
- Extracted
get_matching_enabled_doctypefunction from existing code for reuse - Updated translation files (main.pot and de.po) with new UI strings
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pdf_on_submit/utils.py | New Python utility module providing whitelisted API to fetch print details (format and letterhead) for documents |
| pdf_on_submit/public/js/pdf_button_utils.js | JavaScript utility that registers PDF button handlers for sales/purchase doctypes and handles PDF generation via API calls |
| pdf_on_submit/pdf_on_submit/doctype/pdf_on_submit_settings/pdf_on_submit_settings.json | Added new checkbox field and column break to enable/disable the PDF button feature |
| pdf_on_submit/locale/main.pot | Updated translation template with new strings for the PDF button and related messages |
| pdf_on_submit/locale/de.po | Updated German translations for new UI elements and messages |
| pdf_on_submit/hooks.py | Added JavaScript file inclusion via both app_include_js and doctype_js hooks |
| pdf_on_submit/attach_pdf.py | Extracted get_matching_enabled_doctype function for reuse in the new utils module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -26,7 +26,9 @@ | |||
| # page_js = {"page" : "public/js/file.js"} | |||
|
|
|||
| # include js in doctype views | |||
| # doctype_js = {"doctype" : "public/js/doctype.js"} | |||
| doctype_js = { | |||
| "*": "public/js/pdf_button_utils.js" | |||
| } | |||
There was a problem hiding this comment.
The JavaScript file is included both globally via app_include_js and for all doctypes via doctype_js["*"]. This causes the same file to be loaded twice, which is redundant and could lead to duplicate event handler registrations. Consider using only one of these inclusion methods. Since the file is specifically for doctype forms, doctype_js is more appropriate and app_include_js should be removed.
| def get_print_details(doctype: str, docname: str) -> tuple: | ||
| """ | ||
| Get print format and letter head for a document based on PDF on Submit Settings. | ||
|
|
||
| Returns: (print_format, letter_head) | ||
| """ | ||
| if not frappe.has_permission(doctype, "print", docname): | ||
| frappe.throw(_("No permission to print this document")) | ||
|
|
||
| doc = frappe.get_doc(doctype, docname) | ||
| print_format = doc.meta.default_print_format or "Standard" | ||
| letter_head = doc.letter_head or None | ||
|
|
||
| if "pdf_on_submit" not in frappe.get_installed_apps(): | ||
| return (print_format, letter_head) | ||
|
|
||
| settings = frappe.get_single("PDF on Submit Settings") | ||
| matched_config = get_matching_enabled_doctype(doc, settings) | ||
|
|
||
| if matched_config: | ||
| print_format = matched_config.print_format or print_format | ||
| letter_head = matched_config.letter_head or letter_head | ||
|
|
||
| return (print_format, letter_head) |
There was a problem hiding this comment.
The new function get_print_details is whitelisted and exposed as an API endpoint but lacks test coverage. Since the repository has a test suite (pdf_on_submit/tests/test_pdf_on_submit.py), consider adding tests for this function to verify permission checks, fallback behavior, and correct return values for different document types.
HenningWendtland
left a comment
There was a problem hiding this comment.
Review Summary
(AI-generated review) This is a well-implemented feature with clean code! I've analyzed the implementation and identified a few security improvements that would strengthen it before merging.
CI Status
✅ All checks passing (CodeQL, JavaScript analysis, Python analysis)
PR Description
The description is accurate but quite minimal. Consider adding:
- What the feature does (adds optional PDF button to sales/purchase forms)
- How it works (calls whitelisted method to get print details, opens PDF in new tab)
- How to enable it (via PDF on Submit Settings checkbox)
What Works Well
- Clean implementation following existing Frappe patterns
- Permission check with
frappe.has_permission()before document access - Safe URL construction using
URLSearchParams - German translations properly added to
.pofile - Good error handling with user-friendly messages
- Proper check for new documents (
frm.is_new()) - Follows conventional commit format
Main Concerns
1. Missing Server-Side DocType Allowlist (High Priority)
The whitelisted method in utils.py accepts any arbitrary doctype parameter. While there's a client-side ALLOWED_DOCTYPES list in JavaScript, this provides no server-side protection. An attacker could call the API directly with any doctype they have print permission for.
Current code (utils.py:6-7):
@frappe.whitelist()
def get_print_details(doctype: str, docname: str) -> tuple:Issue: Someone could call:
frappe.call({
method: "pdf_on_submit.utils.get_print_details",
args: {doctype: "User", docname: "Administrator"}
})Suggested fix: Add server-side doctype validation:
ALLOWED_DOCTYPES = [
"Quotation", "Sales Order", "Sales Invoice", "Delivery Note", "Dunning",
"Request for Quotation", "Supplier Quotation",
"Purchase Order", "Purchase Invoice", "Purchase Receipt",
]
@frappe.whitelist()
def get_print_details(doctype: str, docname: str) -> tuple:
if doctype not in ALLOWED_DOCTYPES:
frappe.throw(
_("PDF button not available for doctype {0}").format(doctype),
frappe.PermissionError
)
# ... rest of function2. Missing Server-Side Setting Validation (Medium Priority)
The show_pdf_button setting is only checked client-side (pdf_button_utils.js:36-40). Users can bypass this by calling the whitelisted method directly via browser console or API.
Suggested fix: Add server-side check in utils.py:
@frappe.whitelist()
def get_print_details(doctype: str, docname: str) -> tuple:
# ... doctype validation ...
# Check if feature is enabled
show_button = frappe.db.get_single_value("PDF on Submit Settings", "show_pdf_button")
if not show_button:
frappe.throw(_("PDF button is currently disabled"), frappe.ValidationError)
# ... rest of function3. No HTTP Method Restriction (Medium Priority)
The whitelisted method doesn't specify allowed HTTP methods, making it accessible via GET requests. While this is a read-only operation, best practice is to use POST for methods that require authentication and return user-specific data.
Suggested fix:
@frappe.whitelist(methods=["POST"])
def get_print_details(doctype: str, docname: str) -> tuple:
# ... function bodyAdditional Suggestions (Optional)
- Input validation: Consider validating input lengths (doctype/docname max 140 chars per Frappe standards)
- Rate limiting: Consider adding
@rate_limit()decorator to prevent abuse - Security logging: Consider logging permission failures for security monitoring
Testing Recommendations
Please test:
- Direct API calls with unauthorized doctypes
- API calls with
show_pdf_buttondisabled - User Permission scenarios (users restricted to specific companies/territories)
Let me know if you have questions about any of these suggestions! I think addressing the doctype allowlist (#1) would be the most important improvement.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Usecase:
PDF on Submit tries to minimize the clicks needed to create and attach a PDF. In most cases users want to see the PDF as a form of Preview before submission though. Since the PDF On Submit Settings allow now multiple prints with filters and differ from the ERPNext default print it can become cumbersome for the User to go to "Print -> Select the correct format and letterhead -> Press PDF Button"
Solution:
Add a simple PDF Button in widely used Doctypes that respects the PDF on Submit Settings
Features of the Button:
Fetches first Print Format in PDF on Submit settings that matches, or falls back to ERPNext Default Format
Add a Checkbox in PDF On Submit Settings to toggle the feature
Autoadd the Button for 10 most common Sales & Purchase Doctypes
Allow Users to add the Button for other Doctypes via simple Client Script
Check Print Permissions
feat: add Optional PDF Button for Sales and Purchase Doctypes
chore: update translation files