feat: print format for sales invoice #49508
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
khushi8112
force-pushed
the
print-format-for-sales-invoice
branch
from
ad27994 to
7888500
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
erpnext/fixtures/letter_head.json (1)
4-4: Fix broken inline CSS on logo (missing semicolon) and add alt text.The first fixture still has
style="width: 90px height: 90px;"which breaks height; also add analtfor accessibility/PDF fallbacks. This was flagged earlier and appears unresolved here.- <img src="{{ frappe.utils.get_url(frappe.db.get_value('Company', doc.company, 'company_logo')) }}" style="width: 90px height: 90px;"> + <img src="{{ frappe.utils.get_url(frappe.db.get_value('Company', doc.company, 'company_logo')) }}" style="width: 90px; height: 90px;" alt="{{ doc.company }} logo">
🧹 Nitpick comments (5)
erpnext/fixtures/letter_head.json (5)
4-4: wkhtmltopdf compatibility: avoid flex gap; add margin fallback.wkhtmltopdf (Qt WebKit) often ignores
gap. Add a child-sibling margin fallback to preserve spacing in PDFs.- .logo-address { - display: flex; - align-items: center; - gap: 15px; - width: 50%; - } + .logo-address { + display: flex; + align-items: center; + gap: 15px; /* modern browsers */ + width: 50%; + } + .logo-address > * + * { margin-left: 15px; } /* wkhtmltopdf fallback */
4-4: Minor: reduce redundant
tags inside flex rows.
<br>within.letterhead-right-div(a flex row) isn’t needed and can add stray line breaks in PDFs.- <div>{{ doc.name }}</div><br> + <div>{{ doc.name }}</div>(Apply similarly to Website/Email/Contact blocks.)
27-27: wkhtmltopdf compatibility: useflex-endinstead ofend.
justify-content: end;may not be supported by wkhtmltopdf; useflex-end.- justify-content: end; + justify-content: flex-end;
27-27: Add alt text and ensure logo sizing applies to the image.Add an
altattribute for accessibility and set explicit dimensions on the<img>or via.logo img {}to avoid layout shifts/oversized logos in PDFs.- <img src="{{ frappe.utils.get_url(frappe.db.get_value('Company', doc.company, 'company_logo')) }}"> + <img src="{{ frappe.utils.get_url(frappe.db.get_value('Company', doc.company, 'company_logo')) }}" alt="{{ doc.company }} logo" style="width: 75px; height: 75px;">Optionally, move sizing to CSS:
- .logo { + .logo { border: 1px solid #EDEDED; border-radius: 15px; - width: 75px; + width: 75px; } + .logo img { width: 75px; height: 75px; }
27-27: Add gap fallback for flex rows in contact blocks.As above,
gapcan be ignored by wkhtmltopdf; add sibling margins to keep spacing consistent.- .letterhead-right-div { - display: flex; - align-items: flex-start; - gap: 0 10px; - } + .letterhead-right-div { + display: flex; + align-items: flex-start; + gap: 0 10px; /* modern browsers */ + } + .letterhead-right-div > * + * { margin-left: 10px; } /* wkhtmltopdf fallback */
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/fixtures/letter_head.json(1 hunks)
⏰ 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). (6)
- GitHub Check: Patch Test
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html (7)
3-3: Render letterhead HTML safely.
Output is escaped; mark trusted HTML as safe.-<div class="letter-head">{{ letter_head }}</div> +<div class="letter-head">{{ letter_head | safe }}</div>
18-20: wkhtmltopdf: add CSS var fallback for color.
wkhtmltopdf may ignore CSS vars; add a literal fallback.- color: var(--black-overlay-700); + color: #171717; + color: var(--black-overlay-700, #171717);
145-151: Normalize non‑standard font-weight.
420 isn’t a standard weight; use 400.- font-weight: 420; + font-weight: 400;
281-286: Render Terms HTML safely.
Ensure stored HTML isn’t escaped.- {{ doc.terms}} + {{ doc.terms | safe }}
206-206: Pass parent doc to get_formatted for qty.
Using 0 as context can mis-format quantities.- <td class="text-right text-dark">{{ item.get_formatted("qty", 0) }} {{ item.uom }}</td> + <td class="text-right text-dark">{{ item.get_formatted("qty", doc) }} {{ item.uom }}</td>
268-272: Use item_wise_tax_detail for per‑item tax.
Current calc ignores item‑wise rules; use the mapping from each tax row.- {% for row in doc.taxes %} {% set tax_amount = item.base_net_amount * (row.rate / 100) %} - <td class="text-right"> - ({{ (row.rate) }}%) {{ frappe.format_value(tax_amount, {"fieldtype": "Currency"}, doc) - }} - </td> - {% endfor %} + {% for row in doc.taxes %} + {% set detail = frappe.parse_json(row.item_wise_tax_detail or '{}') %} + {% set per_item = detail.get(item.item_code) or [row.rate or 0, 0] %} + {% set per_item_rate = per_item[0] %} + {% set per_item_amount = per_item[1] %} + <td class="text-right"> + ({{ per_item_rate }}%) {{ frappe.format_value(per_item_amount, {"fieldtype": "Currency"}, doc) }} + </td> + {% endfor %}
174-180: “Bill From”/“Bill To” swapped;
trimming off-by-one.
Swap address variables and trim "
", "
", "
" correctly; also coalesce None to "".- {% set address = (doc.address_display) %} {% if address and address.rstrip().endswith("<br />") %} {% set - address = address[:-5] %} {% endif %} {% set company_address_display = - (doc.company_address_display) %} {% if company_address_display and company_address_display.rstrip().endswith("<br />") %} {% - set company_address_display = company_address_display[:-5] %} {% endif %} - <td><span class="heading">{{ _("Bill From") }}:</span><br />{{ address }}</td> - <td><span class="heading">{{ _("Bill To") }}:</span><br />{{ company_address_display }}</td> + {% set address = (doc.address_display or "").rstrip() %} + {% if address.endswith("<br />") %}{% set address = address[:-6] %} + {% elif address.endswith("<br/>") %}{% set address = address[:-5] %} + {% elif address.endswith("<br>") %}{% set address = address[:-4] %}{% endif %} + {% set company_address_display = (doc.company_address_display or "").rstrip() %} + {% if company_address_display.endswith("<br />") %}{% set company_address_display = company_address_display[:-6] %} + {% elif company_address_display.endswith("<br/>") %}{% set company_address_display = company_address_display[:-5] %} + {% elif company_address_display.endswith("<br>") %}{% set company_address_display = company_address_display[:-4] %}{% endif %} + <td><span class="heading">{{ _("Bill From") }}:</span><br />{{ company_address_display }}</td> + <td><span class="heading">{{ _("Bill To") }}:</span><br />{{ address }}</td>
🧹 Nitpick comments (3)
erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html (3)
162-164: Guard missing due date.
Avoid rendering “None” or empty formatting.- <span class="heading">{{ _("Payment Due Date") }}:</span> {{ - frappe.utils.format_date(doc.due_date) }} + <span class="heading">{{ _("Payment Due Date") }}:</span> + {% if doc.due_date %}{{ frappe.utils.format_date(doc.due_date) }}{% else %}-{% endif %}
229-233: Show discount row only when applicable; avoid “None%”.
Guard on discount_amount and format percentage safely.- <tr> - <td class="text-right text-muted"> - {{ _("Discount") }} ({{ doc.additional_discount_percentage }}%): - </td> - <td class="text-right">{{ doc.get_formatted("discount_amount", doc) }}</td> - </tr> + {% if doc.discount_amount %} + <tr> + <td class="text-right text-muted"> + {{ _("Discount") }}{% if doc.additional_discount_percentage %} ({{ frappe.format_value(doc.additional_discount_percentage, {"fieldtype":"Percent"}, doc) }}){% endif %}: + </td> + <td class="text-right">{{ doc.get_formatted("discount_amount", doc) }}</td> + </tr> + {% endif %}
234-239: Composite “Tax (%)” is misleading.
Summing rates across rows isn’t accurate. Either drop “(x%)” or compute an effective rate from totals.Option A (drop percent):
- {% set tax_rate = doc.taxes | selectattr("tax_amount") | sum(attribute="rate") %} {% if - tax_rate > 0 %} + {% if doc.total_taxes_and_charges %} <tr> - <td class="text-right text-muted">{{ _("Tax") }} ({{ tax_rate }}%):</td> + <td class="text-right text-muted">{{ _("Tax") }}:</td> <td class="text-right">{{ doc.get_formatted("total_taxes_and_charges", doc) }}</td> </tr> {% endif %}Option B (effective rate):
{% set base_total = doc.base_total or doc.total %} {% set eff = (doc.total_taxes_and_charges / base_total * 100) if base_total else None %} ... <td class="text-right text-muted">{{ _("Tax") }}{% if eff is not none %} ({{ frappe.format_value(eff, {"fieldtype":"Percent"}, doc) }}){% endif %}:</td>
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html(1 hunks)
⏰ 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). (6)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Patch Test
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html (7)
12-12: Custom heading not rendered: passprint_heading_templateinto macro call.The macro supports
print_heading_templatebut the invocation omits it, so custom headings will never show.Apply this diff:
- {{ add_header(loop.index, layout|len, doc, letter_head, no_letterhead, footer, print_settings) }} + {{ add_header(loop.index, layout|len, doc, letter_head, no_letterhead, footer, print_settings, print_heading_template) }}
11-13: Avoid multipleid="header-html"inside the per‑page loop.Duplicate IDs are invalid and wkhtmltopdf picks only the first header. Render
#header-htmlonce outside the loop whenrepeat_header_footeris enabled; render per‑page headers only when it’s disabled.Proposed structure:
-{% for page in layout %} -<div class="page-break invoice-page"> - <div {% if print_settings.repeat_header_footer %} id="header-html" class="hidden-pdf" {% endif %}> - {{ add_header(loop.index, layout|len, doc, letter_head, no_letterhead, footer, print_settings, print_heading_template) }} - </div> +{% if print_settings.repeat_header_footer %} + <div id="header-html" class="hidden-pdf"> + {{ add_header(1, layout|len, doc, letter_head, no_letterhead, footer, print_settings, print_heading_template) }} + </div> +{% endif %} +{% for page in layout %} +<div class="page-break invoice-page"> + {% if not print_settings.repeat_header_footer %} + {{ add_header(loop.index, layout|len, doc, letter_head, no_letterhead, footer, print_settings, print_heading_template) }} + {% endif %}
113-124: Constrain item images to prevent layout shift.Images aren’t sized; add rules so thumbnails consistently fit the 24px box.
Apply this diff:
.img-thumb { min-width: 24px; min-height: 24px; max-width: 24px; max-height: 24px; border-radius: 6px; overflow: hidden; display: flex; align-items: center; justify-content: center; background: #f0f0f0; } +/* Ensure contained image fills thumbnail consistently */ +.img-thumb img { + width: 100%; + height: 100%; + object-fit: cover; + display: block; +}Also applies to: 141-152
158-176: Localizable labels and address HTML safety.Wrap static labels with
_()and renderaddress_displayas safe HTML to preserve line breaks.Apply this diff:
- <div class="info-text">Customer Name:</div> - <div class="info-text">Bill to:</div> + <div class="info-text">{{ _("Customer Name:") }}</div> + <div class="info-text">{{ _("Bill to:") }}</div> ... - <div class="info-text">Invoice Number:</div> - <div class="info-text">Invoice Date:</div> - <div class="info-text">Payment Due Date:</div> + <div class="info-text">{{ _("Invoice Number:") }}</div> + <div class="info-text">{{ _("Invoice Date:") }}</div> + <div class="info-text">{{ _("Payment Due Date:") }}</div> ... - <div class="info-text">{{ doc.address_display or "" }}</div> + <div class="info-text">{{ (doc.address_display or "") | safe }}</div>
194-201: Use table headers (th) for semantics and better accessibility.Header cells are marked as
td. Switch tothwith scope.Apply this diff:
- <tr> - <td class="text-center">{{ _("No") }}</td> - <td class="text-left">{{ _("Item") }}</td> - <td class="text-center">{{ _("HSN/SAC") }}</td> - <td class="text-right">{{ _("Quantity") }}</td> - <td class="text-right">{{ _("Rate") }}</td> - <td class="text-right">{{ _("Amount") }}</td> - </tr> + <tr> + <th scope="col" class="text-center">{{ _("No") }}</th> + <th scope="col" class="text-left">{{ _("Item") }}</th> + <th scope="col" class="text-center">{{ _("HSN/SAC") }}</th> + <th scope="col" class="text-right">{{ _("Quantity") }}</th> + <th scope="col" class="text-right">{{ _("Rate") }}</th> + <th scope="col" class="text-right">{{ _("Amount") }}</th> + </tr>- <tr> - <td style="width: 5%" class="text-left">{{ _("HSN/SAC") }}</td> - <td style="width: 15%" class="text-right">{{ _("Taxable Amount") }}</td> - {% for row in doc.taxes %} - <td style="width: 10%" class="text-right">{{ row.description }} ({{ row.rate }}%)</td> - {% endfor %} - </tr> + <tr> + <th scope="col" style="width: 5%" class="text-left">{{ _("HSN/SAC") }}</th> + <th scope="col" style="width: 15%" class="text-right">{{ _("Taxable Amount") }}</th> + {% for row in doc.taxes %} + <th scope="col" style="width: 10%" class="text-right">{{ row.description }} ({{ row.rate }}%)</th> + {% endfor %} + </tr>Also applies to: 281-286
261-262: Avoid forced capitalization on dynamic text.
text-capitalizecan distort “In Words” in some locales/scripts.Apply this diff:
- <div class="text-capitalize"><span class="title"> {{ _("In Words: ") }}</span>{{ doc.in_words }}</div> + <div><span class="title"> {{ _("In Words: ") }}</span>{{ doc.in_words }}</div>
306-309: Render terms as HTML.
doc.termsis rich text; without| safeit may render escaped.Apply this diff:
- {{ doc.terms}} + {{ doc.terms | safe }}
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html(1 hunks)erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.json
⏰ 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). (6)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Patch Test
- GitHub Check: Summary
erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html
Outdated
Show resolved
Hide resolved
erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html
Outdated
Show resolved
Hide resolved
erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html
Outdated
Show resolved
Hide resolved
khushi8112
force-pushed
the
print-format-for-sales-invoice
branch
from
2424437 to
dd5277d
Compare
|
Is this ready for review? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/accounts/doctype/process_statement_of_accounts/test_process_statement_of_accounts.py(1 hunks)
⏰ 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). (6)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Patch Test
- GitHub Check: Summary
erpnext/accounts/doctype/process_statement_of_accounts/test_process_statement_of_accounts.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
erpnext/fixtures/letter_head.json (1)
27-27: Invalid HTML attribute: class="font-size: 13px;" → style="font-size: 13px;"Same issue previously noted; still present here.
- <span class="font-size: 13px;"> + <span style="font-size: 13px;">
🧹 Nitpick comments (5)
erpnext/fixtures/letter_head.json (5)
4-4: Fix logo container alignment and image fit (current CSS doesn’t center or constrain the image)..align-content on .logo has no effect without display:flex; the
may overflow the 180x90 box.
- .logo { - border: 1px solid #EDEDED; - border-radius: 15px; - width: 180px; - height: 90px; - align-content: center; - margin-right: 5px; - } + .logo { + border: 1px solid #EDEDED; + border-radius: 15px; + width: 180px; + height: 90px; + display: flex; + align-items: center; + justify-content: center; + margin-right: 5px; + overflow: hidden; + } + .logo img { + max-width: 100%; + max-height: 100%; + object-fit: contain; + display: block; + } @@ - <img src="{{ frappe.utils.get_url(company_logo) }}"> + <img src="{{ frappe.utils.get_url(company_logo) }}" alt="{{ doc.company }} logo">
27-27: Apply the same logo fit/alt improvements to the second fixture.Mirror the .logo/.logo img rules and add alt on the
.
@@ - <div class="logo"> + <div class="logo"> @@ - <img src="{{ frappe.utils.get_url(company_logo) }}"> + <img src="{{ frappe.utils.get_url(company_logo) }}" alt="{{ doc.company }} logo">And include the same CSS block added in the first fixture for .logo and .logo img.
4-4: Minor: drop stray
in flex rows to avoid inconsistent spacing.Line‑breaks after values inside flex rows aren’t needed and can cause odd gaps.
- <div>{{ doc.name }}</div><br> + <div>{{ doc.name }}</div> @@ - <div>{{ website }}</div><br> + <div>{{ website }}</div> @@ - <div>{{ email }}</div><br> + <div>{{ email }}</div> @@ - <div>{{ phone_no }}</div><br> + <div>{{ phone_no }}</div>Also applies to: 27-27
4-4: i18n/layout: avoid fixed 60px label width; allow wrapping for longer locales.Hard width can truncate translated labels. Use min-width or auto layout.
- .letterhead-title { - color: #7C7C7C; - width: 60px; - flex: 1 0 0; - } + .letterhead-title { + color: #7C7C7C; + min-width: 60px; /* acts as a hint */ + flex: 0 0 auto; + word-break: break-word; + }Also applies to: 27-27
4-4: Avoid per-render DB calls in the letterhead templatefrappe.db.get_value, frappe.get_all and frappe.get_doc are executed during Jinja rendering in erpnext/fixtures/letter_head.json (lines 4 and 27). Replace single-field lookups with frappe.get_cached_value and move the address/company lookups into a precomputed print/context helper (fetch once in Python) to eliminate repeated queries.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/fixtures/letter_head.json(1 hunks)
⏰ 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). (6)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Patch Test
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html (4)
235-235: Bug: wrong get_formatted argument for quantity.Second arg should be the document, not 0, to ensure correct locale/precision.
Apply:
- <td class="text-right">{{ item.get_formatted("qty", 0) }} {{ item.uom }}</td> + <td class="text-right">{{ item.get_formatted("qty", doc) }} {{ item.uom }}</td>
302-306: Invalid HTML: div inside table; use caption or move label above.Place “Tax Breakup” as a or above the table.
Apply (preferred):
- <table class="tax-table mt-15"> - <div class="mt-40 pb-8">{{ _("Tax Breakup") }}</div> + <table class="tax-table mt-15"> + <caption class="text-left mt-40 pb-8">{{ _("Tax Breakup") }}</caption> <thead class="highlight-bg">
266-274: Tax percent computation is incorrect; remove synthetic percent.Summing tax.rate is wrong for Actual/zero/compound rows. Show total only.
Apply:
-{% set tax_rate = doc.taxes | selectattr("tax_amount") | sum(attribute="rate") %} {% if - if tax_rate > 0 %} - <tr class="row-divider"> - <td class="text-right title">{{ _("Tax") }} ({{ tax_rate }}%):</td> - <td class="text-right"> - {{ doc.get_formatted("total_taxes_and_charges", doc) }} - </td> - </tr> - {% endif %} +{% if doc.total_taxes_and_charges %} + <tr class="row-divider"> + <td class="text-right title">{{ _("Taxes and Charges:") }}</td> + <td class="text-right">{{ doc.get_formatted("total_taxes_and_charges", doc) }}</td> + </tr> +{% endif %}
318-323: Currency mismatch and misleading per‑item tax math.Use document currency fields; current calc mixes base_* with doc currency and assumes percent charge type.
Apply minimal currency fix:
- <td class="text-right">{{ item.get_formatted("base_net_amount", doc) }}</td> -{% for row in doc.taxes %} {% set tax_amount = item.base_net_amount * (row.rate / 100) %} + <td class="text-right">{{ item.get_formatted("net_amount", doc) }}</td> +{% for row in doc.taxes %} {% set tax_amount = item.net_amount * (row.rate / 100) %} <td class="text-right"> {{ frappe.format_value(tax_amount, {"fieldtype": "Currency"}, doc) }} </td>Better: replace the per‑item breakup with a per‑tax summary to handle Actual/compound rows correctly. Example:
-{% if doc.taxes_and_charges %} - ... current per‑item breakup table ... -{% endif %} +{% if doc.taxes and doc.taxes|length > 0 %} + <div class="mt-40 pb-8">{{ _("Taxes and Charges Breakup") }}</div> + <table class="tax-table mt-15"> + <thead class="highlight-bg"> + <tr> + <th scope="col" class="text-left">{{ _("Description") }}</th> + <th scope="col" class="text-right">{{ _("Rate") }}</th> + <th scope="col" class="text-right">{{ _("Amount") }}</th> + </tr> + </thead> + <tbody> + {% for row in doc.taxes %} + <tr> + <td class="text-left">{{ row.description }}</td> + <td class="text-right"> + {% if row.charge_type in ["On Net Total","On Previous Row Amount","On Previous Row Total"] and row.rate %} + {{ row.rate }}% + {% else %}—{% endif %} + </td> + <td class="text-right">{{ row.get_formatted("tax_amount", doc) }}</td> + </tr> + {% endfor %} + </tbody> + </table> +{% endif %}Also applies to: 319-323
🧹 Nitpick comments (8)
erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html (8)
302-302: Show tax breakup based on presence of tax rows, not template name.doc.taxes_and_charges is a template string; taxes can exist without it.
Apply:
-{% if doc.taxes_and_charges %} +{% if doc.taxes and doc.taxes|length > 0 %}
260-265: Hide discount row when no discount; avoid “(0%)”.Only render when discount exists; conditionally show percent label.
Apply:
- <tr class="row-divider"> - <td class="text-right title"> - {{ _("Discount") }} ({{ doc.additional_discount_percentage or 0 }}%): - </td> - <td class="text-right">{{ doc.get_formatted("discount_amount", doc) }}</td> - </tr> +{% if doc.discount_amount %} + <tr class="row-divider"> + <td class="text-right title"> + {{ _("Discount") }}{% if doc.additional_discount_percentage %} ({{ doc.additional_discount_percentage }}%){% endif %}: + </td> + <td class="text-right">{{ doc.get_formatted("discount_amount", doc) }}</td> + </tr> +{% endif %}
207-215: A11y: use th for table headers.Header cells should be .
Apply:
- <td class="text-center">{{ _("No") }}</td> - <td class="text-left">{{ _("Item") }}</td> - <td class="text-center">{{ _("HSN/SAC") }}</td> - <td class="text-right">{{ _("Quantity") }}</td> - <td class="text-right">{{ _("Rate") }}</td> - <td class="text-right">{{ _("Amount") }}</td> + <th scope="col" class="text-center">{{ _("No") }}</th> + <th scope="col" class="text-left">{{ _("Item") }}</th> + <th scope="col" class="text-center">{{ _("HSN/SAC") }}</th> + <th scope="col" class="text-right">{{ _("Quantity") }}</th> + <th scope="col" class="text-right">{{ _("Rate") }}</th> + <th scope="col" class="text-right">{{ _("Amount") }}</th>
307-312: A11y: use th in tax table header too.Improve semantics for screen readers.
Apply:
- <td style="width: 5%" class="text-left">{{ _("HSN/SAC") }}</td> - <td style="width: 15%" class="text-right">{{ _("Taxable Amount") }}</td> - {% for row in doc.taxes %} - <td style="width: 10%" class="text-right">{{ row.description }} ({{ row.rate }}%)</td> + <th scope="col" style="width: 5%" class="text-left">{{ _("HSN/SAC") }}</th> + <th scope="col" style="width: 15%" class="text-right">{{ _("Taxable Amount") }}</th> + {% for row in doc.taxes %} + <th scope="col" style="width: 10%" class="text-right">{{ row.description }}{% if row.rate %} ({{ row.rate }}%){% endif %}</th> {% endfor %}
173-176: Localize static labels in header.Wrap labels with _() for translation consistency.
Apply:
- <div class="info-text">Customer Name:</div> - <div class="info-text">Bill to:</div> + <div class="info-text">{{ _("Customer Name:") }}</div> + <div class="info-text">{{ _("Bill to:") }}</div> ... - <div class="info-text">Invoice Number:</div> - <div class="info-text">Invoice Date:</div> - <div class="info-text">Payment Due Date:</div> + <div class="info-text">{{ _("Invoice Number:") }}</div> + <div class="info-text">{{ _("Invoice Date:") }}</div> + <div class="info-text">{{ _("Payment Due Date:") }}</div>Also applies to: 184-186
285-287: Tweak translation string for “In Words”.Avoid trailing space inside _(); place space outside.
Apply:
- <span class="title"> {{ _("In Words: ") }}</span>{{ doc.in_words }} + <span class="title">{{ _("In Words:") }}</span> {{ doc.in_words }}
1-5: Unused macro parameter “footer”.Parameter is accepted and passed but never used. Remove to reduce surface area.
Apply:
-{%- macro add_header(page_num, max_pages, doc, letter_head, no_letterhead, footer, print_settings=None, -print_heading_template=None) -%} +{%- macro add_header(page_num, max_pages, doc, letter_head, no_letterhead, print_settings=None, +print_heading_template=None) -%} ... - {{ add_header(loop.index, layout|len, doc, letter_head, no_letterhead, footer, print_settings) }} + {{ add_header(loop.index, layout|len, doc, letter_head, no_letterhead, print_settings) }}Also applies to: 8-8
142-144: Non‑standard font-weight value.420 is unusual; consider 400 (normal) or 500 (medium) for predictability.
Apply:
- font-weight: 420; + font-weight: 400;
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html(1 hunks)erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html
⏰ 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). (6)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (1)
erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html (1)
197-241: Overall: solid structure and styling.Layout, formatting calls, and multi‑page header usage look good. After fixing the above, this will be production‑ready.
Please re-render a sample invoice with: no taxes, percent taxes, actual charges, and zero discount to confirm conditional rows behave as expected.
Also applies to: 243-300, 302-337
khushi8112
force-pushed
the
print-format-for-sales-invoice
branch
from
8cc2bd5 to
7d4daa3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html (2)
268-274: Tax percentage display is misleading for mixed/actual taxes.Summing rates across rows (and filtering by tax_amount) isn’t meaningful; show total taxes without a single “%”.
- {% set tax_rate = doc.taxes | selectattr("tax_amount") | sum(attribute="rate") %} {% if - tax_rate > 0 %} - <tr> - <td class="text-right text-muted">{{ _("Tax") }} ({{ tax_rate }}%):</td> - <td class="text-right">{{ doc.get_formatted("total_taxes_and_charges", doc) }}</td> - </tr> - {% endif %} + {% if doc.total_taxes_and_charges %} + <tr> + <td class="text-right text-muted">{{ _("Taxes:") }}</td> + <td class="text-right">{{ doc.get_formatted("total_taxes_and_charges", doc) }}</td> + </tr> + {% endif %}
263-267: Guard discount percentage when missing; avoid “(None%)”.Only show the percentage when present; optionally hide the row when discount_amount is zero.
- <td class="text-right text-muted"> - {{ _("Discount") }} ({{ doc.additional_discount_percentage }}%): - </td> - <td class="text-right">{{ doc.get_formatted("discount_amount", doc) }}</td> + {% if doc.discount_amount %} + <td class="text-right text-muted"> + {{ _("Discount") }}{% if doc.additional_discount_percentage %} ({{ doc.additional_discount_percentage }}%){% endif %}: + </td> + <td class="text-right">{{ doc.get_formatted("discount_amount", doc) }}</td> + {% endif %}
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html(1 hunks)erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html
⏰ 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). (6)
- GitHub Check: Patch Test
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Summary
🔇 Additional comments (5)
erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html (5)
27-27: wkhtmltopdf compatibility: add CSS var fallback + normalize font-weight.Add a literal fallback for the CSS variable and use a standard weight; 420 isn’t valid across engines.
- color: var(--black-overlay-700); + color: var(--black-overlay-700, #171717);- font-weight: 420; + font-weight: 400;Also applies to: 166-166
252-254: Verify text-uppercase availability.If the base print CSS doesn’t provide .text-uppercase, add
text-transform: uppercase;in this template.
201-212: “Bill From/To” reversed and
trim off-by-one; also handle
variants.Show company address under “Bill From” and customer address under “Bill To”; trim 6 chars for "
" and support "
" and "
".- {% set address = (doc.address_display) %} - {% if address and address.rstrip().endswith("<br />") %} - {% set address = address[:-5] %} - {% endif %} - - {% set company_address_display = (doc.company_address_display) %} - {% if company_address_display and company_address_display.rstrip().endswith("<br />") %} - {% set company_address_display = company_address_display[:-5] %} - {% endif %} - <td><span class="heading">{{ _("Bill From") }}:</span><br />{{ address }}</td> - <td><span class="heading">{{ _("Bill To") }}:</span><br />{{ company_address_display }}</td> + {% set address = (doc.address_display or "").rstrip() %} + {% if address.endswith("<br />") %}{% set address = address[:-6] %}{% elif address.endswith("<br/>") %}{% set address = address[:-5] %}{% elif address.endswith("<br>") %}{% set address = address[:-4] %}{% endif %} + + {% set company_address_display = (doc.company_address_display or "").rstrip() %} + {% if company_address_display.endswith("<br />") %}{% set company_address_display = company_address_display[:-6] %}{% elif company_address_display.endswith("<br/>") %}{% set company_address_display = company_address_display[:-5] %}{% elif company_address_display.endswith("<br>") %}{% set company_address_display = company_address_display[:-4] %}{% endif %} + <td><span class="heading">{{ _("Bill From") }}:</span><br />{{ company_address_display }}</td> + <td><span class="heading">{{ _("Bill To") }}:</span><br />{{ address }}</td>
238-238: Fix qty formatting context.Pass the parent doc to get_formatted; 0 can mis-format quantities.
- <td class="text-right text-dark">{{ item.get_formatted("qty", 0) }} {{ item.uom }}</td> + <td class="text-right text-dark">{{ item.get_formatted("qty", doc) }} {{ item.uom }}</td>
302-307: Use item_wise_tax_detail for per‑item tax breakup.Current math ignores item-wise rules and discounts; read the row’s mapping.
- {% for row in doc.taxes %} {% set tax_amount = item.base_net_amount * (row.rate / 100) - %} - <td class="text-right"> - ({{ (row.rate) }}%) {{ frappe.format_value(tax_amount, {"fieldtype": "Currency"}, - doc) }} - </td> - {% endfor %} + {% for row in doc.taxes %} + {% set detail = frappe.parse_json(row.item_wise_tax_detail or '{}') %} + {% set per_item = (detail.get(item.item_code) if detail else None) %} + {% set per_item_rate = (per_item[0] if per_item else (row.rate or 0)) %} + {% set per_item_amount = (per_item[1] if per_item else 0) %} + <td class="text-right"> + ({{ per_item_rate }}%) {{ frappe.format_value(per_item_amount, {"fieldtype": "Currency"}, doc) }} + </td> + {% endfor %}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
erpnext/hooks.py (1)
604-646: Replace invalid DocType in global_search_doctypes
- In erpnext/hooks.py under the
"Default"group, change
{"doctype": "Sales Taxes and Charges", "index": 27}
to
{"doctype": "Sales Taxes and Charges Template", "index": 27}
🧹 Nitpick comments (5)
erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html (1)
170-176: Wrap static labels with _() for i18nLocalize these user-facing labels.
Apply:
- <div class="info-text">Customer Name:</div> - <div class="info-text">Bill to:</div> + <div class="info-text">{{ _("Customer Name:") }}</div> + <div class="info-text">{{ _("Bill to:") }}</div> @@ - <div class="info-text">Invoice Number:</div> - <div class="info-text">Invoice Date:</div> - <div class="info-text">Payment Due Date:</div> + <div class="info-text">{{ _("Invoice Number:") }}</div> + <div class="info-text">{{ _("Invoice Date:") }}</div> + <div class="info-text">{{ _("Payment Due Date:") }}</div>Also applies to: 181-184
erpnext/accounts/letterhead/letterhead_with_background_colour.html (2)
101-106: Avoid doc-specific fields in letterheadLetterhead should be reusable; don’t hardcode Sales Invoice title and doc.name here (already shown in the print format).
Apply:
- <div style="height: 90px; margin-bottom: 10px; text-align: right;"> - <div class="invoice-title">{{ _("Sales Invoice") }}</div> - <div class="invoice-number">{{ doc.name }}</div> - <br> - </div> + <div style="height: 90px; margin-bottom: 10px; text-align: right;"></div>
108-120: Reduce DB round-trips: fetch website/email/phone in one callConsolidate 3 get_value calls into one for efficiency.
Apply:
- {% set website = frappe.db.get_value("Company", doc.company, "website") %} - {% set email = frappe.db.get_value("Company", doc.company, "email") %} - {% set phone_no = frappe.db.get_value("Company", doc.company, "phone_no") %} + {% set company_info = frappe.db.get_value("Company", doc.company, ["website", "email", "phone_no"], as_dict=True) %} @@ - {% if website %} - <div><span class="contact-title">{{ _("Website:") }}</span><span class="contact-value">{{ website }}</span></div> + {% if company_info and company_info.website %} + <div><span class="contact-title">{{ _("Website:") }}</span><span class="contact-value">{{ company_info.website }}</span></div> {% endif %} - {% if email %} - <div><span class="contact-title">{{ _("Email:") }}</span><span class="contact-value">{{ email }}</span></div> + {% if company_info and company_info.email %} + <div><span class="contact-title">{{ _("Email:") }}</span><span class="contact-value">{{ company_info.email }}</span></div> {% endif %} - {% if phone_no %} - <div><span class="contact-title">{{ _("Contact:") }}</span><span class="contact-value">{{ phone_no }}</span></div> + {% if company_info and company_info.phone_no %} + <div><span class="contact-title">{{ _("Contact:") }}</span><span class="contact-value">{{ company_info.phone_no }}</span></div> {% endif %}erpnext/accounts/letterhead/letterhead_plain.html (2)
98-101: Avoid doc-specific fields in letterheadRemove Invoice/Name from letterhead; it’s already in the print format and reduces reusability.
Apply:
- <div class="invoice-info"> - <span class="invoice-label">{{ _("Invoice:") }}</span> - <span>{{ doc.name }}</span> - </div>
94-96: Reduce DB round-trips for company contact fieldsFetch website/email/phone in one call.
Apply:
- {% set website = frappe.db.get_value("Company", doc.company, "website") %} - {% set email = frappe.db.get_value("Company", doc.company, "email") %} - {% set phone_no = frappe.db.get_value("Company", doc.company, "phone_no") %} + {% set company_info = frappe.db.get_value("Company", doc.company, ["website", "email", "phone_no"], as_dict=True) %} @@ - {% if website %} + {% if company_info and company_info.website %} <div class="invoice-info"> <span class="invoice-label">{{ _("Website:") }}</span> - <span>{{ website }}</span> + <span>{{ company_info.website }}</span> </div> {% endif %} - {% if email %} + {% if company_info and company_info.email %} <div class="invoice-info"> <span class="invoice-label">{{ _("Email:") }}</span> - <span>{{ email }}</span> + <span>{{ company_info.email }}</span> </div> {% endif %} - {% if phone_no %} + {% if company_info and company_info.phone_no %} <div class="invoice-info"> <span class="invoice-label">{{ _("Contact:") }}</span> - <span>{{ phone_no }}</span> + <span>{{ company_info.phone_no }}</span> </div> {% endif %}Also applies to: 103-119
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
erpnext/accounts/letterhead/letterhead_plain.html(1 hunks)erpnext/accounts/letterhead/letterhead_with_background_colour.html(1 hunks)erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html(1 hunks)erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html(1 hunks)erpnext/hooks.py(1 hunks)erpnext/setup/install.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html
🧰 Additional context used
🪛 GitHub Actions: Linters
erpnext/setup/install.py
[error] 268-280: Blocking rule fired: frappe-semgrep-rules.rules.frappe-manual-commit. Manually committing a transaction is highly discouraged. If manual commit is required, add a justification comment and a 'nosemgrep' on the same line; otherwise refactor to use the framework's transaction management.
⏰ 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). (6)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Patch Test
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Summary
🔇 Additional comments (2)
erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html (2)
246-246: Fix get_formatted arg for qtySecond arg must be the document, not 0.
Apply:
- <td class="text-right">{{ item.get_formatted("qty", 0) }} {{ item.uom }}</td> + <td class="text-right">{{ item.get_formatted("qty", doc) }} {{ item.uom }}</td>
277-285: Remove synthetic tax percent; it’s incorrectSumming tax.rate is wrong for Actual/zero/compound rows. Show total taxes without a fake percent.
Apply:
- {% set tax_rate = doc.taxes | selectattr("tax_amount") | sum(attribute="rate") %} {% if tax_rate > 0 %} - <tr class="row-divider"> - <td class="text-right title">{{ _("Tax") }} ({{ tax_rate }}%):</td> - <td class="text-right"> - {{ doc.get_formatted("total_taxes_and_charges", doc) }} - </td> - </tr> - {% endif %} + {% if doc.total_taxes_and_charges %} + <tr class="row-divider"> + <td class="text-right title">{{ _("Taxes and Charges:") }}</td> + <td class="text-right">{{ doc.get_formatted("total_taxes_and_charges", doc) }}</td> + </tr> + {% endif %}
khushi8112
force-pushed
the
print-format-for-sales-invoice
branch
from
3e4a29d to
b22d1d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
erpnext/accounts/letterhead/letterhead_with_background_colour.html (1)
108-121: Reduce DB round-trips and add alt textFetch multiple Company fields in one call and add alt text for the logo for accessibility.
Apply:
- <div style="text-align: left; float: right;" class="other-details"> - {% set website = frappe.db.get_value("Company", doc.company, "website") %} - {% set email = frappe.db.get_value("Company", doc.company, "email") %} - {% set phone_no = frappe.db.get_value("Company", doc.company, "phone_no") %} + <div style="text-align: left; float: right;" class="other-details"> + {% set company = frappe.db.get_value("Company", doc.company, ["website", "email", "phone_no"], as_dict=True) or {} %} - {% if website %} - <div><span class="contact-title">{{ _("Website:") }}</span><span class="contact-value">{{ website }}</span></div> + {% if company.website %} + <div><span class="contact-title">{{ _("Website:") }}</span><span class="contact-value">{{ company.website }}</span></div> {% endif %} - {% if email %} - <div><span class="contact-title">{{ _("Email:") }}</span><span class="contact-value">{{ email }}</span></div> + {% if company.email %} + <div><span class="contact-title">{{ _("Email:") }}</span><span class="contact-value">{{ company.email }}</span></div> {% endif %} - {% if phone_no %} - <div><span class="contact-title">{{ _("Contact:") }}</span><span class="contact-value">{{ phone_no }}</span></div> + {% if company.phone_no %} + <div><span class="contact-title">{{ _("Contact:") }}</span><span class="contact-value">{{ company.phone_no }}</span></div> {% endif %} </div>And at Line 77:
- <img src="{{ frappe.utils.get_url(company_logo) }}"> + <img src="{{ frappe.utils.get_url(company_logo) }}" alt="{{ doc.company }} logo">erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html (2)
6-9: Remove empty else branchNo-op else adds noise.
Apply:
{% if print_heading_template %} {{ frappe.render_template(print_heading_template, {"doc":doc}) }} -{% else %} {% endif %}
263-267: Guard against None for discount percentShow 0% instead of “None%”.
Apply:
- <td class="text-right text-muted"> - {{ _("Discount") }} ({{ doc.additional_discount_percentage }}%): - </td> + <td class="text-right text-muted"> + {{ _("Discount") }} ({{ doc.additional_discount_percentage or 0 }}%): + </td>
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
erpnext/accounts/doctype/process_statement_of_accounts/test_process_statement_of_accounts.py(1 hunks)erpnext/accounts/letterhead/letterhead_plain.html(1 hunks)erpnext/accounts/letterhead/letterhead_with_background_colour.html(1 hunks)erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html(1 hunks)erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.json(1 hunks)erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html(1 hunks)erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.json(1 hunks)erpnext/hooks.py(1 hunks)erpnext/setup/install.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- erpnext/accounts/letterhead/letterhead_plain.html
- erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.json
- erpnext/accounts/doctype/process_statement_of_accounts/test_process_statement_of_accounts.py
⏰ 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). (7)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: semgrep
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (12)
erpnext/setup/install.py (3)
38-38: Good call: create letterheads during installHooking
create_letter_head()intoafter_install()ensures the assets exist post-setup.
251-257: Correct base pathUsing
frappe.get_app_path("erpnext", "accounts", "letterhead")points to the right template directory.
259-272: Remove manual commit (CI blocker) and rely on outer transactionThe explicit
frappe.db.commit()insidecreate_letter_head()violates CI rules and is unnecessary—after_install()commits.Apply:
for name, filename in letterheads.items(): if not frappe.db.exists("Letter Head", name): content = frappe.read_file(os.path.join(base_path, filename)) doc = frappe.get_doc( { "doctype": "Letter Head", "letter_head_name": name, "source": "HTML", "content": content, } ) doc.insert(ignore_permissions=True) - frappe.db.commit() +erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.json (1)
1-32: Print Format metadata looks consistentDocType, module, generator, language, margins, and standard flag are set appropriately.
erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html (5)
21-28: wkhtmltopdf compatibility: add CSS var fallbackCSS variables are unreliable in wkhtmltopdf. Provide a literal fallback.
Apply:
.invoice-wrapper { font-family: "Inter", sans-serif; - color: var(--black-overlay-700); + color: var(--black-overlay-700, #171717); }
162-168: Normalize non-standard font-weightUse standard weight 400 instead of 420 for broader support.
Apply:
.print-format { color: #171717; font-size: 14px; font-style: normal; - font-weight: 420; + font-weight: 400; line-height: 21px; padding: 0px; letter-spacing: 0.14px;
201-212: “Bill From”/“Bill To” swapped; also incorrect trim lengthSwap company/customer addresses and remove the full "
" (6 chars). Handle variants.Apply:
- {% set address = (doc.address_display) %} - {% if address and address.rstrip().endswith("<br />") %} - {% set address = address[:-5] %} - {% endif %} - - {% set company_address_display = (doc.company_address_display) %} - {% if company_address_display and company_address_display.rstrip().endswith("<br />") %} - {% set company_address_display = company_address_display[:-5] %} - {% endif %} - <td><span class="heading">{{ _("Bill From") }}:</span><br />{{ address }}</td> - <td><span class="heading">{{ _("Bill To") }}:</span><br />{{ company_address_display }}</td> + {% set address = (doc.address_display or "").rstrip() %} + {% if address.endswith("<br />") %}{% set address = address[:-6] %}{% elif address.endswith("<br/>") %}{% set address = address[:-5] %}{% elif address.endswith("<br>") %}{% set address = address[:-4] %}{% endif %} + + {% set company_address_display = (doc.company_address_display or "").rstrip() %} + {% if company_address_display.endswith("<br />") %}{% set company_address_display = company_address_display[:-6] %}{% elif company_address_display.endswith("<br/>") %}{% set company_address_display = company_address_display[:-5] %}{% elif company_address_display.endswith("<br>") %}{% set company_address_display = company_address_display[:-4] %}{% endif %} + <td><span class="heading">{{ _("Bill From") }}:</span><br />{{ company_address_display }}</td> + <td><span class="heading">{{ _("Bill To") }}:</span><br />{{ address }}</td>
238-238: Fix qty formatting contextPass the parent doc, not 0.
Apply:
- <td class="text-right text-dark">{{ item.get_formatted("qty", 0) }} {{ item.uom }}</td> + <td class="text-right text-dark">{{ item.get_formatted("qty", doc) }} {{ item.uom }}</td>
268-274: Remove synthetic tax percent; show total taxes onlySumming
rateis incorrect across charge types. Display the total taxes without a derived percent.Apply:
- {% set tax_rate = doc.taxes | selectattr("tax_amount") | sum(attribute="rate") %} {% if - tax_rate > 0 %} - <tr> - <td class="text-right text-muted">{{ _("Tax") }} ({{ tax_rate }}%):</td> - <td class="text-right">{{ doc.get_formatted("total_taxes_and_charges", doc) }}</td> - </tr> - {% endif %} + {% if doc.total_taxes_and_charges %} + <tr> + <td class="text-right text-muted">{{ _("Taxes and Charges:") }}</td> + <td class="text-right">{{ doc.get_formatted("total_taxes_and_charges", doc) }}</td> + </tr> + {% endif %}erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html (2)
12-14: Header wrapper: repeat support looks goodUsing
id="header-html"withhidden-pdfgated byrepeat_header_footeris correct.
246-246: Fix qty formatting contextPass the parent doc, not 0.
Apply:
- <td class="text-right">{{ item.get_formatted("qty", 0) }} {{ item.uom }}</td> + <td class="text-right">{{ item.get_formatted("qty", doc) }} {{ item.uom }}</td>erpnext/hooks.py (1)
605-646: Confirmed unique global_search_doctypes definition — no other assignments found across the codebase.
erpnext/accounts/letterhead/letterhead_with_background_colour.html
Outdated
Show resolved
Hide resolved
erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html
Outdated
Show resolved
Hide resolved
khushi8112
force-pushed
the
print-format-for-sales-invoice
branch
from
b22d1d4 to
d4a459e
Compare
📝 WalkthroughWalkthroughThis change introduces a new Sales Invoice print flow. A before_print hook on SalesInvoice checks company details and emits a realtime event when data is missing. A client script (page_js for print page) listens to this event, opens a dialog to collect company/logo/contact/address, and saves via a new whitelisted method that updates Company, creates/links Address, and updates the invoice’s address display. Two new Sales Invoice print formats are added. Two letterhead HTML templates are introduced. Setup now creates default letterheads. Global search doctypes and page_js are added in hooks. One test gains a blank line. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 7
♻️ Duplicate comments (4)
erpnext/accounts/letterhead/letterhead_with_background_colour.html (1)
102-106: Letterhead should not hardcode document-specific content.Lines 103-104 embed "Sales Invoice" and
doc.namein the letterhead, reducing reusability across DocTypes. Move document-specific title and number into the print format template, keeping the letterhead focused on branding only.Apply this diff:
- <div style="height: 90px; margin-bottom: 10px; text-align: right;"> - <div class="invoice-title">{{ _("Sales Invoice") }}</div> - <div class="invoice-number">{{ doc.name }}</div> - <br> - </div> + <!-- Document title/number belongs in print format, not letterhead -->erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html (1)
246-246: Fix incorrectget_formattedargument for quantity.The second argument to
get_formattedshould be the document, not0. Passing0risks incorrect formatting.Apply this diff:
- <td class="text-right">{{ item.get_formatted("qty", 0) }} {{ item.uom }}</td> + <td class="text-right">{{ item.get_formatted("qty", doc) }} {{ item.uom }}</td>erpnext/setup/install.py (1)
272-272: Remove manual commit (violates CI rule).Manual commits are flagged by CI. The outer
after_install()transaction already commits at line 39, so this commit is unnecessary.Apply this diff:
- frappe.db.commit()erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html (1)
237-247: Qty formatting still passes incorrect context.As noted in previous review, Line 241 passes
0as the doc/context toget_formatted("qty", 0), which may mis-format quantities.Apply this fix:
-<td class="text-right text-dark">{{ item.get_formatted("qty", 0) }} {{ item.uom }}</td> +<td class="text-right text-dark">{{ item.get_formatted("qty", doc) }} {{ item.uom }}</td>
🧹 Nitpick comments (4)
erpnext/accounts/letterhead/letterhead_plain.html (1)
93-120: Invoice information rendering is correct.The conditional rendering of website, email, and phone number properly handles optional fields.
The three separate
frappe.db.get_valuecalls could be combined into a single query for slight efficiency:-{% set website = frappe.db.get_value("Company", doc.company, "website") %} -{% set email = frappe.db.get_value("Company", doc.company, "email") %} -{% set phone_no = frappe.db.get_value("Company", doc.company, "phone_no") %} +{% set company_info = frappe.db.get_value("Company", doc.company, ["website", "email", "phone_no"], as_dict=True) %} +{% set website = company_info.website if company_info else None %} +{% set email = company_info.email if company_info else None %} +{% set phone_no = company_info.phone_no if company_info else None %}erpnext/public/js/print.js (3)
97-99: Reconsider the hard-coded reload delay.The 1-second timeout before reloading is arbitrary and could cause issues:
- If the server save is slow or the network is congested, the reload might occur before the transaction commits, showing stale data.
- The timeout provides no user control and ignores actual completion status.
Consider one of these approaches:
- Reload immediately after the successful callback (the server has already responded, so the data is committed).
- Use
frappe.set_route()to navigate away and back, triggering a clean refresh without a full page reload.- If a delay is truly necessary, increase it or poll for completion.
Example for approach 1:
callback: function () { companyDetailsDialog.hide(); - frappe.msgprint(__("Saving document...")); - setTimeout(() => { - window.location.reload(); - }, 1000); + frappe.show_alert({ + message: __("Company details saved successfully"), + indicator: "green" + }); + window.location.reload(); },
15-16: Use boolean literals for field properties.The dialog field definitions use
0and1for thereqdandhiddenproperties instead offalseandtrue. While this works (JavaScript is truthy/falsy), using proper booleans is more idiomatic and improves readability.Example for the first field:
{ label: "Company Logo", fieldname: "company_logo", fieldtype: "Attach Image", - reqd: data.company_logo ? 0 : 1, - hidden: data.company_logo ? 1 : 0, + reqd: !data.company_logo, + hidden: !!data.company_logo, },Apply similar changes to all other fields with conditional
reqd/hiddenproperties.Also applies to: 22-23, 29-30, 38-39, 45-46, 52-52, 59-60, 66-66, 81-82
86-102: Consider adding a loading indicator during save.When the user clicks "Save", there's no visual feedback that the operation is in progress until the
frappe.msgprintappears. For slow networks or large transactions, this could leave the user uncertain whether their action registered.Add a loading state to the primary action:
primary_action(values) { + companyDetailsDialog.set_primary_action("Saving...", null); + companyDetailsDialog.disable_primary_action(); frappe.call({ method: "erpnext.accounts.doctype.sales_invoice.sales_invoice.save_company_master_details", args: { name: data.name, company: data.company, details: values, }, callback: function () { companyDetailsDialog.hide(); frappe.msgprint(__("Saving document...")); setTimeout(() => { window.location.reload(); }, 1000); }, error: function () { + companyDetailsDialog.set_primary_action("Save", companyDetailsDialog.primary_action); + companyDetailsDialog.enable_primary_action(); beforePrintHandled = false; frappe.msgprint(__("Failed to save company details. Please try again.")); }, }); },
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
erpnext/accounts/doctype/process_statement_of_accounts/test_process_statement_of_accounts.py(1 hunks)erpnext/accounts/doctype/sales_invoice/sales_invoice.py(2 hunks)erpnext/accounts/letterhead/letterhead_plain.html(1 hunks)erpnext/accounts/letterhead/letterhead_with_background_colour.html(1 hunks)erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html(1 hunks)erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.json(1 hunks)erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html(1 hunks)erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.json(1 hunks)erpnext/hooks.py(2 hunks)erpnext/public/js/print.js(1 hunks)erpnext/setup/install.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
erpnext/accounts/doctype/sales_invoice/sales_invoice.py (2)
erpnext/stock/doctype/delivery_note/delivery_note.py (1)
before_print(233-251)erpnext/controllers/accounts_controller.py (1)
before_print(607-629)
🪛 GitHub Actions: Linters
erpnext/setup/install.py
[error] 272-272: frappe-semgrep-rules.rules.frappe-manual-commit: Manually committing a transaction is highly discouraged. If manual commit is required, add a comment explaining why and add '# nosemgrep' on the same line. (See https://frappeframework.com/docs/user/en/api/database#database-transaction-model)
🪛 Ruff (0.13.1)
erpnext/accounts/doctype/sales_invoice/sales_invoice.py
282-282: Unused method argument: settings
(ARG002)
⏰ 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). (6)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (16)
erpnext/accounts/doctype/process_statement_of_accounts/test_process_statement_of_accounts.py (1)
84-84: LGTM – formatting improvement.The added blank line improves readability with no functional impact.
erpnext/hooks.py (2)
54-54: LGTM – print script wired correctly.The
page_jsmapping loads the new print flow script on the print page.
605-648: LGTM – global search configuration added.The
global_search_doctypesconfiguration establishes search priority for common ERPNext DocTypes.erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.html (2)
269-276: LGTM – tax display corrected.The tax display now iterates individual tax rows without synthetic percent computation, aligning with the previous fix.
288-304: LGTM – grand total section correct.The grand total and in-words display use correct field references and formatting.
erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.json (1)
1-33: LGTM! Print format configuration is well-structured.The JSON configuration for the Sales Invoice print format is properly structured with appropriate settings for a Jinja-based template using wkhtmltopdf.
erpnext/accounts/print_format/print_format_sales_invoice/print_format_sales_invoice.json (1)
1-33: Clarify the need for two similar print formats.Two print format configurations are being added with nearly identical settings:
- "Sales Invoice Print Format" (in
sales_invoice_print_format/)- "Print Format Sales Invoice" (in
print_format_sales_invoice/)Both target the same DocType ("Sales Invoice") with the same settings (Jinja type, wkhtmltopdf, font size 14, margins 15.0). Please verify whether both are intentional or if one should be removed to avoid duplication and potential user confusion.
erpnext/accounts/letterhead/letterhead_plain.html (3)
1-55: CSS styling looks appropriate for letterhead.The inline styles are well-structured for a print/email letterhead template with appropriate spacing, borders, and layout.
57-67: Logo rendering logic is correct.The conditional rendering and URL handling for the company logo are implemented properly.
69-91: Address resolution logic is correct.The Dynamic Link query pattern properly finds the company's address, and the conditional rendering safely handles cases where no address exists.
erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html (5)
1-19: Macro and page layout structure is correct.The
add_headermacro properly handles letterhead, print heading, and document status, and the page loop correctly iterates through the layout.
21-184: CSS styling is comprehensive and well-organized.The styles appropriately handle tables, text formatting, spacing, and print-specific layout needs.
251-270: Totals section structure and Net Total discount logic are correct.The "Total in words" display and conditional Net Total discount rendering are properly implemented.
271-286: Tax rendering logic is correct.The tax loop properly displays each tax's description, rate, and amount, respecting the conditions for zero-amount taxes and inclusive taxes. The conditional Grand Total discount rendering is also correct.
287-303: Grand Total and Terms sections are correct.The grand total display and conditional terms rendering are properly implemented.
erpnext/public/js/print.js (1)
3-5: Validate data object structure.The code accesses properties on the
dataobject (e.g.,data.company_logo,data.address_line,data.company,data.name) without validating that the object or these properties exist. If the server-sidebefore_printhook publishes an incomplete payload, this will cause runtime errors.Add defensive checks or validate the data structure at the start of the handler:
frappe.realtime.on("sales_invoice_before_print", (data) => { if (!data || !data.name || !data.company) { console.error("Invalid sales_invoice_before_print data:", data); return; } const route = frappe.get_route(); // ... rest of the code });Please verify that the server-side
before_printhook insales_invoice.pyalways publishes the required fields (name,company,company_logo,address_line) to ensure this client code receives valid data.
erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html
Outdated
Show resolved
Hide resolved
erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html
Outdated
Show resolved
Hide resolved
erpnext/accounts/print_format/sales_invoice_print_format/sales_invoice_print_format.html
Outdated
Show resolved
Hide resolved
khushi8112
force-pushed
the
print-format-for-sales-invoice
branch
from
6a1176e to
6e07aac
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
erpnext/accounts/doctype/sales_invoice/sales_invoice.py (1)
2850-2899: Harden save_company_master_details against unauthorized updates.Any logged-in user can call this endpoint and update Company fields or mutate a Sales Invoice regardless of permissions or company ownership, because we never verify the invoice, its company, or writer rights before issuing raw
db.set_valuecalls. That’s a security issue.Please guard the entry point before mutating anything:
@frappe.whitelist() def save_company_master_details(name, company, details): from frappe.utils import validate_email_address + si = frappe.get_doc("Sales Invoice", name) + si.check_permission("write") + + if si.company != company: + frappe.throw( + _("Sales Invoice {0} does not belong to Company {1}.").format(name, company) + ) + + company_doc = frappe.get_doc("Company", company) + company_doc.check_permission("write") + if isinstance(details, str): details = frappe.parse_json(details)With the guard in place, only users who can actually edit the target Sales Invoice and Company will get through, and cross-company tampering is blocked.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
erpnext/accounts/doctype/process_statement_of_accounts/test_process_statement_of_accounts.py(1 hunks)erpnext/accounts/doctype/sales_invoice/sales_invoice.py(2 hunks)erpnext/accounts/letterhead/letterhead_plain.html(1 hunks)erpnext/accounts/letterhead/letterhead_with_background_colour.html(1 hunks)erpnext/accounts/print_format/sales_invoice_standard/sales_invoice_standard.json(1 hunks)erpnext/accounts/print_format/sales_invoice_with_item_image/sales_invoice_with_item_image.json(1 hunks)erpnext/hooks.py(2 hunks)erpnext/public/js/print.js(1 hunks)erpnext/setup/install.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- erpnext/accounts/letterhead/letterhead_with_background_colour.html
- erpnext/accounts/doctype/process_statement_of_accounts/test_process_statement_of_accounts.py
- erpnext/public/js/print.js
- erpnext/accounts/letterhead/letterhead_plain.html
🧰 Additional context used
🧬 Code graph analysis (1)
erpnext/accounts/doctype/sales_invoice/sales_invoice.py (1)
erpnext/controllers/accounts_controller.py (1)
before_print(607-629)
⏰ 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). (6)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
erpnext/accounts/print_format/sales_invoice_standard/sales_invoice_standard.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
erpnext/accounts/letterhead/company_letterhead.html (1)
80-105: Remove invoice-specific content from letterhead for reusability.The letterhead template currently includes invoice-specific fields at lines 83-86:
<div class="invoice-info"> <span class="invoice-label">{{ _("Invoice:") }}</span> <span>{{ doc.name }}</span> </div>Letterheads should be generic and reusable across different document types (Purchase Orders, Delivery Notes, etc.). Document-specific metadata like the invoice number belongs in the print format, not the letterhead.
Apply this diff to remove the invoice-specific block:
{% set company_details = frappe.db.get_value("Company", doc.company, ["website", "email", "phone_no"], as_dict=True) %} - <div class="invoice-info"> - <span class="invoice-label">{{ _("Invoice:") }}</span> - <span>{{ doc.name }}</span> - </div> {% if company_details.website %} <div class="invoice-info"> <span class="invoice-label">{{ _("Website:") }}</span>This ensures the letterhead can be used across all document types in ERPNext.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
erpnext/accounts/doctype/sales_invoice/sales_invoice.py(2 hunks)erpnext/accounts/letterhead/company_letterhead.html(1 hunks)erpnext/accounts/letterhead/company_letterhead_grey.html(1 hunks)erpnext/accounts/print_format/sales_invoice_standard/sales_invoice_standard.json(1 hunks)erpnext/setup/install.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
erpnext/accounts/doctype/sales_invoice/sales_invoice.py (1)
erpnext/controllers/accounts_controller.py (1)
before_print(607-629)
⏰ 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). (6)
- GitHub Check: Patch Test
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Summary
🔇 Additional comments (4)
erpnext/setup/install.py (1)
285-305: LGTM! Previous issues have been addressed.The
create_letter_headfunction correctly:
- Uses the proper path with "accounts" and "letterhead" subdirectories
- Avoids manual commits (relies on outer transaction)
- Uses consistent letterhead names in both the exists check and document creation
The implementation properly reads HTML templates and creates Letter Head documents during installation.
erpnext/accounts/doctype/sales_invoice/sales_invoice.py (1)
282-334: LGTM! Previous concerns have been addressed.The
before_printmethod now properly:
- Calls
super().before_print(settings)to ensure parent class logic executes- Uses safe dict access with
.get()when extractingaddress_line1- Checks permissions before prompting for missing company details
- Publishes the realtime event only when required fields are missing and the user has appropriate permissions
erpnext/accounts/letterhead/company_letterhead_grey.html (1)
1-125: LGTM! Well-structured letterhead template.The grey-themed letterhead template follows good practices:
- Uses conditional rendering to show sections only when data exists
- Employs safe dictionary access with
.get()to avoid KeyError- Properly fetches company details via
frappe.db.get_value()- Includes appropriate styling with rounded corners and padding
- Displays company logo, name, address, and contact information in a clean layout
The template integrates well with the broader print flow where
before_printgathers company details and thesave_company_master_detailsendpoint can update them.erpnext/accounts/print_format/sales_invoice_standard/sales_invoice_standard.json (1)
1-33: LGTM! Bill From/Bill To sections are correctly implemented.The print format correctly assigns:
- Bill From: Company name and company address (who is issuing the invoice)
- Bill To: Customer name and customer address (who should pay the invoice)
This is the standard and correct convention for invoices. The past review comment suggesting these should be swapped was incorrect.
The print format also includes:
- Proper conditional rendering for optional fields
- Dynamic Item Code column based on naming settings
- Well-structured totals section with discount and tax handling
- Appropriate styling and layout
4 participants
no-docs