Skip to content

fix: display sales invoice fields in POS additional fields when invoi…#53993

Open
sudarsan2001 wants to merge 1 commit intofrappe:developfrom
aerele:pos-settings-sales-invoice
Open

fix: display sales invoice fields in POS additional fields when invoi…#53993
sudarsan2001 wants to merge 1 commit intofrappe:developfrom
aerele:pos-settings-sales-invoice

Conversation

@sudarsan2001
Copy link
Copy Markdown
Contributor

Ref: #53794

Description: In POS Settings, the POS Additional Fields child table shows fields only from the POS Invoice, even when the invoice type is selected as Sales Invoice instead of Sales Invoice fields

Before:
image

After:
image

backport needed in v16

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 2, 2026

📝 Walkthrough

Walkthrough

This change modifies the POS Settings form handler to support dynamic invoice type selection. A new invoice_type change event handler is added that triggers get_invoice_fields when the invoice type value changes. The get_invoice_fields handler is updated to load fields from the doctype specified by invoice_type, defaulting to "POS Invoice" if not set. The POS Field fieldname handler is similarly updated to resolve field metadata (label, required status, options, field type, default value) from the dynamically selected doctype instead of always using "POS Invoice".

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: displaying Sales Invoice fields in POS additional fields when the invoice type is Sales Invoice.
Description check ✅ Passed The description is directly related to the changeset, referencing issue #53794, explaining the problem (fields showing only from POS Invoice regardless of invoice type selection), and providing before/after screenshots demonstrating the fix.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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
Copy Markdown
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: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@erpnext/accounts/doctype/pos_settings/pos_settings.js`:
- Around line 107-112: Guard against df being undefined after mapping the
DocType fields: after creating df (from frappe.get_doc("DocType",
frm.doc.invoice_type ? frm.doc.invoice_type : "POS Invoice").fields) add a
null/undefined check before accessing df.label, df.reqd, df.bold, df.in_filter.
If df is falsy, skip the property assignments (or provide safe defaults) so the
later code that reads df.label/df.reqd/df.bold/df.in_filter does not throw;
adjust the surrounding logic that uses df to handle the missing-field case
gracefully.
- Around line 44-50: When invoice_type changes, clear or reconcile the
invoice_fields child table to avoid stale fieldnames: inside the invoice_type
handler (and at the start of get_invoice_fields) call
frm.clear_table('invoice_fields') (or programmatically filter existing
frm.doc.invoice_fields to only keep rows whose fieldname exists in the new
DocType fields) then call frm.refresh_field('invoice_fields') before
repopulating; use the existing get_invoice_fields logic to repopulate from
frappe.get_doc("DocType", invoice_type).fields so saved child rows always match
the selected invoice_type.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: f4cb5b3d-18b2-4042-98e8-164abda8651d

📥 Commits

Reviewing files that changed from the base of the PR and between efd716e and d2dd411.

📒 Files selected for processing (1)
  • erpnext/accounts/doctype/pos_settings/pos_settings.js

Comment on lines +44 to +50
invoice_type: function (frm, doctype, name) {
frm.trigger("get_invoice_fields");
},
get_invoice_fields: function (frm) {
frappe.model.with_doctype("POS Invoice", () => {
var fields = $.map(frappe.get_doc("DocType", "POS Invoice").fields, function (d) {
var invoice_type = frm.doc.invoice_type ? frm.doc.invoice_type : "POS Invoice";
frappe.model.with_doctype(invoice_type, () => {
var fields = $.map(frappe.get_doc("DocType", invoice_type).fields, function (d) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Clear/reconcile invoice_fields when invoice_type changes.

Line 45 refreshes available options, but existing child rows are left intact. This allows stale fieldnames from the previous doctype to remain and be saved, and backend validation currently only checks duplicates (not doctype membership).

Suggested fix
 invoice_type: function (frm, doctype, name) {
+	const invoice_type = frm.doc.invoice_type || "POS Invoice";
+	frappe.model.with_doctype(invoice_type, () => {
+		const allowed = new Set(
+			(frappe.get_doc("DocType", invoice_type).fields || []).map((d) => d.fieldname)
+		);
+		frm.doc.invoice_fields = (frm.doc.invoice_fields || []).filter((row) =>
+			allowed.has(row.fieldname)
+		);
+		frm.refresh_field("invoice_fields");
+		frm.trigger("get_invoice_fields");
+	});
-	frm.trigger("get_invoice_fields");
 },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@erpnext/accounts/doctype/pos_settings/pos_settings.js` around lines 44 - 50,
When invoice_type changes, clear or reconcile the invoice_fields child table to
avoid stale fieldnames: inside the invoice_type handler (and at the start of
get_invoice_fields) call frm.clear_table('invoice_fields') (or programmatically
filter existing frm.doc.invoice_fields to only keep rows whose fieldname exists
in the new DocType fields) then call frm.refresh_field('invoice_fields') before
repopulating; use the existing get_invoice_fields logic to repopulate from
frappe.get_doc("DocType", invoice_type).fields so saved child rows always match
the selected invoice_type.

Comment on lines +107 to +112
var df = $.map(
frappe.get_doc("DocType", frm.doc.invoice_type ? frm.doc.invoice_type : "POS Invoice").fields,
function (d) {
return doc.fieldname == d.fieldname ? d : null;
}
)[0];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Guard against missing field metadata before property access.

If doc.fieldname is stale/invalid for the current invoice_type, df becomes undefined; then Line 114-118 will throw on df.label, df.reqd, etc.

Suggested fix
 		var df = $.map(
 			frappe.get_doc("DocType", frm.doc.invoice_type ? frm.doc.invoice_type : "POS Invoice").fields,
 			function (d) {
 				return doc.fieldname == d.fieldname ? d : null;
 			}
 		)[0];
+		if (!df) {
+			return;
+		}
 
 		doc.label = df.label;
 		doc.reqd = df.reqd;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@erpnext/accounts/doctype/pos_settings/pos_settings.js` around lines 107 -
112, Guard against df being undefined after mapping the DocType fields: after
creating df (from frappe.get_doc("DocType", frm.doc.invoice_type ?
frm.doc.invoice_type : "POS Invoice").fields) add a null/undefined check before
accessing df.label, df.reqd, df.bold, df.in_filter. If df is falsy, skip the
property assignments (or provide safe defaults) so the later code that reads
df.label/df.reqd/df.bold/df.in_filter does not throw; adjust the surrounding
logic that uses df to handle the missing-field case gracefully.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant