raise a validation error when not all duped versions are blocked together (#21013)

* raise a validation error when not all duped versions are blocked together

* re-add self.changed_version_ids_choices that was refactored away

src/olympia/blocklist/forms.py

@@ -1,3 +1,4 @@
+from collections import defaultdict
 from datetime import datetime, timedelta
 
 from django import forms
@@ -178,17 +179,18 @@ class BlocklistSubmissionForm(AMOModelForm):
                 for block in self.blocks
             ]
 
-            field.widget.choices = [
+            self.changed_version_ids_choices = [
                 v_id for _guid, opts in field.choices for (v_id, _text) in opts
             ]
             if not data and not self.initial.get('changed_version_ids'):
                 # preselect all the options
-                self.initial['changed_version_ids'] = field.widget.choices
+                self.initial['changed_version_ids'] = self.changed_version_ids_choices
         else:
             field.choices = list(
                 (v_id, v_id) for v_id in self.instance.changed_version_ids
             )
-            field.widget.choices = self.instance.changed_version_ids
+            self.changed_version_ids_choices = self.instance.changed_version_ids
+        field.widget.choices = self.changed_version_ids_choices
         field.widget.blocks = self.blocks
 
     def get_value(self, field_name, default):
@@ -201,6 +203,8 @@ class BlocklistSubmissionForm(AMOModelForm):
     def clean_changed_version_ids(self):
         data = self.cleaned_data.get('changed_version_ids', [])
         errors = []
+
+        # First, check we're not creating empty blocks
         # we're checking new blocks for add/change; and all blocks for delete
         for block in (bl for bl in self.blocks if not bl.id or not self.is_add_change):
             version_ids = [v.id for v in block.addon_versions]
@@ -211,6 +215,25 @@ class BlocklistSubmissionForm(AMOModelForm):
             if (self.is_add_change or any(blocked_ids)) and not any(changed_ids):
                 errors.append(ValidationError(f'{block.guid} has no changed versions'))
 
+        # Second, check for duplicate version strings in reused guids.
+        error_string = (
+            '{}:{} exists more than once. All {} versions must be '
+            f'{"blocked" if self.is_add_change else "unblocked"} together'
+        )
+        for block in self.blocks:
+            version_strs = defaultdict(set)  # collect version strings together
+            for version in block.addon_versions:
+                if version.id in self.changed_version_ids_choices:
+                    version_strs[version.version].add(version.id)
+            for version_str, ids in version_strs.items():
+                # i.e. dupe version string & some ids, but not all, are being changed.
+                if len(ids) > 1 and not ids.isdisjoint(data) and not ids.issubset(data):
+                    errors.append(
+                        ValidationError(
+                            error_string.format(block.guid, version_str, version_str)
+                        )
+                    )
+
         if errors:
             raise ValidationError(errors)
         return data

src/olympia/blocklist/tests/test_forms.py

@@ -13,7 +13,7 @@ from olympia.reviewers.models import ReviewActionReason
 
 from ..admin import BlocklistSubmissionAdmin
 from ..forms import MultiAddForm, MultiDeleteForm
-from ..models import Block, BlocklistSubmission
+from ..models import Block, BlocklistSubmission, BlockVersion
 
 
 class TestBlocklistSubmissionForm(TestCase):
@@ -316,6 +316,53 @@ class TestBlocklistSubmissionForm(TestCase):
         form = Form(data=data)
         assert form.is_valid(), form.errors
 
+    def test_duplicate_version_strings_must_all_be_changed(self):
+        Form = self.get_form()
+        # this version string will exist twice for this guid
+        ver_string = self.partial_existing_addon_v_notblocked.version
+        # create a previous addon instance that reused the same version string
+        old_addon = addon_factory(version_kw={'version': ver_string})
+        old_addon_version = old_addon.current_version
+        old_addon.delete()
+        old_addon.addonguid.update(guid=self.partial_existing_addon.guid)
+        # repeat but this time the version was blocked already so not a choice
+        old_blocked_addon = addon_factory(version_kw={'version': ver_string})
+        old_blocked_addon_version = old_blocked_addon.current_version
+        old_blocked_addon.delete()
+        old_blocked_addon.addonguid.update(guid=self.partial_existing_addon.guid)
+        BlockVersion.objects.create(
+            version=old_blocked_addon_version, block=self.existing_block_partial
+        )
+        data = {
+            'action': str(BlocklistSubmission.ACTION_ADDCHANGE),
+            'input_guids': f'{self.new_addon.guid}\n'
+            f'{self.existing_block_full.guid}\n'
+            f'{self.existing_block_partial.guid}',
+            'changed_version_ids': [
+                self.new_addon.current_version.id,
+            ],
+        }
+
+        # it's valid when neither of the versions are being changed
+        form = Form(data=data)
+        assert form.is_valid()
+
+        # but not when only one is
+        data['changed_version_ids'].append(self.partial_existing_addon_v_notblocked.id)
+        form = Form(data=data)
+        assert not form.is_valid()
+        assert form.errors == {
+            'changed_version_ids': [
+                f'{self.partial_existing_addon.guid}:{ver_string} exists more than once'
+                f'. All {ver_string} versions must be blocked together'
+            ]
+        }
+
+        # and it's valid again if both are being changed
+        data['changed_version_ids'].append(old_addon_version.id)
+        form = Form(data=data)
+        assert form.is_valid()
+
     def test_clean(self):
         Form = self.get_form()
         data = {