Merge lp:~jtv/launchpad/bug-802840 into lp:launchpad

[Jeroen T. Vermeulen]

Ignore None component/section on sync upload source override.

= Summary =

When setting an override on a sync-type package upload, leaving out the component causes an oops during the component permissions check.

(Background note: an override sets the upload's component and section).

== Proposed fix ==

Treat the None component setting as "no change," which is permitted.

== Pre-implementation notes ==

Discussed with Julian. Proper behaviour is to permit the change as long as the usual permission check on the existing component still passes. If it doesn't, then the entire override attempt will (and should) still fail. There is no similar check for sections; the component permission check basically verifies whether you are allowed to set overrides in the current component.

== Implementation details ==

An "or" in the "if" would have required a line break or a helper variable. I just appended None to the list of valid components.

== Tests ==

{{{
./bin/test -vvc lp.soyuz.tests.test_packagecopyjob -t Override
./bin/test -vvc lp.soyuz.tests.test_packageupload -t Override
}}}

== Demo and Q/A ==

Set an override on a sync-type package upload on the +queue page (you recognize these by the fact that the name of the upload is not a link) but set no component. This should now work.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/model/queue.py
  lib/lp/soyuz/model/packagecopyjob.py
  lib/lp/soyuz/tests/test_packagecopyjob.py
  lib/lp/soyuz/tests/test_packageupload.py

[Graham Binns (gmb)]

From IRC:

<gmb> jtv: Lazy reviewer alert: Those tests could do with some leading comments. I know you've tried to make the test names meaningful and descriptive, but it still took me several seconds to parse them, and I'm fundamentally lazy.

Other than that, r=me.

review: Approve (code)

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2011-06-24 06:56:17 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2011-07-05 09:26:17 +0000
@@ -371,16 +371,12 @@
 
     def addSourceOverride(self, override):
         """Add an `ISourceOverride` to the metadata."""
-        component = ""
-        section = ""
+        metadata_changes = {}
         if override.component is not None:
-            component = override.component.name
+            metadata_changes['component_override'] = override.component.name
         if override.section is not None:
-            section = override.section.name
-        metadata_dict = dict(
-            component_override=component,
-            section_override=section)
-        self.context.extendMetadata(metadata_dict)
+            metadata_changes['section_override'] = override.section.name
+        self.context.extendMetadata(metadata_changes)
 
     def getSourceOverride(self):
         """Fetch an `ISourceOverride` from the metadata."""

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2011-06-28 13:30:55 +0000
+++ lib/lp/soyuz/model/queue.py	2011-07-05 09:26:17 +0000
@@ -929,7 +929,7 @@
             # Nothing needs overriding, bail out.
             return False
 
-        if new_component not in allowed_components:
+        if new_component not in list(allowed_components) + [None]:
             raise QueueInconsistentStateError(
                 "No rights to override to %s" % new_component.name)
 

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2011-06-24 05:57:31 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2011-07-05 09:26:17 +0000
@@ -795,6 +795,40 @@
             section=Equals(metadata_section))
         self.assertThat(override, matcher)
 
+    def test_addSourceOverride_accepts_None_component_as_no_change(self):
+        # When given an override with None as the component,
+        # addSourceOverride will update the section but not the
+        # component.
+        pcj = self.factory.makePlainPackageCopyJob()
+        old_component = self.factory.makeComponent()
+        old_section = self.factory.makeSection()
+        pcj.addSourceOverride(SourceOverride(
+            source_package_name=pcj.package_name,
+            component=old_component, section=old_section))
+        new_section = self.factory.makeSection()
+        pcj.addSourceOverride(SourceOverride(
+            source_package_name=pcj.package_name,
+            component=None, section=new_section))
+        self.assertEqual(old_component.name, pcj.component_name)
+        self.assertEqual(new_section.name, pcj.section_name)
+
+    def test_addSourceOverride_accepts_None_section_as_no_change(self):
+        # When given an override with None for the section,
+        # addSourceOverride will update the component but not the
+        # section.
+        pcj = self.factory.makePlainPackageCopyJob()
+        old_component = self.factory.makeComponent()
+        old_section = self.factory.makeSection()
+        pcj.addSourceOverride(SourceOverride(
+            source_package_name=pcj.package_name,
+            component=old_component, section=old_section))
+        new_component = self.factory.makeComponent()
+        pcj.addSourceOverride(SourceOverride(
+            source_package_name=pcj.package_name,
+            component=new_component, section=None))
+        self.assertEqual(new_component.name, pcj.component_name)
+        self.assertEqual(old_section.name, pcj.section_name)
+
     def test_getSourceOverride(self):
         # Test the getSourceOverride which gets an ISourceOverride from
         # the metadata.

=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py	2011-06-23 13:05:52 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py	2011-07-05 09:26:17 +0000
@@ -428,6 +428,16 @@
             pu.overrideSource,
             disallowed_component, section, [current_component])
 
+    def test_overrideSource_ignores_None_component_change(self):
+        # overrideSource accepts None as a component; it will not object
+        # based on permissions for the new component.
+        pu, pcj = self.makeUploadWithPackageCopyJob()
+        current_component = getUtility(IComponentSet)[pcj.component_name]
+        new_section = self.factory.makeSection()
+        pu.overrideSource(None, new_section, [current_component])
+        self.assertEqual(current_component.name, pcj.component_name)
+        self.assertEqual(new_section.name, pcj.section_name)
+
     def test_acceptFromQueue_with_copy_job(self):
         # acceptFromQueue should accept the upload and resume the copy
         # job.