Merge ~cjwatson/launchpad:distribution-edit-restricted-processors into launchpad:master

[Colin Watson]

Fix Distribution:+edit crash with enabled restricted processors

The main archive of a distribution may have an enabled processor which is restricted, such that the owners of that distribution aren't necessarily authorized to enable or disable it; riscv64 is one such for Ubuntu at the moment. This shouldn't prevent distribution owners from making other changes in the distribution's +edit view.

PPAs already had a similar validation tweak.

[Jürgen Gmach (jugmac00)]

I assume you have thought through all the alternatives.

LGishTM :-)

review: Approve

diff --git a/lib/lp/oci/browser/tests/test_ocirecipe.py b/lib/lp/oci/browser/tests/test_ocirecipe.py
index 97fc3c9..b135ed8 100644
--- a/lib/lp/oci/browser/tests/test_ocirecipe.py
+++ b/lib/lp/oci/browser/tests/test_ocirecipe.py
@@ -336,7 +336,8 @@ class TestOCIRecipeAddView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
         self.assertContentEqual(["386", "amd64", "hppa"], processors.value)
 
     def test_create_new_recipe_display_restricted_processors(self):
-        # A restricted processor is shown disabled in the UI.
+        # A restricted processor is shown with a disabled (greyed out)
+        # checkbox in the UI.
         self.setUpDistroSeries()
         oci_project = self.factory.makeOCIProject(pillar=self.distribution)
         proc_armhf = self.factory.makeProcessor(
@@ -900,8 +901,8 @@ class TestOCIRecipeEditView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
         self.assertRecipeProcessors(recipe, ["amd64", "armel"])
 
     def test_edit_processors_restricted(self):
-        # A restricted processor is shown disabled in the UI and cannot be
-        # enabled.
+        # A restricted processor is shown with a disabled (greyed out)
+        # checkbox in the UI, and the processor cannot be enabled.
         self.setUpDistroSeries()
         proc_armhf = self.factory.makeProcessor(
             name="armhf", restricted=True, build_by_default=False)
@@ -929,10 +930,11 @@ class TestOCIRecipeEditView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
             browser.getControl("Update OCI recipe").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 CannotModifyOCIRecipeProcessor.
+        # A restricted processor that is already enabled is shown with a
+        # disabled (greyed out) checkbox 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
+        # CannotModifyOCIRecipeProcessor.
         proc_386 = getUtility(IProcessorSet).getByName("386")
         proc_amd64 = getUtility(IProcessorSet).getByName("amd64")
         proc_armhf = self.factory.makeProcessor(
diff --git a/lib/lp/registry/browser/distribution.py b/lib/lp/registry/browser/distribution.py
index 303ea20..5fccb6c 100644
--- a/lib/lp/registry/browser/distribution.py
+++ b/lib/lp/registry/browser/distribution.py
@@ -1078,6 +1078,14 @@ class DistributionEditView(RegistryEditFormView,
         official_malone = data.get('official_malone', False)
         if not official_malone:
             data['enable_bug_expiration'] = False
+        if 'processors' in data:
+            widget = self.widgets['processors']
+            for processor in self.context.main_archive.processors:
+                if processor not in data['processors']:
+                    if processor.name in widget.disabled_items:
+                        # This processor is restricted and currently
+                        # enabled.  Leave it untouched.
+                        data['processors'].append(processor)
 
     def change_archive_fields(self, data):
         # Update context.main_archive.
diff --git a/lib/lp/registry/browser/tests/test_distribution_views.py b/lib/lp/registry/browser/tests/test_distribution_views.py
index 44ddf0d..e50d458 100644
--- a/lib/lp/registry/browser/tests/test_distribution_views.py
+++ b/lib/lp/registry/browser/tests/test_distribution_views.py
@@ -1,8 +1,12 @@
 # Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from fixtures import FakeLogger
 import soupmatchers
-from testtools.matchers import MatchesStructure
+from testtools.matchers import (
+    MatchesSetwise,
+    MatchesStructure,
+    )
 import transaction
 from zope.component import getUtility
 
@@ -17,12 +21,15 @@ from lp.registry.interfaces.distributionmirror import (
     MirrorContent,
     MirrorStatus,
     )
+from lp.services.webapp.publisher import canonical_url
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.services.worlddata.interfaces.country import ICountrySet
+from lp.soyuz.interfaces.archive import CannotModifyArchiveProcessor
 from lp.testing import (
     login,
     login_celebrity,
     login_person,
+    person_logged_in,
     record_two_runs,
     TestCaseWithFactory,
     )
@@ -255,6 +262,70 @@ class TestDistroEditView(OCIConfigHelperMixin, TestCaseWithFactory):
 
         self.assertContentEqual([], self.distribution.main_archive.processors)
 
+    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_edit_processors_restricted(self):
+        # A restricted processor is shown with a disabled (greyed out)
+        # checkbox in the UI, and the processor cannot be enabled.
+        self.useFixture(FakeLogger())
+        self.factory.makeProcessor(
+            name="riscv64", restricted=True, build_by_default=False)
+        login_person(self.distribution.owner)
+        browser = self.getUserBrowser(
+            canonical_url(self.distribution) + "/+edit",
+            user=self.distribution.owner)
+        processors = browser.getControl(name="field.processors")
+        self.assertContentEqual(["386", "amd64", "hppa"], processors.value)
+        self.assertProcessorControls(
+            processors, ["386", "amd64", "hppa"], ["riscv64"])
+        # 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:
+            if control.optionValue == "riscv64":
+                del control._control.attrs["disabled"]
+        processors.value = ["386", "amd64", "riscv64"]
+        self.assertRaises(
+            CannotModifyArchiveProcessor,
+            browser.getControl(name="field.actions.change").click)
+
+    def test_edit_processors_restricted_already_enabled(self):
+        # A restricted processor that is already enabled is shown with a
+        # disabled (greyed out) checkbox 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_riscv64 = self.factory.makeProcessor(
+            name="riscv64", restricted=True, build_by_default=False)
+        login_person(self.distribution.owner)
+        archive = self.distribution.main_archive
+        archive.setProcessors([proc_386, proc_amd64, proc_riscv64])
+        self.assertArchiveProcessors(archive, ["386", "amd64", "riscv64"])
+        browser = self.getUserBrowser(
+            canonical_url(self.distribution) + "/+edit",
+            user=self.distribution.owner)
+        processors = browser.getControl(name="field.processors")
+        # riscv64 is checked but disabled.
+        self.assertContentEqual(["386", "amd64", "riscv64"], processors.value)
+        self.assertProcessorControls(
+            processors, ["386", "amd64", "hppa"], ["riscv64"])
+        processors.value = ["386"]
+        browser.getControl(name="field.actions.change").click()
+        self.assertArchiveProcessors(archive, ["386", "riscv64"])
+
     def test_package_derivatives_email(self):
         # Test that the edit form allows changing package_derivatives_email
         edit_form = self.getDefaultEditDict()
diff --git a/lib/lp/snappy/browser/tests/test_snap.py b/lib/lp/snappy/browser/tests/test_snap.py
index d3f4bca..fc683c4 100644
--- a/lib/lp/snappy/browser/tests/test_snap.py
+++ b/lib/lp/snappy/browser/tests/test_snap.py
@@ -629,7 +629,8 @@ class TestSnapAddView(BaseTestSnapView):
         self.assertContentEqual(["386", "amd64", "hppa"], processors.value)
 
     def test_create_new_snap_display_restricted_processors(self):
-        # A restricted processor is shown disabled in the UI.
+        # A restricted processor is shown with a disabled (greyed out)
+        # checkbox in the UI.
         self.useFixture(BranchHostingFixture(blob=b""))
         branch = self.factory.makeAnyBranch()
         distroseries = self.setUpDistroSeries()
@@ -1311,8 +1312,8 @@ class TestSnapEditView(BaseTestSnapView):
         self.assertSnapProcessors(snap, ["amd64", "armel"])
 
     def test_edit_processors_restricted(self):
-        # A restricted processor is shown disabled in the UI and cannot be
-        # enabled.
+        # A restricted processor is shown with a disabled (greyed out)
+        # checkbox in the UI, and the processor cannot be enabled.
         distroseries, snappyseries = self.setUpSeries()
         proc_armhf = self.factory.makeProcessor(
             name="armhf", restricted=True, build_by_default=False)
@@ -1339,10 +1340,10 @@ class TestSnapEditView(BaseTestSnapView):
             browser.getControl("Update snap package").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 CannotModifySnapProcessor.
+        # A restricted processor that is already enabled is shown with a
+        # disabled (greyed out) checkbox 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 CannotModifySnapProcessor.
         proc_386 = getUtility(IProcessorSet).getByName("386")
         proc_amd64 = getUtility(IProcessorSet).getByName("amd64")
         proc_armhf = self.factory.makeProcessor(
diff --git a/lib/lp/soyuz/browser/tests/test_archive.py b/lib/lp/soyuz/browser/tests/test_archive.py
index 07e8614..040a1e4 100644
--- a/lib/lp/soyuz/browser/tests/test_archive.py
+++ b/lib/lp/soyuz/browser/tests/test_archive.py
@@ -125,8 +125,8 @@ class TestArchiveEditView(TestCaseWithFactory):
         self.assertArchiveProcessors(ppa, ["amd64", "armel"])
 
     def test_edit_processors_restricted(self):
-        # A restricted processor is shown disabled in the UI and cannot be
-        # enabled.
+        # A restricted processor is shown with a disabled (greyed out)
+        # checkbox in the UI, and the processor cannot be enabled.
         self.useFixture(FakeLogger())
         proc_armhf = self.factory.makeProcessor(
             name="armhf", restricted=True, build_by_default=False)
@@ -151,10 +151,11 @@ class TestArchiveEditView(TestCaseWithFactory):
             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.
+        # A restricted processor that is already enabled is shown with a
+        # disabled (greyed out) checkbox 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(