Merge lp:~julian-edwards/launchpad/publish-warning-bug-715116 into lp:launchpad

[Julian Edwards]

[r=leonardr][bug=715116] Add a big informational notice on the PPA pages if publishing is disabled.

= 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.

[Leonard Richardson (leonardr)]

The code and tests look good but I think the wording has some problems. Let me nitpick your wording to try and save you a full UI review.

1. "Publishing has been disabled for this archive" is a full sentence and needs a period after it.

2. "Go to the Change details page..." is also a full sentence sentence.

3. Should "no builds will be dispatched" be "no builds are being dispatched"? Isn't it something that's (not) happening continuously, right now?

4. I'm not happy with the wording "Go to the Change details page", or with sticking a "Note:" in there with no special markup. How about one of these:

"Publishing has been disabled for this archive. Since this archive is private, no builds are being dispatched. You can <a>re-enable publishing</a>."

"Publishing has been disabled for this archive. <a>(Re-enable it.)</a> Since this archive is private, no builds are being dispatched."

What do you think?

I also don't understand why "no builds are being dispatched" follows from "this archive is private". Why are builds dispatched for public archives that have publishing disabled? Will the actual users understand?

review: Approve

[William Grant (wgrant)]

Could you add a full stop to the end of "Publishing has been disabled for this archive"? I'm also a little wary about having "Whether" in the field description.

[Julian Edwards (julian-edwards)]

On Tuesday 08 February 2011 18:36:10 you wrote:
> Review: Approve

Thanks for the review Leonard, and thanks for the critique of the wording.

> What do you think?

Much better, I've changed it a little more so it will look like:

            "Publishing has been disabled for this archive. (re-enable "
            "publishing) Since this archive is private, no builds are being "
            "dispatched."

> I also don't understand why "no builds are being dispatched" follows from
> "this archive is private". Why are builds dispatched for public archives
> that have publishing disabled? Will the actual users understand?

The reason for this is too technical to put on that page really, but the
upshot is that for security reasons the builders require authenticated access
to a place where they can download the source before building it. The only
place we have right now is the repo itself, thusly requiring the sources to be
published first.

The branch is in ec2 now, thanks.

=== 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-09 10:13:20 +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,24 @@
 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 += (
+                    " <a href=%s>(re-enable publishing)</a>" % edit_url)
+            if self.context.private:
+                notification += (
+                    " Since this archive is private, no builds are "
+                    "being 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-09 10:13:20 +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. (re-enable "
+            "publishing)",
+            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. (re-enable "
+            "publishing) Since this archive is private, no builds are being "
+            "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-09 10:13:20 +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.