launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-307269 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #307269 in Launchpad itself: "Bug tasks nominations can be accepted and declined simultaneously"
  https://bugs.launchpad.net/launchpad/+bug/307269

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-307269/+merge/60721

BugNomination:+edit-form, as used on BugTask:+index, OOPSes when told to decline an approved nomination. This branch fixes declining/approving an approved nomination to display a pleasant message instead of OOPSing.

+edit-form and +editstatus both use BugNominationEditView, which badly reimplements much of LaunchpadFormView. I've ported both to use LaunchpadFormView, restricted the Approve and Decline actions to only appear when appropriate, and changed the template to display a message when the nomination is already approved.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-307269/+merge/60721
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-307269 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugnomination.py'
--- lib/lp/bugs/browser/bugnomination.py	2011-03-23 16:28:51 +0000
+++ lib/lp/bugs/browser/bugnomination.py	2011-05-12 05:07:27 +0000
@@ -15,6 +15,7 @@
 
 import pytz
 from zope.component import getUtility
+from zope.interface import Interface
 from zope.publisher.interfaces import implements
 
 from canonical.launchpad import _
@@ -202,58 +203,53 @@
         return self.request.getNearest(ICveSet) == (None, None)
 
 
-class BugNominationEditView(LaunchpadView):
+class BugNominationEditView(LaunchpadFormView):
     """Browser view class for approving and declining nominations."""
 
-    def __init__(self, context, request):
-        LaunchpadView.__init__(self, context, request)
+    schema = Interface
+    field_names = []
+
+    @property
+    def title(self):
+        return 'Approve or decline nomination for bug #%d in %s' % (
+            self.context.bug.id, self.context.target.bugtargetdisplayname)
+    label = title
+
+    def initialize(self):
         self.current_bugtask = getUtility(ILaunchBag).bugtask
-
-    def getFormAction(self):
-        """Get the string used as the form action."""
-        return (
-            "%s/nominations/%d/+edit-form" % (
-                canonical_url(self.current_bugtask), self.context.id))
-
-    def processNominationDecision(self):
-        """Process the decision made on this nomination."""
-        form = self.request.form
-        approve_nomination = form.get("approve")
-        decline_nomination = form.get("decline")
-
-        if not (approve_nomination or decline_nomination):
-            return
-
-        if approve_nomination:
-            self.context.approve(self.user)
-            self.request.response.addNotification(
-                "Approved nomination for %s" %
-                    self.context.target.bugtargetdisplayname)
-        elif decline_nomination:
-            self.context.decline(self.user)
-            self.request.response.addNotification(
-                "Declined nomination for %s" %
-                    self.context.target.bugtargetdisplayname)
-
-        self.request.response.redirect(
-            canonical_url(getUtility(ILaunchBag).bugtask))
-
-    def shouldShowApproveButton(self):
+        super(BugNominationEditView, self).initialize()
+
+    @property
+    def action_url(self):
+        return "%s/nominations/%d/+editstatus" % (
+            canonical_url(self.current_bugtask),
+            self.context.id)
+
+    def shouldShowApproveButton(self, action):
         """Should the approve button be shown?"""
         return self.context.isProposed() or self.context.isDeclined()
 
-    def shouldShowDeclineButton(self):
+    def shouldShowDeclineButton(self, action):
         """Should the decline button be shown?"""
         return self.context.isProposed()
 
-    def getCurrentBugTaskURL(self):
-        """Return the URL of the current bugtask."""
-        return canonical_url(getUtility(ILaunchBag).bugtask)
+    @action(_("Approve"), name="approve", condition=shouldShowApproveButton)
+    def approve(self, action, data):
+        self.context.approve(self.user)
+        self.request.response.addNotification(
+            "Approved nomination for %s" %
+                self.context.target.bugtargetdisplayname)
+
+    @action(_("Decline"), name="decline", condition=shouldShowDeclineButton)
+    def decline(self, action, data):
+        self.context.decline(self.user)
+        self.request.response.addNotification(
+            "Declined nomination for %s" %
+                self.context.target.bugtargetdisplayname)
 
     @property
-    def title(self):
-        return 'Approve or decline nomination for bug #%d in %s' % (
-            self.context.bug.id, self.context.target.bugtargetdisplayname)
+    def next_url(self):
+        return canonical_url(self.current_bugtask)
 
 
 class BugNominationContextMenu(BugContextMenu):

=== modified file 'lib/lp/bugs/browser/tests/test_bugnomination.py'
--- lib/lp/bugs/browser/tests/test_bugnomination.py	2010-12-21 18:53:42 +0000
+++ lib/lp/bugs/browser/tests/test_bugnomination.py	2011-05-12 05:07:27 +0000
@@ -8,12 +8,14 @@
 from zope.component import getUtility
 
 from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.launchpad.webapp.interaction import get_current_principal
 from canonical.launchpad.webapp.interfaces import ILaunchBag
 from lp.testing import (
     login_person,
     person_logged_in,
     TestCaseWithFactory,
     )
+from lp.testing.matchers import Contains
 from lp.testing.views import create_initialized_view
 
 
@@ -50,3 +52,51 @@
         view = create_initialized_view(self.bug_task, name='+nominate')
         action = view.__class__.actions.byname['actions.submit']
         self.assertEqual('Target', action.label)
+
+
+class TestBugNominationEditView(TestCaseWithFactory):
+    """Tests for BugNominationEditView."""
+
+    layer = DatabaseFunctionalLayer
+
+    def getNomination(self):
+        nomination = self.factory.makeBugNomination(
+            target=self.factory.makeProductSeries())
+        login_person(nomination.productseries.product.owner)
+        return nomination
+
+    def getNominationEditView(self, nomination, form):
+        getUtility(ILaunchBag).add(nomination.bug.default_bugtask)
+        view = create_initialized_view(
+            nomination, name='+editstatus',
+            current_request=True,
+            principal=get_current_principal(),
+            form=form)
+        return view
+
+    def assertApproves(self, nomination):
+        self.assertEquals(
+            302,
+            self.getNominationEditView(
+                nomination,
+                {'field.actions.approve': 'Approve'},
+                ).request.response.getStatus())
+        self.assertTrue(nomination.isApproved())
+
+    def test_approving_twice_is_noop(self):
+        nomination = self.getNomination()
+        self.assertApproves(nomination)
+        self.assertThat(
+            self.getNominationEditView(
+                nomination,
+                {'field.actions.approve': 'Approve'}).render(),
+            Contains("This nomination has already been approved."))
+
+    def test_declining_approved_is_noop(self):
+        nomination = self.getNomination()
+        self.assertApproves(nomination)
+        self.assertThat(
+            self.getNominationEditView(
+                nomination,
+                {'field.actions.decline': 'Decline'}).render(),
+            Contains("This nomination has already been approved."))

=== modified file 'lib/lp/bugs/templates/bugnomination-edit-form.pt'
--- lib/lp/bugs/templates/bugnomination-edit-form.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/bugs/templates/bugnomination-edit-form.pt	2011-05-12 05:07:27 +0000
@@ -1,13 +1,6 @@
 <tal:root
   xmlns:tal="http://xml.zope.org/namespaces/tal";
+  xmlns:metal="http://xml.zope.org/namespaces/metal";
   omit-tag="">
-<tal:process condition="view/processNominationDecision" />
-<form action="#"
-      method="post"
-      tal:attributes="action view/getFormAction">
-  <input tal:condition="view/shouldShowApproveButton"
-         type="submit" name="approve" value="Approve" id="approve" />
-  <input tal:condition="view/shouldShowDeclineButton"
-         type="submit" name="decline" value="Decline" id="decline" />
-</form>
+  <div metal:use-macro="context/@@launchpad_form/form" />
 </tal:root>

=== modified file 'lib/lp/bugs/templates/bugnomination-edit.pt'
--- lib/lp/bugs/templates/bugnomination-edit.pt	2010-12-20 22:33:43 +0000
+++ lib/lp/bugs/templates/bugnomination-edit.pt	2011-05-12 05:07:27 +0000
@@ -10,15 +10,19 @@
 <body>
   <metal:main metal:fill-slot="main">
     <div class="top-portlet">
-      <h1 tal:content="view/title">
-        Manage nomination for bug #1 in Ubuntu Hoary
-      </h1>
-      <p>
-        Approving a nomination will target the bug to be fixed in a specific
-        series. Declining a nomination will show on the bug page that this bug will
-        not be fixed in that specific series.
-      </p>
-      <div tal:content="structure context/@@+edit-form" />
+      <div metal:use-macro="context/@@launchpad_form/form">
+        <div metal:fill-slot="extra_info">
+          <p>
+            Approving a nomination will target the bug to be fixed in a
+            specific series. Declining a nomination will show on the bug
+            page that this bug will not be fixed in that specific
+            series.
+          </p>
+          <p tal:condition="context/isApproved">
+            This nomination has already been approved.
+          </p>
+        </div>
+      </div>
     </div>
   </metal:main>
 </body>