launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/archive-edit-fix-restricted-processors into lp:launchpad.

Commit message:
Ensure that enabled and restricted processors are left untouched when submitting Archive:+edit.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1501519 in Launchpad itself: "Editing archives with enabled restricted processors OOPSes"
  https://bugs.launchpad.net/launchpad/+bug/1501519

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/archive-edit-fix-restricted-processors/+merge/272999

Ensure that enabled and restricted processors are left untouched when submitting Archive:+edit.  This is much the same strategy that we use for enabled and unavailable processors.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/archive-edit-fix-restricted-processors into lp:launchpad.

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2015-09-23 12:15:53 +0000
+++ lib/lp/soyuz/browser/archive.py	2015-09-30 23:39:16 +0000
@@ -2085,12 +2085,17 @@
     def validate(self, data):
         if 'processors' in data:
             available_processors = set(self.context.available_processors)
+            widget = self.widgets['processors']
             for processor in self.context.processors:
-                if (processor not in available_processors and
-                        processor not in data['processors']):
-                    # This processor is not currently available for
-                    # selection, but is enabled.  Leave it untouched.
-                    data['processors'].append(processor)
+                if processor not in data['processors']:
+                    if processor not in available_processors:
+                        # This processor is not currently available for
+                        # selection, but is enabled.  Leave it untouched.
+                        data['processors'].append(processor)
+                    elif processor.name in widget.disabled_items:
+                        # This processor is restricted and currently
+                        # enabled.  Leave it untouched.
+                        data['processors'].append(processor)
 
 
 class ArchiveAdminView(BaseArchiveEditView, EnableProcessorsMixin):

=== modified file 'lib/lp/soyuz/browser/tests/test_archive.py'
--- lib/lp/soyuz/browser/tests/test_archive.py	2015-09-25 13:54:46 +0000
+++ lib/lp/soyuz/browser/tests/test_archive.py	2015-09-30 23:39:16 +0000
@@ -18,6 +18,7 @@
 from lp.testing import (
     admin_logged_in,
     login_person,
+    person_logged_in,
     record_two_runs,
     TestCaseWithFactory,
     )
@@ -44,6 +45,20 @@
             distroseries=self.ubuntu.getSeries("breezy-autotest"),
             architecturetag="amd64", processor=proc_amd64)
 
+    def assertArchiveProcessors(self, archive, names):
+        with person_logged_in(archive.owner):
+            self.assertContentEqual(
+                names, [processor.name for processor in archive.processors])
+
+    def assertProcessorControls(self, processors_control, enabled, disabled):
+        matchers = [
+            MatchesStructure.byEquality(optionValue=name, disabled=False)
+            for name in enabled]
+        matchers.extend([
+            MatchesStructure.byEquality(optionValue=name, disabled=True)
+            for name in disabled])
+        self.assertThat(processors_control.controls, MatchesSetwise(*matchers))
+
     def test_display_processors(self):
         ppa = self.factory.makeArchive()
         owner = login_person(ppa.owner)
@@ -57,20 +72,14 @@
 
     def test_edit_processors(self):
         ppa = self.factory.makeArchive()
-        owner = login_person(ppa.owner)
-        self.assertContentEqual(
-            ["386", "amd64", "hppa"],
-            [processor.name for processor in ppa.processors])
+        self.assertArchiveProcessors(ppa, ["386", "amd64", "hppa"])
         browser = self.getUserBrowser(
-            canonical_url(ppa) + "/+edit", user=owner)
+            canonical_url(ppa) + "/+edit", user=ppa.owner)
         processors = browser.getControl(name="field.processors")
         self.assertContentEqual(["386", "amd64", "hppa"], processors.value)
         processors.value = ["386", "amd64"]
         browser.getControl("Save").click()
-        login_person(ppa.owner)
-        self.assertContentEqual(
-            ["386", "amd64"],
-            [processor.name for processor in ppa.processors])
+        self.assertArchiveProcessors(ppa, ["386", "amd64"])
 
     def test_edit_with_invisible_processor(self):
         # It's possible for existing archives to have an enabled processor
@@ -84,19 +93,14 @@
         proc_armel = self.factory.makeProcessor(
             name="armel", restricted=True, build_by_default=False)
         ppa = self.factory.makeArchive()
-        with admin_logged_in():
-            ppa.setProcessors([proc_386, proc_amd64, proc_armel])
-        owner = login_person(ppa.owner)
+        ppa.setProcessors([proc_386, proc_amd64, proc_armel])
         browser = self.getUserBrowser(
-            canonical_url(ppa) + "/+edit", user=owner)
+            canonical_url(ppa) + "/+edit", user=ppa.owner)
         processors = browser.getControl(name="field.processors")
         self.assertContentEqual(["386", "amd64"], processors.value)
         processors.value = ["amd64"]
         browser.getControl("Save").click()
-        login_person(ppa.owner)
-        self.assertContentEqual(
-            ["amd64", "armel"],
-            [processor.name for processor in ppa.processors])
+        self.assertArchiveProcessors(ppa, ["amd64", "armel"])
 
     def test_edit_processors_restricted(self):
         # A restricted processor is shown disabled in the UI and cannot be
@@ -108,25 +112,13 @@
             distroseries=self.ubuntu.getSeries("breezy-autotest"),
             architecturetag="armhf", processor=proc_armhf)
         ppa = self.factory.makeArchive()
-        owner = login_person(ppa.owner)
-        self.assertContentEqual(
-            ["386", "amd64", "hppa"],
-            [processor.name for processor in ppa.processors])
+        self.assertArchiveProcessors(ppa, ["386", "amd64", "hppa"])
         browser = self.getUserBrowser(
-            canonical_url(ppa) + "/+edit", user=owner)
+            canonical_url(ppa) + "/+edit", user=ppa.owner)
         processors = browser.getControl(name="field.processors")
         self.assertContentEqual(["386", "amd64", "hppa"], processors.value)
-        self.assertThat(
-            processors.controls, MatchesSetwise(
-                MatchesStructure.byEquality(
-                    optionValue="386", disabled=False),
-                MatchesStructure.byEquality(
-                    optionValue="amd64", disabled=False),
-                MatchesStructure.byEquality(
-                    optionValue="armhf", disabled=True),
-                MatchesStructure.byEquality(
-                    optionValue="hppa", disabled=False),
-                ))
+        self.assertProcessorControls(
+            processors, ["386", "amd64", "hppa"], ["armhf"])
         # Even if the user works around the disabled checkbox and forcibly
         # enables it, they can't enable the restricted processor.
         for control in processors.controls:
@@ -136,6 +128,31 @@
         self.assertRaises(
             CannotModifyArchiveProcessor, browser.getControl("Save").click)
 
+    def test_edit_processors_restricted_already_enabled(self):
+        # A restricted processor that is already enabled is shown disabled
+        # in the UI.  This causes form submission to omit it, but the
+        # validation code fixes that up behind the scenes so that we don't
+        # get CannotModifyArchiveProcessor.
+        proc_386 = getUtility(IProcessorSet).getByName("386")
+        proc_amd64 = getUtility(IProcessorSet).getByName("amd64")
+        proc_armhf = self.factory.makeProcessor(
+            name="armhf", restricted=True, build_by_default=False)
+        self.factory.makeDistroArchSeries(
+            distroseries=self.ubuntu.getSeries("breezy-autotest"),
+            architecturetag="armhf", processor=proc_armhf)
+        ppa = self.factory.makeArchive()
+        ppa.setProcessors([proc_386, proc_amd64, proc_armhf])
+        self.assertArchiveProcessors(ppa, ["386", "amd64", "armhf"])
+        browser = self.getUserBrowser(
+            canonical_url(ppa) + "/+edit", user=ppa.owner)
+        processors = browser.getControl(name="field.processors")
+        self.assertContentEqual(["386", "amd64"], processors.value)
+        self.assertProcessorControls(
+            processors, ["386", "amd64", "hppa"], ["armhf"])
+        processors.value = ["386"]
+        browser.getControl("Save").click()
+        self.assertArchiveProcessors(ppa, ["386", "armhf"])
+
 
 class TestArchiveCopyPackagesView(TestCaseWithFactory):