launchpad-reviewers team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

Tim Penhey has proposed merging lp:~thumper/launchpad/show-recipe-upload-issue into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #700864 Recipes don't indicate that there are upload permission problems
  https://bugs.launchpad.net/bugs/700864

For more details, see:
https://code.launchpad.net/~thumper/launchpad/show-recipe-upload-issue/+merge/45804

This issue was discovered while debugging a problem with the awn-testing recipe.

If the recipe owner does not have upload rights to the daily build PPA the daily recipe jobs are not created.

Here is an example of the error shown:
  http://people.canonical.com/~tim/recipe_upload_issue.png

This is not something we can always check on creation or editing as the person may be removed from a team that has a PPA that was having the builds working.
-- 
https://code.launchpad.net/~thumper/launchpad/show-recipe-upload-issue/+merge/45804
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/show-recipe-upload-issue into lp:launchpad.

=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py	2010-11-03 08:17:36 +0000
+++ lib/lp/app/browser/tales.py	2011-01-11 01:27:00 +0000
@@ -75,6 +75,12 @@
 SEPARATOR = ' : '
 
 
+def format_link(obj, view_name=None):
+    """Return the equivalent of obj/fmt:link as a string."""
+    adapter = queryAdapter(obj, IPathAdapter, 'fmt')
+    return adapter.link(view_name)
+
+
 class MenuLinksDict(dict):
     """A dict class to construct menu links when asked for and not before.
 

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2010-12-22 02:01:36 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2011-01-11 01:27:00 +0000
@@ -73,6 +73,7 @@
     LaunchpadFormView,
     render_radio_widget_part,
     )
+from lp.app.browser.tales import format_link
 from lp.code.errors import (
     BuildAlreadyPending,
     NoSuchBranch,
@@ -194,6 +195,16 @@
         # are put into production. spec=sourcepackagerecipes
         super(SourcePackageRecipeView, self).initialize()
         self.request.response.addWarningNotification(RECIPE_BETA_MESSAGE)
+        recipe = self.context
+        if self.dailyBuildWithoutUploadPermission():
+            self.request.response.addWarningNotification(
+                structured(
+                    "Daily builds for this recipe will <strong>not</strong> "
+                    "occur.<br/><br/>The owner of the recipe (%s) does not "
+                    "have permission to upload packages into the daily "
+                    "build PPA (%s)" % (
+                        format_link(recipe.owner),
+                        format_link(recipe.daily_build_archive))))
 
     @property
     def page_title(self):
@@ -217,6 +228,18 @@
                 break
         return builds
 
+    def dailyBuildWithoutUploadPermission(self):
+        """Returns true if there are upload permissions to the daily archive.
+
+        If the recipe isn't built daily, we don't consider this a problem.
+        """
+        recipe = self.context
+        ppa = recipe.daily_build_archive
+        if recipe.build_daily:
+            has_upload = ppa.checkArchivePermission(recipe.owner)
+            return not has_upload
+        return False
+
 
 class SourcePackageRecipeRequestBuildsView(LaunchpadFormView):
     """A view for requesting builds of a SourcePackageRecipe."""

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-12-22 02:01:36 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2011-01-11 01:27:00 +0000
@@ -25,6 +25,7 @@
     extract_text,
     find_main_content,
     find_tags_by_class,
+    get_feedback_messages,
     get_radio_button_text_for_field,
     )
 from canonical.launchpad.webapp import canonical_url
@@ -53,6 +54,7 @@
     person_logged_in,
     )
 from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
+from lp.testing.views import create_initialized_view
 
 
 class TestCaseForRecipe(BrowserTestCase):
@@ -1113,6 +1115,44 @@
             "An identical build is already pending for ubuntu woody.",
             extract_text(find_main_content(browser.contents)))
 
+    def makeRecipeWithUploadIssues(self):
+        """Make a recipe where the owner can't upload to the PPA."""
+        # This occurs when the PPA that the recipe is being built daily into
+        # is owned by a team, and the owner of the recipe isn't in the team
+        # that owns the PPA.
+        registrant = self.factory.makePerson()
+        owner_team = self.factory.makeTeam(members=[registrant], name='team1')
+        ppa_team = self.factory.makeTeam(members=[registrant], name='team2')
+        ppa = self.factory.makeArchive(owner=ppa_team, name='ppa')
+        return self.factory.makeSourcePackageRecipe(
+            registrant=registrant, owner=owner_team, daily_build_archive=ppa,
+            build_daily=True)
+
+    def test_owner_with_no_ppa_upload_permission(self):
+        # Daily build with upload issues are a problem.
+        recipe = self.makeRecipeWithUploadIssues()
+        view = create_initialized_view(recipe, '+index')
+        self.assertTrue(view.dailyBuildWithoutUploadPermission())
+
+    def test_owner_with_no_ppa_upload_permission_non_daily(self):
+        # Non-daily builds with upload issues are not so much of an issue.
+        recipe = self.makeRecipeWithUploadIssues()
+        with person_logged_in(recipe.registrant):
+            recipe.build_daily = False
+        view = create_initialized_view(recipe, '+index')
+        self.assertFalse(view.dailyBuildWithoutUploadPermission())
+
+    def test_owner_with_no_ppa_upload_permission_message(self):
+        # If there is an issue, a message is shown.
+        recipe = self.makeRecipeWithUploadIssues()
+        browser = self.getViewBrowser(recipe, '+index')
+        messages = get_feedback_messages(browser.contents)
+        self.assertEqual(
+            "Daily builds for this recipe will not occur.\n"
+            "The owner of the recipe (Team1) does not have permission to "
+            "upload packages into the daily build PPA (PPA for Team2)",
+            messages[-1])
+
 
 class TestSourcePackageRecipeBuildView(BrowserTestCase):
     """Test behaviour of SourcePackageReciptBuildView."""