[Irfan778899]

• Add Subscription Details section with Subscription Type, Is New Subscription, Subscription Remarks, and Subscription Item child table to Quotation, Sales Order, and Sales Invoice
• Add dynamic Old Invoice / Old Proforma Invoice link based on Export checkbox
• Enforce mandatory Subscription Type and conditional Old Invoice requirement
• Configure read-only controls for Old Invoice and Export on Sales Order and Sales Invoice
• Update renewal workflow to mark old document renewed on export Sales Order or domestic Sales Invoice submission
• Add client scripts for item filtering and amount syncing

[mujeerhashmi]

1. Critical Bugs & Correctness Issues

A. Broken string interpolation in quotation.js and sales_order.js

In spollex/public/js/quotation.js (line 66) and spollex/public/js/sales_order.js (line 66), the placeholder {0} is missing its curly braces:

// Current (quotation.js & sales_order.js)
frappe.msgprint(__("Item 0 is not present in the items table.", [row.item_code]));

// Correct (as in sales_invoice.js)
frappe.msgprint(__("Item {0} is not present in the items table.", [row.item_code]));

B. Amount discrepancy between item_code selection and before_save when duplicate items exist

In all three client scripts (quotation.js, sales_order.js, sales_invoice.js):

• On Subscription Item item_code selection, .find() picks only the first matching row's amount from doc.items.
• On before_save, item_amount_map sums the amounts of all rows in doc.items with the same item_code and overwrites row.amount.
• Impact: The user sees one amount upon selecting the item, which silently changes on save if the item appears on multiple rows in items. Additionally, because before_save unconditionally overwrites row.amount with item_amount_map[row.item_code], any manual adjustment the user makes to Subscription Item.amount (which is an editable field) is silently wiped out on save.

C. before_save runs only on the client — server-side validate does not sync or validate Subscription Item

All Subscription Item validation (checking that item_code exists in doc.items, syncing amount, or validating from_date <= to_date) is implemented solely in client-side JS:

• Documents created or updated via make_sales_order / make_sales_invoice server mappings, background jobs, Data Import, or REST API will bypass before_save and item_code filter checks.
• Neither from_date nor to_date is validated (e.g., from_date > to_date is allowed).
• Move the Subscription Item amount calculation and item membership check into the Python validate_subscription_details hook.

D. Cancelling or Amending a Document leaves custom_renewed and custom_old_invoice in an inconsistent state

1. no_copy is 0 on custom_renewed and custom_old_invoice (spollex/fixtures/custom_field.json):
   • If a renewed Sales Order or Sales Invoice is duplicated or amended, custom_renewed = 1 will be copied onto the new draft document because no_copy is 0. Set "no_copy": 1 on Sales Order-custom_renewed and Sales Invoice-custom_renewed.
2. Multiple documents pointing to the same custom_old_invoice:
   • Nothing prevents two different Quotations/Orders/Invoices from referencing the same custom_old_invoice. If a second draft is created and cancelled, on_cancel unconditionally sets custom_renewed = 0 on the old invoice even if another submitted invoice already renewed it.
   • Furthermore, the link field custom_old_invoice has no set_query filter to restrict selection to submitted (docstatus = 1), Same Customer, and unrenewed (custom_renewed = 0) documents.

E. Dirty form on load when viewing existing / submitted documents

In update_old_invoice_link(frm) (called on onload and refresh):

if (frm.doc.custom_old_doctype !== target_dt) {
    frm.set_value("custom_old_doctype", target_dt);
}

If an existing or submitted (docstatus == 1) document recorded prior to this PR (where custom_old_doctype is null or empty in DB) is opened in Desk, frm.set_value will execute during onload/refresh and immediately mark the form Not Saved / Dirty (or fail on submitted docs where allow_on_submit is 0).

• Fix: Guard frm.set_value with if (!frm.is_new() && frm.doc.docstatus !== 0) return; or only set custom_old_doctype when the document is editable (frm.doc.docstatus === 0).

---

2. Schema & Migration Risk

A. Adding reqd: 1 (custom_subscription_type) to core transaction DocTypes

custom_subscription_type is configured with "reqd": 1 and "default": null across Quotation, Sales Order, and Sales Invoice.

• Impact: Every existing draft Quotation, Sales Order, or Sales Invoice, as well as any automated creation path (e.g., e-commerce, API integrations, Auto Repeat, POS, or standard ERPNext tests/mappings from Opportunity/Delivery Note) that does not supply custom_subscription_type, will now fail with MandatoryError.
• Recommendation: Either set "default": "None" in custom_field.json so non-subscription flows work out of the box without breaking background/API creation, or make the field conditionally mandatory.

B. read_only: 1 + mandatory_depends_on deadlock on Sales Order and Sales Invoice

In custom_field.json, for both Sales Order and Sales Invoice:

• custom_old_invoice has "read_only": 1 AND "mandatory_depends_on": "eval:in_list(['AMC', 'License'], doc.custom_subscription_type) && !doc.custom_is_new_subscription".
• custom_export also has "read_only": 1.
• Impact: If a user creates a Sales Order or Sales Invoice directly (not mapped from a Quotation) and selects Subscription Type = AMC with Is New Subscription = 0, custom_old_invoice becomes mandatory AND read-only at the same time. The user cannot type into custom_old_invoice and cannot save the document!
• Fix: Make read_only_depends_on conditional on whether the row was mapped from an upstream document, or allow editing when custom_old_invoice is empty.

---

3. Code Duplication & Maintainability

• Python (spollex/doctype_events/): validate_subscription_details is copy-pasted identically across quotation.py, sales_order.py, and sales_invoice.py, and update_renewal_invoice_status is nearly identical between sales_order.py and sales_invoice.py. Consolidate this into a single shared module (e.g., spollex/doctype_events/subscription.py) and reference it from hooks.py.
• JavaScript (spollex/public/js/): All 70 lines of quotation.js, sales_order.js, and the additions in sales_invoice.js are 100% identical. Extract a shared helper or controller to avoid maintaining three diverging copies (which already caused the "Item 0" bug in two of the three files).
• Avoid setTimeout(..., 100) in Subscription Item.item_code: Using an arbitrary 100ms timer to set amount after item_code selection is brittle. If there is no standard ERPNext fetch overriding Subscription Item.amount, frappe.model.set_value(cdt, cdn, "amount", ...) can be called directly.

---

Summary Checklist of Recommended Changes

Priority	File(s)	Action
High (Blocker)	custom_field.json	Fix deadlock where custom_old_invoice is read_only: 1 while also having mandatory_depends_on on Sales Order and Sales Invoice (breaks standalone SO/SI creation).
High (Blocker)	custom_field.json	Add "default": "None" to custom_subscription_type (or relax reqd: 1) so programmatic/mapped SO & SI creation doesn't crash with MandatoryError.
High	quotation.js, sales_order.js	Fix "Item 0 is not present..." → "Item {0} is not present...".
High	custom_field.json	Set "no_copy": 1 on Sales Order-custom_renewed and Sales Invoice-custom_renewed so duplicating/amending doesn't copy the renewed flag.
Medium	quotation.js, sales_order.js, sales_invoice.js	Add set_query on custom_old_invoice to filter by docstatus: 1, customer: frm.doc.customer, and custom_renewed: 0.
Medium	quotation.js, sales_order.js, sales_invoice.js	Prevent update_old_invoice_link from calling frm.set_value when frm.doc.docstatus !== 0 (prevents marking submitted docs as dirty on load).
Medium	quotation.py, sales_order.py, sales_invoice.py	Move Subscription Item amount syncing & date/item validation (from_date <= to_date) to Python validate hook, and deduplicate across the 3 files.

[Irfan778899]

The PR is updated with these changes

[mujeerhashmi]

What Was Fixed Well

• Shared Python Module (spollex/doctype_events/subscription.py): Validation, Subscription Item amount syncing, and date range checks (from_date > to_date) are now consolidated and enforced server-side.
• Default on custom_subscription_type: Added "default": "None" across Quotation, Sales Order, and Sales Invoice.
• no_copy: 1 on custom_renewed: Set on both Sales Order-custom_renewed and Sales Invoice-custom_renewed.
• String interpolation & setTimeout removal: Fixed "Item {0}..." in quotation.js and sales_order.js, and removed the 100ms setTimeout.
• Submitted-doc dirty guard: update_old_invoice_link now checks frm.doc.docstatus === 0 before calling frm.set_value.

---

Remaining Issues Found (Including 2 Regressions Introduced by the Fixes)

1. Critical Regression: update_renewal_invoice_status no longer checks doc.get("custom_export")

In the previous version:

• Sales Order only marked the old invoice renewed if doc.get("custom_export"):
• Sales Invoice only marked the old invoice renewed if not doc.get("custom_export"):

In the refactored spollex/doctype_events/subscription.py (lines 51–58):

def update_renewal_invoice_status(doc, method=None, is_export=False):
	old_invoice = doc.get("custom_old_invoice")
	if not old_invoice:
		return

	is_renewed = 1 if method == "on_submit" else 0
	target_dt = doc.get("custom_old_doctype") or ("Sales Order" if is_export else "Sales Invoice")
	if frappe.db.exists(target_dt, old_invoice):
		frappe.db.set_value(target_dt, old_invoice, "custom_renewed", is_renewed)

Because sales_order.py passes is_export=True and sales_invoice.py passes is_export=False, without checking bool(doc.get("custom_export")) == is_export:

1. Submitting a domestic Sales Order (custom_export = 0) will now prematurely mark the old Sales Invoice as renewed (custom_renewed = 1) before the Sales Invoice is even submitted!
2. If that domestic Sales Order is later billed to a Sales Invoice and the Sales Invoice is cancelled, or vice versa, both Sales Order and Sales Invoice will fight over custom_renewed.
3. Submitting an export Sales Invoice (custom_export = 1) will also toggle custom_renewed on the Sales Order.

Fix in spollex/doctype_events/subscription.py:

def update_renewal_invoice_status(doc, method=None, is_export=False):
	old_invoice = doc.get("custom_old_invoice")
	if not old_invoice or bool(doc.get("custom_export")) != is_export:
		return

(Or even simpler: point hooks.py directly to spollex.doctype_events.subscription.update_renewal_invoice_status, check (doc.doctype == "Sales Order" and doc.get("custom_export")) or (doc.doctype == "Sales Invoice" and not doc.get("custom_export")), and delete the wrapper functions in sales_order.py and sales_invoice.py altogether.)

---

2. Bug in quotation.js: doc.customer does not exist on Quotation

In spollex/public/js/quotation.js (lines 46–55):

    frm.set_query("custom_old_invoice", function(doc) {
        let target_dt = doc.custom_export ? "Sales Order" : "Sales Invoice";
        return {
            filters: {
                customer: doc.customer,
                docstatus: 1,
                custom_renewed: 0
            }
        };
    });

Standard ERPNext Quotation does not have a customer field — it uses party_name (when quotation_to === "Customer") and customer_name.

• Impact: On Quotation, doc.customer is undefined, so the link filter either fails to filter by customer at all or passes customer: undefined.
• Fix in quotation.js: Use customer: doc.party_name (when doc.quotation_to === "Customer").

---

3. UX Trap: "read_only_depends_on": "eval:doc.custom_old_invoice" locks the field immediately on selection

In spollex/fixtures/custom_field.json, on Sales Order and Sales Invoice, both custom_old_invoice and custom_export have:

"read_only_depends_on": "eval:doc.custom_old_invoice"

• Impact: When creating a standalone Sales Order or Sales Invoice, the moment the user selects a value in custom_old_invoice (or even types and blurs), doc.custom_old_invoice becomes truthy and immediately locks both custom_old_invoice and custom_export into read-only mode before the document is even saved. If the user accidentally picked the wrong invoice, they can never change or clear it!
• Fix: If the intent is that custom_old_invoice and custom_export should only be read-only when mapped from an upstream Quotation / Sales Order, check the upstream reference in read_only_depends_on:
  • On Sales Order: "eval:doc.items && doc.items.some(r => r.prevdoc_docname)"
  • On Sales Invoice: "eval:doc.items && doc.items.some(r => r.sales_order || r.delivery_note)"

---

4. Minor: Date comparison in _validate_subscription_item_dates can fail on mixed types (str vs datetime.date)

In spollex/doctype_events/subscription.py (line 41):

if row.from_date and row.to_date and row.from_date > row.to_date:

When a document is saved via the UI, row.from_date and row.to_date are ISO strings ("YYYY-MM-DD"), whereas in programmatic creation or after certain controller hooks they may be datetime.date objects. Comparing str > datetime.date raises TypeError: '>' not supported between instances of 'str' and 'datetime.date'.

• Fix: Use getdate(row.from_date) > getdate(row.to_date) (importing getdate from frappe.utils).

[Irfan778899]

Updated the PR