Delete both valid and invalid basket lines marked for deletion

Currently, if a valid basket line is marked for deletion, any other
invalid basket lines on the formset prevent it from being deleted.

docs/source/releases/v2.1.rst

@@ -120,6 +120,10 @@ Bug fixes
   in a discount being applied for all items in the basket rather than the
   cheapest one.
 
+- Fixed a bug where a line could not be deleted from the basket, if the basket
+  had another line with a validation error - i.e. its product was out of stock
+  (see :issue:`2791` and :issue:`1654`).
+
 Removal of deprecated features
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

src/oscar/apps/basket/views.py

@@ -1,7 +1,7 @@
 from django import shortcuts
 from django.contrib import messages
 from django.core.exceptions import ObjectDoesNotExist
-from django.http import JsonResponse
+from django.http import JsonResponse, QueryDict
 from django.shortcuts import redirect
 from django.template.loader import render_to_string
 from django.urls import reverse
@@ -229,9 +229,17 @@ class BasketView(ModelFormSetView):
         saved_basket.merge_line(line)
 
     def formset_invalid(self, formset):
+        has_deletion = any(formset._should_delete_form(form) for form in formset.forms)
+        has_no_invalid_non_deletion = all(form.is_valid() or formset._should_delete_form(form)
+                                          for form in formset.forms)
+        if has_deletion:
+            self.remove_deleted_forms(formset)
+            if has_no_invalid_non_deletion:
+                return self.formset_valid(formset)
+
         flash_messages = ajax.FlashMessages()
         flash_messages.warning(_(
-            "Your basket couldn't be updated. "
+            "Your basket has got some issues. "
             "Please correct any validation errors below."))
 
         if self.request.is_ajax():
@@ -242,6 +250,53 @@ class BasketView(ModelFormSetView):
         flash_messages.apply_to_request(self.request)
         return super().formset_invalid(formset)
 
+    def remove_deleted_forms(self, formset):
+        """
+        Removes forms marked for deletion, from the formset, as well as deletes
+        their model instance objects; and modifies the formset's request data,
+        to match the state of the data in the database, for the remaining forms.
+
+        This is useful for redisplaying a formset containing other invalid
+        forms, after deleting one of the forms from it.
+        """
+        form_data = {}
+        form_index = 0
+        for form in formset.forms:
+            # Delete forms marked for deletion, and retain the request data
+            # for the other forms
+            if formset._should_delete_form(form):
+                if form.instance.id is not None:
+                    form.instance.delete()
+            else:
+                old_form_prefix = form.prefix
+                new_form_prefix = formset.add_prefix(form_index)
+                for field_name in form.fields:
+                    form.prefix = old_form_prefix
+                    old_prefixed_field_name = form.add_prefix(field_name)
+                    form.prefix = new_form_prefix
+                    new_prefixed_field_name = form.add_prefix(field_name)
+                    try:
+                        form_data[new_prefixed_field_name] = formset.data[old_prefixed_field_name]
+                    except KeyError:
+                        pass
+                form_index += 1
+        for field_name in formset.management_form.fields:
+            prefixed_field_name = formset.management_form.add_prefix(field_name)
+            if field_name in ['INITIAL_FORMS', 'TOTAL_FORMS']:
+                form_data[prefixed_field_name] = str(form_index)
+            else:
+                form_data[prefixed_field_name] = formset.data[prefixed_field_name]
+
+        query_dict = QueryDict(mutable=True)
+        query_dict.update(form_data)
+        formset.data = query_dict
+        # Clear cached values, so that they are recomputed using the modified
+        # request data
+        del formset.management_form
+        del formset.forms
+        # Clean the formset's modified request data
+        formset.full_clean()
+
 
 class BasketAddView(FormView):
     """

tests/functional/test_basket.py

@@ -318,6 +318,7 @@ class BasketFormSetTests(WebTestCase):
         management_form = formset.management_form
         data = {
             formset.add_prefix('INITIAL_FORMS'): management_form.initial['INITIAL_FORMS'],
+            formset.add_prefix('MIN_NUM_FORMS'): management_form.initial['MIN_NUM_FORMS'],
             formset.add_prefix('MAX_NUM_FORMS'): management_form.initial['MAX_NUM_FORMS'],
             formset.add_prefix('TOTAL_FORMS'): management_form.initial['TOTAL_FORMS'],
             'form-0-quantity': 1,
@@ -330,7 +331,151 @@ class BasketFormSetTests(WebTestCase):
         response = self.post(reverse('basket:summary'), params=data)
         self.assertEqual(response.status_code, 200)
         formset = response.context['formset']
-        self.assertEqual(len(formset.forms), 3)
+        self.assertEqual(len(formset.forms), 2)
         self.assertEqual(len(formset.forms_with_instances), 2)
         self.assertEqual(basket.lines.all()[0].quantity, 1)
         self.assertEqual(basket.lines.all()[1].quantity, 1)
+
+    def test_deleting_valid_line_with_other_valid_line(self):
+        product_1 = create_product()
+        product_2 = create_product()
+
+        basket = factories.create_basket(empty=True)
+        basket.owner = self.user
+        basket.save()
+        add_product(basket, product=product_1)
+        add_product(basket, product=product_2)
+
+        response = self.get(reverse('basket:summary'))
+        formset = response.context['formset']
+        self.assertEqual(len(formset.forms), 2)
+
+        data = {
+            formset.add_prefix('TOTAL_FORMS'): formset.management_form.initial['TOTAL_FORMS'],
+            formset.add_prefix('INITIAL_FORMS'): formset.management_form.initial['INITIAL_FORMS'],
+            formset.add_prefix('MIN_NUM_FORMS'): formset.management_form.initial['MIN_NUM_FORMS'],
+            formset.add_prefix('MAX_NUM_FORMS'): formset.management_form.initial['MAX_NUM_FORMS'],
+            formset.forms[0].add_prefix('id'): formset.forms[0].instance.pk,
+            formset.forms[0].add_prefix('quantity'): formset.forms[0].instance.quantity,
+            formset.forms[0].add_prefix('DELETE'): 'on',
+            formset.forms[1].add_prefix('id'): formset.forms[1].instance.pk,
+            formset.forms[1].add_prefix('quantity'): formset.forms[1].instance.quantity,
+        }
+        response = self.post(reverse('basket:summary'), params=data, xhr=True)
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(len(response.context['formset'].forms), 1)
+        self.assertFalse(response.context['formset'].is_bound)  # new formset is rendered
+        self.assertEqual(basket.lines.count(), 1)
+        self.assertEqual(basket.lines.all()[0].quantity, 1)
+
+    def test_deleting_valid_line_with_other_invalid_line(self):
+        product_1 = create_product()
+        product_2 = create_product()
+
+        basket = factories.create_basket(empty=True)
+        basket.owner = self.user
+        basket.save()
+        add_product(basket, product=product_1)
+        add_product(basket, product=product_2)
+
+        response = self.get(reverse('basket:summary'))
+        formset = response.context['formset']
+        self.assertEqual(len(formset.forms), 2)
+
+        # Render product for other line out of stock
+        product_2.stockrecords.update(num_in_stock=0)
+
+        data = {
+            formset.add_prefix('TOTAL_FORMS'): formset.management_form.initial['TOTAL_FORMS'],
+            formset.add_prefix('INITIAL_FORMS'): formset.management_form.initial['INITIAL_FORMS'],
+            formset.add_prefix('MIN_NUM_FORMS'): formset.management_form.initial['MIN_NUM_FORMS'],
+            formset.add_prefix('MAX_NUM_FORMS'): formset.management_form.initial['MAX_NUM_FORMS'],
+            formset.forms[0].add_prefix('id'): formset.forms[0].instance.pk,
+            formset.forms[0].add_prefix('quantity'): formset.forms[0].instance.quantity,
+            formset.forms[0].add_prefix('DELETE'): 'on',
+            formset.forms[1].add_prefix('id'): formset.forms[1].instance.pk,
+            formset.forms[1].add_prefix('quantity'): formset.forms[1].instance.quantity,
+        }
+        response = self.post(reverse('basket:summary'), params=data, xhr=True)
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(len(response.context['formset'].forms), 1)
+        self.assertTrue(response.context['formset'].is_bound)  # formset with errors is rendered
+        self.assertFalse(response.context['formset'].forms[0].is_valid())
+        self.assertEqual(basket.lines.count(), 1)
+        self.assertEqual(basket.lines.all()[0].quantity, 1)
+
+    def test_deleting_invalid_line_with_other_valid_line(self):
+        product_1 = create_product()
+        product_2 = create_product()
+
+        basket = factories.create_basket(empty=True)
+        basket.owner = self.user
+        basket.save()
+        add_product(basket, product=product_1)
+        add_product(basket, product=product_2)
+
+        response = self.get(reverse('basket:summary'))
+        formset = response.context['formset']
+        self.assertEqual(len(formset.forms), 2)
+
+        # Render product for to-be-deleted line out of stock
+        product_1.stockrecords.update(num_in_stock=0)
+
+        data = {
+            formset.add_prefix('TOTAL_FORMS'): formset.management_form.initial['TOTAL_FORMS'],
+            formset.add_prefix('INITIAL_FORMS'): formset.management_form.initial['INITIAL_FORMS'],
+            formset.add_prefix('MIN_NUM_FORMS'): formset.management_form.initial['MIN_NUM_FORMS'],
+            formset.add_prefix('MAX_NUM_FORMS'): formset.management_form.initial['MAX_NUM_FORMS'],
+            formset.forms[0].add_prefix('id'): formset.forms[0].instance.pk,
+            formset.forms[0].add_prefix('quantity'): formset.forms[0].instance.quantity,
+            formset.forms[0].add_prefix('DELETE'): 'on',
+            formset.forms[1].add_prefix('id'): formset.forms[1].instance.pk,
+            formset.forms[1].add_prefix('quantity'): formset.forms[1].instance.quantity,
+        }
+        response = self.post(reverse('basket:summary'), params=data, xhr=True)
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(len(response.context['formset'].forms), 1)
+        self.assertFalse(response.context['formset'].is_bound)  # new formset is rendered
+        self.assertEqual(basket.lines.count(), 1)
+        self.assertEqual(basket.lines.all()[0].quantity, 1)
+
+    def test_deleting_invalid_line_with_other_invalid_line(self):
+        product_1 = create_product()
+        product_2 = create_product()
+
+        basket = factories.create_basket(empty=True)
+        basket.owner = self.user
+        basket.save()
+        add_product(basket, product=product_1)
+        add_product(basket, product=product_2)
+
+        response = self.get(reverse('basket:summary'))
+        formset = response.context['formset']
+        self.assertEqual(len(formset.forms), 2)
+
+        # Render products for both lines out of stock
+        product_1.stockrecords.update(num_in_stock=0)
+        product_2.stockrecords.update(num_in_stock=0)
+
+        data = {
+            formset.add_prefix('TOTAL_FORMS'): formset.management_form.initial['TOTAL_FORMS'],
+            formset.add_prefix('INITIAL_FORMS'): formset.management_form.initial['INITIAL_FORMS'],
+            formset.add_prefix('MIN_NUM_FORMS'): formset.management_form.initial['MIN_NUM_FORMS'],
+            formset.add_prefix('MAX_NUM_FORMS'): formset.management_form.initial['MAX_NUM_FORMS'],
+            formset.forms[0].add_prefix('id'): formset.forms[0].instance.pk,
+            formset.forms[0].add_prefix('quantity'): formset.forms[0].instance.quantity,
+            formset.forms[0].add_prefix('DELETE'): 'on',
+            formset.forms[1].add_prefix('id'): formset.forms[1].instance.pk,
+            formset.forms[1].add_prefix('quantity'): formset.forms[1].instance.quantity,
+        }
+        response = self.post(reverse('basket:summary'), params=data, xhr=True)
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(len(response.context['formset'].forms), 1)
+        self.assertTrue(response.context['formset'].is_bound)  # formset with errors is rendered
+        self.assertFalse(response.context['formset'].forms[0].is_valid())
+        self.assertEqual(basket.lines.count(), 1)
+        self.assertEqual(basket.lines.all()[0].quantity, 1)