From 23f4b51e3e260aadafc8fc4611f1bf1015782271 Mon Sep 17 00:00:00 2001 From: marination Date: Mon, 10 Jan 2022 18:04:03 +0530 Subject: [PATCH 1/4] fix: Behaviour if Item Variants Cache generation - Revert to old behaviour where variant doesnt need to be published - Delete unused functions --- .../doctype/website_item/website_item.py | 108 +----------------- .../variant_selector/item_variants_cache.py | 33 ++++-- 2 files changed, 25 insertions(+), 116 deletions(-) diff --git a/erpnext/e_commerce/doctype/website_item/website_item.py b/erpnext/e_commerce/doctype/website_item/website_item.py index 2e60dfd9459..5aedd686149 100644 --- a/erpnext/e_commerce/doctype/website_item/website_item.py +++ b/erpnext/e_commerce/doctype/website_item/website_item.py @@ -203,16 +203,15 @@ class WebsiteItem(WebsiteGenerator): context.body_class = "product-page" context.parents = get_parent_item_groups(self.item_group, from_item=True) # breadcumbs - self.attributes = frappe.get_all("Item Variant Attribute", + self.attributes = frappe.get_all( + "Item Variant Attribute", fields=["attribute", "attribute_value"], - filters={"parent": self.item_code}) + filters={"parent": self.item_code} + ) if self.slideshow: context.update(get_slideshow(self)) - self.set_variant_context(context) - self.set_attribute_context(context) - self.set_disabled_attributes(context) self.set_metatags(context) self.set_shopping_cart_data(context) @@ -237,61 +236,6 @@ class WebsiteItem(WebsiteGenerator): return context - def set_variant_context(self, context): - if not self.has_variants: - return - - context.no_cache = True - variant = frappe.form_dict.variant - - # load variants - # also used in set_attribute_context - context.variants = frappe.get_all( - "Item", - filters={ - "variant_of": self.item_code, - "published_in_website": 1 - }, - order_by="name asc") - - # the case when the item is opened for the first time from its list - if not variant and context.variants: - variant = context.variants[0] - - if variant: - context.variant = frappe.get_doc("Item", variant) - fields = ("website_image", "website_image_alt", "web_long_description", "description", - "website_specifications") - - for fieldname in fields: - if context.variant.get(fieldname): - value = context.variant.get(fieldname) - if isinstance(value, list): - value = [d.as_dict() for d in value] - - context[fieldname] = value - - if self.slideshow and context.variant and context.variant.slideshow: - context.update(get_slideshow(context.variant)) - - - def set_attribute_context(self, context): - if not self.has_variants: - return - - attribute_values_available = {} - context.attribute_values = {} - context.selected_attributes = {} - - # load attributes - self.set_selected_attributes(context.variants, context, attribute_values_available) - - # filter attributes, order based on attribute table - item = frappe.get_cached_doc("Item", self.item_code) - self.set_attribute_values(item.attributes, context, attribute_values_available) - - context.variant_info = json.dumps(context.variants) - def set_selected_attributes(self, variants, context, attribute_values_available): for variant in variants: variant.attributes = frappe.get_all( @@ -328,50 +272,6 @@ class WebsiteItem(WebsiteGenerator): if attr_value.attribute_value in attribute_values_available.get(attr.attribute, []): values.append(attr_value.attribute_value) - def set_disabled_attributes(self, context): - """Disable selection options of attribute combinations that do not result in a variant""" - - if not self.attributes or not self.has_variants: - return - - context.disabled_attributes = {} - attributes = [attr.attribute for attr in self.attributes] - - def find_variant(combination): - for variant in context.variants: - if len(variant.attributes) < len(attributes): - continue - - if "combination" not in variant: - ref_combination = [] - - for attr in variant.attributes: - idx = attributes.index(attr.attribute) - ref_combination.insert(idx, attr.attribute_value) - - variant["combination"] = ref_combination - - if not (set(combination) - set(variant["combination"])): - # check if the combination is a subset of a variant combination - # eg. [Blue, 0.5] is a possible combination if exists [Blue, Large, 0.5] - return True - - for i, attr in enumerate(self.attributes): - if i == 0: - continue - - combination_source = [] - - # loop through previous attributes - for prev_attr in self.attributes[:i]: - combination_source.append([context.selected_attributes.get(prev_attr.attribute)]) - - combination_source.append(context.attribute_values[attr.attribute]) - - for combination in itertools.product(*combination_source): - if not find_variant(combination): - context.disabled_attributes.setdefault(attr.attribute, []).append(combination[-1]) - def set_metatags(self, context): context.metatags = frappe._dict({}) diff --git a/erpnext/e_commerce/variant_selector/item_variants_cache.py b/erpnext/e_commerce/variant_selector/item_variants_cache.py index 39eb9155d5e..bb6b3ef37fe 100644 --- a/erpnext/e_commerce/variant_selector/item_variants_cache.py +++ b/erpnext/e_commerce/variant_selector/item_variants_cache.py @@ -44,7 +44,7 @@ class ItemVariantsCacheManager: val = frappe.cache().get_value('ordered_attribute_values_map') if val: return val - all_attribute_values = frappe.db.get_all('Item Attribute Value', + all_attribute_values = frappe.get_all('Item Attribute Value', ['attribute_value', 'idx', 'parent'], order_by='idx asc') ordered_attribute_values_map = frappe._dict({}) @@ -57,25 +57,34 @@ class ItemVariantsCacheManager: def build_cache(self): parent_item_code = self.item_code - attributes = [a.attribute for a in frappe.db.get_all('Item Variant Attribute', - {'parent': parent_item_code}, ['attribute'], order_by='idx asc') + attributes = [ + a.attribute for a in frappe.get_all( + 'Item Variant Attribute', + {'parent': parent_item_code}, + ['attribute'], + order_by='idx asc' + ) ] - item_variants_data = frappe.db.get_all('Item Variant Attribute', - {'variant_of': parent_item_code}, ['parent', 'attribute', 'attribute_value'], + # join with Website Item + item_variants_data = frappe.get_all( + 'Item Variant Attribute', + {'variant_of': parent_item_code}, + ['parent', 'attribute', 'attribute_value'], order_by='name', as_list=1 ) - unpublished_items = set([i.item_code for i in frappe.db.get_all('Website Item', filters={'published': 0}, fields=["item_code"])]) + disabled_items = set( + [i.name for i in frappe.db.get_all('Item', {'disabled': 1})] + ) - attribute_value_item_map = frappe._dict({}) - item_attribute_value_map = frappe._dict({}) + attribute_value_item_map = frappe._dict() + item_attribute_value_map = frappe._dict() - # dont consider variants that are unpublished - # (either have no Website Item or are unpublished in Website Item) - item_variants_data = [r for r in item_variants_data if r[0] not in unpublished_items] - item_variants_data = [r for r in item_variants_data if frappe.db.exists("Website Item", {"item_code": r[0]})] + # dont consider variants that are disabled + # pull all other variants + item_variants_data = [r for r in item_variants_data if r[0] not in disabled_items] for row in item_variants_data: item_code, attribute, attribute_value = row From 577de39ca6c8fb1709a7fd14354840ade8d5a42a Mon Sep 17 00:00:00 2001 From: marination Date: Tue, 11 Jan 2022 18:19:29 +0530 Subject: [PATCH 2/4] test: Variant Selector - Attribute Value fetching --- .../doctype/website_item/website_item.py | 1 - .../variant_selector/test_variant_selector.py | 87 +++++++++++++++++-- 2 files changed, 82 insertions(+), 6 deletions(-) diff --git a/erpnext/e_commerce/doctype/website_item/website_item.py b/erpnext/e_commerce/doctype/website_item/website_item.py index 5aedd686149..181aaa1292e 100644 --- a/erpnext/e_commerce/doctype/website_item/website_item.py +++ b/erpnext/e_commerce/doctype/website_item/website_item.py @@ -1,7 +1,6 @@ # Copyright (c) 2021, Frappe Technologies Pvt. Ltd. and contributors # For license information, please see license.txt -import itertools import json import frappe diff --git a/erpnext/e_commerce/variant_selector/test_variant_selector.py b/erpnext/e_commerce/variant_selector/test_variant_selector.py index c70fee5fff5..d9f56216333 100644 --- a/erpnext/e_commerce/variant_selector/test_variant_selector.py +++ b/erpnext/e_commerce/variant_selector/test_variant_selector.py @@ -1,11 +1,88 @@ -# import frappe +import frappe import unittest -# from erpnext.e_commerce.product_data_engine.query import ProductQuery -# from erpnext.e_commerce.doctype.website_item.website_item import make_website_item +from erpnext.controllers.item_variant import create_variant +from erpnext.e_commerce.doctype.website_item.website_item import make_website_item +from erpnext.stock.doctype.item.test_item import make_item test_dependencies = ["Item"] class TestVariantSelector(unittest.TestCase): - # TODO: Variant Selector Tests - pass \ No newline at end of file + + def setUp(self) -> None: + self.template_item = make_item("Test-Tshirt-Temp", { + "has_variant": 1, + "variant_based_on": "Item Attribute", + "attributes": [ + { + "attribute": "Test Size" + }, + { + "attribute": "Test Colour" + } + ] + }) + + # create L-R, L-G, M-R, M-G and S-R + for size in ("Large", "Medium",): + for colour in ("Red", "Green",): + variant = create_variant("Test-Tshirt-Temp", { + "Test Size": size, + "Test Colour": colour + }) + variant.save() + + variant = create_variant("Test-Tshirt-Temp", { + "Test Size": "Small", + "Test Colour": "Red" + }) + variant.save() + + @classmethod + def tearDownClass(cls): + frappe.db.rollback() + + def test_item_attributes(self): + """ + Test if the right attributes are fetched in the popup. + (Attributes must only come from active items) + + Attribute selection must not be linked to Website Items. + """ + from erpnext.e_commerce.variant_selector.utils import get_attributes_and_values + + make_website_item(self.template_item) # publish template not variants + + attr_data = get_attributes_and_values("Test-Tshirt-Temp") + + self.assertEqual(attr_data[0]["attribute"], "Test Size") + self.assertEqual(attr_data[1]["attribute"], "Test Colour") + self.assertEqual(len(attr_data[0]["values"]), 3) # ['Small', 'Medium', 'Large'] + self.assertEqual(len(attr_data[1]["values"]), 2) # ['Red', 'Green'] + + # disable small red tshirt, now there are no small tshirts. + # but there are some red tshirts + small_variant = frappe.get_doc("Item", "Test-Tshirt-Temp-S-R") + small_variant.disabled = 1 + small_variant.save() # trigger cache rebuild + + attr_data = get_attributes_and_values("Test-Tshirt-Temp") + + # Only L and M attribute values must be fetched since S is disabled + self.assertEqual(len(attr_data[0]["values"]), 2) # ['Medium', 'Large'] + + # def test_next_item_variant_values(self): + # """ + # Test if on selecting an attribute value, the next possible values + # are filtered accordingly. + # Values that dont apply should not be fetched. + # E.g. + # There is a ** Small-Red ** Tshirt. No other colour in this size. + # On selecting ** Small **, only ** Red ** should be selectable next. + # """ + # from erpnext.e_commerce.variant_selector.utils import get_next_attribute_and_values + + # next_values = get_next_attribute_and_values("Test-Tshirt-Temp", selected_attributes={ + # "Colour": "Red" + # }) + # print(next_values) \ No newline at end of file From 746bb6277f2f6c240df226012ceb90a6baf5b339 Mon Sep 17 00:00:00 2001 From: marination Date: Tue, 11 Jan 2022 20:15:24 +0530 Subject: [PATCH 3/4] test: Variant Selector - Fetch next valid Attribute values fetching after selection of one --- .../variant_selector/test_variant_selector.py | 35 +++++++++++-------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/erpnext/e_commerce/variant_selector/test_variant_selector.py b/erpnext/e_commerce/variant_selector/test_variant_selector.py index d9f56216333..608ff230a67 100644 --- a/erpnext/e_commerce/variant_selector/test_variant_selector.py +++ b/erpnext/e_commerce/variant_selector/test_variant_selector.py @@ -1,6 +1,7 @@ -import frappe import unittest +import frappe + from erpnext.controllers.item_variant import create_variant from erpnext.e_commerce.doctype.website_item.website_item import make_website_item from erpnext.stock.doctype.item.test_item import make_item @@ -71,18 +72,22 @@ class TestVariantSelector(unittest.TestCase): # Only L and M attribute values must be fetched since S is disabled self.assertEqual(len(attr_data[0]["values"]), 2) # ['Medium', 'Large'] - # def test_next_item_variant_values(self): - # """ - # Test if on selecting an attribute value, the next possible values - # are filtered accordingly. - # Values that dont apply should not be fetched. - # E.g. - # There is a ** Small-Red ** Tshirt. No other colour in this size. - # On selecting ** Small **, only ** Red ** should be selectable next. - # """ - # from erpnext.e_commerce.variant_selector.utils import get_next_attribute_and_values + def test_next_item_variant_values(self): + """ + Test if on selecting an attribute value, the next possible values + are filtered accordingly. + Values that dont apply should not be fetched. + E.g. + There is a ** Small-Red ** Tshirt. No other colour in this size. + On selecting ** Small **, only ** Red ** should be selectable next. + """ + from erpnext.e_commerce.variant_selector.utils import get_next_attribute_and_values - # next_values = get_next_attribute_and_values("Test-Tshirt-Temp", selected_attributes={ - # "Colour": "Red" - # }) - # print(next_values) \ No newline at end of file + next_values = get_next_attribute_and_values("Test-Tshirt-Temp", selected_attributes={"Test Size": "Small"}) + next_colours = next_values["valid_options_for_attributes"]["Test Colour"] + filtered_items = next_values["filtered_items"] + + self.assertEqual(len(next_colours), 1) + self.assertEqual(next_colours.pop(), "Red") + self.assertEqual(len(filtered_items), 1) + self.assertEqual(filtered_items.pop(), "Test-Tshirt-Temp-S-R") From a0bd82e99f617071ce21124c9cd3638c917a74b7 Mon Sep 17 00:00:00 2001 From: marination Date: Tue, 11 Jan 2022 22:43:39 +0530 Subject: [PATCH 4/4] fix: Server Side test - teardown correctly --- .../e_commerce/variant_selector/test_variant_selector.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/erpnext/e_commerce/variant_selector/test_variant_selector.py b/erpnext/e_commerce/variant_selector/test_variant_selector.py index 608ff230a67..31c86f1fd53 100644 --- a/erpnext/e_commerce/variant_selector/test_variant_selector.py +++ b/erpnext/e_commerce/variant_selector/test_variant_selector.py @@ -39,8 +39,7 @@ class TestVariantSelector(unittest.TestCase): }) variant.save() - @classmethod - def tearDownClass(cls): + def tearDown(self): frappe.db.rollback() def test_item_attributes(self): @@ -72,6 +71,10 @@ class TestVariantSelector(unittest.TestCase): # Only L and M attribute values must be fetched since S is disabled self.assertEqual(len(attr_data[0]["values"]), 2) # ['Medium', 'Large'] + # teardown + small_variant.disabled = 1 + small_variant.save() + def test_next_item_variant_values(self): """ Test if on selecting an attribute value, the next possible values