From 44abf309473258a7beedc7cc7245d21e874ac22a Mon Sep 17 00:00:00 2001 From: Voxin Muyli Date: Mon, 9 Oct 2023 16:32:37 +0200 Subject: [PATCH 01/18] Optimize attribute values (#321) * Implemented faster attributes save * renamed for clarity --- oscarapi/serializers/admin/product.py | 5 +- oscarapi/serializers/fields.py | 98 +++++++++--------------- oscarapi/serializers/product.py | 106 +++++++++++++++++++++++++- oscarapi/tests/unit/testproduct.py | 2 + oscarapi/utils/attributes.py | 78 +++++++++++++++++++ oscarapi/utils/deprecations.py | 2 + 6 files changed, 229 insertions(+), 62 deletions(-) create mode 100644 oscarapi/utils/attributes.py create mode 100644 oscarapi/utils/deprecations.py diff --git a/oscarapi/serializers/admin/product.py b/oscarapi/serializers/admin/product.py index ee2869c0..3984bef4 100644 --- a/oscarapi/serializers/admin/product.py +++ b/oscarapi/serializers/admin/product.py @@ -145,12 +145,15 @@ def update(self, instance, validated_data): if ( self.partial ): # we need to clean up all the attributes with wrong product class + attribute_codes = product_class.attributes.values_list( + "code", flat=True + ) for attribute_value in instance.attribute_values.exclude( attribute__product_class=product_class ): code = attribute_value.attribute.code if ( - code in pclass_option_codes + code in attribute_codes ): # if the attribute exist also on the new product class, update the attribute attribute_value.attribute = product_class.attributes.get( code=code diff --git a/oscarapi/serializers/fields.py b/oscarapi/serializers/fields.py index 1e0599f8..09a5180c 100644 --- a/oscarapi/serializers/fields.py +++ b/oscarapi/serializers/fields.py @@ -1,6 +1,7 @@ # pylint: disable=W0212, W0201, W0632 import logging import operator +import warnings from os.path import basename, join from urllib.parse import urlsplit, parse_qs @@ -15,8 +16,10 @@ from rest_framework.fields import get_attribute from oscar.core.loading import get_model, get_class +from oscarapi.utils.deprecations import RemovedInOScarAPI4 from oscarapi import settings +from oscarapi.utils.attributes import AttributeFieldBase, attribute_details from oscarapi.utils.loading import get_api_class from oscarapi.utils.exists import bound_unique_together_get_or_create from .exceptions import FieldError @@ -27,7 +30,6 @@ create_from_breadcrumbs = get_class("catalogue.categories", "create_from_breadcrumbs") entity_internal_value = get_api_class("serializers.hooks", "entity_internal_value") RetrieveFileMixin = get_api_class(settings.FILE_DOWNLOADER_MODULE, "RetrieveFileMixin") -attribute_details = operator.itemgetter("code", "value") class TaxIncludedDecimalField(serializers.DecimalField): @@ -93,7 +95,7 @@ def use_pk_only_optimization(self): return False -class AttributeValueField(serializers.Field): +class AttributeValueField(AttributeFieldBase, serializers.Field): """ This field is used to handle the value of the ProductAttributeValue model @@ -103,30 +105,46 @@ class AttributeValueField(serializers.Field): """ def __init__(self, **kwargs): + warnings.warn( + "AttributeValueField is deprecated and will be removed in a future version of oscarapi", + RemovedInOScarAPI4, + stacklevel=2, + ) # this field always needs the full object kwargs["source"] = "*" - kwargs["error_messages"] = { - "no_such_option": _("{code}: Option {value} does not exist."), - "invalid": _("Wrong type, {error}."), - "attribute_validation_error": _( - "Error assigning `{value}` to {code}, {error}." - ), - "attribute_required": _("Attribute {code} is required."), - "attribute_missing": _( - "No attribute exist with code={code}, " - "please define it in the product_class first." - ), - "child_without_parent": _( - "Can not find attribute if product_class is empty and " - "parent is empty as well, child without parent?" - ), - } super(AttributeValueField, self).__init__(**kwargs) def get_value(self, dictionary): # return all the data because this field uses everything return dictionary + def to_product_attribute(self, data): + if "product" in data: + # we need the attribute to determine the type of the value + return ProductAttribute.objects.get( + code=data["code"], product_class__products__id=data["product"] + ) + elif "product_class" in data and data["product_class"] is not None: + return ProductAttribute.objects.get( + code=data["code"], product_class__slug=data.get("product_class") + ) + elif "parent" in data: + return ProductAttribute.objects.get( + code=data["code"], product_class__products__id=data["parent"] + ) + + def to_attribute_type_value(self, attribute, code, value): + internal_value = super().to_attribute_type_value(attribute, code, value) + if attribute.type in [ + attribute.IMAGE, + attribute.FILE, + ]: + image_field = ImageUrlField() + image_field._context = self.context + internal_value = image_field.to_internal_value(value) + + return internal_value + def to_internal_value(self, data): # noqa assert "product" in data or "product_class" in data or "parent" in data @@ -134,49 +152,9 @@ def to_internal_value(self, data): # noqa code, value = attribute_details(data) internal_value = value - if "product" in data: - # we need the attribute to determine the type of the value - attribute = ProductAttribute.objects.get( - code=code, product_class__products__id=data["product"] - ) - elif "product_class" in data and data["product_class"] is not None: - attribute = ProductAttribute.objects.get( - code=code, product_class__slug=data.get("product_class") - ) - elif "parent" in data: - attribute = ProductAttribute.objects.get( - code=code, product_class__products__id=data["parent"] - ) + attribute = self.to_product_attribute(data) - if attribute.required and value is None: - self.fail("attribute_required", code=code) - - # some of these attribute types need special processing, or their - # validation will fail - if attribute.type == attribute.OPTION: - internal_value = attribute.option_group.options.get(option=value) - elif attribute.type == attribute.MULTI_OPTION: - if attribute.required and not value: - self.fail("attribute_required", code=code) - internal_value = attribute.option_group.options.filter(option__in=value) - if len(value) != internal_value.count(): - non_existing = set(value) - set( - internal_value.values_list("option", flat=True) - ) - non_existing_as_error = ",".join(sorted(non_existing)) - self.fail("no_such_option", value=non_existing_as_error, code=code) - elif attribute.type == attribute.DATE: - date_field = serializers.DateField() - internal_value = date_field.to_internal_value(value) - elif attribute.type == attribute.DATETIME: - date_field = serializers.DateTimeField() - internal_value = date_field.to_internal_value(value) - elif attribute.type == attribute.ENTITY: - internal_value = entity_internal_value(attribute, value) - elif attribute.type in [attribute.IMAGE, attribute.FILE]: - image_field = ImageUrlField() - image_field._context = self.context - internal_value = image_field.to_internal_value(value) + internal_value = self.to_attribute_type_value(attribute, code, value) # the rest of the attribute types don't need special processing try: diff --git a/oscarapi/serializers/product.py b/oscarapi/serializers/product.py index b480d6f3..cc71d01d 100644 --- a/oscarapi/serializers/product.py +++ b/oscarapi/serializers/product.py @@ -2,6 +2,7 @@ import logging from copy import deepcopy +from django.db.models.manager import Manager from django.utils.translation import gettext as _ from rest_framework import serializers @@ -15,7 +16,7 @@ from oscarapi.utils.files import file_hash from oscarapi.utils.exists import find_existing_attribute_option_group from oscarapi.utils.accessors import getitems - +from oscarapi.utils.attributes import AttributeConverter from oscarapi.serializers.fields import DrillDownHyperlinkedIdentityField from oscarapi.serializers.utils import ( OscarModelSerializer, @@ -195,6 +196,71 @@ class Meta: class ProductAttributeValueListSerializer(UpdateListSerializer): + # pylint: disable=unused-argument + def shortcut_to_internal_value(self, data, productclass, attributes): + difficult_attributes = { + at.code: at + for at in productclass.attributes.filter( + type__in=[ + ProductAttribute.OPTION, + ProductAttribute.MULTI_OPTION, + ProductAttribute.DATE, + ProductAttribute.DATETIME, + ProductAttribute.ENTITY, + ] + ) + } + cv = AttributeConverter(self.context) + internal_value = [] + for item in data: + code, value = getitems(item, "code", "value") + if code is None: # delegate error state to child serializer + internal_value.append(self.child.to_internal_value(item)) + + if code in difficult_attributes: + attribute = difficult_attributes[code] + converted_value = cv.to_attribute_type_value(attribute, code, value) + internal_value.append( + { + "value": converted_value, + "attribute": attribute, + "product_class": productclass, + } + ) + else: + internal_value.append( + { + "value": value, + "attribute": code, + "product_class": productclass, + } + ) + + return internal_value + + def to_internal_value(self, data): + productclasses = set() + attributes = set() + + for item in data: + product_class, code = getitems(item, "product_class", "code") + if product_class: + productclasses.add(product_class) + attributes.add(code) + + # if all attributes belong to the same productclass, everything is just + # as expected and we can take a shortcut by only resolving the + # productclass to the model instance and nothing else. + try: + if len(productclasses) == 1 and all(attributes): + (product_class,) = productclasses + pc = ProductClass.objects.get(slug=product_class) + return self.shortcut_to_internal_value(data, pc, attributes) + except ProductClass.DoesNotExist: + pass + + return super().to_internal_value(data) + def get_value(self, dictionary): values = super(ProductAttributeValueListSerializer, self).get_value(dictionary) if values is empty: @@ -205,6 +271,44 @@ def get_value(self, dictionary): dict(value, product_class=product_class, parent=parent) for value in values ] + def to_representation(self, data): + if isinstance(data, Manager): + # use a cached query from product.attr to get the attributes instead + # if an silly .all() that clones the queryset and performs a new query + _, product = self.get_name_and_rel_instance(data) + iterable = product.attr.get_values() + else: + iterable = data + + return [self.child.to_representation(item) for item in iterable] + + def update(self, instance, validated_data): + assert isinstance(instance, Manager) + + _, product = self.get_name_and_rel_instance(instance) + + attr_codes = [] + product.attr.initialize() + for validated_datum in validated_data: + # leave all the attribute saving to the ProductAttributesContainer instead + # of the child serializers + attribute, value = getitems(validated_datum, "attribute", "value") + if hasattr( + attribute, "code" + ): # if the attribute is a model instance use the code + product.attr.set(attribute.code, value, validate_identifier=False) + attr_codes.append(attribute.code) + else: + product.attr.set(attribute, value, validate_identifier=False) + attr_codes.append(attribute) + + # if we don't clear the dirty attributes all parent attributes + # are marked as explicitly set, so they will be copied to the + # child product. + product.attr._dirty.clear() # pylint: disable=protected-access + product.attr.save() + return list(product.attr.get_values().filter(attribute__code__in=attr_codes)) + class ProductAttributeValueSerializer(OscarModelSerializer): # we declare the product as write_only since this serializer is meant to be diff --git a/oscarapi/tests/unit/testproduct.py b/oscarapi/tests/unit/testproduct.py index 0767cba9..9ec1b59d 100644 --- a/oscarapi/tests/unit/testproduct.py +++ b/oscarapi/tests/unit/testproduct.py @@ -994,6 +994,7 @@ def test_switch_product_class_patch(self): they may cause errors. """ product = Product.objects.get(pk=3) + self.assertEqual(product.attribute_values.count(), 11) ser = AdminProductSerializer( data={ "product_class": "t-shirt", @@ -1006,6 +1007,7 @@ def test_switch_product_class_patch(self): ) self.assertTrue(ser.is_valid(), "Something wrong %s" % ser.errors) obj = ser.save() + self.assertEqual( obj.attribute_values.count(), 2, diff --git a/oscarapi/utils/attributes.py b/oscarapi/utils/attributes.py new file mode 100644 index 00000000..fdef848c --- /dev/null +++ b/oscarapi/utils/attributes.py @@ -0,0 +1,78 @@ +import operator +from django.utils.translation import gettext_lazy as _ +from rest_framework import serializers +from rest_framework.fields import MISSING_ERROR_MESSAGE +from rest_framework.exceptions import ErrorDetail +from oscarapi.utils.loading import get_api_class + +attribute_details = operator.itemgetter("code", "value") +entity_internal_value = get_api_class("serializers.hooks", "entity_internal_value") + + +class AttributeFieldBase: + default_error_messages = { + "no_such_option": _("{code}: Option {value} does not exist."), + "invalid": _("Wrong type, {error}."), + "attribute_validation_error": _( + "Error assigning `{value}` to {code}, {error}." + ), + "attribute_required": _("Attribute {code} is required."), + "attribute_missing": _( + "No attribute exist with code={code}, " + "please define it in the product_class first." + ), + "child_without_parent": _( + "Can not find attribute if product_class is empty and " + "parent is empty as well, child without parent?" + ), + } + + def to_attribute_type_value(self, attribute, code, value): + internal_value = value + # pylint: disable=no-member + if attribute.required and value is None: + self.fail("attribute_required", code=code) + + # some of these attribute types need special processing, or their + # validation will fail + if attribute.type == attribute.OPTION: + internal_value = attribute.option_group.options.get(option=value) + elif attribute.type == attribute.MULTI_OPTION: + if attribute.required and not value: + self.fail("attribute_required", code=code) + internal_value = attribute.option_group.options.filter(option__in=value) + if len(value) != internal_value.count(): + non_existing = set(value) - set( + internal_value.values_list("option", flat=True) + ) + non_existing_as_error = ",".join(sorted(non_existing)) + self.fail("no_such_option", value=non_existing_as_error, code=code) + elif attribute.type == attribute.DATE: + date_field = serializers.DateField() + internal_value = date_field.to_internal_value(value) + elif attribute.type == attribute.DATETIME: + date_field = serializers.DateTimeField() + internal_value = date_field.to_internal_value(value) + elif attribute.type == attribute.ENTITY: + internal_value = entity_internal_value(attribute, value) + + return internal_value + + +class AttributeConverter(AttributeFieldBase): + def __init__(self, context): + self.context = context + self.errors = [] + + def fail(self, key, **kwargs): + """ + An implementation of fail that collects errors instead of raising them + """ + try: + msg = self.default_error_messages[key] + except KeyError: + class_name = self.__class__.__name__ + msg = MISSING_ERROR_MESSAGE.format(class_name=class_name, key=key) + raise AssertionError(msg) + message_string = msg.format(**kwargs) + self.errors.append(ErrorDetail(message_string, code=key)) diff --git a/oscarapi/utils/deprecations.py b/oscarapi/utils/deprecations.py new file mode 100644 index 00000000..88ccef88 --- /dev/null +++ b/oscarapi/utils/deprecations.py @@ -0,0 +1,2 @@ +class RemovedInOScarAPI4(PendingDeprecationWarning): + pass From 13ce8f4fbc0242f41fd55937fce7c71355aeca21 Mon Sep 17 00:00:00 2001 From: Lars van de Kerkhof Date: Mon, 9 Oct 2023 16:29:31 +0200 Subject: [PATCH 02/18] Renamed warning --- oscarapi/serializers/fields.py | 4 ++-- oscarapi/utils/deprecations.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/oscarapi/serializers/fields.py b/oscarapi/serializers/fields.py index 09a5180c..e5c9cce1 100644 --- a/oscarapi/serializers/fields.py +++ b/oscarapi/serializers/fields.py @@ -16,7 +16,7 @@ from rest_framework.fields import get_attribute from oscar.core.loading import get_model, get_class -from oscarapi.utils.deprecations import RemovedInOScarAPI4 +from oscarapi.utils.deprecations import RemovedInFutureRelease from oscarapi import settings from oscarapi.utils.attributes import AttributeFieldBase, attribute_details @@ -107,7 +107,7 @@ class AttributeValueField(AttributeFieldBase, serializers.Field): def __init__(self, **kwargs): warnings.warn( "AttributeValueField is deprecated and will be removed in a future version of oscarapi", - RemovedInOScarAPI4, + RemovedInFutureRelease, stacklevel=2, ) # this field always needs the full object diff --git a/oscarapi/utils/deprecations.py b/oscarapi/utils/deprecations.py index 88ccef88..88677141 100644 --- a/oscarapi/utils/deprecations.py +++ b/oscarapi/utils/deprecations.py @@ -1,2 +1,2 @@ -class RemovedInOScarAPI4(PendingDeprecationWarning): +class RemovedInFutureRelease(PendingDeprecationWarning): pass From 6b6713b6613bcf91907cc0b44fb556c2621bd0c0 Mon Sep 17 00:00:00 2001 From: Voxin Muyli Date: Tue, 10 Oct 2023 11:59:23 +0200 Subject: [PATCH 03/18] Enable shortcut when saving child products. (#322) --- oscarapi/serializers/product.py | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/oscarapi/serializers/product.py b/oscarapi/serializers/product.py index cc71d01d..31094c94 100644 --- a/oscarapi/serializers/product.py +++ b/oscarapi/serializers/product.py @@ -241,24 +241,34 @@ def shortcut_to_internal_value(self, data, productclass, attributes): def to_internal_value(self, data): productclasses = set() attributes = set() + parent = None for item in data: product_class, code = getitems(item, "product_class", "code") if product_class: productclasses.add(product_class) + if "parent" in item and item["parent"] is not None: + parent = item["parent"] attributes.add(code) # if all attributes belong to the same productclass, everything is just # as expected and we can take a shortcut by only resolving the # productclass to the model instance and nothing else. - try: - if len(productclasses) == 1 and all(attributes): - (product_class,) = productclasses - pc = ProductClass.objects.get(slug=product_class) - return self.shortcut_to_internal_value(data, pc, attributes) - except ProductClass.DoesNotExist: - pass - + attrs_valid = all(attributes) # no missing attribute codes? + if attrs_valid: + try: + if len(productclasses): + (product_class,) = productclasses + pc = ProductClass.objects.get(slug=product_class) + return self.shortcut_to_internal_value(data, pc, attributes) + elif parent: + pc = ProductClass.objects.get(products__id=parent) + return self.shortcut_to_internal_value(data, pc, attributes) + except ProductClass.DoesNotExist: + pass + + # if we get here we can't take the shortcut, just let everything be + # processed by the original serializer and handle the errors. return super().to_internal_value(data) def get_value(self, dictionary): From 20ba73b04e96a18a882c69a0de0a5c020e715294 Mon Sep 17 00:00:00 2001 From: Voxin Muyli Date: Wed, 24 Jan 2024 14:55:09 +0100 Subject: [PATCH 04/18] Make sure to only return attributevalues that are on the actual product to avoid transferring (#331) attributesvalues from parent to child. --- oscarapi/serializers/product.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oscarapi/serializers/product.py b/oscarapi/serializers/product.py index 31094c94..06c11fd1 100644 --- a/oscarapi/serializers/product.py +++ b/oscarapi/serializers/product.py @@ -317,7 +317,7 @@ def update(self, instance, validated_data): # child product. product.attr._dirty.clear() # pylint: disable=protected-access product.attr.save() - return list(product.attr.get_values().filter(attribute__code__in=attr_codes)) + return list(product.attribute_values.filter(attribute__code__in=attr_codes)) class ProductAttributeValueSerializer(OscarModelSerializer): From 8ea9bf5e55951c48c92db51a4058bb39ff47d0cb Mon Sep 17 00:00:00 2001 From: Voxin Muyli Date: Thu, 22 Feb 2024 14:38:06 +0100 Subject: [PATCH 05/18] Improve multi db support (#335) Use the same database as the passed-in manager --- oscarapi/serializers/product.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/oscarapi/serializers/product.py b/oscarapi/serializers/product.py index 06c11fd1..41435c04 100644 --- a/oscarapi/serializers/product.py +++ b/oscarapi/serializers/product.py @@ -317,7 +317,12 @@ def update(self, instance, validated_data): # child product. product.attr._dirty.clear() # pylint: disable=protected-access product.attr.save() - return list(product.attribute_values.filter(attribute__code__in=attr_codes)) + # we have to make sure to use the correct db_manager in a multidatabase + # context, we make sure to use the same database as the passed in manager + local_attribute_values = product.attribute_values.db_manager( + instance.db + ).filter(attribute__code__in=attr_codes) + return list(local_attribute_values) class ProductAttributeValueSerializer(OscarModelSerializer): From f3f041e9ecd38b6a860aadf423ebcae4f5a1498f Mon Sep 17 00:00:00 2001 From: Voxin Muyli Date: Mon, 18 Mar 2024 11:49:30 +0100 Subject: [PATCH 06/18] The image and file fields where nolonger working. (#337) * The image and file fields where nolonger working. This PR restores that functionality. * nergens voor --- oscarapi/serializers/fields.py | 10 ++++++---- oscarapi/serializers/product.py | 2 ++ oscarapi/utils/attributes.py | 6 ++++++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/oscarapi/serializers/fields.py b/oscarapi/serializers/fields.py index e5c9cce1..eabf21c7 100644 --- a/oscarapi/serializers/fields.py +++ b/oscarapi/serializers/fields.py @@ -199,10 +199,12 @@ def to_representation(self, value): return value.value.option elif obj_type == value.attribute.MULTI_OPTION: return value.value.values_list("option", flat=True) - elif obj_type == value.attribute.FILE: - return value.value.url - elif obj_type == value.attribute.IMAGE: - return value.value.url + elif obj_type in [value.attribute.FILE, value.attribute.IMAGE]: + url = value.value.url + request = self.context.get("request", None) + if request is not None: + url = request.build_absolute_uri(url) + return url elif obj_type == value.attribute.ENTITY: if hasattr(value.value, "json"): return value.value.json() diff --git a/oscarapi/serializers/product.py b/oscarapi/serializers/product.py index 41435c04..a7eb9b24 100644 --- a/oscarapi/serializers/product.py +++ b/oscarapi/serializers/product.py @@ -207,6 +207,8 @@ def shortcut_to_internal_value(self, data, productclass, attributes): ProductAttribute.DATE, ProductAttribute.DATETIME, ProductAttribute.ENTITY, + ProductAttribute.FILE, + ProductAttribute.IMAGE, ] ) } diff --git a/oscarapi/utils/attributes.py b/oscarapi/utils/attributes.py index fdef848c..bd5b9989 100644 --- a/oscarapi/utils/attributes.py +++ b/oscarapi/utils/attributes.py @@ -4,6 +4,7 @@ from rest_framework.fields import MISSING_ERROR_MESSAGE from rest_framework.exceptions import ErrorDetail from oscarapi.utils.loading import get_api_class +from oscarapi.serializers import fields as oscarapi_fields attribute_details = operator.itemgetter("code", "value") entity_internal_value = get_api_class("serializers.hooks", "entity_internal_value") @@ -55,6 +56,11 @@ def to_attribute_type_value(self, attribute, code, value): internal_value = date_field.to_internal_value(value) elif attribute.type == attribute.ENTITY: internal_value = entity_internal_value(attribute, value) + elif attribute.type in [attribute.IMAGE, attribute.FILE]: + image_field = oscarapi_fields.ImageUrlField() + # pylint: disable=protected-access + image_field._context = self.context + internal_value = image_field.to_internal_value(value) return internal_value From a561162d3cd76e1149631fe8987fdc18d407466f Mon Sep 17 00:00:00 2001 From: Lars van de Kerkhof Date: Mon, 18 Mar 2024 12:20:11 +0100 Subject: [PATCH 07/18] fixes tests --- oscarapi/tests/unit/testproduct.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/oscarapi/tests/unit/testproduct.py b/oscarapi/tests/unit/testproduct.py index 9ec1b59d..7a1eaa0a 100644 --- a/oscarapi/tests/unit/testproduct.py +++ b/oscarapi/tests/unit/testproduct.py @@ -237,7 +237,7 @@ def test_product_attributes(self): self.assertEqual(attributes_by_name["datetime"], "2018-01-02T10:45:00Z") self.assertIsInstance(attributes_by_name["file"], str) self.assertEqual( - attributes_by_name["file"], "/media/images/products/2018/01/sony-xa50ES.pdf" + attributes_by_name["file"], "http://testserver/media/images/products/2018/01/sony-xa50ES.pdf" ) self.assertIsInstance(attributes_by_name["float"], float) self.assertEqual(attributes_by_name["float"], 3.2) @@ -247,7 +247,7 @@ def test_product_attributes(self): ) self.assertIsInstance(attributes_by_name["image"], str) self.assertEqual( - attributes_by_name["image"], "/media/images/products/2018/01/IMG_3777.JPG" + attributes_by_name["image"], "http://testserver/media/images/products/2018/01/IMG_3777.JPG" ) self.assertIsInstance(attributes_by_name["integer"], int) self.assertEqual(attributes_by_name["integer"], 7) From 68c7f4cc31b9caa9d79a37a34ad2e1fd103c1bdf Mon Sep 17 00:00:00 2001 From: Lars van de Kerkhof Date: Mon, 18 Mar 2024 12:25:18 +0100 Subject: [PATCH 08/18] black --- oscarapi/tests/unit/testproduct.py | 6 ++++-- setup.py | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/oscarapi/tests/unit/testproduct.py b/oscarapi/tests/unit/testproduct.py index 7a1eaa0a..634e9dc4 100644 --- a/oscarapi/tests/unit/testproduct.py +++ b/oscarapi/tests/unit/testproduct.py @@ -237,7 +237,8 @@ def test_product_attributes(self): self.assertEqual(attributes_by_name["datetime"], "2018-01-02T10:45:00Z") self.assertIsInstance(attributes_by_name["file"], str) self.assertEqual( - attributes_by_name["file"], "http://testserver/media/images/products/2018/01/sony-xa50ES.pdf" + attributes_by_name["file"], + "http://testserver/media/images/products/2018/01/sony-xa50ES.pdf", ) self.assertIsInstance(attributes_by_name["float"], float) self.assertEqual(attributes_by_name["float"], 3.2) @@ -247,7 +248,8 @@ def test_product_attributes(self): ) self.assertIsInstance(attributes_by_name["image"], str) self.assertEqual( - attributes_by_name["image"], "http://testserver/media/images/products/2018/01/IMG_3777.JPG" + attributes_by_name["image"], + "http://testserver/media/images/products/2018/01/IMG_3777.JPG", ) self.assertIsInstance(attributes_by_name["integer"], int) self.assertEqual(attributes_by_name["integer"], 7) diff --git a/setup.py b/setup.py index 4f5e62c0..7225e09f 100755 --- a/setup.py +++ b/setup.py @@ -1,6 +1,6 @@ from setuptools import find_packages, setup -__version__ = "3.2.4" +__version__ = "3.2.6" setup( # package name in pypi From 2a9eca51eca94a1675ec3af6bb9bb1945d45250c Mon Sep 17 00:00:00 2001 From: Lars van de Kerkhof Date: Wed, 20 Mar 2024 10:23:24 +0100 Subject: [PATCH 09/18] Fixes cyclic import error --- oscarapi/serializers/product.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oscarapi/serializers/product.py b/oscarapi/serializers/product.py index a7eb9b24..aaec8c0f 100644 --- a/oscarapi/serializers/product.py +++ b/oscarapi/serializers/product.py @@ -16,8 +16,8 @@ from oscarapi.utils.files import file_hash from oscarapi.utils.exists import find_existing_attribute_option_group from oscarapi.utils.accessors import getitems -from oscarapi.utils.attributes import AttributeConverter from oscarapi.serializers.fields import DrillDownHyperlinkedIdentityField +from oscarapi.utils.attributes import AttributeConverter from oscarapi.serializers.utils import ( OscarModelSerializer, OscarHyperlinkedModelSerializer, From d4f99944c9c22db7241b3581c0468f67510236a8 Mon Sep 17 00:00:00 2001 From: Craig Weber Date: Wed, 10 Apr 2024 15:18:32 -0400 Subject: [PATCH 10/18] Fixes handling of null product image attributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an image product attribute is `null`, the product API would throw the following exception: ``` ValueError: The 'value_image' attribute has no file associated with it. […] File "rest_framework/serializers.py", line 522, in to_representation ret[field.field_name] = field.to_representation(attribute) File "rest_framework/serializers.py", line 686, in to_representation return [ File "rest_framework/serializers.py", line 687, in self.child.to_representation(item) for item in iterable File "rest_framework/serializers.py", line 522, in to_representation ret[field.field_name] = field.to_representation(attribute) File "oscarapi/serializers/fields.py", line 227, in to_representation return value.value.url File "django/db/models/fields/files.py", line 66, in url self._require_file() File "django/db/models/fields/files.py", line 41, in _require_file raise ValueError( ``` This patch fixes this by checking if the image field has a value before trying to generate a URL. --- oscarapi/serializers/fields.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/oscarapi/serializers/fields.py b/oscarapi/serializers/fields.py index eabf21c7..b426811c 100644 --- a/oscarapi/serializers/fields.py +++ b/oscarapi/serializers/fields.py @@ -200,6 +200,8 @@ def to_representation(self, value): elif obj_type == value.attribute.MULTI_OPTION: return value.value.values_list("option", flat=True) elif obj_type in [value.attribute.FILE, value.attribute.IMAGE]: + if not value.value: + return None url = value.value.url request = self.context.get("request", None) if request is not None: From 410dd8c2238252493f86f73441349da6b6916c6c Mon Sep 17 00:00:00 2001 From: Viggo de Vries Date: Thu, 18 Apr 2024 13:23:12 +0200 Subject: [PATCH 11/18] Add category bulk admin api --- oscarapi/urls.py | 3 +++ oscarapi/utils/categories.py | 35 +++++++++++++++++++++++++++++++++ oscarapi/views/admin/product.py | 13 ++++++++++++ oscarapi/views/root.py | 1 + 4 files changed, 52 insertions(+) diff --git a/oscarapi/urls.py b/oscarapi/urls.py index 01f68868..cbf3b829 100644 --- a/oscarapi/urls.py +++ b/oscarapi/urls.py @@ -114,6 +114,7 @@ AttributeOptionGroupAdminDetail, CategoryAdminList, CategoryAdminDetail, + CategoryBulkAdminApi, ) = get_api_classes( "views.admin.product", [ @@ -127,6 +128,7 @@ "AttributeOptionGroupAdminDetail", "CategoryAdminList", "CategoryAdminDetail", + "CategoryBulkAdminApi", ], ) @@ -204,6 +206,7 @@ path("ranges//", RangeDetail.as_view(), name="range-detail"), path("categories/", CategoryList.as_view(), name="category-list"), path("categories//", CategoryDetail.as_view(), name="category-detail"), + path("categories-bulk/", CategoryBulkAdminApi.as_view(), name="admin-category-bulk"), re_path( "^categories/(?P.*)/$", CategoryList.as_view(), diff --git a/oscarapi/utils/categories.py b/oscarapi/utils/categories.py index 539b7d6c..8b30ca0a 100644 --- a/oscarapi/utils/categories.py +++ b/oscarapi/utils/categories.py @@ -71,3 +71,38 @@ def find_from_full_slug(breadcrumb_str, separator="/"): category_names = [x.strip() for x in breadcrumb_str.split(separator)] categories = create_from_sequence(category_names, False) return categories[-1] + + + +def upsert_categories(data, parent_category=None): + if parent_category is None: + # Starting from root, we want the first category in the root + sibling = Category.get_first_root_node() + else: + # We are further down the category tree, we want to get the first child from the parent + sibling = parent_category.get_first_child() + + for cat in data: + children = cat.pop("children", None) + + try: + category = Category.objects.get(code=cat["data"]["code"]) + except Category.DoesNotExist: + # Category with code does not exist, create it on the root + category = Category.add_root(**cat["data"]) + + if sibling is not None: + if category.pk != sibling.pk: + # Move the category to the right of the sibling + category.move(sibling, pos="right") + elif parent_category is not None: + if category.get_parent().pk != parent_category.pk: + # Move the category as the first child under the parent category since we have not sibling + category.move(parent_category, pos="first-child") + + # The category is now the sibling, new categories will be moved to the right of this category + sibling = category + + if children: + # Add children under this category + upsert_categories(children, parent_category=category) \ No newline at end of file diff --git a/oscarapi/views/admin/product.py b/oscarapi/views/admin/product.py index b54b39c3..ce64b029 100644 --- a/oscarapi/views/admin/product.py +++ b/oscarapi/views/admin/product.py @@ -2,10 +2,13 @@ from django.http import Http404 from rest_framework import generics from rest_framework.exceptions import NotFound +from rest_framework.views import APIView +from rest_framework.response import Response from oscar.core.loading import get_model from oscarapi.utils.loading import get_api_classes, get_api_class from oscarapi.utils.exists import construct_id_filter +from oscarapi.utils.categories import upsert_categories APIAdminPermission = get_api_class("permissions", "APIAdminPermission") ProductAttributeSerializer, AttributeOptionGroupSerializer = get_api_classes( @@ -140,3 +143,13 @@ class CategoryAdminDetail(generics.RetrieveUpdateDestroyAPIView): queryset = Category.objects.all() serializer_class = AdminCategorySerializer permission_classes = (APIAdminPermission,) + + +class CategoryBulkAdminApi(APIView): + def get(self, request, format=None): + return Response(Category.dump_bulk(keep_ids=False)) + + def post(self, request): + upsert_categories(request.data) + + return self.get(request) diff --git a/oscarapi/views/root.py b/oscarapi/views/root.py index ed8a6b1a..002e08d2 100644 --- a/oscarapi/views/root.py +++ b/oscarapi/views/root.py @@ -37,6 +37,7 @@ def ADMIN_APIS(r, f): ("productclasses", reverse("admin-productclass-list", request=r, format=f)), ("products", reverse("admin-product-list", request=r, format=f)), ("categories", reverse("admin-category-list", request=r, format=f)), + ("categories-bulk", reverse("admin-category-bulk", request=r, format=f)), ("orders", reverse("admin-order-list", request=r, format=f)), ("partners", reverse("admin-partner-list", request=r, format=f)), ("users", reverse("admin-user-list", request=r, format=f)), From 3b2238a843fad4612cf60e99f89cf8c2c67ad98e Mon Sep 17 00:00:00 2001 From: Viggo de Vries Date: Mon, 29 Apr 2024 16:04:12 +0200 Subject: [PATCH 12/18] Add tests to prove how it works --- oscarapi/tests/unit/testproduct.py | 281 +++++++++++++++++++++++++++++ oscarapi/tests/utils.py | 14 +- oscarapi/urls.py | 4 +- oscarapi/utils/categories.py | 44 ++++- oscarapi/views/admin/product.py | 9 +- 5 files changed, 337 insertions(+), 15 deletions(-) diff --git a/oscarapi/tests/unit/testproduct.py b/oscarapi/tests/unit/testproduct.py index 634e9dc4..23c3902a 100644 --- a/oscarapi/tests/unit/testproduct.py +++ b/oscarapi/tests/unit/testproduct.py @@ -2135,3 +2135,284 @@ def test_create_or_update_root_category(self): self.assertEqual(Category.objects.count(), 3) self.response.assertStatusEqual(201) self.assertEqual(self.response["description"], "Klakaa") + + +@skipIf(settings.OSCARAPI_BLOCK_ADMIN_API_ACCESS, "Admin API is not enabled") +class AdminBulkCategoryApiTest(APITest): + def test_create_category_tree(self): + Category.objects.all().delete() + data = [ + { + "data": {"code": "henk", "name": "Henk"}, + "children": [ + {"data": {"code": "klaas", "name": "Klaas"}}, + {"data": {"code": "klaas2", "name": "Klaas 2"}}, + ], + }, + { + "data": { + "code": "harrie", + "name": "Harrie", + "description": "Dit is de description", + } + }, + ] + + self.response = self.post("admin-category-bulk", manual_data=data) + + self.assertEqual(Category.objects.count(), 4) + + self.assertTrue(Category.objects.filter(name="Henk", code="henk").exists()) + + klaas2 = Category.objects.get(code="klaas2") + self.assertEqual(klaas2.get_parent().code, "henk") + self.assertEqual(klaas2.depth, 2) + self.assertEqual( + Category.dump_bulk(), + [ + { + "data": { + "name": "Henk", + "code": "henk", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "henk", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 1, + "children": [ + { + "data": { + "name": "Klaas", + "code": "klaas", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 2, + }, + { + "data": { + "name": "Klaas 2", + "code": "klaas2", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas-2", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 3, + }, + ], + }, + { + "data": { + "name": "Harrie", + "code": "harrie", + "description": "Dit is de description", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "harrie", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 4, + }, + ], + ) + + data = [ + { + "data": {"code": "henk", "name": "Henk nieuw"}, + "children": [ + {"data": {"code": "klaas", "name": "Klaas"}}, + ], + }, + { + "data": { + "code": "harrie", + "name": "Harrie", + "description": "Dit is de description", + } + }, + {"data": {"code": "klaas2", "name": "Klaas 2"}}, + ] + + self.response = self.post("admin-category-bulk", manual_data=data) + + self.assertTrue( + Category.objects.filter(name="Henk nieuw", code="henk").exists() + ) + + self.assertEqual(Category.objects.count(), 4) + + klaas2.refresh_from_db() + self.assertEqual(klaas2.get_parent(), None) + self.assertEqual(klaas2.depth, 1) + + self.assertEqual( + Category.dump_bulk(), + [ + { + "data": { + "name": "Henk nieuw", + "code": "henk", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "henk", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 1, + "children": [ + { + "data": { + "name": "Klaas", + "code": "klaas", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 2, + } + ], + }, + { + "data": { + "name": "Harrie", + "code": "harrie", + "description": "Dit is de description", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "harrie", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 4, + }, + { + "data": { + "name": "Klaas 2", + "code": "klaas2", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas-2", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 3, + }, + ], + ) + + data = [ + { + "data": { + "code": "harrie", + "name": "Harrie", + "description": "Dit is de description", + } + }, + { + "data": {"code": "henk", "name": "Henk nieuw"}, + "children": [ + {"data": {"code": "klaas", "name": "Klaas"}}, + {"data": {"code": "klaas2", "name": "Klaas 2"}}, + ], + }, + ] + + self.response = self.post("admin-category-bulk", manual_data=data) + + self.assertTrue( + Category.objects.filter(name="Henk nieuw", code="henk").exists() + ) + + self.assertEqual(Category.objects.count(), 4) + + klaas2.refresh_from_db() + self.assertEqual(klaas2.get_parent().code, "henk") + self.assertEqual(klaas2.depth, 2) + + self.assertEqual( + Category.dump_bulk(), + [ + { + "data": { + "name": "Harrie", + "code": "harrie", + "description": "Dit is de description", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "harrie", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 4, + }, + { + "data": { + "name": "Henk nieuw", + "code": "henk", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "henk", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 1, + "children": [ + { + "data": { + "name": "Klaas", + "code": "klaas", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 2, + }, + { + "data": { + "name": "Klaas 2", + "code": "klaas2", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas-2", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 3, + }, + ], + }, + ], + ) diff --git a/oscarapi/tests/utils.py b/oscarapi/tests/utils.py index a275f290..a8712070 100644 --- a/oscarapi/tests/utils.py +++ b/oscarapi/tests/utils.py @@ -53,7 +53,15 @@ def hlogin(self, username, password, session_id): ) return True - def api_call(self, url_name, method, session_id=None, authenticated=False, **data): + def api_call( + self, + url_name, + method, + session_id=None, + authenticated=False, + manual_data=None, + **data + ): try: url = reverse(url_name) except NoReverseMatch: @@ -65,7 +73,9 @@ def api_call(self, url_name, method, session_id=None, authenticated=False, **dat kwargs["HTTP_SESSION_ID"] = "SID:%s:testserver:%s" % (auth_type, session_id) response = None - if data: + if manual_data is not None: + response = method(url, json.dumps(manual_data), **kwargs) + elif data: response = method(url, json.dumps(data), **kwargs) else: response = method(url, **kwargs) diff --git a/oscarapi/urls.py b/oscarapi/urls.py index cbf3b829..084843ea 100644 --- a/oscarapi/urls.py +++ b/oscarapi/urls.py @@ -206,7 +206,9 @@ path("ranges//", RangeDetail.as_view(), name="range-detail"), path("categories/", CategoryList.as_view(), name="category-list"), path("categories//", CategoryDetail.as_view(), name="category-detail"), - path("categories-bulk/", CategoryBulkAdminApi.as_view(), name="admin-category-bulk"), + path( + "categories-bulk/", CategoryBulkAdminApi.as_view(), name="admin-category-bulk" + ), re_path( "^categories/(?P.*)/$", CategoryList.as_view(), diff --git a/oscarapi/utils/categories.py b/oscarapi/utils/categories.py index 8b30ca0a..8fc81f21 100644 --- a/oscarapi/utils/categories.py +++ b/oscarapi/utils/categories.py @@ -73,8 +73,14 @@ def find_from_full_slug(breadcrumb_str, separator="/"): return categories[-1] +def upsert_categories(data): + categories_to_update, fields_to_update = _upsert_categories(data) -def upsert_categories(data, parent_category=None): + if categories_to_update and fields_to_update: + Category.objects.bulk_update(categories_to_update, fields_to_update) + + +def _upsert_categories(data, parent_category=None): if parent_category is None: # Starting from root, we want the first category in the root sibling = Category.get_first_root_node() @@ -82,27 +88,51 @@ def upsert_categories(data, parent_category=None): # We are further down the category tree, we want to get the first child from the parent sibling = parent_category.get_first_child() + categories_to_update = [] + category_fields_to_update = set() + for cat in data: children = cat.pop("children", None) - + try: category = Category.objects.get(code=cat["data"]["code"]) + + for key, value in cat["data"].items(): + setattr(category, key, value) + category_fields_to_update.add(key) + + categories_to_update.append(category) except Category.DoesNotExist: - # Category with code does not exist, create it on the root - category = Category.add_root(**cat["data"]) + # Category with code does not exist, create it on the root or under the parent + if parent_category: + category = parent_category.add_child(**cat["data"]) + else: + category = Category.add_root(**cat["data"]) if sibling is not None: + if category.pk != sibling.pk: # Move the category to the right of the sibling category.move(sibling, pos="right") elif parent_category is not None: - if category.get_parent().pk != parent_category.pk: + get_parent = category.get_parent() + if (get_parent is None and parent_category is not None) or ( + get_parent.pk != parent_category.pk + ): # Move the category as the first child under the parent category since we have not sibling category.move(parent_category, pos="first-child") + # category.save() # The category is now the sibling, new categories will be moved to the right of this category sibling = category - + if children: # Add children under this category - upsert_categories(children, parent_category=category) \ No newline at end of file + _categories_to_update, _category_fields_to_update = _upsert_categories( + children, parent_category=category + ) + categories_to_update.extend(_categories_to_update) + if _category_fields_to_update: + category_fields_to_update.update(_category_fields_to_update) + + return categories_to_update, category_fields_to_update diff --git a/oscarapi/views/admin/product.py b/oscarapi/views/admin/product.py index ce64b029..d1926516 100644 --- a/oscarapi/views/admin/product.py +++ b/oscarapi/views/admin/product.py @@ -143,13 +143,12 @@ class CategoryAdminDetail(generics.RetrieveUpdateDestroyAPIView): queryset = Category.objects.all() serializer_class = AdminCategorySerializer permission_classes = (APIAdminPermission,) - - + + class CategoryBulkAdminApi(APIView): - def get(self, request, format=None): + def get(self, request, *args, **kwargs): return Response(Category.dump_bulk(keep_ids=False)) - + def post(self, request): upsert_categories(request.data) - return self.get(request) From 236f6d5b40722864fcf0e61e90a81ec283c47473 Mon Sep 17 00:00:00 2001 From: Viggo de Vries Date: Mon, 29 Apr 2024 16:05:59 +0200 Subject: [PATCH 13/18] wops --- oscarapi/utils/categories.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/oscarapi/utils/categories.py b/oscarapi/utils/categories.py index 8fc81f21..1485b7a6 100644 --- a/oscarapi/utils/categories.py +++ b/oscarapi/utils/categories.py @@ -110,7 +110,6 @@ def _upsert_categories(data, parent_category=None): category = Category.add_root(**cat["data"]) if sibling is not None: - if category.pk != sibling.pk: # Move the category to the right of the sibling category.move(sibling, pos="right") @@ -121,7 +120,6 @@ def _upsert_categories(data, parent_category=None): ): # Move the category as the first child under the parent category since we have not sibling category.move(parent_category, pos="first-child") - # category.save() # The category is now the sibling, new categories will be moved to the right of this category sibling = category From 94b66d30e05b0594ae017445881823cb4ad23a70 Mon Sep 17 00:00:00 2001 From: Lars van de Kerkhof Date: Wed, 15 May 2024 14:32:13 +0200 Subject: [PATCH 14/18] Added Jenkinsfile --- Jenkinsfile | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 Jenkinsfile diff --git a/Jenkinsfile b/Jenkinsfile new file mode 100644 index 00000000..fb38bc40 --- /dev/null +++ b/Jenkinsfile @@ -0,0 +1,70 @@ +#!/usr/bin/env groovy + +pipeline { + agent { label 'GEITENPETRA' } + options { disableConcurrentBuilds() } + + stages { + stage('Build') { + steps { + withPythonEnv('System-CPython-3.10') { + withEnv(['PIP_INDEX_URL=PIP_INDEX_URL=https://pypi.uwkm.nl/voxyan/testing/+simple/']) { + pysh "make install" + } + } + } + } + stage('Lint') { + steps { + withPythonEnv('System-CPython-3.10') { + pysh "make lint" + } + } + } + stage('Test') { + steps { + withPythonEnv('System-CPython-3.10') { + pysh "make test" + } + } + post { + always { + junit allowEmptyResults: true, testResults: '**/nosetests.xml' + } + success { + step([ + $class: 'CoberturaPublisher', + coberturaReportFile: '**/coverage.xml', + ]) + } + } + } + } + post { + always { + echo 'This will always run' + } + success { + echo 'This will run only if successful' + withPythonEnv('System-CPython-3.10') { + echo 'This will run only if successful' + pysh "version --plugin=wheel -B${env.BUILD_NUMBER} --skip-build" + sh "which git" + sh "git push --tags" + } + } + failure { + emailext subject: "JENKINS-NOTIFICATION: ${currentBuild.currentResult}: Job '${env.JOB_NAME} #${env.BUILD_NUMBER}'", + body: '${SCRIPT, template="groovy-text.template"}', + recipientProviders: [culprits(), brokenBuildSuspects(), brokenTestsSuspects()] + + } + unstable { + echo 'This will run only if the run was marked as unstable' + } + changed { + echo 'This will run only if the state of the Pipeline has changed' + echo 'For example, if the Pipeline was previously failing but is now successful' + } + } +} From 65ae98ff6de083e2c1703385b8273d34f1dce852 Mon Sep 17 00:00:00 2001 From: Lars van de Kerkhof Date: Wed, 15 May 2024 14:34:25 +0200 Subject: [PATCH 15/18] hehe --- Jenkinsfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Jenkinsfile b/Jenkinsfile index fb38bc40..8fda4900 100644 --- a/Jenkinsfile +++ b/Jenkinsfile @@ -8,7 +8,7 @@ pipeline { stage('Build') { steps { withPythonEnv('System-CPython-3.10') { - withEnv(['PIP_INDEX_URL=PIP_INDEX_URL=https://pypi.uwkm.nl/voxyan/testing/+simple/']) { + withEnv(['PIP_INDEX_URL=https://pypi.uwkm.nl/voxyan/testing/+simple/']) { pysh "make install" } } From e665d2df6964f1c9ac5e49c59c3bd5da14a92de9 Mon Sep 17 00:00:00 2001 From: Lars van de Kerkhof Date: Wed, 15 May 2024 17:03:13 +0200 Subject: [PATCH 16/18] set to next release version --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 7225e09f..57f76902 100755 --- a/setup.py +++ b/setup.py @@ -1,6 +1,6 @@ from setuptools import find_packages, setup -__version__ = "3.2.6" +__version__ = "3.2.7" setup( # package name in pypi From 01051070211a09b8118c44ccd9ca415517d1f8e6 Mon Sep 17 00:00:00 2001 From: Voxin Muyli Date: Thu, 16 May 2024 08:10:41 +0200 Subject: [PATCH 17/18] Setup local CI (#348) --- Jenkinsfile | 4 ++-- Makefile | 10 +++------- oscarapi/utils/models.py | 5 +---- setup.py | 13 ++++++++++--- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/Jenkinsfile b/Jenkinsfile index 8fda4900..934ee5f0 100644 --- a/Jenkinsfile +++ b/Jenkinsfile @@ -8,7 +8,7 @@ pipeline { stage('Build') { steps { withPythonEnv('System-CPython-3.10') { - withEnv(['PIP_INDEX_URL=https://pypi.uwkm.nl/voxyan/testing/+simple/']) { + withEnv(['PIP_INDEX_URL=https://pypi.uwkm.nl/voxyan/oscar/+simple/']) { pysh "make install" } } @@ -24,7 +24,7 @@ pipeline { stage('Test') { steps { withPythonEnv('System-CPython-3.10') { - pysh "make test" + pysh "make coverage" } } post { diff --git a/Makefile b/Makefile index fa5a2a65..d6b47ce6 100644 --- a/Makefile +++ b/Makefile @@ -8,7 +8,7 @@ clean: rm -Rf build/ install: - pip install -e .[dev] + pip install -e .[dev] --upgrade --upgrade-strategy=eager --pre sandbox: install python sandbox/manage.py migrate @@ -43,15 +43,11 @@ publish_release_testpypi: build_release publish_release: build_release twine upload dist/* -lint.installed: - pip install -e .[lint] - touch $@ - -lint: lint.installed +lint: black --check --exclude "migrations/*" oscarapi/ pylint setup.py oscarapi/ -black: lint.installed +black: black --exclude "/migrations/" oscarapi/ uwsgi: diff --git a/oscarapi/utils/models.py b/oscarapi/utils/models.py index 22c6d120..2b96ff70 100644 --- a/oscarapi/utils/models.py +++ b/oscarapi/utils/models.py @@ -1,9 +1,6 @@ from contextlib import contextmanager -try: - from unittest.mock import patch -except ImportError: - from mock import patch +from unittest.mock import patch @contextmanager diff --git a/setup.py b/setup.py index 57f76902..6c118a1f 100755 --- a/setup.py +++ b/setup.py @@ -50,11 +50,18 @@ "setuptools", "django-oscar>=3.2", "Django>=3.2", - "djangorestframework>=3.9" + "djangorestframework>=3.9", ], # mark test target to require extras. extras_require={ - "dev": ["coverage", "mock", "twine", "wheel", "easy_thumbnails"], - "lint": ["pylint", "pylint-django", "black>=23.1.0"], + "dev": [ + "coverage", + "wheel", + "easy_thumbnails", + "vdt.versionplugin.wheel", + "pylint", + "pylint-django", + "black>=23.1.0", + ], }, ) From cdab4d27e649ec6550f4833d65d7f5bdbdf730e2 Mon Sep 17 00:00:00 2001 From: Viggo de Vries Date: Tue, 17 Sep 2024 13:35:54 +0200 Subject: [PATCH 18/18] Improve category upsert and add test (#352) * Doe some updating and add test * refrhes_from_db only path * Add transaction that rollsback on error * update versions --- .github/workflows/ci.yml | 10 +- oscarapi/tests/unit/testproduct.py | 588 +++++++++++++++++++++++++++++ oscarapi/utils/categories.py | 23 +- 3 files changed, 608 insertions(+), 13 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 62adcda0..f5d208d6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,11 +10,11 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout the repository - uses: actions/checkout@v2 + uses: actions/checkout@v4 with: path: ./src - name: Setup Python 3.11 - uses: actions/setup-python@v2 + uses: actions/setup-python@v5 with: python-version: '3.11' - name: Install all dependencies @@ -22,7 +22,7 @@ jobs: - name: Run all linting run: make lint - name: Upload src dir as artefact - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v4 with: name: src path: ./src @@ -38,12 +38,12 @@ jobs: oscar-version: ["3.2"] steps: - name: Download src dir - uses: actions/download-artifact@v2 + uses: actions/download-artifact@v4 with: name: src path: ./src - name: Setup Python 3.x - uses: actions/setup-python@v2 + uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} - name: Install all dependencies diff --git a/oscarapi/tests/unit/testproduct.py b/oscarapi/tests/unit/testproduct.py index 23c3902a..caa3fb29 100644 --- a/oscarapi/tests/unit/testproduct.py +++ b/oscarapi/tests/unit/testproduct.py @@ -2416,3 +2416,591 @@ def test_create_category_tree(self): }, ], ) + + def test_crazy_stuff(self): + Category.objects.all().delete() + data = [ + { + "data": {"code": "henk", "name": "Henk"}, + "children": [ + {"data": {"code": "klaas", "name": "Klaas"}}, + {"data": {"code": "klaas2", "name": "Klaas 2"}}, + {"data": {"code": "klaas3", "name": "Klaas 3"}}, + {"data": {"code": "klaas4", "name": "Klaas 4"}}, + {"data": {"code": "klaas5", "name": "Klaas 5"}}, + {"data": {"code": "klaas6", "name": "Klaas 6"}}, + ], + }, + { + "data": { + "code": "harrie", + "name": "Harrie", + "description": "Dit is de description", + }, + "children": [ + {"data": {"code": "koe", "name": "Koe"}}, + {"data": {"code": "koe2", "name": "Koe 2"}}, + {"data": {"code": "koe3", "name": "Koe 3"}}, + {"data": {"code": "koe4", "name": "Koe 4"}}, + {"data": {"code": "koe5", "name": "Koe 5"}}, + {"data": {"code": "koe6", "name": "Koe 6"}}, + ], + }, + ] + + self.response = self.post("admin-category-bulk", manual_data=data) + + self.assertEqual(Category.objects.count(), 14) + + self.assertEqual( + Category.dump_bulk(), + [ + { + "data": { + "name": "Henk", + "code": "henk", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "henk", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 1, + "children": [ + { + "data": { + "name": "Klaas", + "code": "klaas", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 2, + }, + { + "data": { + "name": "Klaas 2", + "code": "klaas2", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas-2", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 3, + }, + { + "data": { + "name": "Klaas 3", + "code": "klaas3", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas-3", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 4, + }, + { + "data": { + "name": "Klaas 4", + "code": "klaas4", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas-4", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 5, + }, + { + "data": { + "name": "Klaas 5", + "code": "klaas5", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas-5", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 6, + }, + { + "data": { + "name": "Klaas 6", + "code": "klaas6", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas-6", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 7, + }, + ], + }, + { + "data": { + "name": "Harrie", + "code": "harrie", + "description": "Dit is de description", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "harrie", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 8, + "children": [ + { + "data": { + "name": "Koe", + "code": "koe", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "koe", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 9, + }, + { + "data": { + "name": "Koe 2", + "code": "koe2", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "koe-2", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 10, + }, + { + "data": { + "name": "Koe 3", + "code": "koe3", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "koe-3", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 11, + }, + { + "data": { + "name": "Koe 4", + "code": "koe4", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "koe-4", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 12, + }, + { + "data": { + "name": "Koe 5", + "code": "koe5", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "koe-5", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 13, + }, + { + "data": { + "name": "Koe 6", + "code": "koe6", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "koe-6", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 14, + }, + ], + }, + ], + ) + + data = [ + { + "data": {"code": "nieuwe1", "name": "Nieuwe 1"}, + "children": [ + {"data": {"code": "klaas", "name": "Klaas"}}, + {"data": {"code": "klaas2", "name": "Klaas 2"}}, + ], + }, + { + "data": {"code": "nieuwe2", "name": "Nieuwe 2"}, + "children": [ + {"data": {"code": "klaas3", "name": "Klaas 3"}}, + {"data": {"code": "klaas4", "name": "Klaas 4"}}, + ], + }, + { + "data": {"code": "nieuwe3", "name": "Nieuwe 3"}, + "children": [ + {"data": {"code": "klaas5", "name": "Klaas 5"}}, + {"data": {"code": "klaas6", "name": "Klaas 6"}}, + ], + }, + { + "data": {"code": "gekke1", "name": "Gekke 1"}, + "children": [ + {"data": {"code": "koe", "name": "Koe"}}, + {"data": {"code": "koe2", "name": "Koe 2"}}, + ], + }, + { + "data": {"code": "gekke2", "name": "Gekke 2"}, + "children": [ + {"data": {"code": "koe3", "name": "Koe 3"}}, + {"data": {"code": "koe4", "name": "Koe 4"}}, + ], + }, + { + "data": {"code": "gekke3", "name": "Gekke 3"}, + "children": [ + {"data": {"code": "koe5", "name": "Koe 5"}}, + {"data": {"code": "koe6", "name": "Koe 6"}}, + ], + }, + ] + + self.response = self.post("admin-category-bulk", manual_data=data) + + self.assertEqual(Category.objects.count(), 20) + + self.assertEqual( + Category.dump_bulk(), + [ + { + "data": { + "name": "Henk", + "code": "henk", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "henk", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 1, + }, + { + "data": { + "name": "Harrie", + "code": "harrie", + "description": "Dit is de description", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "harrie", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 8, + }, + { + "data": { + "name": "Nieuwe 1", + "code": "nieuwe1", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "nieuwe-1", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 15, + "children": [ + { + "data": { + "name": "Klaas", + "code": "klaas", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 2, + }, + { + "data": { + "name": "Klaas 2", + "code": "klaas2", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas-2", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 3, + }, + ], + }, + { + "data": { + "name": "Nieuwe 2", + "code": "nieuwe2", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "nieuwe-2", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 16, + "children": [ + { + "data": { + "name": "Klaas 3", + "code": "klaas3", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas-3", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 4, + }, + { + "data": { + "name": "Klaas 4", + "code": "klaas4", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas-4", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 5, + }, + ], + }, + { + "data": { + "name": "Nieuwe 3", + "code": "nieuwe3", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "nieuwe-3", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 17, + "children": [ + { + "data": { + "name": "Klaas 5", + "code": "klaas5", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas-5", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 6, + }, + { + "data": { + "name": "Klaas 6", + "code": "klaas6", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "klaas-6", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 7, + }, + ], + }, + { + "data": { + "name": "Gekke 1", + "code": "gekke1", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "gekke-1", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 18, + "children": [ + { + "data": { + "name": "Koe", + "code": "koe", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "koe", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 9, + }, + { + "data": { + "name": "Koe 2", + "code": "koe2", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "koe-2", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 10, + }, + ], + }, + { + "data": { + "name": "Gekke 2", + "code": "gekke2", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "gekke-2", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 19, + "children": [ + { + "data": { + "name": "Koe 3", + "code": "koe3", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "koe-3", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 11, + }, + { + "data": { + "name": "Koe 4", + "code": "koe4", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "koe-4", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 12, + }, + ], + }, + { + "data": { + "name": "Gekke 3", + "code": "gekke3", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "gekke-3", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 20, + "children": [ + { + "data": { + "name": "Koe 5", + "code": "koe5", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "koe-5", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 13, + }, + { + "data": { + "name": "Koe 6", + "code": "koe6", + "description": "", + "meta_title": None, + "meta_description": None, + "image": "", + "slug": "koe-6", + "is_public": True, + "ancestors_are_public": True, + }, + "id": 14, + }, + ], + }, + ], + ) diff --git a/oscarapi/utils/categories.py b/oscarapi/utils/categories.py index 1485b7a6..cca77e57 100644 --- a/oscarapi/utils/categories.py +++ b/oscarapi/utils/categories.py @@ -1,4 +1,5 @@ from django.utils.translation import gettext as _ +from django.db import transaction from rest_framework.exceptions import NotFound @@ -74,19 +75,22 @@ def find_from_full_slug(breadcrumb_str, separator="/"): def upsert_categories(data): - categories_to_update, fields_to_update = _upsert_categories(data) + with transaction.atomic(): + categories_to_update, fields_to_update = _upsert_categories(data) - if categories_to_update and fields_to_update: - Category.objects.bulk_update(categories_to_update, fields_to_update) + if categories_to_update and fields_to_update: + Category.objects.bulk_update(categories_to_update, fields_to_update) + + Category.fix_tree() def _upsert_categories(data, parent_category=None): if parent_category is None: - # Starting from root, we want the first category in the root - sibling = Category.get_first_root_node() + # Get the last category in the root + sibling = Category.get_last_root_node() else: - # We are further down the category tree, we want to get the first child from the parent - sibling = parent_category.get_first_child() + # Set sibling to None if there is a parent category, we want the first category in the data to be added as a first-child of the parent + sibling = None categories_to_update = [] category_fields_to_update = set() @@ -118,9 +122,12 @@ def _upsert_categories(data, parent_category=None): if (get_parent is None and parent_category is not None) or ( get_parent.pk != parent_category.pk ): - # Move the category as the first child under the parent category since we have not sibling + # Move the category as the first child under the parent category since we have no sibling category.move(parent_category, pos="first-child") + # Update the new path from the database after moving the category to it's new home + category.refresh_from_db(fields=["path"]) + # The category is now the sibling, new categories will be moved to the right of this category sibling = category