Skip to content

Conversation

@khushi8112
Copy link
Member

Please provide enough information so that others can review your pull request:

Explain the details for making this change. What existing problem does the pull request solve?

Screenshots/GIFs

@github-actions github-actions bot added skip-release-notes This PR should not be mentioned in the release notes accounts needs-tests This PR needs automated unit-tests. labels Nov 26, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 26, 2025

📝 Walkthrough

Walkthrough

This PR refactors company details handling during the print workflow. It removes the before_print hook and save_company_master_details function from the SalesInvoice document, migrating this functionality to generalized whitelisted functions in accounts_controller (get_missing_company_details and update_company_master_and_address). The JavaScript print handling is restructured from event-driven to route-driven logic with new helper functions for dialog construction and management, including doctype filtering and a 500ms delayed check for fetching missing details.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • erpnext/public/js/print.js: Significant restructuring of print event flow with new route-based logic, multiple helper functions for dialog lifecycle management, and changes to timing/control flow. Verify the 500ms delay and conditional logic work as intended.
  • Cross-file integration: Ensure the moved Python functions in accounts_controller properly replace the removed SalesInvoice methods, with consistent permission checks and return values.
  • Dialog field building: Review the dynamic field construction and the make_field helper function for correctness across various field types.

Possibly related PRs

  • feat: print format for sales invoice #49508: Directly related PR that modifies the same print-related flow, removing SalesInvoice.before_print and save_company_master_details while adding complementary changes to public/js/print.js.

Suggested labels

needs-tests

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)
Check name Status Explanation Resolution
Title check ⚠️ Warning The title mentions replacing publish_realtime with msgprint, but the actual changes show a much broader refactoring: removing before_print hooks from SalesInvoice, moving logic to AccountsController functions, and restructuring print.js with route-driven logic and dialog helpers. Update the title to reflect the primary refactoring scope: moving company details collection from before_print hooks to dedicated AccountsController functions and restructuring the print flow with route-driven dialog logic.
Description check ⚠️ Warning The description field contains only the repository's PR template and contributor guidelines without any actual explanation of the changes, objectives, or problems being solved. Provide a meaningful description explaining: what problem this refactoring solves, why the architectural changes were necessary, and how the new route-driven approach improves upon the previous event-driven flow.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (2)
erpnext/controllers/accounts_controller.py (2)

4173-4223: LGTM! Consider explicit return for permission failures.

The permission checks and logic are correct. However, when permission checks fail (lines 4190-4196 and 4198-4204), the function returns None implicitly after showing the message. While the JS handles this gracefully (checks r && r.message), an explicit return False would improve clarity.

 		frappe.msgprint(
 			_(
 				"Some required Company details are missing. You don't have permission to update them. Please contact your System Manager."
 			)
 		)
-		return
+		return False

4262-4274: Hardcoded doctype limits extensibility.

The function hardcodes "Sales Invoice" for the update query, but the doctype_list in print.js suggests this could be extended to other doctypes. Consider passing the doctype as a parameter or deriving it from the name.

 @frappe.whitelist()
-def update_company_master_and_address(name, company, details):
+def update_company_master_and_address(doctype, name, company, details):
     ...
     if company_address:
-        company_address_display = frappe.db.get_value("Sales Invoice", name, "company_address_display")
+        company_address_display = frappe.db.get_value(doctype, name, "company_address_display")
         if not company_address_display or details.get("address_line1"):
             from frappe.query_builder import DocType
 
-            SalesInvoice = DocType("Sales Invoice")
+            DocTable = DocType(doctype)
 
             (
-                frappe.qb.update(SalesInvoice)
-                .set(SalesInvoice.company_address, company_address)
-                .set(SalesInvoice.company_address_display, get_address_display(company_address))
-                .where(SalesInvoice.name == name)
+                frappe.qb.update(DocTable)
+                .set(DocTable.company_address, company_address)
+                .set(DocTable.company_address_display, get_address_display(company_address))
+                .where(DocTable.name == name)
             ).run()
📜 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 a8d3e9b and fc182a8.

📒 Files selected for processing (3)
  • erpnext/accounts/doctype/sales_invoice/sales_invoice.py (0 hunks)
  • erpnext/controllers/accounts_controller.py (2 hunks)
  • erpnext/public/js/print.js (1 hunks)
💤 Files with no reviewable changes (1)
  • erpnext/accounts/doctype/sales_invoice/sales_invoice.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: barredterra
Repo: frappe/erpnext PR: 50159
File: erpnext/public/js/utils/sales_common.js:122-125
Timestamp: 2025-11-16T16:00:08.157Z
Learning: In ERPNext sales transactions (erpnext/public/js/utils/sales_common.js and erpnext/controllers/selling_controller.py), the company_contact_person field has different update behaviors: Frontend set_default_company_contact_person() should update the contact when the company field changes (intentional override on user action), while backend set_company_contact_person() in set_missing_values() should only fill empty fields (no override during save operations).
📚 Learning: 2025-11-16T16:00:08.157Z
Learnt from: barredterra
Repo: frappe/erpnext PR: 50159
File: erpnext/public/js/utils/sales_common.js:122-125
Timestamp: 2025-11-16T16:00:08.157Z
Learning: In ERPNext sales transactions (erpnext/public/js/utils/sales_common.js and erpnext/controllers/selling_controller.py), the company_contact_person field has different update behaviors: Frontend set_default_company_contact_person() should update the contact when the company field changes (intentional override on user action), while backend set_company_contact_person() in set_missing_values() should only fill empty fields (no override during save operations).

Applied to files:

  • erpnext/controllers/accounts_controller.py
📚 Learning: 2025-11-25T18:35:10.132Z
Learnt from: chrnas
Repo: frappe/erpnext PR: 50654
File: erpnext/templates/print_formats/includes/serial_and_batch_no_barcodes.html:21-25
Timestamp: 2025-11-25T18:35:10.132Z
Learning: In ERPNext, print views are low-traffic parts of the system, so performance optimizations that add complexity (like pre-computing data to avoid N+1 queries in print templates) may not be necessary unless documents regularly contain large numbers of items.

Applied to files:

  • erpnext/public/js/print.js
🪛 GitHub Actions: Linters
erpnext/public/js/print.js

[error] 130-130: frappe-semgrep-rules.rules.frappe-missing-translate-function-js All user facing text must be wrapped in translate function. Please refer to translation documentation. https://frappeframework.com/docs/user/en/guides/basics/translations

⏰ 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
🔇 Additional comments (3)
erpnext/controllers/accounts_controller.py (1)

10-10: LGTM!

The import of get_address_display is correctly placed and used in the new update_company_master_and_address function at line 4272.

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

21-26: LGTM!

The function safely handles missing DOM elements by returning false, which is acceptable behavior for the print context.


28-38: Update API call if update_company_master_and_address signature changes.

If the suggested refactor to add doctype parameter to update_company_master_and_address is accepted, update save_company_details to pass the doctype.

 function save_company_details(dialog, data, values) {
 	frappe.call({
 		method: "erpnext.controllers.accounts_controller.update_company_master_and_address",
 		args: {
+			doctype: data.doctype,  // Add doctype to data in fetch_company_details
 			name: data.name,
 			company: data.company,
 			details: values,
 		},

And ensure get_missing_company_details returns the doctype in its response.

Comment on lines +4239 to +4240
if company_fields_to_update:
frappe.db.set_value("Company", company, company_fields_to_update)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Verify write permission before updating Company.

The function is whitelisted but updates Company fields directly via frappe.db.set_value, which bypasses document-level permission checks. While get_missing_company_details checks permissions before showing the dialog, a malicious caller could invoke this endpoint directly.

+	if company_fields_to_update:
+		if not frappe.has_permission("Company", "write", company):
+			frappe.throw(_("You don't have permission to update Company details."))
 		frappe.db.set_value("Company", company, company_fields_to_update)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if company_fields_to_update:
frappe.db.set_value("Company", company, company_fields_to_update)
if company_fields_to_update:
if not frappe.has_permission("Company", "write", company):
frappe.throw(_("You don't have permission to update Company details."))
frappe.db.set_value("Company", company, company_fields_to_update)
🤖 Prompt for AI Agents
In erpnext/controllers/accounts_controller.py around lines 4239-4240, the code
calls frappe.db.set_value to update Company fields which bypasses document-level
permission checks; load the Company document via frappe.get_doc, apply the
company_fields_to_update to the document, and save it using the document API
(doc.save) without ignoring permissions or explicitly call
doc.check_permission("write") / raise a PermissionError if the current user
lacks write access so that updates enforce Frappe's permission system instead of
directly using frappe.db.set_value.

Comment on lines +1 to +5
const doctype_list = ["Sales Invoice"];
const allowed_print_formats = ["Sales Invoice Standard", "Sales Invoice with Item Image"];
const allowed_letterheads = ["Company Letterhead", "Company Letterhead - Grey"];

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

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

Script executed:

#!/bin/bash
# First, find and examine the print.js file
find . -name "print.js" -path "*/public/js/*" | head -20

Repository: frappe/erpnext


<shell_result>
Repository: frappe/erpnext
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository frappe/erpnext: Clone operation failed
</shell_result>


frappe.ready callback documentation event lifecycle


<web_search_result>
Short summary — what frappe.ready is and when its callbacks run:

  • frappe.ready(fn) registers fn by pushing it onto frappe.ready_events; it does not run immediately. (shodan.io)
  • Frappe triggers those registered callbacks during the website/page initialization sequence (after document ready → page-change → apply_permissions). The framework calls trigger_ready() which executes the functions in frappe.ready_events in order. (about.lovia.id)
  • For Web Forms there are extra lifecycle hooks (web_form events like after_load) that are fired asynchronously after trigger_ready; many users put web_form handlers inside frappe.ready so they get registered before the form-specific events fire. (about.lovia.id)

Practical notes / pitfalls:

  • If your script runs before frappe.ready is defined, registration will fail; frappe.ready is usually defined very early in page HTML and then run later by trigger_ready. (shodan.io)
  • spa/desk pages and some custom HTML contexts can load earlier or later — some custom-code reports show frappe.ready callbacks not firing because of when/where the JS file is loaded; in those cases use document-ready, window.onload, or observe DOM mutations as a fallback. (discuss.frappe.io)
  • Web-form-specific handlers (frappe.web_form.after_load, on, validate, etc.) sometimes need to be set outside or carefully inside frappe.ready depending on Frappe version and how the webform JS is loaded; if a handler doesn’t run, verify the file is actually loaded and register the handler before the webform triggers its events. (discuss.frappe.io)

Example pattern:

  • Register generic init code:
    frappe.ready(function() {
    // register web_form events or attach DOM handlers
    });
    (about.lovia.id)

If you want, I can fetch the exact trigger_ready implementation in the Frappe GitHub repo for your Frappe version and show the call order (document ready → page-change → apply_permissions → trigger_ready). Which Frappe/ERPNext version are you using?

Citations:


frappe.get_route() function when available initialization


<web_search_result>
Short answer: frappe.get_route() is a client-side function available once the Frappe JS runtime is loaded in the browser (i.e. after window.frappe is defined and the Desk/web page JS has initialized). Put calls to it in places that run after Frappe initializes — e.g. form/list view scripts, on_page_load/onload callbacks, or inside frappe.ready / page-change handlers — or guard by checking window.frappe before calling. [1][2][3]

Concrete pointers:

  • Docs: frappe.get_route() returns the current route (available in client JS on desk pages). [1]
  • Common usage: call from list/form scripts or after routing (frappe.set_route / route-change); many examples use it inside onload/page-load handlers. [2][4]
  • If your code runs too early, wrap it (or wait) using frappe.ready or the page-change/event hooks, or check for window.frappe before using frappe.get_route(). Forum threads showing timing issues and workarounds: use frappe.ready / page-change / on_page_load, or defer until frappe is defined. [3][5]

References:
[1] Frappe Common Utilities API — frappe.get_route (docs).
[2] Frappe routing / wiki examples (frappe.get_route usage).
[3] Forum threads about frappe.ready / timing and when JS runs.
[4] Developer cheatsheet / routing notes.
[5] Examples of placing code in on_page_load / listview/form scripts.
</web_search_result>


Immediate execution at script load may cause frappe.get_route() to return invalid or unavailable data.

handle_route_event() is called directly when the script loads, before Frappe's initialization sequence completes. According to Frappe's lifecycle, frappe.get_route() and DOM elements (print format/letterhead inputs) may not be ready at that point. Callbacks registered via frappe.ready() execute after the full initialization sequence (document ready → page-change → apply_permissions → trigger_ready).

Wrap the initialization in frappe.ready():

 const doctype_list = ["Sales Invoice"];
 const allowed_print_formats = ["Sales Invoice Standard", "Sales Invoice with Item Image"];
 const allowed_letterheads = ["Company Letterhead", "Company Letterhead - Grey"];
 
-handle_route_event();
+frappe.ready(() => {
+	handle_route_event();
+});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const doctype_list = ["Sales Invoice"];
const allowed_print_formats = ["Sales Invoice Standard", "Sales Invoice with Item Image"];
const allowed_letterheads = ["Company Letterhead", "Company Letterhead - Grey"];
frappe.realtime.on("sales_invoice_before_print", (data) => {
let print_format = $('input[data-fieldname="print_format"]').val();
let letterhead = $('input[data-fieldname="letterhead"]').val();
handle_route_event();
const doctype_list = ["Sales Invoice"];
const allowed_print_formats = ["Sales Invoice Standard", "Sales Invoice with Item Image"];
const allowed_letterheads = ["Company Letterhead", "Company Letterhead - Grey"];
frappe.ready(() => {
handle_route_event();
});
🤖 Prompt for AI Agents
In erpnext/public/js/print.js around lines 1 to 5, the script calls
handle_route_event() immediately at load which can run before Frappe
initialization and cause frappe.get_route() or print-format/letterhead DOM
elements to be unavailable; change this to call handle_route_event() from within
a frappe.ready() callback (i.e., register the initialization inside
frappe.ready(...) so it runs after document
ready/page-change/apply_permissions/trigger_ready), ensuring any route checks or
DOM queries happen only after Frappe is fully initialized.

Comment on lines +7 to +19
function handle_route_event() {
const route = frappe.get_route();
const current_doctype = route[1];
const current_docname = route[2];

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

const route = frappe.get_route();
setTimeout(() => {
if (should_fetch_company_details()) {
fetch_company_details(current_doctype, current_docname);
}
}, 500);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Add null check for current_docname.

If the route doesn't include a document name (e.g., navigating to a list view), current_docname will be undefined, and the API call will fail.

 function handle_route_event() {
 	const route = frappe.get_route();
 	const current_doctype = route[1];
 	const current_docname = route[2];
 
-	if (!doctype_list.includes(current_doctype)) return;
+	if (!doctype_list.includes(current_doctype) || !current_docname) return;
 
 	setTimeout(() => {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function handle_route_event() {
const route = frappe.get_route();
const current_doctype = route[1];
const current_docname = route[2];
if (!allowed_print_formats.includes(print_format) && !allowed_letterheads.includes(letterhead)) {
return;
}
if (!doctype_list.includes(current_doctype)) return;
const route = frappe.get_route();
setTimeout(() => {
if (should_fetch_company_details()) {
fetch_company_details(current_doctype, current_docname);
}
}, 500);
}
function handle_route_event() {
const route = frappe.get_route();
const current_doctype = route[1];
const current_docname = route[2];
if (!doctype_list.includes(current_doctype) || !current_docname) return;
setTimeout(() => {
if (should_fetch_company_details()) {
fetch_company_details(current_doctype, current_docname);
}
}, 500);
}
🤖 Prompt for AI Agents
In erpnext/public/js/print.js around lines 7 to 19, the handler uses
current_docname from the route without checking for null/undefined which causes
the API call to fail for list views; add a guard to ensure current_docname is
present before scheduling or calling fetch_company_details (e.g., return early
if !current_docname or check route length), so fetch_company_details is only
invoked for valid document pages.

Comment on lines +40 to +51
function open_company_details_dialog(data) {
const dialog = new frappe.ui.Dialog({
title: "Enter Company Details",
fields: build_dialog_fields(data),
primary_action_label: "Save",
primary_action(values) {
save_company_details(dialog, data, values);
},
});

dialog.show();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Missing translation for dialog title.

The dialog title should be wrapped with __() for translation support.

 function open_company_details_dialog(data) {
 	const dialog = new frappe.ui.Dialog({
-		title: "Enter Company Details",
+		title: __("Enter Company Details"),
 		fields: build_dialog_fields(data),
-		primary_action_label: "Save",
+		primary_action_label: __("Save"),
🤖 Prompt for AI Agents
In erpnext/public/js/print.js around lines 40 to 51, the dialog title is a plain
string ("Enter Company Details"); wrap that title in the translation function
__() so it becomes __("Enter Company Details") to enable i18n support for the
dialog header and preserve existing behavior.

Comment on lines +53 to +108
function build_dialog_fields(data) {
return [
make_field("Company Logo", "company_logo", "Attach Image", data.company_logo),
make_field("Website", "website", "Data", data.website),
make_field("Phone No", "phone_no", "Data", data.phone_no),
{
label: "Email",
fieldname: "email",
fieldtype: "Data",
options: "Email",
reqd: data.email ? 0 : 1,
hidden: data.email ? 1 : 0,
},
{ fieldtype: "Section Break" },

make_field("Address Title", "address_title", "Data", data.address_line),
{
label: "Address Type",
fieldname: "address_type",
fieldtype: "Select",
options: ["Billing", "Shipping"],
default: "Billing",
reqd: data.address_line ? 0 : 1,
hidden: data.address_line ? 1 : 0,
},
make_field("Address Line 1", "address_line1", "Data", data.address_line),
make_field("Address Line 2", "address_line2", "Data", data.address_line, false),
make_field("City", "city", "Data", data.address_line),
make_field("State", "state", "Data", data.address_line, false),
{
label: "Country",
fieldname: "country",
fieldtype: "Link",
options: "Country",
reqd: data.address_line ? 0 : 1,
hidden: data.address_line ? 1 : 0,
},
make_field("Postal Code", "pincode", "Data", data.address_line, false),

{
label: "Select Company Address",
fieldname: "company_address",
fieldtype: "Link",
options: "Address",
get_query: () => ({
query: "frappe.contacts.doctype.address.address.address_query",
filters: {
link_doctype: "Company",
link_name: data.company,
},
],
primary_action_label: "Save",
primary_action(values) {
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(__("Updating details."));
setTimeout(() => {
window.location.reload();
}, 1000);
},
});
},
});
companyDetailsDialog.show();
}
});
frappe.router.on("change", () => {
const route = frappe.get_route();
if (route[0] !== "print" || route[1] !== "Sales Invoice") {
beforePrintHandled = false;
}
});
}),
reqd: data.address_line && !data.company_address ? 1 : 0,
hidden: data.address_line && !data.company_address ? 0 : 1,
},
];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Missing translations for field labels and options.

All user-facing strings should be wrapped with __() for i18n support.

 function build_dialog_fields(data) {
 	return [
-		make_field("Company Logo", "company_logo", "Attach Image", data.company_logo),
-		make_field("Website", "website", "Data", data.website),
-		make_field("Phone No", "phone_no", "Data", data.phone_no),
+		make_field(__("Company Logo"), "company_logo", "Attach Image", data.company_logo),
+		make_field(__("Website"), "website", "Data", data.website),
+		make_field(__("Phone No"), "phone_no", "Data", data.phone_no),
 		{
-			label: "Email",
+			label: __("Email"),
 			...
 		},
 		...
 		{
-			label: "Address Type",
+			label: __("Address Type"),
 			fieldname: "address_type",
 			fieldtype: "Select",
-			options: ["Billing", "Shipping"],
+			options: [__("Billing"), __("Shipping")],
 			...
 		},

Apply similar changes to all other labels and the make_field calls.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In erpnext/public/js/print.js around lines 53 to 108, user-facing labels and
option strings are not wrapped for translation; update every visible string
(first argument to make_field calls, object label values like "Email",
"Country", "Select Company Address", the Select options array
["Billing","Shipping"], the default "Billing", Link options like "Country" and
"Address", and any other literal UI text in this block) to use the translation
helper __('...') and ensure make_field calls pass translated labels; keep all
logic (reqd/hidden/default) unchanged and import/use the existing __() function
as used elsewhere in the codebase.

Comment on lines +120 to +137
function save_company_details(dialog, data, values) {
frappe.call({
method: "erpnext.controllers.accounts_controller.update_company_master_and_address",
args: {
name: data.name,
company: data.company,
details: values,
},
callback() {
dialog.hide();
frappe.msgprint("Updating details.");

setTimeout(() => {
location.reload();
}, 1000);
},
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix translation and add error handling.

This addresses the pipeline failure and improves robustness.

 function save_company_details(dialog, data, values) {
 	frappe.call({
 		method: "erpnext.controllers.accounts_controller.update_company_master_and_address",
 		args: {
 			name: data.name,
 			company: data.company,
 			details: values,
 		},
-		callback() {
+		callback(r) {
+			if (r.exc) return;
 			dialog.hide();
-			frappe.msgprint("Updating details.");
-
-			setTimeout(() => {
-				location.reload();
-			}, 1000);
+			frappe.msgprint({
+				title: __("Success"),
+				message: __("Company details updated. Reloading..."),
+				indicator: "green",
+			});
+			setTimeout(() => location.reload(), 500);
 		},
+		error() {
+			frappe.msgprint({
+				title: __("Error"),
+				message: __("Failed to update company details."),
+				indicator: "red",
+			});
+		},
 	});
 }
🧰 Tools
🪛 GitHub Actions: Linters

[error] 130-130: frappe-semgrep-rules.rules.frappe-missing-translate-function-js All user facing text must be wrapped in translate function. Please refer to translation documentation. https://frappeframework.com/docs/user/en/guides/basics/translations

🤖 Prompt for AI Agents
In erpnext/public/js/print.js around lines 120 to 137, update the frappe.call
callback to handle errors and make the user message translatable: change the
callback to accept the response (e.g., function(r)) and only hide the dialog,
show the success message, and trigger reload when r.exc is falsy; if r.exc (or
another error object) is present, show a translated error message and do not
reload; wrap displayed strings with __(...) for translation and surface the
error details in the msgprint for debugging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accounts needs-tests This PR needs automated unit-tests. skip-release-notes This PR should not be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant