Skip to content

feat: add optional PDF Button for Sales and Purchase Doctypes#75

Open
HenningWendtland wants to merge 7 commits intoalyf-de:version-16from
HenningWendtland:add_pdf_button
Open

feat: add optional PDF Button for Sales and Purchase Doctypes#75
HenningWendtland wants to merge 7 commits intoalyf-de:version-16from
HenningWendtland:add_pdf_button

Conversation

@HenningWendtland
Copy link
Copy Markdown
Member

@HenningWendtland HenningWendtland commented Feb 20, 2026

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

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_doctype function 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.

Comment thread pdf_on_submit/public/js/pdf_button_utils.js
Comment thread pdf_on_submit/hooks.py
Comment on lines 19 to +31
@@ -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"
}
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment thread pdf_on_submit/utils.py Outdated
Comment thread pdf_on_submit/utils.py Outdated
Comment on lines +7 to +30
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)
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment thread pdf_on_submit/public/js/pdf_button_utils.js
Comment thread pdf_on_submit/utils.py Outdated
Comment thread pdf_on_submit/utils.py Outdated
Copy link
Copy Markdown
Member Author

@HenningWendtland HenningWendtland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .po file
  • 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 function

2. 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 function

3. 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 body

Additional Suggestions (Optional)

  1. Input validation: Consider validating input lengths (doctype/docname max 140 chars per Frappe standards)
  2. Rate limiting: Consider adding @rate_limit() decorator to prevent abuse
  3. Security logging: Consider logging permission failures for security monitoring

Testing Recommendations

Please test:

  • Direct API calls with unauthorized doctypes
  • API calls with show_pdf_button disabled
  • 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.

HenningWendtland and others added 3 commits February 20, 2026 16:38
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants