launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-802840 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #802840 in Launchpad itself: "Oops when not overriding sync upload's component"
  https://bugs.launchpad.net/launchpad/+bug/802840

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-802840/+merge/66775

= 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
-- 
https://code.launchpad.net/~jtv/launchpad/bug-802840/+merge/66775
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-802840 into lp:launchpad.

=== 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-04 11:34:47 +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-04 11:34:47 +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-04 11:34:47 +0000
@@ -795,6 +795,34 @@
             section=Equals(metadata_section))
         self.assertThat(override, matcher)
 
+    def test_addSourceOverride_accepts_None_component_as_no_change(self):
+        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):
+        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-04 11:34:47 +0000
@@ -428,6 +428,14 @@
             pu.overrideSource,
             disallowed_component, section, [current_component])
 
+    def test_overrideSource_ignores_None_component_change(self):
+        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.