launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/set-initial-archive-daily-build into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1128655 in Launchpad itself: "Creating a recipe and then requesting a build has an incorrect PPA selected by default in the build request dialog"
  https://bugs.launchpad.net/launchpad/+bug/1128655

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/set-initial-archive-daily-build/+merge/181440

Currently, when requesting a build for a recipe, the default PPA is the first one that the user can upload too, or the last archive the recipe was built into if the user can see the details of the build. This leads to the situation when the user has to change the archive almost every time they request a build.

This branch changes the default so it is nothing, except if a daily build archive is set and the user can upload to it, in which case it becomes the default.

Refactoring some of the tests in the test module I touched to force this branch to be net-negative.
-- 
https://code.launchpad.net/~stevenk/launchpad/set-initial-archive-daily-build/+merge/181440
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/set-initial-archive-daily-build into lp:launchpad.

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2013-04-11 00:51:46 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2013-08-22 01:45:47 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """SourcePackageRecipe views."""
@@ -394,16 +394,15 @@
         The distroseries function as defaults for requesting a build.
         """
         initial_values = {'distroseries': self.context.distroseries}
-        build = self.context.last_build
-        if build:
-            # If the build can't be viewed, the archive can't.
-            if check_permission('launchpad.View', build):
-                initial_values['archive'] = build.archive
+        if self.context.daily_build_archive and check_permission(
+            'launchpad.Append', self.context.daily_build_archive):
+            initial_values['archive'] = self.context.daily_build_archive
         return initial_values
 
     class schema(Interface):
         """Schema for requesting a build."""
-        archive = Choice(vocabulary='TargetPPAs', title=u'Archive')
+        archive = Choice(
+            vocabulary='TargetPPAs', title=u'Archive', required=False)
         distroseries = List(
             Choice(vocabulary='BuildableDistroSeries'),
             title=u'Distribution series')
@@ -411,6 +410,10 @@
     custom_widget('distroseries', LabeledMultiCheckBoxWidget)
 
     def validate(self, data):
+        if not data['archive']:
+            self.setFieldError(
+                'archive', "You must specify the archive to build into.")
+            return
         distros = data.get('distroseries', [])
         if not len(distros):
             self.setFieldError('distroseries',

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2013-05-03 18:08:31 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2013-08-22 01:45:47 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the source package recipe view classes and templates."""
@@ -57,7 +57,6 @@
     time_counter,
     )
 from lp.testing.deprecated import LaunchpadFormHarness
-from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
@@ -104,8 +103,7 @@
         self.squirrel = self.factory.makeDistroSeries(
             displayname='Secret Squirrel', name='secret', version='100.04',
             distribution=self.ppa.distribution)
-        naked_squirrel = remove_security_proxy_and_shout_at_engineer(
-            self.squirrel)
+        naked_squirrel = removeSecurityProxy(self.squirrel)
         naked_squirrel.nominatedarchindep = self.squirrel.newArch(
             'i386', ProcessorFamily.get(1), False, self.chef,
             supports_virtualized=True)
@@ -1447,6 +1445,7 @@
             cake_recipe
             Request builds for cake_recipe
             Archive:
+            (nothing selected)
             Secret PPA [~chef/ppa]
             Distribution series:
             Secret Squirrel
@@ -1459,14 +1458,7 @@
 
     def test_request_builds_action(self):
         """Requesting a build creates pending builds."""
-        woody = self.factory.makeDistroSeries(
-            name='woody', displayname='Woody',
-            distribution=self.ppa.distribution)
-        naked_woody = remove_security_proxy_and_shout_at_engineer(woody)
-        naked_woody.nominatedarchindep = woody.newArch(
-            'i386', ProcessorFamily.get(1), False, self.factory.makePerson(),
-            supports_virtualized=True)
-
+        self._makeWoodyDistroSeries()
         recipe = self.makeRecipe()
         browser = self.getViewBrowser(recipe, '+request-builds')
         browser.getControl('Woody').click()
@@ -1479,44 +1471,49 @@
         build_distros.sort()
         # Secret Squirrel is checked by default.
         self.assertEqual(['Secret Squirrel', 'Woody'], build_distros)
-        self.assertEqual(
-            set([2605]),
-            set(build.buildqueue_record.lastscore for build in builds))
+        self.assertContentEqual(
+            [2605], [build.buildqueue_record.lastscore for build in builds])
 
     def test_request_builds_action_not_logged_in(self):
         """Requesting a build creates pending builds."""
-        woody = self.factory.makeDistroSeries(
-            name='woody', displayname='Woody',
-            distribution=self.ppa.distribution)
-        naked_woody = removeSecurityProxy(woody)
-        naked_woody.nominatedarchindep = woody.newArch(
-            'i386', ProcessorFamily.get(1), False, self.factory.makePerson(),
-            supports_virtualized=True)
+        self._makeWoodyDistroSeries()
         recipe = self.makeRecipe()
-
         browser = self.getViewBrowser(recipe, no_login=True)
         self.assertRaises(
             Unauthorized, browser.getLink('Request build(s)').click)
 
-    def test_request_builds_archive(self):
+    def test_request_builds_archive_no_daily_build_archive(self):
         recipe = self.factory.makeSourcePackageRecipe()
-        ppa2 = self.factory.makeArchive(
-            displayname='Secret PPA', owner=self.chef, name='ppa2')
         view = SourcePackageRecipeRequestBuildsView(recipe, None)
         self.assertIs(None, view.initial_values.get('archive'))
-        self.factory.makeSourcePackageRecipeBuild(recipe=recipe, archive=ppa2)
-        self.assertEqual(ppa2, view.initial_values.get('archive'))
+
+    def test_request_builds_archive_daily_build_archive_unuploadable(self):
+        ppa = self.factory.makeArchive()
+        recipe = self.factory.makeSourcePackageRecipe(daily_build_archive=ppa)
+        with person_logged_in(self.chef):
+            view = SourcePackageRecipeRequestBuildsView(recipe, None)
+            self.assertIs(None, view.initial_values.get('archive'))
+
+    def test_request_builds_archive(self):
+        ppa = self.factory.makeArchive(
+            displayname='Secret PPA', owner=self.chef, name='ppa2')
+        recipe = self.factory.makeSourcePackageRecipe(daily_build_archive=ppa)
+        with person_logged_in(self.chef):
+            view = SourcePackageRecipeRequestBuildsView(recipe, None)
+            self.assertEqual(ppa, view.initial_values.get('archive'))
+
+    def _makeWoodyDistroSeries(self):
+        woody = self.factory.makeDistroSeries(
+            name='woody', displayname='Woody',
+            distribution=self.ppa.distribution)
+        removeSecurityProxy(woody).nominatedarchindep = woody.newArch(
+            'i386', ProcessorFamily.get(1), False, self.factory.makePerson(),
+            supports_virtualized=True)
+        return woody
 
     def test_request_build_rejects_over_quota(self):
         """Over-quota build requests cause validation failures."""
-        woody = self.factory.makeDistroSeries(
-            name='woody', displayname='Woody',
-            distribution=self.ppa.distribution)
-        naked_woody = remove_security_proxy_and_shout_at_engineer(woody)
-        naked_woody.nominatedarchindep = woody.newArch(
-            'i386', ProcessorFamily.get(1), False, self.factory.makePerson(),
-            supports_virtualized=True)
-
+        woody = self._makeWoodyDistroSeries()
         recipe = self.makeRecipe()
         for x in range(5):
             build = recipe.requestBuild(
@@ -1532,14 +1529,7 @@
 
     def test_request_builds_rejects_duplicate(self):
         """Over-quota build requests cause validation failures."""
-        woody = self.factory.makeDistroSeries(
-            name='woody', displayname='Woody',
-            distribution=self.ppa.distribution)
-        naked_woody = remove_security_proxy_and_shout_at_engineer(woody)
-        naked_woody.nominatedarchindep = woody.newArch(
-            'i386', ProcessorFamily.get(1), False, self.factory.makePerson(),
-            supports_virtualized=True)
-
+        woody = self._makeWoodyDistroSeries()
         recipe = self.makeRecipe()
         recipe.requestBuild(
             self.ppa, self.chef, woody, PackagePublishingPocket.RELEASE)

=== modified file 'lib/lp/code/vocabularies/sourcepackagerecipe.py'
--- lib/lp/code/vocabularies/sourcepackagerecipe.py	2012-09-05 05:08:26 +0000
+++ lib/lp/code/vocabularies/sourcepackagerecipe.py	2013-08-22 01:45:47 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Source Package Recipe vocabularies used in the lp/code modules."""
@@ -61,5 +61,4 @@
     """Return a vocabulary of ppas that the current user can target."""
     ppas = getUtility(IArchiveSet).getPPAsForUser(getUtility(ILaunchBag).user)
     return make_archive_vocabulary(
-        ppa for ppa in ppas
-        if check_permission('launchpad.Append', ppa))
+        ppa for ppa in ppas if check_permission('launchpad.Append', ppa))