Make sure to only save attributes when they have been changed. (#3995)

* Make sure to only save attributes when they have been saved.

This makes sure attributes on the parent are not needlessly saved to the child.

* Renamed testcase

* Added tests for child product attributes not copying parent attributes

* Make ProductAttributesContainer even more lazy and initialize only when an attribute is being read.

* Changed docstring

src/oscar/apps/catalogue/abstract_models.py

@@ -16,7 +16,7 @@ from django.db.models.fields import Field
 from django.db.models.lookups import StartsWith
 from django.template.defaultfilters import striptags
 from django.urls import reverse
-from django.utils.functional import SimpleLazyObject, cached_property
+from django.utils.functional import cached_property
 from django.utils.html import strip_tags
 from django.utils.safestring import mark_safe
 from django.utils.translation import get_language
@@ -445,7 +445,7 @@ class AbstractProduct(models.Model):
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
-        self.attr = SimpleLazyObject(lambda: ProductAttributesContainer(product=self))
+        self.attr = ProductAttributesContainer(product=self)
 
     def __str__(self):
         if self.title:

src/oscar/apps/catalogue/product_attributes.py

@@ -1,4 +1,4 @@
-from django.core.exceptions import ValidationError
+from django.core.exceptions import ObjectDoesNotExist, ValidationError
 from django.utils.translation import gettext_lazy as _
 
 
@@ -19,14 +19,35 @@ class ProductAttributesContainer:
         self.__dict__ = state
 
     def __init__(self, product):
-        self.product = product
-        self.refresh()
+        self._product = product
+        self._initialized = False
+
+    @property
+    def product(self):
+        return self._product
+
+    def initialize(self):
+        self._initialized = True
+        # initialize should not overwrite any values that have allready been set
+        attrs = self.__dict__
+        for v in self.get_values().select_related('attribute'):
+            attrs.setdefault(v.attribute.code, v.value)
 
     def refresh(self):
-        values = self.get_values().select_related('attribute')
-        for v in values:
+        for v in self.get_values().select_related('attribute'):
             setattr(self, v.attribute.code, v.value)
 
+    def __getattribute__(self, name):
+        try:
+            return super().__getattribute__(name)
+        except AttributeError:
+            if self._initialized:
+                raise
+            else:
+                self.initialize()
+                # try again, whever happens then will just be the result.
+                return super().__getattribute__(name)
+
     def __getattr__(self, name):
         raise AttributeError(
             _("%(obj)s has no attribute named '%(attr)s'") % {'obj': self.product.get_product_class(), 'attr': name})
@@ -66,4 +87,15 @@ class ProductAttributesContainer:
         for attribute in self.get_all_attributes():
             if hasattr(self, attribute.code):
                 value = getattr(self, attribute.code)
+                # Make sure that if a value comes from a parent product, it is not
+                # copied to the child, we do this by checking if a value has been
+                # changed, which would not be the case if the value comes from the
+                # parent.
+                try:
+                    attribute_value_current = self.get_value_by_attribute(attribute)
+                    if attribute_value_current.value == value:
+                        continue  # no new value needs to be saved
+                except ObjectDoesNotExist:
+                    pass  # there is no existing value, so a value needs to be saved.
+
                 attribute.save_value(self.product, value)

tests/unit/catalogue/test_product_attributes.py

@@ -0,0 +1,258 @@
+from django.core.exceptions import ValidationError
+from django.test import TestCase
+
+from oscar.core.loading import get_model
+from oscar.test.factories import (
+    ProductAttributeFactory, ProductClassFactory, ProductFactory)
+
+Product = get_model("catalogue", "Product")
+ProductAttribute = get_model("catalogue", "ProductAttribute")
+ProductAttributeValue = get_model("catalogue", "ProductAttributeValue")
+
+
+class ProductAttributeTest(TestCase):
+
+    def setUp(self):
+        super().setUp()
+
+        # setup the productclass
+        self.product_class = product_class = ProductClassFactory(name='Cows', slug='cows')
+        self.name_attr = ProductAttributeFactory(
+            type=ProductAttribute.TEXT, product_class=product_class, name="name", code="name")
+        self.weight_attrs = ProductAttributeFactory(
+            type=ProductAttribute.INTEGER, name="weight", code="weight", product_class=product_class)
+
+        # create the parent product
+        self.product = product = ProductFactory(
+            title="I am your father", stockrecords=None, product_class=product_class, structure="parent", upc="1234")
+        product.attr.weight = 3
+        product.full_clean()
+        product.save()
+
+        # create the child product
+        self.child_product = ProductFactory(
+            parent=product, structure="child", categories=None, product_class=None,
+            title="You are my father", upc="child-1234")
+        self.child_product.full_clean()
+
+    def test_update_child_with_attributes(self):
+        "Attributes preseent on the parent should not be copied to the child "
+        "when title of the child is modified"
+        self.assertEqual(
+            ProductAttributeValue.objects.filter(product_id=self.product.pk).count(),
+            1,
+            "The parent has 1 attributes",
+        )
+
+        # establish baseline
+        self.assertEqual(
+            ProductAttributeValue.objects.filter(product=self.child_product).count(),
+            0,
+            "The child has no attributes",
+        )
+        self.assertEqual(self.child_product.parent_id, self.product.pk)
+        self.assertIsNone(self.child_product.product_class)
+        self.assertEqual(self.child_product.upc, "child-1234")
+        self.assertEqual(self.child_product.slug, "you-are-my-father")
+        self.assertNotEqual(self.child_product.title, "Klaas is my real father")
+
+        self.child_product.title = "Klaas is my real father"
+        self.child_product.save()
+
+        self.child_product.refresh_from_db()
+        self.assertEqual(self.child_product.title, "Klaas is my real father")
+        self.assertEqual(
+            ProductAttributeValue.objects.filter(product=self.child_product).count(),
+            0,
+            "The child has no attributes",
+        )
+
+    def test_update_child_attributes(self):
+        "Attributes preseent on the parent should not be copied to the child "
+        "when the child attributes are modified"
+        self.assertEqual(
+            ProductAttributeValue.objects.filter(product_id=self.product.pk).count(),
+            1,
+            "The parent has 1 attributes",
+        )
+
+        # establish baseline
+        self.assertEqual(
+            ProductAttributeValue.objects.filter(product=self.child_product).count(),
+            0,
+            "The child has no attributes",
+        )
+        self.assertEqual(self.child_product.parent_id, self.product.pk)
+        self.assertIsNone(self.child_product.product_class)
+        self.assertEqual(self.child_product.upc, "child-1234")
+        self.assertEqual(self.child_product.slug, "you-are-my-father")
+        self.assertNotEqual(self.child_product.title, "Klaas is my real father")
+
+        self.child_product.title = "Klaas is my real father"
+        self.child_product.attr.name = "Berta"
+        self.child_product.save()
+
+        self.child_product.refresh_from_db()
+        self.assertEqual(self.child_product.title, "Klaas is my real father")
+        self.assertEqual(
+            ProductAttributeValue.objects.filter(product=self.child_product).count(),
+            1,
+            "The child now has 1 attribute",
+        )
+
+    def test_update_attributes_to_parent_and_child(self):
+        "Attributes present on the parent should not be copied to the child "
+        "ever, not even newly added attributes"
+        self.assertEqual(
+            ProductAttributeValue.objects.filter(product_id=self.product.pk).count(),
+            1,
+            "The parent has 1 attributes",
+        )
+        self.product.attr.name = "Greta"
+        self.product.save()
+        self.product.refresh_from_db()
+        self.product.attr.refresh()
+
+        self.assertEqual(
+            ProductAttributeValue.objects.filter(product_id=self.product.pk).count(),
+            2,
+            "The parent now has 2 attributes",
+        )
+
+        # establish baseline
+        self.assertEqual(
+            ProductAttributeValue.objects.filter(product=self.child_product).count(),
+            0,
+            "The child has no attributes",
+        )
+        self.assertEqual(self.child_product.parent_id, self.product.pk)
+        self.assertIsNone(self.child_product.product_class)
+        self.assertEqual(self.child_product.upc, "child-1234")
+        self.assertEqual(self.child_product.slug, "you-are-my-father")
+        self.assertNotEqual(self.child_product.title, "Klaas is my real father")
+
+        self.child_product.title = "Klaas is my real father"
+        self.child_product.attr.name = "Berta"
+        self.child_product.save()
+
+        self.child_product.refresh_from_db()
+        self.assertEqual(self.child_product.title, "Klaas is my real father")
+        self.assertEqual(
+            ProductAttributeValue.objects.filter(product=self.child_product).count(),
+            1,
+            "The child now has 1 attribute",
+        )
+
+
+class ProductAttributeQuerysetTest(TestCase):
+    fixtures = ["productattributes"]
+
+    def test_query_multiple_producttypes(self):
+        "We should be able to query over multiple product classes"
+        result = Product.objects.filter_by_attributes(henkie="bah bah")
+        self.assertEqual(result.count(), 2)
+        result1, result2 = list(result)
+
+        self.assertNotEqual(result1.product_class, result2.product_class)
+        self.assertEqual(result1.attr.henkie, result2.attr.henkie)
+
+    def test_further_filtering(self):
+        "The returned queryset should be ready for further filtering"
+        result = Product.objects.filter_by_attributes(henkie="bah bah")
+        photo = result.filter(title__contains="Photo")
+        self.assertEqual(photo.count(), 1)
+
+    def test_empty_results(self):
+        "Empty results are possible without errors"
+        result = Product.objects.filter_by_attributes(doesnotexist=True)
+        self.assertFalse(result.exists(), "querying with bulshit attributes should give no results")
+        result = Product.objects.filter_by_attributes(henkie="zulthoofd")
+        self.assertFalse(result.exists(), "querying with non existing values should give no results")
+        result = Product.objects.filter_by_attributes(henkie=True)
+        self.assertFalse(result.exists(), "querying with wring value type should give no results")
+
+    def test_text_value(self):
+        result = Product.objects.filter_by_attributes(subtitle="superhenk")
+        self.assertTrue(result.exists())
+        result = Product.objects.filter_by_attributes(subtitle="kekjo")
+        self.assertTrue(result.exists())
+        result = Product.objects.filter_by_attributes(subtitle=True)
+        self.assertFalse(result.exists())
+
+    def test_formatted_text(self):
+        html = "<p style=\"margin: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Helvetica;\">Vivamus auctor leo vel dui. Aliquam erat volutpat. Phasellus nibh. Vestibulum ante ipsum primis in faucibus orci luctus et ultrices posuere cubilia Curae; Cras tempor. Morbi egestas, <em>urna</em> non consequat tempus, <strong>nunc</strong> arcu mollis enim, eu aliquam erat nulla non nibh. Duis consectetuer malesuada velit. Nam ante nulla, interdum vel, tristique ac, condimentum non, tellus. Proin ornare feugiat nisl. Suspendisse dolor nisl, ultrices at, eleifend vel, consequat at, dolor.</p>"  # noqa
+        result = Product.objects.filter_by_attributes(additional_info=html)
+        self.assertTrue(result.exists())
+
+    def test_boolean(self):
+        result = Product.objects.filter_by_attributes(available=True)
+        self.assertTrue(result.exists())
+        result = Product.objects.filter_by_attributes(available=0)
+        self.assertTrue(result.exists())
+        with self.assertRaises(ValidationError):
+            result = Product.objects.filter_by_attributes(available="henk")
+
+    def test_number(self):
+        result = Product.objects.filter_by_attributes(facets=4)
+        self.assertTrue(result.exists())
+        with self.assertRaises(ValueError):
+            result = Product.objects.filter_by_attributes(facets="four")
+
+        result = Product.objects.filter_by_attributes(facets=1)
+        self.assertFalse(result.exists())
+
+    def test_float(self):
+        result = Product.objects.filter_by_attributes(hypothenusa=1.25)
+        self.assertTrue(result.exists())
+        with self.assertRaises(ValueError):
+            result = Product.objects.filter_by_attributes(facets="four")
+
+        result = Product.objects.filter_by_attributes(hypothenusa=1)
+        self.assertFalse(result.exists())
+
+    def test_option(self):
+        result = Product.objects.filter_by_attributes(kind="totalitarian")
+        self.assertTrue(result.exists())
+        result = Product.objects.filter_by_attributes(kind=True)
+        self.assertFalse(result.exists())
+
+        result = Product.objects.filter_by_attributes(kind="omnimous")
+        self.assertFalse(result.exists())
+
+    def test_multi_option(self):
+        result = Product.objects.filter_by_attributes(subkinds="megalomane")
+        self.assertTrue(result.exists())
+        self.assertEqual(result.count(), 2)
+        result = Product.objects.filter_by_attributes(subkinds=True)
+        self.assertFalse(result.exists())
+
+        result = Product.objects.filter_by_attributes(subkinds="omnimous")
+        self.assertFalse(result.exists())
+
+        result = Product.objects.filter_by_attributes(subkinds__contains="om")
+        self.assertTrue(result.exists(), "megalomane conains om")
+        self.assertEqual(result.count(), 2)
+
+    def test_multiple_attributes(self):
+        result = Product.objects.filter_by_attributes(subkinds="megalomane", available=True)
+        self.assertTrue(result.exists())
+
+        result = Product.objects.filter_by_attributes(
+            kind="totalitarian",
+            hypothenusa=1.25,
+            facets=8,
+            subtitle="superhenk",
+            subkinds="megalomane",
+            available=False
+        )
+        self.assertTrue(result.exists())
+
+    def test_lookups(self):
+        result = Product.objects.filter_by_attributes(facets__lte=4)
+        self.assertEqual(result.count(), 1)
+
+        result = Product.objects.filter_by_attributes(facets__lte=8)
+        self.assertEqual(result.count(), 2)
+
+        result = Product.objects.filter_by_attributes(facets__lt=8)
+        self.assertEqual(result.count(), 1)

tests/unit/catalogue/test_product_attributes_query.py

@@ -1,120 +0,0 @@
-from django.core.exceptions import ValidationError
-from django.test import TestCase
-
-from oscar.core.loading import get_model
-
-Product = get_model("catalogue", "Product")
-
-
-class ProductAttributeQuerysetTest(TestCase):
-    fixtures = ["productattributes"]
-
-    def test_query_multiple_producttypes(self):
-        "We should be able to query over multiple product classes"
-        result = Product.objects.filter_by_attributes(henkie="bah bah")
-        self.assertEqual(result.count(), 2)
-        result1, result2 = list(result)
-
-        self.assertNotEqual(result1.product_class, result2.product_class)
-        self.assertEqual(result1.attr.henkie, result2.attr.henkie)
-
-    def test_further_filtering(self):
-        "The returned queryset should be ready for further filtering"
-        result = Product.objects.filter_by_attributes(henkie="bah bah")
-        photo = result.filter(title__contains="Photo")
-        self.assertEqual(photo.count(), 1)
-
-    def test_empty_results(self):
-        "Empty results are possible without errors"
-        result = Product.objects.filter_by_attributes(doesnotexist=True)
-        self.assertFalse(result.exists(), "querying with bulshit attributes should give no results")
-        result = Product.objects.filter_by_attributes(henkie="zulthoofd")
-        self.assertFalse(result.exists(), "querying with non existing values should give no results")
-        result = Product.objects.filter_by_attributes(henkie=True)
-        self.assertFalse(result.exists(), "querying with wring value type should give no results")
-
-    def test_text_value(self):
-        result = Product.objects.filter_by_attributes(subtitle="superhenk")
-        self.assertTrue(result.exists())
-        result = Product.objects.filter_by_attributes(subtitle="kekjo")
-        self.assertTrue(result.exists())
-        result = Product.objects.filter_by_attributes(subtitle=True)
-        self.assertFalse(result.exists())
-
-    def test_formatted_text(self):
-        html = "<p style=\"margin: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Helvetica;\">Vivamus auctor leo vel dui. Aliquam erat volutpat. Phasellus nibh. Vestibulum ante ipsum primis in faucibus orci luctus et ultrices posuere cubilia Curae; Cras tempor. Morbi egestas, <em>urna</em> non consequat tempus, <strong>nunc</strong> arcu mollis enim, eu aliquam erat nulla non nibh. Duis consectetuer malesuada velit. Nam ante nulla, interdum vel, tristique ac, condimentum non, tellus. Proin ornare feugiat nisl. Suspendisse dolor nisl, ultrices at, eleifend vel, consequat at, dolor.</p>"  # noqa
-        result = Product.objects.filter_by_attributes(additional_info=html)
-        self.assertTrue(result.exists())
-
-    def test_boolean(self):
-        result = Product.objects.filter_by_attributes(available=True)
-        self.assertTrue(result.exists())
-        result = Product.objects.filter_by_attributes(available=0)
-        self.assertTrue(result.exists())
-        with self.assertRaises(ValidationError):
-            result = Product.objects.filter_by_attributes(available="henk")
-
-    def test_number(self):
-        result = Product.objects.filter_by_attributes(facets=4)
-        self.assertTrue(result.exists())
-        with self.assertRaises(ValueError):
-            result = Product.objects.filter_by_attributes(facets="four")
-
-        result = Product.objects.filter_by_attributes(facets=1)
-        self.assertFalse(result.exists())
-
-    def test_float(self):
-        result = Product.objects.filter_by_attributes(hypothenusa=1.25)
-        self.assertTrue(result.exists())
-        with self.assertRaises(ValueError):
-            result = Product.objects.filter_by_attributes(facets="four")
-
-        result = Product.objects.filter_by_attributes(hypothenusa=1)
-        self.assertFalse(result.exists())
-
-    def test_option(self):
-        result = Product.objects.filter_by_attributes(kind="totalitarian")
-        self.assertTrue(result.exists())
-        result = Product.objects.filter_by_attributes(kind=True)
-        self.assertFalse(result.exists())
-
-        result = Product.objects.filter_by_attributes(kind="omnimous")
-        self.assertFalse(result.exists())
-
-    def test_multi_option(self):
-        result = Product.objects.filter_by_attributes(subkinds="megalomane")
-        self.assertTrue(result.exists())
-        self.assertEqual(result.count(), 2)
-        result = Product.objects.filter_by_attributes(subkinds=True)
-        self.assertFalse(result.exists())
-
-        result = Product.objects.filter_by_attributes(subkinds="omnimous")
-        self.assertFalse(result.exists())
-
-        result = Product.objects.filter_by_attributes(subkinds__contains="om")
-        self.assertTrue(result.exists(), "megalomane conains om")
-        self.assertEqual(result.count(), 2)
-
-    def test_multiple_attributes(self):
-        result = Product.objects.filter_by_attributes(subkinds="megalomane", available=True)
-        self.assertTrue(result.exists())
-
-        result = Product.objects.filter_by_attributes(
-            kind="totalitarian",
-            hypothenusa=1.25,
-            facets=8,
-            subtitle="superhenk",
-            subkinds="megalomane",
-            available=False
-        )
-        self.assertTrue(result.exists())
-
-    def test_lookups(self):
-        result = Product.objects.filter_by_attributes(facets__lte=4)
-        self.assertEqual(result.count(), 1)
-
-        result = Product.objects.filter_by_attributes(facets__lte=8)
-        self.assertEqual(result.count(), 2)
-
-        result = Product.objects.filter_by_attributes(facets__lt=8)
-        self.assertEqual(result.count(), 1)