fix: show company-details popup only for the targeted print format/le…

[khushi8112]

Issue:

• The company details popup appeared for all print formats and letterheads whenever company details were missing. This caused unnecessary interruptions when users used other formats.

Before

Screen.Recording.2025-11-07.at.1.01.30.PM.mov

After

Screen.Recording.2025-11-07.at.12.58.59.PM.mov

[coderabbitai]

📝 Walkthrough

Walkthrough

A guard condition has been added to the sales_invoice_before_print handler in the print.js file. This guard reads print_format and letterhead values, checks them against defined allowed values, and returns early if neither matches the specified conditions. The existing route-based print logic and dialog flow remain intact but are now only reached if the guard condition passes, effectively filtering which invoices proceed through the print dialog.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Guard logic validation: Confirm the allowed values for print_format and letterhead are correctly specified and represent the intended behavior
• Early return impact: Verify that the early return doesn't skip necessary initialization or side effects required for subsequent operations
• Intent verification: Clarify why these specific conditions warrant filtering and whether all edge cases are properly handled
• Backward compatibility: Assess whether existing invoices with different print_format/letterhead combinations may be unexpectedly blocked

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the fix: restricting the company-details popup to only targeted print formats/letterheads, which matches the core change in the PR.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check	✅ Passed	The PR description directly addresses the changeset by explaining the bug (unwanted popups) and the fix (targeted popup behavior for specific print formats/letterheads).

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

erpnext/public/js/print.js (2)

7-8: Hardcoded configuration reduces maintainability.

The allowed print formats and letterheads are hardcoded, requiring code changes whenever new formats need to be added to the allowlist. This couples configuration with implementation.

Consider moving these to a configurable location (e.g., DocType settings, system settings, or at minimum, a well-documented constant at the top of the file):

+// Configuration: Print formats and letterheads that should trigger the company details dialog
+const COMPANY_DETAILS_PRINT_FORMATS = ["Sales Invoice Standard", "Sales Invoice with Item Image"];
+const COMPANY_DETAILS_LETTERHEADS = ["Company Letterhead", "Company Letterhead - Grey"];
+
 let beforePrintHandled = false;

 frappe.realtime.on("sales_invoice_before_print", (data) => {
 	let print_format = $('input[data-fieldname="print_format"]').val();
 	let letterhead = $('input[data-fieldname="letterhead"]').val();

-	let allowed_print_formats = ["Sales Invoice Standard", "Sales Invoice with Item Image"];
-	let allowed_letterheads = ["Company Letterhead", "Company Letterhead - Grey"];
+	let allowed_print_formats = COMPANY_DETAILS_PRINT_FORMATS;
+	let allowed_letterheads = COMPANY_DETAILS_LETTERHEADS;

Even better, consider making this configurable through a DocType setting so administrators can manage it without code changes.

---

4-12: Additional robustness suggestions.

Consider these improvements to make the guard more robust:

1. Case-insensitive matching: The current includes() check is case-sensitive. If the actual values have different casing, they won't match.

2. Trim whitespace: Input values might have leading/trailing whitespace.

3. Early validation: Check if the elements exist before querying.

 	let print_format = $('input[data-fieldname="print_format"]').val();
 	let letterhead = $('input[data-fieldname="letterhead"]').val();
+	
+	// Normalize values: trim and convert to lowercase for case-insensitive comparison
+	print_format = (print_format || "").trim().toLowerCase();
+	letterhead = (letterhead || "").trim().toLowerCase();

-	let allowed_print_formats = ["Sales Invoice Standard", "Sales Invoice with Item Image"];
-	let allowed_letterheads = ["Company Letterhead", "Company Letterhead - Grey"];
+	// Convert allowed values to lowercase for comparison
+	let allowed_print_formats = ["Sales Invoice Standard", "Sales Invoice with Item Image"].map(f => f.toLowerCase());
+	let allowed_letterheads = ["Company Letterhead", "Company Letterhead - Grey"].map(l => l.toLowerCase());

 	if (!allowed_print_formats.includes(print_format) && !allowed_letterheads.includes(letterhead)) {
 		return;
 	}

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9eabaf0 and 4c8226e.

📒 Files selected for processing (1)

• erpnext/public/js/print.js (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). (1)

• GitHub Check: Summary

🔇 Additional comments (2)

erpnext/public/js/print.js (2)

10-12: Verify the guard condition logic matches requirements.

The condition !allowed_print_formats.includes(print_format) && !allowed_letterheads.includes(letterhead) will show the dialog when either the print format or letterhead matches the allowed lists. Please confirm this is the intended behavior, as the PR description mentions "show company-details popup only for the targeted print format/le[tterhead]."

If the requirement is to show the dialog only when both match (more restrictive), the logic should be:

-	if (!allowed_print_formats.includes(print_format) && !allowed_letterheads.includes(letterhead)) {
+	if (!allowed_print_formats.includes(print_format) || !allowed_letterheads.includes(letterhead)) {
 		return;
 	}

However, if showing the dialog when either matches is correct, consider adding a comment for clarity:

+	// Show dialog if either the print format OR letterhead is in the allowed lists
 	if (!allowed_print_formats.includes(print_format) && !allowed_letterheads.includes(letterhead)) {
 		return;
 	}

---

4-5: Verify DOM selector timing and defensive checks for external framework fields.

The print dialog exposes print_format and letterhead as standard controls in the print preview toolbar. However, these input fields are queried from the Frappe framework (not defined in the erpnext codebase) and may or may not exist in the DOM when the event fires.

The guard condition lacks defensive checks for undefined values. If the selectors fail to find elements, .val() returns undefined, and the condition silently skips showing the dialog. Recommend adding null coalescing:

-	let print_format = $('input[data-fieldname="print_format"]').val();
-	let letterhead = $('input[data-fieldname="letterhead"]').val();
+	let print_format = $('input[data-fieldname="print_format"]').val() || "";
+	let letterhead = $('input[data-fieldname="letterhead"]').val() || "";

Verify through manual testing that these input fields exist and are populated when the sales_invoice_before_print event fires.