Merge pull request #56742 from frappe/mergify/bp/version-16-hotfix/pr-56741

fix(item): rework multiple variant dialog for large numeric ranges (backport #56741)

erpnext/stock/doctype/item/item.js

@@ -777,62 +777,141 @@ $.extend(erpnext.item, {
 
 		function make_fields_from_attribute_values(attr_dict) {
 			let fields = [];
-			let att_key = frm.doc.attributes.map((idx) => idx.attribute);
+			let attributes = frm.doc.attributes.filter((row) => !row.disabled);
-			att_key.forEach((name, i) => {
+			attributes.forEach((row, i) => {
+				let name = row.attribute;
 				if (i % 3 === 0) {
 					fields.push({ fieldtype: "Section Break" });
 				}
-				fields.push({ fieldtype: "Column Break", label: name });
+				fields.push({ fieldtype: "Column Break" });
 				fields.push({
-					fieldtype: "Data",
+					fieldtype: "MultiSelectPills",
-					placeholder: "Search",
+					label: name,
-					fieldname: `search_${frappe.scrub(name)}`,
+					fieldname: frappe.scrub(name),
-					onchange: function (e) {
+					placeholder: __("Search values..."),
-						let value = e.target.value;
+					get_data: (txt) => get_attribute_suggestions(attr_dict[name], txt),
-						let result = attr_dict[name].filter((attr_value) =>
+					onchange: update_primary_action,
-							attr_value.toString().toLowerCase().includes(value.toLowerCase())
-						);
-						attr_dict[name].forEach((attr_value) => {
-							if (result.includes(attr_value)) {
-								me.multiple_variant_dialog.set_df_property(attr_value, "hidden", 0);
-							} else {
-								me.multiple_variant_dialog.set_df_property(attr_value, "hidden", 1);
-							}
-						});
-					},
-				});
-				attr_dict[name].forEach((value) => {
-					fields.push({
-						fieldtype: "Check",
-						label: value,
-						fieldname: value,
-						default: 0,
-						onchange: function () {
-							let selected_attributes = get_selected_attributes();
-							let lengths = Object.keys(selected_attributes).map((key) => {
-								return selected_attributes[key].length;
-							});
-							if (!lengths.length) {
-								me.multiple_variant_dialog.get_primary_btn().html(__("Create Variants"));
-								me.multiple_variant_dialog.disable_primary_action();
-							} else {
-								let no_of_combinations = lengths.reduce((a, b) => a * b, 1);
-								let msg;
-								if (no_of_combinations === 1) {
-									msg = __("Make {0} Variant", [no_of_combinations]);
-								} else {
-									msg = __("Make {0} Variants", [no_of_combinations]);
-								}
-								me.multiple_variant_dialog.get_primary_btn().html(msg);
-								me.multiple_variant_dialog.enable_primary_action();
-							}
-						},
-					});
 				});
 			});
 			return fields;
 		}
 
+		function get_attribute_suggestions(spec, txt) {
+			if (!spec) return [];
+			return Array.isArray(spec) ? filter_list(spec, txt) : numeric_suggestions(spec, txt);
+		}
+
+		// Cap matches so a long value list never hands everything to Awesomplete,
+		// which would freeze the browser.
+		function filter_list(values, txt) {
+			txt = (txt || "").toLowerCase();
+			let matches = [];
+			for (let value of values) {
+				if (!txt || value.toLowerCase().includes(txt)) {
+					matches.push(value);
+					if (matches.length >= 50) break;
+				}
+			}
+			return matches;
+		}
+
+		// Numeric ranges aren't enumerated. With no input, preview the first few
+		// values; once the user types, accept it only if it lies on the increment
+		// within [from, to]. Both paths are cheap even for huge ranges.
+		function numeric_suggestions(range, txt) {
+			let { from_range: from, to_range: to, increment } = range;
+			if (!(increment > 0) || from > to) return [];
+
+			txt = (txt || "").trim();
+			if (!txt) {
+				let preview = [];
+				for (
+					let value = from;
+					value <= to && preview.length < 50;
+					value = flt(value + increment, 6)
+				) {
+					preview.push(String(value));
+				}
+				return preview;
+			}
+
+			return is_valid_attribute_value(range, txt) ? [String(flt(txt, 6))] : [];
+		}
+
+		function is_valid_attribute_value(spec, value) {
+			if (!spec || !value) return false;
+			if (Array.isArray(spec)) return spec.includes(value);
+
+			let { from_range: from, to_range: to, increment } = spec;
+			if (!(increment > 0)) return false;
+
+			// Reject anything that isn't cleanly a number ("abc", "5000xyz", "");
+			// flt would coerce these to 0 and wrongly accept them.
+			let text = String(value).trim();
+			let num = Number(text);
+			if (text === "" || !Number.isFinite(num)) return false;
+
+			if (num < from || num > to) return false;
+			let steps = (num - from) / increment;
+			return Math.abs(Math.round(steps) - steps) <= 1e-6;
+		}
+
+		// Block variant creation if anything is wrong: an invalid committed pill, or
+		// text typed but not added as a pill (which get_selected_attributes would
+		// otherwise drop silently). The user must fix each before creation proceeds.
+		function validate_selected_attributes() {
+			let errors = [];
+			frm.doc.attributes.forEach((row) => {
+				if (row.disabled) return;
+				let field = me.multiple_variant_dialog.get_field(frappe.scrub(row.attribute));
+				if (!field) return;
+
+				let attribute = frappe.utils.escape_html(row.attribute);
+				let spec = attr_val_fields[row.attribute];
+
+				let invalid = [
+					...new Set((field.get_value() || []).filter((v) => !is_valid_attribute_value(spec, v))),
+				];
+				if (invalid.length) {
+					let values = invalid.map(frappe.utils.escape_html).join(", ");
+					errors.push(__("{0}: remove invalid value(s) {1}", [attribute, values]));
+				}
+
+				let pending = (field.$input?.val() || "").trim();
+				if (pending) {
+					let value = frappe.utils.escape_html(pending);
+					errors.push(
+						__("{0}: select the typed value {1} from the list or clear it", [attribute, value])
+					);
+				}
+			});
+
+			if (errors.length) {
+				frappe.throw({
+					title: __("Invalid Attribute Values"),
+					message: errors.join("<br>"),
+					indicator: "red",
+				});
+			}
+		}
+
+		function update_primary_action() {
+			let selected_attributes = get_selected_attributes();
+			let counts = Object.keys(selected_attributes).map((key) => selected_attributes[key].length);
+			if (!counts.length) {
+				me.multiple_variant_dialog.get_primary_btn().html(__("Create Variants"));
+				me.multiple_variant_dialog.disable_primary_action();
+			} else {
+				let no_of_combinations = counts.reduce((a, b) => a * b, 1);
+				let msg =
+					no_of_combinations === 1
+						? __("Make {0} Variant", [no_of_combinations])
+						: __("Make {0} Variants", [no_of_combinations]);
+				me.multiple_variant_dialog.get_primary_btn().html(msg);
+				me.multiple_variant_dialog.enable_primary_action();
+			}
+		}
+
 		function make_and_show_dialog(fields) {
 			me.multiple_variant_dialog = new frappe.ui.Dialog({
 				title: __("Select Attribute Values"),
@@ -858,6 +937,8 @@ $.extend(erpnext.item, {
 			});
 
 			me.multiple_variant_dialog.set_primary_action(__("Create Variants"), () => {
+				validate_selected_attributes();
+
 				let selected_attributes = get_selected_attributes();
 				let use_template_image = me.multiple_variant_dialog.get_value("use_template_image");
 
@@ -885,72 +966,70 @@ $.extend(erpnext.item, {
 				});
 			});
 
-			$($(me.multiple_variant_dialog.$wrapper.find(".form-column")).find(".frappe-control")).css(
-				"margin-bottom",
-				"0px"
-			);
-
 			me.multiple_variant_dialog.disable_primary_action();
 			me.multiple_variant_dialog.clear();
 			me.multiple_variant_dialog.show();
-			me.multiple_variant_dialog.$wrapper
-				.find("div[data-fieldname^='search_']")
-				.find(".clearfix")
-				.hide();
 		}
 
 		function get_selected_attributes() {
 			let selected_attributes = {};
-			me.multiple_variant_dialog.$wrapper.find(".form-column").each((i, col) => {
+			frm.doc.attributes.forEach((row) => {
-				if (i === 0) return;
+				if (row.disabled) return;
-				let attribute_name = $(col).find(".column-label").html().trim();
+				let values = me.multiple_variant_dialog.get_value(frappe.scrub(row.attribute));
-				selected_attributes[attribute_name] = [];
+				if (values && values.length) {
-				let checked_opts = $(col).find(".checkbox input");
+					selected_attributes[row.attribute] = values;
-				checked_opts.each((i, opt) => {
-					if ($(opt).is(":checked")) {
-						selected_attributes[attribute_name].push($(opt).attr("data-fieldname"));
-					}
-				});
-				if (!selected_attributes[attribute_name].length) {
-					delete selected_attributes[attribute_name];
 				}
 			});
-
 			return selected_attributes;
 		}
 
 		frm.doc.attributes.forEach(function (d) {
 			if (!d.disabled) {
 				let p = new Promise((resolve) => {
-					if (!d.numeric_values) {
+					// Read the numeric configuration from the Item Attribute master
-						frappe
+					// instead of the variant attribute row, which may be stale or
-							.call({
+					// blank if the attribute was made numeric after it was added here.
-								method: "frappe.client.get_list",
+					frappe.db
-								args: {
+						.get_value("Item Attribute", d.attribute, [
-									doctype: "Item Attribute Value",
+							"numeric_values",
-									filters: [["parent", "=", d.attribute]],
+							"from_range",
-									fields: ["attribute_value"],
+							"to_range",
-									limit_page_length: 0,
+							"increment",
-									parent: "Item Attribute",
+						])
-									order_by: "idx",
+						.then((res) => {
-								},
+							let attr = res.message || {};
-							})
+
-							.then((r) => {
+							if (!attr.numeric_values) {
-								if (r.message) {
+								frappe
-									attr_val_fields[d.attribute] = r.message.map(function (d) {
+									.call({
-										return d.attribute_value;
+										method: "frappe.client.get_list",
+										args: {
+											doctype: "Item Attribute Value",
+											filters: [["parent", "=", d.attribute]],
+											fields: ["attribute_value"],
+											limit_page_length: 0,
+											parent: "Item Attribute",
+											order_by: "idx",
+										},
+									})
+									.then((r) => {
+										attr_val_fields[d.attribute] = (r.message || []).map(
+											(row) => row.attribute_value
+										);
+										resolve();
 									});
-									resolve();
+							} else {
-								}
+								// Store the range instead of enumerating it; a large range
-							});
+								// (e.g. 1-100000) is slow to build and to search. Values are
-					} else {
+								// validated against the range on demand while typing.
-						let values = [];
+								attr_val_fields[d.attribute] = {
-						for (var i = d.from_range; i <= d.to_range; i = flt(i + d.increment, 6)) {
+									from_range: flt(attr.from_range),
-							values.push(i);
+									to_range: flt(attr.to_range),
-						}
+									increment: flt(attr.increment),
-						attr_val_fields[d.attribute] = values;
+								};
-						resolve();
+								resolve();
-					}
+							}
+						});
 				});
 
 				promises.push(p);

erpnext/stock/doctype/item_attribute/item_attribute.js

@@ -1,4 +1,13 @@
 // Copyright (c) 2019, Frappe Technologies Pvt. Ltd. and contributors
 // For license information, please see license.txt
 
-frappe.ui.form.on("Item Attribute", {});
+frappe.ui.form.on("Item Attribute", {
+	numeric_values(frm) {
+		// Numeric attributes have no discrete values; drop the rows so their
+		// mandatory Attribute Value / Abbreviation don't block the save.
+		if (frm.doc.numeric_values) {
+			frm.clear_table("item_attribute_values");
+			frm.refresh_field("item_attribute_values");
+		}
+	},
+});