Merge lp:~stevenk/launchpad/reject-reason-plus-queue into lp:launchpad

[Steve Kowalik]

Make use of the new comment argument to IPackageUpload.rejectFromQueue() in DistroSeries:+queue by showing a textbox for a reason and require it to be specified when rejecting packages.

Make use of the new comment argument to IPackageUpload.rejectFromQueue() by showing a textbox for a reason and require it to be specified when rejecting packages.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/soyuz/browser/queue.py'
--- lib/lp/soyuz/browser/queue.py	2013-04-17 08:13:18 +0000
+++ lib/lp/soyuz/browser/queue.py	2013-05-28 06:37:30 +0000
@@ -309,6 +309,7 @@
         # Retrieve the form data.
         accept = self.request.form.get('Accept', '')
         reject = self.request.form.get('Reject', '')
+        rejection_comment = self.request.form.get('rejection_comment', '')
         component_override = self.request.form.get('component_override', '')
         section_override = self.request.form.get('section_override', '')
         priority_override = self.request.form.get('priority_override', '')
@@ -318,6 +319,11 @@
         if (not accept and not reject) or not queue_ids:
             return
 
+        # If we're asked to reject with no comment, bail.
+        if reject and not rejection_comment:
+            self.error = 'Rejection comment required.'
+            return
+
         # Determine if there is a source override requested.
         new_component = None
         new_section = None
@@ -421,7 +427,11 @@
                     'priority'] = new_priority.title.lower()
 
             try:
-                getattr(self, 'queue_action_' + action)(queue_item)
+                if action == 'accept':
+                    queue_item.acceptFromQueue(user=self.user)
+                elif action == 'reject':
+                    queue_item.rejectFromQueue(
+                        user=self.user, comment=rejection_comment)
             except (QueueAdminUnauthorizedError,
                     QueueInconsistentStateError) as info:
                 failure.append('FAILED: %s (%s)' %
@@ -447,14 +457,6 @@
         url = str(self.request.URL) + "?queue_state=%s" % self.state.value
         self.request.response.redirect(url)
 
-    def queue_action_accept(self, queue_item):
-        """Accept the queue item passed."""
-        queue_item.acceptFromQueue(user=self.user)
-
-    def queue_action_reject(self, queue_item):
-        """Reject the queue item passed."""
-        queue_item.rejectFromQueue(user=self.user)
-
     def sortedSections(self):
         """Possible sections for the context distroseries.
 

=== modified file 'lib/lp/soyuz/browser/tests/test_queue.py'
--- lib/lp/soyuz/browser/tests/test_queue.py	2013-04-17 11:07:33 +0000
+++ lib/lp/soyuz/browser/tests/test_queue.py	2013-05-28 06:37:30 +0000
@@ -42,17 +42,15 @@
 from lp.testing.views import create_initialized_view
 
 
-class TestAcceptQueueUploads(TestCaseWithFactory):
-    """Uploads for the partner archive can be accepted with the relevant
-    permissions.
-    """
+class TestAcceptRejectQueueUploads(TestCaseWithFactory):
+    """Uploads can be accepted or rejected with the relevant permissions."""
 
     layer = LaunchpadFunctionalLayer
 
     def setUp(self):
         """Create two new uploads in the new state and a person with
         permission to upload to the partner archive."""
-        super(TestAcceptQueueUploads, self).setUp()
+        super(TestAcceptRejectQueueUploads, self).setUp()
         login('[e-mail]')
         self.test_publisher = SoyuzTestPublisher()
         self.test_publisher.prepareBreezyAutotest()
@@ -132,6 +130,11 @@
         view.performQueueAction()
         return view
 
+    def assertStatus(self, package_upload_id, status):
+        self.assertEqual(
+            status,
+            getUtility(IPackageUploadSet).get(package_upload_id).status)
+
     def test_main_admin_can_accept_main_upload(self):
         # A person with queue admin access for main
         # can accept uploads to the main archive.
@@ -145,10 +148,7 @@
         request = LaunchpadTestRequest(form=self.form)
         request.method = 'POST'
         self.setupQueueView(request)
-
-        self.assertEquals(
-            'DONE',
-            getUtility(IPackageUploadSet).get(package_upload_id).status.name)
+        self.assertStatus(package_upload_id, PackageUploadStatus.DONE)
 
     def test_main_admin_cannot_accept_partner_upload(self):
         # A person with queue admin access for main cannot necessarily
@@ -169,9 +169,7 @@
                 "FAILED: partner-upload (You have no rights to accept "
                 "component(s) 'partner')"),
             view.request.response.notifications[0].message)
-        self.assertEquals(
-            'NEW',
-            getUtility(IPackageUploadSet).get(package_upload_id).status.name)
+        self.assertStatus(package_upload_id, PackageUploadStatus.NEW)
 
     def test_admin_can_accept_partner_upload(self):
         # An admin can always accept packages, even for the
@@ -183,10 +181,7 @@
         request = LaunchpadTestRequest(form=self.form)
         request.method = 'POST'
         self.setupQueueView(request)
-
-        self.assertEquals(
-            'DONE',
-            getUtility(IPackageUploadSet).get(package_upload_id).status.name)
+        self.assertStatus(package_upload_id, PackageUploadStatus.DONE)
 
     def test_partner_admin_can_accept_partner_upload(self):
         # A person with queue admin access for partner
@@ -201,10 +196,7 @@
         request = LaunchpadTestRequest(form=self.form)
         request.method = 'POST'
         self.setupQueueView(request)
-
-        self.assertEquals(
-            'DONE',
-            getUtility(IPackageUploadSet).get(package_upload_id).status.name)
+        self.assertStatus(package_upload_id, PackageUploadStatus.DONE)
 
     def test_partner_admin_cannot_accept_main_upload(self):
         # A person with queue admin access for partner cannot necessarily
@@ -225,9 +217,7 @@
                 "FAILED: main-upload (You have no rights to accept "
                 "component(s) 'main')"),
             view.request.response.notifications[0].message)
-        self.assertEquals(
-            'NEW',
-            getUtility(IPackageUploadSet).get(package_upload_id).status.name)
+        self.assertStatus(package_upload_id, PackageUploadStatus.NEW)
 
     def test_proposed_admin_can_accept_proposed_upload(self):
         # A person with queue admin access for proposed can accept uploads
@@ -243,7 +233,6 @@
                     self.proposed_queue_admin,
                     pocket=PackagePublishingPocket.PROPOSED,
                     distroseries=distroseries))
-        package_upload_set = getUtility(IPackageUploadSet)
 
         for spr in (self.proposed_spr, self.proposed_series_spr):
             package_upload_id = spr.package_upload.id
@@ -251,9 +240,7 @@
             request = LaunchpadTestRequest(form=self.form)
             request.method = 'POST'
             self.setupQueueView(request, series=spr.upload_distroseries)
-
-            self.assertEqual(
-                'DONE', package_upload_set.get(package_upload_id).status.name)
+            self.assertStatus(package_upload_id, PackageUploadStatus.DONE)
 
     def test_proposed_admin_cannot_accept_release_upload(self):
         # A person with queue admin access for proposed cannot necessarly
@@ -275,9 +262,7 @@
                 "FAILED: main-upload (You have no rights to accept "
                 "component(s) 'main')"),
             view.request.response.notifications[0].message)
-        self.assertEqual(
-            'NEW',
-            getUtility(IPackageUploadSet).get(package_upload_id).status.name)
+        self.assertStatus(package_upload_id, PackageUploadStatus.NEW)
 
     def test_proposed_series_admin_can_accept_that_series_upload(self):
         # A person with queue admin access for proposed for one series can
@@ -294,10 +279,7 @@
         request = LaunchpadTestRequest(form=self.form)
         request.method = 'POST'
         self.setupQueueView(request, series=self.second_series)
-
-        self.assertEqual(
-            'DONE',
-            getUtility(IPackageUploadSet).get(package_upload_id).status.name)
+        self.assertStatus(package_upload_id, PackageUploadStatus.DONE)
 
     def test_proposed_series_admin_cannot_accept_other_series_upload(self):
         # A person with queue admin access for proposed for one series
@@ -317,9 +299,31 @@
 
         self.assertEqual(
             "You do not have permission to act on queue items.", view.error)
-        self.assertEqual(
-            'NEW',
-            getUtility(IPackageUploadSet).get(package_upload_id).status.name)
+        self.assertStatus(package_upload_id, PackageUploadStatus.NEW)
+
+    def test_cannot_reject_without_comment(self):
+        login_person(self.proposed_queue_admin)
+        package_upload_id = self.proposed_spr.package_upload.id
+        form = {
+            'Reject': 'Reject',
+            'QUEUE_ID': [package_upload_id]}
+        request = LaunchpadTestRequest(form=form)
+        request.method = 'POST'
+        view = self.setupQueueView(request)
+        self.assertEqual('Rejection comment required.', view.error)
+        self.assertStatus(package_upload_id, PackageUploadStatus.NEW)
+
+    def test_reject_with_comment(self):
+       login_person(self.proposed_queue_admin)
+       package_upload_id = self.proposed_spr.package_upload.id
+       form = {
+           'Reject': 'Reject',
+           'rejection_comment': 'Because I can.',
+           'QUEUE_ID': [package_upload_id]}
+       request = LaunchpadTestRequest(form=form)
+       request.method = 'POST'
+       self.setupQueueView(request)
+       self.assertStatus(package_upload_id, PackageUploadStatus.REJECTED)
 
 
 class TestQueueItemsView(TestCaseWithFactory):

=== modified file 'lib/lp/soyuz/templates/distroseries-queue.pt'
--- lib/lp/soyuz/templates/distroseries-queue.pt	2013-04-18 08:49:33 +0000
+++ lib/lp/soyuz/templates/distroseries-queue.pt	2013-05-28 06:37:30 +0000
@@ -183,15 +183,25 @@
              </select>
            </td>
            <td colspan="2" style="vertical-align: bottom">
+             <input value="Accept" name="Accept" type="submit"/>
+           </td>
+         </tr>
+         <tr tal:condition="view/availableActions">
+           <td colspan="4" style="text-align: right;
+                                  vertical-align: bottom;
+                                  padding-bottom: 5px">
+             <label>Rejection comment:</label>
+           </td>
+           <td colspan="3" style="vertical-align: bottom">
+             <input size="80" type="text" name="rejection_comment" />
+           </td>
+           <td style="vertical-align: bottom">
              <input
                tal:condition="view/state/enumvalue:REJECTED"
                disabled="true" value="Reject" name="Reject" type="submit"/>
              <input
                tal:condition="not:view/state/enumvalue:REJECTED"
                value="Reject" name="Reject" type="submit"/>
-             <br/>
-             <br/>
-             <input value="Accept" name="Accept" type="submit"/>
            </td>
          </tr>
        </tbody>