launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/publish-warning-bug-715116 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #715116 The PPA page should show a big warning when publishing is disabled
  https://bugs.launchpad.net/bugs/715116

For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/publish-warning-bug-715116/+merge/48952

= Summary =
Fix bug 715116

== Proposed fix ==
A PPA can have apparently "stuck" publications and if it's private, stuck 
builds, if someone has turned the publishing flag off.

This branch adds a big informational notice on the PPA pages if the publishing 
is disabled.

== Implementation details ==

There's three different message segments added depending on some criteria:
 * The basic "publishing is disabled" if that's the case
 * Add a link to "Change details" if the requesing user has lp.edit
 * Add a message about stuck builds if the PPA is private.

As a bonus, fix the really crappy description text on the "publish" flag on 
the +edit page.

== Tests ==
bin/test -cvv test_archive_packages test_warning_for_disabled_publishing

== Demo and Q/A ==


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/browser/tests/test_archive_packages.py
  lib/lp/soyuz/interfaces/archive.py
  lib/lp/soyuz/browser/archive.py

./lib/lp/soyuz/interfaces/archive.py
     586: E301 expected 1 blank line, found 0
     714: E301 expected 1 blank line, found 0
     737: E301 expected 1 blank line, found 0
     760: E301 expected 1 blank line, found 0
     860: E301 expected 1 blank line, found 0
     894: E501 line too long (84 characters)
     894: Line exceeds 78 characters.
-- 
https://code.launchpad.net/~julian-edwards/launchpad/publish-warning-bug-715116/+merge/48952
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/publish-warning-bug-715116 into lp:launchpad.

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2011-02-04 18:28:14 +0000
+++ lib/lp/soyuz/browser/archive.py	2011-02-08 17:35:54 +0000
@@ -55,7 +55,6 @@
     SimpleVocabulary,
     )
 from zope.security.interfaces import Unauthorized
-from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad import _
 from canonical.launchpad.browser.librarian import FileNavigationMixin
@@ -89,7 +88,6 @@
     TextAreaEditorWidget,
     TextLineEditorWidget,
     )
-from lp.app.browser.stringformatter import FormattersAPI
 from lp.app.errors import NotFoundError
 from lp.app.widgets.itemswidgets import (
     LabeledMultiCheckBoxWidget,
@@ -547,6 +545,25 @@
 class ArchiveViewBase(LaunchpadView):
     """Common features for Archive view classes."""
 
+    def initialize(self):
+        # If the archive has publishing disabled, present a warning.  If
+        # the current user has lp.Edit then add a link to +edit to fix
+        # this.
+        if not self.context.publish and self.context.is_active:
+            can_edit = check_permission('launchpad.Edit', self.context)
+            notification = "Publishing has been disabled for this archive"
+            if can_edit:
+                edit_url = canonical_url(self.context) + '/+edit'
+                notification += (
+                    ", go to the <a href=%s>Change details</a> page if you "
+                    "need to re-enable it." % edit_url)
+            if self.context.private:
+                notification += (
+                    "\nNote: since this archive is private, no builds will "
+                    "be dispatched.")
+            self.request.response.addNotification(structured(notification))
+        super(ArchiveViewBase, self).initialize()
+
     @cachedproperty
     def private(self):
         return self.context.private

=== modified file 'lib/lp/soyuz/browser/tests/test_archive_packages.py'
--- lib/lp/soyuz/browser/tests/test_archive_packages.py	2010-12-08 19:03:17 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_packages.py	2011-02-08 17:35:54 +0000
@@ -19,7 +19,10 @@
     MatchesAny,
     )
 from zope.security.interfaces import Unauthorized
+from zope.security.proxy import removeSecurityProxy
 
+from canonical.launchpad.webapp.authentication import LaunchpadPrincipal
+from canonical.launchpad.testing.pages import get_feedback_messages
 from canonical.launchpad.webapp import canonical_url
 from canonical.testing.layers import LaunchpadFunctionalLayer
 from canonical.launchpad.utilities.celebrities import ILaunchpadCelebrities
@@ -117,6 +120,52 @@
         return create_initialized_view(
             ppa, "+packages", query_string=query_string)
 
+    def assertNotifications(self, ppa, notification, person=None):
+        # Assert that while requesting a 'ppa' page as 'person', the
+        # 'notification' appears.
+        if person is not None:
+            login_person(ppa.owner)
+            principal = LaunchpadPrincipal(
+                ppa.owner.account.id, ppa.owner.displayname,
+                ppa.owner.displayname, ppa.owner)
+        else:
+            principal = None
+        page = create_initialized_view(
+            ppa, "+packages", principal=principal).render()
+        notifications = get_feedback_messages(page)
+        self.assertIn(notification, notifications)
+
+    def test_warning_for_disabled_publishing(self):
+        # Ensure that a notification is shown when archive.publish
+        # is False.
+        ppa = self.factory.makeArchive()
+        removeSecurityProxy(ppa).publish = False
+        self.assertNotifications(
+            ppa,
+            'Publishing has been disabled for this archive, go to the '
+            'Change details page if you need to re-enable it.',
+            person=ppa.owner)
+
+    def test_warning_for_disabled_publishing_with_private_ppa(self):
+        # Ensure that a notification is shown when archive.publish
+        # is False warning that builds won't get dispatched.
+        ppa = self.factory.makeArchive(private=True)
+        removeSecurityProxy(ppa).publish = False
+        self.assertNotifications(
+            ppa,
+            'Publishing has been disabled for this archive, go to the '
+            'Change details page if you need to re-enable it.\nNote: since '
+            'this archive is private, no builds will be dispatched.',
+            person=ppa.owner)
+
+    def test_warning_for_disabled_publishing_with_anonymous_user(self):
+        # The warning notification doesn't mention the Change details
+        # page.
+        ppa = self.factory.makeArchive()
+        removeSecurityProxy(ppa).publish = False
+        self.assertNotifications(
+            ppa, 'Publishing has been disabled for this archive')
+
     def test_ppa_packages_menu_is_enabled(self):
         joe = self.factory.makePerson()
         ppa = self.factory.makeArchive()

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2011-02-03 17:38:38 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2011-02-08 17:35:54 +0000
@@ -282,7 +282,9 @@
 
     publish = Bool(
         title=_("Publish"), required=False,
-        description=_("Update the APT archive."))
+        description=_("Whether or not to update the APT repository.  If "
+            "disabled, nothing will be published.  If the archive is "
+            "private then additionally no builds will be dispatched."))
 
     # This is redefined from IPrivacy.private because the attribute is
     # read-only. The value is guarded by a validator.