Merge branch 'feature/variants_dashboard'

docs/source/releases/v0.8.rst

@@ -82,6 +82,15 @@ has been altered to be:
 Some properties and method names have also been updated to the new naming. The
 old ones will throw a deprecation warning.
 
+Better handling of child products in product dashboard
+------------------------------------------------------
+Together with the changes above, the dashboard experience for child products
+has been improved. The difference between a parent product and a stand-alone
+product is hidden from the user; a user can now add and remove child products
+on any suitable product. When the first child product is added, a stand-alone
+product becomes a parent product; and vice versa.
+In the front-end, the old name of "product variants" has been kept.
+
 Django 1.7 support
 ------------------
 
@@ -296,6 +305,13 @@ the following renaming pattern:
 * ``variants`` becomes ``children``
 * ``variant`` becomes ``child``
 
+Product editing
+---------------
+The dashboard improvements for child products meant slight changes to both
+``ProductCreateUpdateView`` and ``ProductForm``. Notably, ``ProductForm`` now
+gets a ``parent`` kwarg. Please review your customisations for compatibility
+with the updated code.
+
 .. _incompatible_shipping_changes_in_0.8:
 
 Shipping

oscar/apps/catalogue/abstract_models.py

@@ -292,6 +292,8 @@ class AbstractProduct(models.Model):
     #: Determines if a product may be used in an offer. It is illegal to
     #: discount some types of product (e.g. ebooks) and this field helps
     #: merchants from avoiding discounting such products
+    #: Note that this flag is ignored for child products; they inherit from
+    #: the parent product.
     is_discountable = models.BooleanField(
         _("Is discountable?"), default=True, help_text=_(
             "This flag indicates if this product can be used in an offer "
@@ -327,19 +329,23 @@ class AbstractProduct(models.Model):
         """
         Validate a product. Those are the rules:
 
-        +---------------+-------------+--------------+-----------+
-        |               | stand alone | parent       | child     |
-        +---------------+-------------+--------------+-----------+
-        | title         | required    | required     | optional  |
-        +---------------+-------------+--------------+-----------+
-        | product class | required    | must be None | required  |
-        +---------------+-------------+--------------+-----------+
-        | parent        | forbidden   | forbidden    | required  |
-        +---------------+-------------+--------------+-----------+
-        | stockrecords  | 0 or more   | forbidden    | required  |
-        +---------------+-------------+--------------+-----------+
-        | categories    | 1 or more   | 1 or more    | forbidden |
-        +---------------+-------------+--------------+-----------+
+        +---------------+-------------+--------------+--------------+
+        |               | stand alone | parent       | child        |
+        +---------------+-------------+--------------+--------------+
+        | title         | required    | required     | optional     |
+        +---------------+-------------+--------------+--------------+
+        | product class | required    | required     | must be None |
+        +---------------+-------------+--------------+--------------+
+        | parent        | forbidden   | forbidden    | required     |
+        +---------------+-------------+--------------+--------------+
+        | stockrecords  | 0 or more   | forbidden    | required     |
+        +---------------+-------------+--------------+--------------+
+        | categories    | 1 or more   | 1 or more    | forbidden    |
+        +---------------+-------------+--------------+--------------+
+        | attributes    | optional    | optional     | optional     |
+        +---------------+-------------+--------------+--------------+
+        | rec. products | optional    | optional     | unsupported  |
+        +---------------+-------------+--------------+--------------+
 
         Because the validation logic is quite complex, validation is delegated
         to the sub method appropriate for the product's structure.
@@ -368,6 +374,9 @@ class AbstractProduct(models.Model):
         if self.parent_id and not self.parent.is_parent:
             raise ValidationError(
                 _("You can only assign child products to parent products."))
+        if self.product_class:
+            raise ValidationError(
+                _("A child product can't have a product class."))
 
     def _clean_parent(self):
         """
@@ -398,6 +407,23 @@ class AbstractProduct(models.Model):
     def is_child(self):
         return self.structure == self.CHILD
 
+    def can_be_parent(self, give_reason=False):
+        """
+        Helps decide if a the product can be turned into a parent product.
+        """
+        reason = None
+        if self.is_child:
+            reason = _('The specified parent product is a child product.')
+        if self.has_stockrecords:
+            reason = _(
+                "One can't add a child product to a product with stock"
+                " records.")
+        is_valid = reason is None
+        if give_reason:
+            return is_valid, reason
+        else:
+            return is_valid
+
     @property
     def options(self):
         pclass = self.get_product_class()
@@ -506,7 +532,7 @@ class AbstractProduct(models.Model):
         """
         return self._min_child_price('price_excl_tax')
 
-    # Wrappers
+    # Wrappers for child products
 
     def get_title(self):
         """
@@ -520,15 +546,24 @@ class AbstractProduct(models.Model):
 
     def get_product_class(self):
         """
-        Return a product's item class
+        Return a product's item class. Child products inherit their parent's.
         """
-        if self.product_class_id or self.product_class:
-            return self.product_class
-        if self.parent and self.parent.product_class:
+        if self.is_child:
             return self.parent.product_class
-        return None
+        else:
+            return self.product_class
     get_product_class.short_description = _("Product class")
 
+    def get_is_discountable(self):
+        """
+        At the moment, is_discountable can't be set individually for child
+        products; they inherit it from their parent.
+        """
+        if self.is_child:
+            return self.parent.is_discountable
+        else:
+            return self.is_discountable
+
     def get_categories(self):
         """
         Return a product's categories or parent's if there is a parent product.

oscar/apps/dashboard/catalogue/app.py

@@ -64,6 +64,9 @@ class CatalogueApplication(Application):
             url(r'^products/create/(?P<product_class_slug>[\w-]+)/$',
                 self.product_createupdate_view.as_view(),
                 name='catalogue-product-create'),
+            url(r'^products/(?P<parent_pk>[-\d]+)/create-variant/$',
+                self.product_createupdate_view.as_view(),
+                name='catalogue-product-create-child'),
             url(r'^products/(?P<pk>\d+)/delete/$',
                 self.product_delete_view.as_view(),
                 name='catalogue-product-delete'),
@@ -101,7 +104,6 @@ class CatalogueApplication(Application):
             url(r'^product-type/(?P<pk>\d+)/delete/$',
                 self.product_class_delete_view.as_view(),
                 name='catalogue-class-delete'),
-
         ]
         return self.post_process_urls(urls)
 

oscar/apps/dashboard/catalogue/forms.py

@@ -1,5 +1,5 @@
 from django import forms
-from django.core.exceptions import ValidationError, MultipleObjectsReturned
+from django.core import exceptions
 from django.forms.models import inlineformset_factory
 from django.utils.translation import ugettext_lazy as _
 from treebeard.forms import MoveNodeForm, movenodeform_factory
@@ -12,8 +12,6 @@ Product = get_model('catalogue', 'Product')
 ProductClass = get_model('catalogue', 'ProductClass')
 Category = get_model('catalogue', 'Category')
 StockRecord = get_model('partner', 'StockRecord')
-Partner = get_model('partner', 'Partner')
-ProductAttributeValue = get_model('catalogue', 'ProductAttributeValue')
 ProductCategory = get_model('catalogue', 'ProductCategory')
 ProductImage = get_model('catalogue', 'ProductImage')
 ProductRecommendation = get_model('catalogue', 'ProductRecommendation')
@@ -147,7 +145,8 @@ class StockRecordFormSet(BaseStockRecordFormSet):
         if self.require_user_stockrecord:
             try:
                 user_partner = self.user.partners.get()
-            except (Partner.DoesNotExist, MultipleObjectsReturned):
+            except (exceptions.ObjectDoesNotExist,
+                    exceptions.MultipleObjectsReturned):
                 pass
             else:
                 partner_field = self.forms[0].fields.get('partner', None)
@@ -172,9 +171,9 @@ class StockRecordFormSet(BaseStockRecordFormSet):
                                         for form in self.forms])
             user_partners = set(self.user.partners.all())
             if not user_partners & stockrecord_partners:
-                raise ValidationError(_("At least one stock record must be set"
-                                        " to a partner that you're associated"
-                                        " with."))
+                raise exceptions.ValidationError(
+                    _("At least one stock record must be set to a partner that"
+                      "you're associated with."))
 
 
 def _attr_text_field(attribute):
@@ -265,52 +264,67 @@ class ProductForm(forms.ModelForm):
     class Meta:
         model = Product
         fields = [
-            'title', 'upc', 'description',
-            'is_discountable',
-            'structure', 'parent']
+            'title', 'upc', 'description', 'is_discountable', 'structure']
         widgets = {
-            'parent': ProductSelect,
+            'structure': forms.HiddenInput()
         }
 
-    def __init__(self, product_class, data=None, *args, **kwargs):
-        self.set_initial_attribute_values(product_class, kwargs)
+    def __init__(self, product_class, data=None, parent=None, *args, **kwargs):
+        self.set_initial(product_class, parent, kwargs)
         super(ProductForm, self).__init__(data, *args, **kwargs)
-        self.instance.product_class = product_class
-
-        # This is quite nasty.  We use the raw posted data to determine if the
-        # product is a parent product, as this changes the validation rules we
-        # want to apply.
-        is_parent = data and data.get('structure', '') == 'parent'
-        self.add_attribute_fields(is_parent)
-
-        parent = self.fields.get('parent', None)
-        if parent is not None:
-            parent.queryset = self.get_parent_products_queryset()
+        if parent:
+            self.instance.parent = parent
+            # We need to set the correct product structures explicitly to pass
+            # attribute validation and child product validation. Note that
+            # those changes are not persisted.
+            self.instance.structure = Product.CHILD
+            self.instance.parent.structure = Product.PARENT
+
+            self.delete_non_child_fields()
+        else:
+            # Only set product class for non-child products
+            self.instance.product_class = product_class
+        self.add_attribute_fields(product_class, self.instance.is_parent)
 
         if 'title' in self.fields:
             self.fields['title'].widget = forms.TextInput(
                 attrs={'autocomplete': 'off'})
 
+    def set_initial(self, product_class, parent, kwargs):
+        """
+        Set initial data for the form. Sets the correct product structure
+        and fetches initial values for the dynamically constructed attribute
+        fields.
+        """
+        if 'initial' not in kwargs:
+            kwargs['initial'] = {}
+        self.set_initial_attribute_values(product_class, kwargs)
+        if parent:
+            kwargs['initial']['structure'] = Product.CHILD
+
     def set_initial_attribute_values(self, product_class, kwargs):
         """
         Update the kwargs['initial'] value to have the initial values based on
         the product instance's attributes
         """
-        if kwargs.get('instance', None) is None:
+        instance = kwargs.get('instance')
+        if instance is None:
             return
-        if 'initial' not in kwargs:
-            kwargs['initial'] = {}
         for attribute in product_class.attributes.all():
             try:
-                value = kwargs['instance'].attribute_values.get(
+                value = instance.attribute_values.get(
                     attribute=attribute).value
-            except ProductAttributeValue.DoesNotExist:
+            except exceptions.ObjectDoesNotExist:
                 pass
             else:
                 kwargs['initial']['attr_%s' % attribute.code] = value
 
-    def add_attribute_fields(self, is_parent=False):
-        for attribute in self.instance.product_class.attributes.all():
+    def add_attribute_fields(self, product_class, is_parent=False):
+        """
+        For each attribute specified by the product class, this method
+        dynamically adds form fields to the product form.
+        """
+        for attribute in product_class.attributes.all():
             field = self.get_attribute_field(attribute)
             if field:
                 self.fields['attr_%s' % attribute.code] = field
@@ -319,31 +333,30 @@ class ProductForm(forms.ModelForm):
                     self.fields['attr_%s' % attribute.code].required = False
 
     def get_attribute_field(self, attribute):
+        """
+        Gets the correct form field for a given attribute type.
+        """
         return self.FIELD_FACTORIES[attribute.type](attribute)
 
-    def get_parent_products_queryset(self):
+    def delete_non_child_fields(self):
         """
-        :return: Parent products, minus this product
+        Deletes any fields not needed for child products. Override this if
+        you want to e.g. keep the description field.
         """
-        # Not using Product.browsable because a deployment might override
-        # that manager to respect a status field or such like
-        queryset = Product._default_manager.filter(structure=Product.PARENT)
-        if self.instance.pk is not None:
-            # Prevent selecting itself as parent
-            queryset = queryset.exclude(pk=self.instance.pk)
-        return queryset
-
-    def save(self):
+        for field_name in ['description', 'is_discountable']:
+            if field_name in self.fields:
+                del self.fields[field_name]
+
+    def _post_clean(self):
         """
-        Set product class and attributes before saving
+        Set attributes before ModelForm calls the product's clean method
+        (which it does in _post_clean), which in turn validates attributes.
         """
-        product = super(ProductForm, self).save(commit=False)
-        for attribute in self.instance.product_class.attributes.all():
+        product_class = self.instance.get_product_class()
+        for attribute in product_class.attributes.all():
             value = self.cleaned_data['attr_%s' % attribute.code]
-            setattr(product.attr, attribute.code, value)
-        product.save()
-        self.save_m2m()
-        return product
+            setattr(self.instance.attr, attribute.code, value)
+        super(ProductForm, self)._post_clean()
 
 
 class StockAlertSearchForm(forms.Form):

oscar/apps/dashboard/catalogue/tables.py

@@ -12,28 +12,36 @@ Category = get_model('catalogue', 'Category')
 
 class ProductTable(Table):
     title = TemplateColumn(
+        verbose_name=_('Title'),
         template_name='dashboard/catalogue/product_row_title.html',
-        order_by='title', accessor=A('get_title'))
+        order_by='title', accessor=A('title'))
     image = TemplateColumn(
+        verbose_name=_('Image'),
         template_name='dashboard/catalogue/product_row_image.html',
         orderable=False)
-    product_class = Column(verbose_name=_("Type"),
-                           accessor=A('get_product_class.name'),
-                           order_by=('product_class__name'))
-    parent = LinkColumn('dashboard:catalogue-product',
-                        verbose_name=_("Parent"), args=[A('parent.pk')],
-                        accessor=A('parent.title'))
-    children = Column(accessor=A('children.count'), orderable=False)
-    stock_records = Column(accessor=A('stockrecords.count'), orderable=False)
+    product_class = Column(
+        verbose_name=_('Product type'),
+        accessor=A('product_class'),
+        order_by='product_class__name')
+    variants = TemplateColumn(
+        verbose_name=_("Variants"),
+        template_name='dashboard/catalogue/product_row_variants.html',
+        orderable=False
+    )
+    stock_records = Column(
+        verbose_name=_('Stock records'),
+        accessor=A('stockrecords.count'),
+        orderable=False)
     actions = TemplateColumn(
+        verbose_name=_('Actions'),
         template_name='dashboard/catalogue/product_row_actions.html',
         orderable=False)
 
     class Meta(DashboardTable.Meta):
         model = Product
         fields = ('upc', 'date_created')
-        sequence = ('title', 'upc', 'image', 'product_class',
-                    'parent', 'children', 'stock_records', '...', 'date_created', 'actions')
+        sequence = ('title', 'upc', 'image', 'product_class', 'variants',
+                    'stock_records', '...', 'date_created', 'actions')
         order_by = '-date_created'
 
 

oscar/apps/dashboard/catalogue/views.py

@@ -1,13 +1,13 @@
-from django.core.exceptions import ObjectDoesNotExist
 from django.views import generic
 from django.db.models import Q
-from django.http import HttpResponseRedirect, Http404
+from django.http import HttpResponseRedirect
 from django.contrib import messages
 from django.core.urlresolvers import reverse
 from django.utils.translation import ugettext_lazy as _
+from django.shortcuts import get_object_or_404, redirect
 from django.template.loader import render_to_string
 
-from oscar.core.loading import get_class, get_classes, get_model
+from oscar.core.loading import get_classes, get_model
 
 from django_tables2 import SingleTableMixin
 
@@ -106,7 +106,7 @@ class ProductListView(SingleTableMixin, generic.TemplateView):
         """
         Build the queryset for this list
         """
-        queryset = Product.objects.base_queryset()
+        queryset = Product.browsable.base_queryset()
         queryset = self.filter_queryset(queryset)
         queryset = self.apply_search(queryset)
         queryset = self.apply_ordering(queryset)
@@ -187,7 +187,17 @@ class ProductCreateRedirectView(generic.RedirectView):
 
 class ProductCreateUpdateView(generic.UpdateView):
     """
-    Dashboard view that bundles both creating and updating single products.
+    Dashboard view that is can both create and update products of all kinds.
+    It can be used in three different ways, each of them with a unique URL
+    pattern:
+    - When creating a new standalone product, this view is called with the
+      desired product class
+    - When editing an existing product, this view is called with the product's
+      primary key. If the product is a child product, the template considerably
+      reduces the available form fields.
+    - When creating a new child product, this view is called with the parent's
+      primary key.
+
     Supports the permission-based dashboard.
     """
 
@@ -219,43 +229,78 @@ class ProductCreateUpdateView(generic.UpdateView):
         This parts allows generic.UpdateView to handle creating products as
         well. The only distinction between an UpdateView and a CreateView
         is that self.object is None. We emulate this behavior.
-        Additionally, self.product_class is set.
+
+        This method is also responsible for setting self.product_class and
+        self.parent.
         """
         self.creating = 'pk' not in self.kwargs
         if self.creating:
-            try:
-                product_class_slug = self.kwargs.get('product_class_slug',
-                                                     None)
-                self.product_class = ProductClass.objects.get(
-                    slug=product_class_slug)
-            except ObjectDoesNotExist:
-                raise Http404
+            # Specifying a parent product is only done when creating a child
+            # product.
+            parent_pk = self.kwargs.get('parent_pk')
+            if parent_pk is None:
+                self.parent = None
+                # A product class needs to be specified when creating a
+                # standalone product.
+                product_class_slug = self.kwargs.get('product_class_slug')
+                self.product_class = get_object_or_404(
+                    ProductClass, slug=product_class_slug)
             else:
-                return None  # success
+                self.parent = self.get_and_check_parent(parent_pk)
+                self.product_class = self.parent.product_class
+
+            return None  # success
         else:
             product = super(ProductCreateUpdateView, self).get_object(queryset)
             self.product_class = product.get_product_class()
+            self.parent = product.parent
             return product
 
+    def get_and_check_parent(self, parent_pk):
+        """
+        Fetches the specified "parent" product and ensures that it can be
+        indeed be turned into a parent product if needed.
+        """
+        parent = get_object_or_404(Product, pk=parent_pk)
+        is_valid, reason = parent.can_be_parent(give_reason=True)
+        if is_valid:
+            return parent
+        else:
+            messages.error(self.request, reason)
+            return redirect('dashboard:catalogue-product-list')
+
     def get_context_data(self, **kwargs):
         ctx = super(ProductCreateUpdateView, self).get_context_data(**kwargs)
         ctx['product_class'] = self.product_class
+        ctx['parent'] = self.parent
+        ctx['title'] = self.get_page_title()
 
         for ctx_name, formset_class in self.formsets.items():
             if ctx_name not in ctx:
                 ctx[ctx_name] = formset_class(self.product_class,
                                               self.request.user,
                                               instance=self.object)
+        return ctx
 
-        if self.object is None:
-            ctx['title'] = _('Create new %s product') % self.product_class.name
+    def get_page_title(self):
+        if self.creating:
+            if self.parent is None:
+                return _('Create new %(product_class)s product') % {
+                    'product_class': self.product_class.name}
+            else:
+                return _('Create new variant of %(parent_product)s') % {
+                    'parent_product': self.parent.title}
         else:
-            ctx['title'] = ctx['product'].get_title()
-        return ctx
+            if self.object.title:
+                return self.object.title
+            else:
+                return _('Editing variant of %(parent_product)s') % {
+                    'parent_product': self.parent.title}
 
     def get_form_kwargs(self):
         kwargs = super(ProductCreateUpdateView, self).get_form_kwargs()
         kwargs['product_class'] = self.product_class
+        kwargs['parent'] = self.parent
         return kwargs
 
     def process_all_forms(self, form):
@@ -305,8 +350,12 @@ class ProductCreateUpdateView(generic.UpdateView):
     def forms_valid(self, form, formsets):
         """
         Save all changes and display a success url.
+        When creating the first child product, this method also sets the new
+        parent's structure accordingly.
         """
-        if not self.creating:
+        if self.creating:
+            self.handle_adding_child(self.parent)
+        else:
             # a just created product was already saved in process_all_forms()
             self.object = form.save()
 
@@ -316,6 +365,19 @@ class ProductCreateUpdateView(generic.UpdateView):
 
         return HttpResponseRedirect(self.get_success_url())
 
+    def handle_adding_child(self, parent):
+        """
+        When creating the first child product, the parent product needs
+        to be implicitly converted from a standalone product to a
+        parent product.
+        """
+        # ProductForm eagerly sets the future parent's structure to PARENT to
+        # pass validation, but it's not persisted in the database. We ensure
+        # it's persisted by calling save()
+        if parent is not None:
+            parent.structure = Product.PARENT
+            parent.save()
+
     def forms_invalid(self, form, formsets):
         # delete the temporary product again
         if self.creating and self.object and self.object.pk is not None:
@@ -335,6 +397,13 @@ class ProductCreateUpdateView(generic.UpdateView):
         return "?".join(url_parts)
 
     def get_success_url(self):
+        """
+        Renders a success message and redirects depending on the button:
+        - Standard case is pressing "Save"; redirects to the product list
+        - When "Save and continue" is pressed, we stay on the same page
+        - When "Create (another) child product" is pressed, it redirects
+          to a new product creation page
+        """
         msg = render_to_string(
             'dashboard/catalogue/messages/product_saved.html',
             {
@@ -343,16 +412,28 @@ class ProductCreateUpdateView(generic.UpdateView):
                 'request': self.request
             })
         messages.success(self.request, msg, extra_tags="safe noicon")
-        url = reverse('dashboard:catalogue-product-list')
-        if self.request.POST.get('action') == 'continue':
-            url = reverse('dashboard:catalogue-product',
-                          kwargs={"pk": self.object.id})
+
+        action = self.request.POST.get('action')
+        if action == 'continue':
+            url = reverse(
+                'dashboard:catalogue-product', kwargs={"pk": self.object.id})
+        elif action == 'create-another-child' and self.parent:
+            url = reverse(
+                'dashboard:catalogue-product-create-child',
+                kwargs={'parent_pk': self.parent.pk})
+        elif action == 'create-child':
+            url = reverse(
+                'dashboard:catalogue-product-create-child',
+                kwargs={'parent_pk': self.object.pk})
+        else:
+            url = reverse('dashboard:catalogue-product-list')
         return self.get_url_with_querystring(url)
 
 
 class ProductDeleteView(generic.DeleteView):
     """
-    Dashboard view to delete a product.
+    Dashboard view to delete a product. Has special logic for deleting the
+    last child product.
     Supports the permission-based dashboard.
     """
     template_name = 'dashboard/catalogue/product_delete.html'
@@ -365,10 +446,63 @@ class ProductDeleteView(generic.DeleteView):
         """
         return filter_products(Product.objects.all(), self.request.user)
 
+    def get_context_data(self, **kwargs):
+        ctx = super(ProductDeleteView, self).get_context_data(**kwargs)
+        if self.object.is_child:
+            ctx['title'] = _("Delete product variant?")
+        else:
+            ctx['title'] = _("Delete product?")
+        return ctx
+
+    def delete(self, request, *args, **kwargs):
+        # We override the core delete method and don't call super in order to
+        # apply more sophisticated logic around handling child products.
+        # Calling super makes it difficult to test if the product being deleted
+        # is the last child.
+
+        self.object = self.get_object()
+
+        # Before performing the delete, record whether this product is the last
+        # child.
+        is_last_child = False
+        if self.object.is_child:
+            parent = self.object.parent
+            is_last_child = parent.children.count() == 1
+
+        self.object.delete()
+
+        # If the product being deleted is the last child, then pass control
+        # to a method than can adjust the parent itself.
+        if is_last_child:
+            self.handle_deleting_last_child(parent)
+
+        return HttpResponseRedirect(self.get_success_url())
+
+    def handle_deleting_last_child(self, parent):
+        # If the last child product is deleted, this view defaults to turning
+        # the parent product into a standalone product. While this is
+        # appropriate for many scenarios, it is intentionally easily
+        # overridable and not automatically done in e.g. a Product's delete()
+        # method as it is more a UX helper than hard business logic.
+        parent.structure = parent.STANDALONE
+        parent.save()
+
     def get_success_url(self):
-        msg = _("Deleted product '%s'") % self.object.title
-        messages.success(self.request, msg)
-        return reverse('dashboard:catalogue-product-list')
+        """
+        When deleting child products, this view redirects to editing the
+        parent product. When deleting any other product, it redirects to the
+        product list view.
+        """
+        if self.object.is_child:
+            msg = _("Deleted product variant '%s'") % self.object.get_title()
+            messages.success(self.request, msg)
+            return reverse(
+                'dashboard:catalogue-product',
+                kwargs={'pk': self.object.parent_id})
+        else:
+            msg = _("Deleted product '%s'") % self.object.title
+            messages.success(self.request, msg)
+            return reverse('dashboard:catalogue-product-list')
 
 
 class StockAlertListView(generic.ListView):

oscar/apps/offer/models.py

@@ -508,7 +508,8 @@ class Condition(models.Model):
         if not line.stockrecord_id:
             return False
         product = line.product
-        return self.range.contains_product(product) and product.is_discountable
+        return (self.range.contains_product(product)
+                and product.get_is_discountable())
 
     def get_applicable_lines(self, offer, basket, most_expensive_first=True):
         """

oscar/templates/oscar/dashboard/catalogue/messages/product_saved.html

@@ -2,15 +2,46 @@
 {% load django_tables2 %}
 
 <p>
-{% if creating %}
-    {% blocktrans with name=product.get_title %}
-        Created product '{{ name }}'.
-    {% endblocktrans %}
-{% else %}
-    {% blocktrans with name=product.get_title %}
-        Updated product '{{ name }}'.
-    {% endblocktrans %}
-{% endif %}
+{% with name=product.title parent_name=product.parent.title %}
+    {% if product.is_child %}
+
+        {% if creating %}
+            {% if product.title %}
+                {% blocktrans %}
+                    Created product variant '{{ name }}'.
+                {% endblocktrans %}
+            {% else %}
+                {% blocktrans %}
+                    Created variant of '{{ parent_name }}'.
+                {% endblocktrans %}
+            {% endif %}
+        {% else %}
+            {% if product.title %}
+                {% blocktrans %}
+                    Updated product variant '{{ name }}'.
+                {% endblocktrans %}
+            {% else %}
+                {% blocktrans %}
+                    Updated a variant of '{{ parent_name }}'.
+                {% endblocktrans %}
+            {% endif %}
+        {% endif %}
+
+    {% else %}
+
+        {% if creating %}
+            {% blocktrans %}
+                Created product '{{ name }}'.
+            {% endblocktrans %}
+        {% else %}
+            {% blocktrans %}
+                Updated product '{{ name }}'.
+            {% endblocktrans %}
+        {% endif %}
+
+    {% endif %}
+{% endwith %}
+
 </p>
 
 <p>

oscar/templates/oscar/dashboard/catalogue/product_delete.html

@@ -3,10 +3,7 @@
 {% load i18n %}
 
 {% block title %}
-    {% blocktrans with title=product.get_title %}
-        Delete {{ title}}?
-    {% endblocktrans %} |
-    {{ block.super }}
+    {{ title }} | {{ block.super }}
 {% endblock %}
 
 {% block body_class %}{{ block.super }} create-page{% endblock %}
@@ -22,35 +19,40 @@
             <span class="divider">/</span>
         </li>
         <li>
-            <a href="{% url 'dashboard:catalogue-product' pk=product.pk %}">{{ product.title }}</a>
+            <a href="{% url 'dashboard:catalogue-product' pk=product.pk %}">{{ product.get_title }}</a>
             <span class="divider">/</span>
         </li>
-        <li class="active">{% trans "Delete?" %}</li>
+        <li class="active">{{ title }}</li>
     </ul>
 {% endblock %}
 
-{% block header %}
-    <div class="page-header">
-        <h1>{% trans "Delete product?" %}</h1>
-    </div>
-{% endblock header%}
+{% block headertext %}{{ title }}{% endblock %}
 
 {% block dashboard_content %}
     <div class="table-header">
-        <h2>{% trans "Delete product" %}</h2>
+        <h2>{{ title }}</h2>
     </div>
     <form action="." method="post" class="well">
         {% csrf_token %}
 
-        {% blocktrans with title=product.title %}
-            <p>Delete product <strong>{{ title }}</strong> - are you sure?</p>
-        {% endblocktrans %}
+        <p>
+            {% if product.is_child %}
+                {% blocktrans with title=product.get_title %}
+                    Delete product variant <strong>{{ title }}</strong> - are you sure?
+                {% endblocktrans %}
+            {% else %}
+                {% blocktrans with title=product.get_title %}
+                    Delete product <strong>{{ title }}</strong> - are you sure?
+                {% endblocktrans %}
+            {% endif %}
+        </p>
 
         {% if product.is_parent %}
-            <p> {% trans "This will also delete the following child products:" %}
+            <p>
+                {% trans "This will also delete the following child products:" %}
                 <ul>
                     {% for child in product.children.all %}
-                        <li><strong>{{ child.title }}</strong></li>
+                        <li><strong>{{ child.get_title }}</strong></li>
                     {% endfor %}
                 </ul>
             </p>

oscar/templates/oscar/dashboard/catalogue/product_row_variants.html

@@ -0,0 +1,5 @@
+{% if record.is_standalone %}
+    -
+{% else %}
+    {{ record.children.count }}
+{% endif %}

oscar/templates/oscar/dashboard/catalogue/product_update.html

@@ -21,8 +21,15 @@
             <a href="{% url 'dashboard:catalogue-product-list' %}">{% trans "Products" %}</a>
             <span class="divider">/</span>
         </li>
+        {% if parent %}
+            <li>
+                <a href="{% url 'dashboard:catalogue-product' parent.id %}">
+                    {{ parent.title }}
+                </a>
+                <span class="divider">/</span>
+            </li>
+        {% endif %}
         <li class="active">{{ title }}</li>
-
     </ul>
 {% endblock %}
 
@@ -31,6 +38,21 @@
 {% block dashboard_content %}
     <form action="{% if request.GET.urlencode %}?{{ request.GET.urlencode }}{% endif %}" method="post" class="form-stacked wysiwyg fixed-actions" enctype="multipart/form-data" data-behaviour="affix-nav-errors" autocomplete="off">
         {% csrf_token %}
+
+        {% if parent %}
+            <div class="row-fluid">
+                <div class="span12">
+                    <div class="alert alert-info">
+                        {% url 'dashboard:catalogue-product' pk=parent.id as parent_url %}
+                        {% blocktrans with title=parent.title %}
+                            You are currently editing a product variant of
+                            <a href="{{ parent_url }}">{{ title }}</a>.
+                        {% endblocktrans %}
+                    </div>
+                </div>
+            </div>
+        {% endif %}
+
         <div class="row-fluid">
 
             {% block tab_nav %}
@@ -40,17 +62,34 @@
                             <h3>{% trans "Sections" %}</h3>
                         </div>
                         <ul class="nav nav-list bs-docs-sidenav" id="product_update_tabs">
-                            <li class="active"><a href="#product_details" data-toggle="tab">{% trans 'Product details' %}</a></li>
-                            <li><a href="#product_category" data-toggle="tab">{% trans 'Category' %}</a></li>
-                            {% if product_class.has_attributes %}
-                                <li><a href="#product_attributes" data-toggle="tab">{% trans 'Attributes' %}</a></li>
-                            {% endif %}
-                            <li><a href="#product_images" data-toggle="tab">{% trans 'Images' %}</a></li>
-                            <li><a href="#product_stock" data-toggle="tab">{% trans 'Stock and pricing' %}</a></li>
-                            {% if not product.is_child %}
-                                <li><a href="#child_products" data-toggle="tab">{% trans 'Variants' %}</a></li>
-                            {% endif %}
-                            <li><a href="#product_recommended" data-toggle="tab">{% trans 'Upselling' %}</a></li>
+                            {% comment %}
+                                The navigation below is filtered heavily on the product structure.
+                                This intentionally is the only place where filtering is done, as
+                                deployments are likely to want to override certain aspects of what
+                                data is stored on products of a certain structure. This means that
+                                only one template block (instead of all affected) has to be altered.
+                            {% endcomment %}
+                            {% block tabs %}
+                                <li class="active"><a href="#product_details" data-toggle="tab">{% trans 'Product details' %}</a></li>
+                                {% if not parent %}
+                                    <li><a href="#product_category" data-toggle="tab">{% trans 'Categories' %}</a></li>
+                                {% endif %}
+                                {% if product_class.has_attributes %}
+                                    <li><a href="#product_attributes" data-toggle="tab">{% trans 'Attributes' %}</a></li>
+                                {% endif %}
+                                {% if not parent %}
+                                    <li><a href="#product_images" data-toggle="tab">{% trans 'Images' %}</a></li>
+                                {% endif %}
+                                {% if not product.is_parent %}
+                                    <li><a href="#product_stock" data-toggle="tab">{% trans 'Stock and pricing' %}</a></li>
+                                {% endif %}
+                                {% if not parent %}
+                                    <li><a href="#child_products" data-toggle="tab">{% trans 'Variants' %}</a></li>
+                                {% endif %}
+                                {% if not parent %}
+                                    <li><a href="#product_recommended" data-toggle="tab">{% trans 'Upselling' %}</a></li>
+                                {% endif %}
+                            {% endblock tabs %}
                         </ul>
                     </div>
                 </div>
@@ -75,17 +114,6 @@
                                             {% include 'partials/form_field.html' with field=field %}
                                         {% endif %}
                                     {% endfor %}
-
-                                    {% with parent=product.parent %}
-                                        {% if parent %}
-                                            <div class="control-group">
-                                                <label class="control-label">{% trans "Parent" %}</label>
-                                                <div class="controls">
-                                                    <a href="{% url 'dashboard:catalogue-product' pk=parent.id %}" title="{% blocktrans with title=parent.title %}Edit {{ title }}{% endblocktrans %}">{{ parent.title }}</a>
-                                                </div>
-                                            </div>
-                                        {% endif %}
-                                    {% endwith %}
                                 {% endblock product_details_content %}
                             </div>
                         </div>
@@ -111,29 +139,27 @@
                     {% endblock product_categories %}
 
                     {% block product_attributes %}
-                        {% if product_class.has_attributes %}
-                            <div class="tab-pane" id="product_attributes">
-                                {% block product_attributes_content %}
-                                    <table class="table table-striped table-bordered">
-                                        <caption>
-                                            {% trans "Attributes" %}
-                                            <span class="label label-success">
-                                                {% trans "Product Type:" %} {{ product_class }}
-                                            </span>
-                                        </caption>
-                                        {% for field in form %}
-                                            {% if 'attr' in field.id_for_label %}
-                                                <tr>
-                                                    <td>
-                                                        {% include "partials/form_field.html" %}
-                                                    </td>
-                                                </tr>
-                                            {% endif %}
-                                        {% endfor %}
-                                    </table>
-                                {% endblock product_attributes_content %}
-                            </div>
-                        {% endif %}
+                        <div class="tab-pane" id="product_attributes">
+                            {% block product_attributes_content %}
+                                <table class="table table-striped table-bordered">
+                                    <caption>
+                                        {% trans "Attributes" %}
+                                        <span class="label label-success">
+                                            {% trans "Product Type:" %} {{ product_class }}
+                                        </span>
+                                    </caption>
+                                    {% for field in form %}
+                                        {% if 'attr' in field.id_for_label %}
+                                            <tr>
+                                                <td>
+                                                    {% include "partials/form_field.html" %}
+                                                </td>
+                                            </tr>
+                                        {% endif %}
+                                    {% endfor %}
+                                </table>
+                            {% endblock product_attributes_content %}
+                        </div>
                     {% endblock product_attributes %}
 
                     {% block product_images %}
@@ -214,35 +240,57 @@
                     {% endblock stockrecords %}
 
                     {% block child_products %}
-                        {% if not product.is_child %}
-                            {% with children=product.children.all %}
-                                <div class="tab-pane" id="child_products">
-                                    {% block child_products_content %}
-                                        <table class='table table-striped table-bordered'>
-                                            <caption>{% trans "Variants" %}</caption>
+                        {% with children=product.children.all %}
+                            <div class="tab-pane" id="child_products">
+                                {% block child_products_content %}
+                                    <table class='table table-striped table-bordered'>
+                                        <caption>
+                                            {% trans "Variants" %}
+                                            <button class="btn btn-primary pull-right{% if not product.can_be_parent %} disabled{% endif %}" name="action" type="submit" value="create-child">
+                                                <i class="icon-plus"></i>
+                                                {% trans "Add variant" %}
+                                            </button>
+                                        </caption>
+                                        {% if children %}
+                                            <tr>
+                                                <th>{% trans "Title" %}</th>
+                                                <th>{% trans "Attributes" %}</th>
+                                                <th>{% trans "Stock records" %}</th>
+                                                <th>&nbsp;</th>
+                                            </tr>
                                             {% for child in children %}
                                                 <tr>
                                                     <td>{{ child.get_title }}</td>
                                                     <td>{{ child.attribute_summary }}</td>
-                                                    <td><a href="{% url 'dashboard:catalogue-product' pk=child.id %}" class="btn btn-primary">{% trans "Edit" %}</a></td>
-                                                </tr>
-                                            {% empty %}
-                                                <tr>
-                                                    <td colspan="3">
-                                                        {% url 'dashboard:catalogue-product-create' product_class.slug as create_url %}
-                                                        {% blocktrans %}
-                                                            This product does not have any variants. To add variants, please ensure that the product structure is set to
-                                                            parent product and finish editing the current product. Then <a href="{{ create_url }}">create</a> one or more
-                                                            child products and select the current product as parent.
-                                                        {% endblocktrans %}
+                                                    <td>{{ child.stockrecords.count }}</td>
+                                                    <td>
+                                                        <a href="{% url 'dashboard:catalogue-product' pk=child.id %}" class="btn btn-primary">
+                                                            {% trans "Edit" %}
+                                                        </a>
+                                                        <a href="{% url 'dashboard:catalogue-product-delete' pk=child.id %}" class="btn btn-danger">
+                                                            {% trans "Delete" %}
+                                                        </a>
                                                     </td>
                                                 </tr>
                                             {% endfor %}
-                                        </table>
-                                    {% endblock child_products_content %}
-                                </div>
-                            {% endwith %}
-                        {% endif %}
+                                        {% else %}
+                                            <tr>
+                                                <td colspan="3">
+                                                    {% if product.can_be_parent %}
+                                                        {% trans 'This product does not have any variants.' %}
+                                                    {% else %}
+                                                        {% trans "One can't add variants to this product at this point." %}
+                                                        {% if product.has_stockrecords %}
+                                                            {% trans 'This is likely because this product still has stock records.' %}
+                                                        {% endif %}
+                                                    {% endif %}
+                                                </td>
+                                            </tr>
+                                        {% endif %}
+                                    </table>
+                                {% endblock child_products_content %}
+                            </div>
+                        {% endwith %}
                     {% endblock child_products %}
 
                     {% block recommended_products %}
@@ -271,10 +319,21 @@
             <div class="fixed-actions-group">
                 <div class="form-actions">
                     <div class="pull-right">
-                        <a href="{% url 'dashboard:catalogue-product-list' %}">{% trans "Cancel" %}</a>
+                        <a href="{% url 'dashboard:catalogue-product-list' %}">
+                            {% trans "Cancel" %}
+                        </a>
                         {% trans "or" %}
-                        <button class="btn btn-secondary btn-large" name="action" type="submit" value="continue">{% trans "Save and continue editing" %}</button>
-                        <button class="btn btn-primary btn-large" name="action" type="submit" value="save">{% trans "Save" %}</button>
+                        {% if parent %}
+                            <button class="btn btn-secondary btn-large" name="action" type="submit" value="create-another-child">
+                                {% trans "Save and add another variant" %}
+                            </button>
+                        {% endif %}
+                        <button class="btn btn-secondary btn-large" name="action" type="submit" value="continue">
+                            {% trans "Save and continue editing" %}
+                        </button>
+                        <button class="btn btn-primary btn-large" name="action" type="submit" value="save">
+                            {% trans "Save" %}
+                        </button>
                     </div>
                     {% if product %}
                         <a class="btn btn-success btn-large" href="{{ product.get_absolute_url }}">{% trans "View on site" %}</a>

oscar/templates/oscar/dashboard/ranges/range_product_list.html

@@ -108,7 +108,7 @@
                                 </td>
                                 <td>{{ product.upc|default:"-" }}</td>
                                 <td><a href="{% url 'dashboard:catalogue-product' pk=product.id %}">{{ product.get_title }}</a></td>
-                                <td>{% if product.is_discountable %}{% trans "Yes" %}{% else %}{% trans "No" %}{% endif %}</td>
+                                <td>{% if product.get_is_discountable %}{% trans "Yes" %}{% else %}{% trans "No" %}{% endif %}</td>
                                 <td>
                                     <a class="btn btn-danger" href="#" data-behaviours="remove">{% trans "Remove" %}</a>
                                     <a href="#" class="btn btn-info btn-handle"><i class="icon-move icon-large"></i> {% trans "Re-order" %}</a>

oscar/test/newfactories.py

@@ -20,8 +20,9 @@ from oscar.core.compat import get_user_model
 
 __all__ = ["UserFactory", "CountryFactory", "UserAddressFactory",
            "BasketFactory", "VoucherFactory", "ProductFactory",
-           "StockRecordFactory", "ProductAttributeFactory",
-           "ProductAttributeValueFactory", "AttributeOptionGroupFactory",
+           "ProductClassFactory", "StockRecordFactory",
+           "ProductAttributeFactory", "ProductAttributeValueFactory",
+           "AttributeOptionGroupFactory",
            "AttributeOptionFactory", "PartnerFactory",
            "ProductCategoryFactory", "CategoryFactory", "RangeFactory",
            "ProductClassFactory"]

sites/sandbox/fixtures/child_products.json

@@ -80,7 +80,7 @@
         "date_created": "2013-12-12T16:34:14.023Z", 
         "product_options": [], 
         "slug": "oscar-t-shirt", 
-        "product_class": 1,
+        "product_class": null,
         "structure": "child"
     }
 },
@@ -98,7 +98,7 @@
         "date_created": "2013-12-12T16:34:32.170Z", 
         "product_options": [], 
         "slug": "oscar-t-shirt", 
-        "product_class": 1,
+        "product_class": null,
         "structure": "child"
     }
 },
@@ -116,7 +116,7 @@
         "date_created": "2013-12-12T17:32:15.016Z", 
         "product_options": [], 
         "slug": "oscar-t-shirt", 
-        "product_class": 1,
+        "product_class": null,
         "structure": "child"
     }
 },
@@ -152,7 +152,7 @@
         "date_created": "2013-12-13T11:37:08.138Z", 
         "product_options": [], 
         "slug": "commandlinefu-t-shirt", 
-        "product_class": 1,
+        "product_class": null,
         "structure": "child"
     }
 },
@@ -170,7 +170,7 @@
         "date_created": "2013-12-13T11:37:45.834Z", 
         "product_options": [], 
         "slug": "commandlinefu-t-shirt", 
-        "product_class": 1,
+        "product_class": null,
         "structure": "child"
     }
 },
@@ -206,7 +206,7 @@
         "date_created": "2013-12-13T11:38:49.769Z", 
         "product_options": [], 
         "slug": "tangent-t-shirt", 
-        "product_class": 1,
+        "product_class": null,
         "structure": "child"
     }
 },
@@ -224,7 +224,7 @@
         "date_created": "2013-12-13T11:39:12.859Z", 
         "product_options": [], 
         "slug": "tangent-t-shirt", 
-        "product_class": 1,
+        "product_class": null,
         "structure": "child"
     }
 },
@@ -242,7 +242,7 @@
         "date_created": "2013-12-13T11:39:53.073Z", 
         "product_options": [], 
         "slug": "tangent-t-shirt", 
-        "product_class": 1,
+        "product_class": null,
         "structure": "child"
     }
 },

tests/functional/dashboard/catalogue_tests.py

@@ -58,7 +58,7 @@ class TestAStaffUser(WebTestCase):
         form['stockrecords-0-partner_sku'] = '14'
         form['stockrecords-0-num_in_stock'] = '555'
         form['stockrecords-0-price_excl_tax'] = '13.99'
-        page = form.submit('action', index=0)
+        page = form.submit(name='action', value='continue')
 
         self.assertEqual(Product.objects.count(), 1)
         product = Product.objects.all()[0]

tests/functional/dashboard/product_tests.py

@@ -113,14 +113,12 @@ class TestCreateChildProduct(ProductWebTest):
         super(TestCreateChildProduct, self).setUp()
 
     def test_categories_are_not_required(self):
-        url = reverse('dashboard:catalogue-product-create',
-                      kwargs={'product_class_slug': self.pclass.slug})
+        url = reverse('dashboard:catalogue-product-create-child',
+                      kwargs={'parent_pk': self.parent.pk})
         page = self.get(url)
 
         product_form = page.form
-        product_form['structure'] = 'child'
         product_form['title'] = expected_title = 'Nice T-Shirt'
-        product_form['parent'] = str(self.parent.id)
         product_form.submit()
 
         try:

tests/integration/catalogue/product_tests.py

@@ -70,13 +70,15 @@ class TopLevelProductTests(ProductTests):
         self.assertEqual(set([product]), set(Product.browsable.all()))
 
 
-class VariantProductTests(ProductTests):
+class ChildProductTests(ProductTests):
 
     def setUp(self):
-        super(VariantProductTests, self).setUp()
+        super(ChildProductTests, self).setUp()
         self.parent = Product.objects.create(
-            title="Parent product", product_class=self.product_class,
-            structure=Product.PARENT)
+            title="Parent product",
+            product_class=self.product_class,
+            structure=Product.PARENT,
+            is_discountable=False)
 
     def test_child_products_dont_need_titles(self):
         Product.objects.create(
@@ -84,19 +86,16 @@ class VariantProductTests(ProductTests):
             structure=Product.CHILD)
 
     def test_child_products_dont_need_a_product_class(self):
-        Product.objects.create(
-            parent=self.parent, structure=Product.CHILD)
+        Product.objects.create(parent=self.parent, structure=Product.CHILD)
 
-    def test_child_products_inherit_parent_titles(self):
+    def test_child_products_inherit_fields(self):
         p = Product.objects.create(
-            parent=self.parent, product_class=self.product_class,
-            structure=Product.CHILD)
+            parent=self.parent,
+            structure=Product.CHILD,
+            is_discountable=True)
         self.assertEqual("Parent product", p.get_title())
-
-    def test_child_products_inherit_product_class(self):
-        p = Product.objects.create(
-            parent=self.parent, structure=Product.CHILD)
         self.assertEqual("Clothing", p.get_product_class().name)
+        self.assertEqual(False, p.get_is_discountable())
 
     def test_child_products_are_not_part_of_browsable_set(self):
         Product.objects.create(

tests/unit/dashboard/catalogue_form_tests.py

@@ -1,24 +1,25 @@
 from django.test import TestCase
-from django_dynamic_fixture import G
 
-from oscar.apps.catalogue import models
 from oscar.apps.dashboard.catalogue import forms
+from oscar.test import factories
 
 
 class TestCreateProductForm(TestCase):
 
     def setUp(self):
-        self.pclass = G(models.ProductClass)
+        self.product_class = factories.ProductClassFactory()
 
-    def submit(self, data):
-        return forms.ProductForm(self.pclass, data=data)
+    def submit(self, data, parent=None):
+        return forms.ProductForm(self.product_class, parent=parent, data=data)
 
     def test_validates_that_parent_products_must_have_title(self):
         form = self.submit({'structure': 'parent'})
         self.assertFalse(form.is_valid())
+        form = self.submit({'structure': 'parent', 'title': 'foo'})
+        self.assertTrue(form.is_valid())
 
     def test_validates_that_child_products_dont_need_a_title(self):
-        parent = G(
-            models.Product, product_class=self.pclass, structure='parent')
-        form = self.submit({'structure': 'child', 'parent': parent.id})
+        parent = factories.ProductFactory(
+            product_class=self.product_class, structure='parent')
+        form = self.submit({'structure': 'child'}, parent=parent)
         self.assertTrue(form.is_valid())