Make ProductAttributesContainer a lazy object so that initialisation is simpler.

Fixes #3258.

docs/source/releases/v3.0.rst

@@ -47,6 +47,10 @@ Backwards incompatible changes
 Bug fixes
 ~~~~~~~~~
 
+- ``catalogue.product_attributes.ProductAttributesContainer`` was refactored to ensure that attributes
+  inside the container are always properly loaded at initialisation. The container is now wrapped in a
+  ``SimpleLazyObject`` when assigned to ``Product.attr``. ``ProductAttributesContainer.initiate_attributes()``
+  was removed as the database query now happens on instantiation.
 
 Removal of deprecated features
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/oscar/apps/catalogue/abstract_models.py

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

src/oscar/apps/catalogue/product_attributes.py

@@ -2,7 +2,7 @@ from django.core.exceptions import ValidationError
 from django.utils.translation import gettext_lazy as _
 
 
-class ProductAttributesContainer(object):
+class ProductAttributesContainer:
     """
     Stolen liberally from django-eav, but simplified to be product-specific
 
@@ -13,25 +13,16 @@ class ProductAttributesContainer(object):
 
     def __setstate__(self, state):
         self.__dict__ = state
-        self.initialised = False
 
     def __init__(self, product):
         self.product = product
-        self.initialised = False
-
-    def initiate_attributes(self):
         values = self.get_values().select_related('attribute')
         for v in values:
             setattr(self, v.attribute.code, v.value)
-        self.initialised = True
 
     def __getattr__(self, name):
-        if not name.startswith('_') and not self.initialised:
-            self.initiate_attributes()
-            return getattr(self, name)
         raise AttributeError(
-            _("%(obj)s has no attribute named '%(attr)s'") % {
-                'obj': self.product.get_product_class(), 'attr': name})
+            _("%(obj)s has no attribute named '%(attr)s'") % {'obj': self.product.get_product_class(), 'attr': name})
 
     def validate_attributes(self):
         for attribute in self.get_all_attributes():

src/oscar/apps/dashboard/catalogue/forms.py

@@ -286,7 +286,6 @@ class ProductForm(forms.ModelForm):
         Set attributes before ModelForm calls the product's clean method
         (which it does in _post_clean), which in turn validates attributes.
         """
-        self.instance.attr.initiate_attributes()
         for attribute in self.instance.attr.get_all_attributes():
             field_name = 'attr_%s' % attribute.code
             # An empty text field won't show up in cleaned_data.

tests/integration/catalogue/test_attributes.py

@@ -4,9 +4,33 @@ from django.core.exceptions import ValidationError
 from django.core.files.uploadedfile import SimpleUploadedFile
 from django.test import TestCase
 
+from oscar.apps.catalogue.models import Product
 from oscar.test import factories
 
 
+class TestContainer(TestCase):
+
+    def test_attributes_initialised_before_write(self):
+        # Regression test for https://github.com/django-oscar/django-oscar/issues/3258
+        product_class = factories.ProductClassFactory()
+        product_class.attributes.create(name='a1', code='a1', required=True)
+        product_class.attributes.create(name='a2', code='a2', required=False)
+        product_class.attributes.create(name='a3', code='a3', required=True)
+        product = factories.ProductFactory(product_class=product_class)
+        product.attr.a1 = "v1"
+        product.attr.a3 = "v3"
+        product.attr.save()
+
+        product = Product.objects.get(pk=product.pk)
+        product.attr.a1 = "v2"
+        product.attr.a3 = "v6"
+        product.attr.save()
+
+        product = Product.objects.get(pk=product.pk)
+        assert product.attr.a1 == "v2"
+        assert product.attr.a3 == "v6"
+
+
 class TestBooleanAttributes(TestCase):
 
     def setUp(self):
@@ -52,7 +76,7 @@ class TestMultiOptionAttributes(TestCase):
         product = factories.ProductFactory()
         # We'll save two out of the three available options
         self.attr.save_value(product, [self.options[0], self.options[2]])
-        product.refresh_from_db()
+        product = Product.objects.get(pk=product.pk)
         self.assertEqual(list(product.attr.sizes), [self.options[0], self.options[2]])
 
     def test_delete_multi_option_value(self):
@@ -60,7 +84,7 @@ class TestMultiOptionAttributes(TestCase):
         self.attr.save_value(product, [self.options[0], self.options[1]])
         # Now delete these values
         self.attr.save_value(product, None)
-        product.refresh_from_db()
+        product = Product.objects.get(pk=product.pk)
         self.assertFalse(hasattr(product.attr, 'sizes'))
 
     def test_multi_option_value_as_text(self):