launchpad-reviewers team mailing list archive

[Raphaël Victor Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Victor Badin has proposed merging lp:~rvb/launchpad/blacklist-disabled-bug-798613 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #798613 in Launchpad itself: "Showing the blacklisting (ignoring) radio selection even if you can't use it would be useful"
  https://bugs.launchpad.net/launchpad/+bug/798613

For more details, see:
https://code.launchpad.net/~rvb/launchpad/blacklist-disabled-bug-798613/+merge/65367

This branch fixes the display of the blacklist slot (on the difference pages) so that it's always present. The slot is disabled if the user has not the right to change the blacklist status of the difference.

= Test =

./bin/test -cvv test_distroseriesdifference_views test_blacklist_options_disabled

= Q/A =

On DF: 
- Go to https://dogfood.launchpad.net/ubuntu/oneiric/+localpackagediffs as a non-privileged user;
- Make sure the blacklist slot is displayed and disabled.
-- 
https://code.launchpad.net/~rvb/launchpad/blacklist-disabled-bug-798613/+merge/65367
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/blacklist-disabled-bug-798613 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/distroseriesdifference.py'
--- lib/lp/registry/browser/distroseriesdifference.py	2011-05-23 16:50:14 +0000
+++ lib/lp/registry/browser/distroseriesdifference.py	2011-06-21 16:02:39 +0000
@@ -160,10 +160,10 @@
         return self.request.is_ajax and self.can_request_diffs
 
     @cachedproperty
-    def show_blacklist_options(self):
-        """Should we show the blacklisting (ignore) radio widget options.
+    def enable_blacklist_options(self):
+        """Should we enable the blacklisting (ignore) radio widget options.
 
-        Only show the options if an editor requests via JS and the user
+        Only enable the options if an editor requests via JS and the user
         is an archive admin.
         """
         return self.request.is_ajax and check_permission(

=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_views.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifference_views.py	2011-06-17 11:39:04 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifference_views.py	2011-06-21 16:02:39 +0000
@@ -156,11 +156,11 @@
             view = create_initialized_view(
                 ds_diff, '+listing-distroseries-extra')
         self.assertFalse(view.show_add_comment)
-        self.assertFalse(view.show_blacklist_options)
+        self.assertFalse(view.enable_blacklist_options)
 
     def test_show_edit_options_editor(self):
-        # Blacklist options and "Add comment" are shown if requested by
-        # an editor via ajax.
+        # The "Add comment" link is shown and the blacklist options are
+        # enabled if requested by an editor via ajax.
         ds_diff = self.factory.makeDistroSeriesDifference()
 
         request = LaunchpadTestRequest(HTTP_X_REQUESTED_WITH='XMLHttpRequest')
@@ -168,10 +168,10 @@
             view = create_initialized_view(
                 ds_diff, '+listing-distroseries-extra', request=request)
             self.assertTrue(view.show_add_comment)
-            self.assertFalse(view.show_blacklist_options)
+            self.assertFalse(view.enable_blacklist_options)
 
-    def test_show_blacklist_options_for_archive_admin(self):
-        # To see the blacklist options the the user needs to be an
+    def test_enable_blacklist_options_for_archive_admin(self):
+        # To see the blacklist options enabled the user needs to be an
         # archive admin.
         ds_diff = self.factory.makeDistroSeriesDifference()
         archive_admin = self.factory.makeArchiveAdmin(
@@ -181,7 +181,7 @@
         with person_logged_in(archive_admin):
             view = create_initialized_view(
                 ds_diff, '+listing-distroseries-extra', request=request)
-            self.assertTrue(view.show_blacklist_options)
+            self.assertTrue(view.enable_blacklist_options)
 
     def test_show_add_comment_non_editor(self):
         # Even with a JS request, non-editors do not see the 'add
@@ -507,7 +507,7 @@
                 "VERSION LINK", 'a',
                 attrs=dict(href=url)))
 
-        self.assertThat(page, anchor_matcher) 
+        self.assertThat(page, anchor_matcher)
 
     def test_blacklist_options(self):
         # Blacklist options are presented to the users who are archive
@@ -526,6 +526,23 @@
         self.assertEqual(
             1, len(soup.findAll('div', {'class': 'blacklist-options'})))
 
+    def test_blacklist_options_disabled(self):
+        # Blacklist options are disabled to the users who are *not* archive
+        # admins.
+        ds_diff = self.factory.makeDistroSeriesDifference()
+        person = self.factory.makePerson()
+
+        with person_logged_in(person):
+            request = LaunchpadTestRequest(
+                HTTP_X_REQUESTED_WITH='XMLHttpRequest')
+            view = create_initialized_view(
+                ds_diff, '+listing-distroseries-extra', request=request)
+            soup = BeautifulSoup(view())
+
+        self.assertEqual(
+            1,
+            len(soup.findAll('div', {'class': 'blacklist-options-disabled'})))
+
     def test_blacklist_options_initial_values_none(self):
         ds_diff = self.factory.makeDistroSeriesDifference()
         view = create_initialized_view(ds_diff, '+listing-distroseries-extra')

=== modified file 'lib/lp/registry/javascript/distroseriesdifferences_details.js'
--- lib/lp/registry/javascript/distroseriesdifferences_details.js	2011-06-13 19:52:15 +0000
+++ lib/lp/registry/javascript/distroseriesdifferences_details.js	2011-06-21 16:02:39 +0000
@@ -294,12 +294,20 @@
                 parent_distro_name,
                 parent_series_name
                 ].join('/');
-            // The blacklist slot is only available when the user has
-            // the right to blacklist.
+            // The blacklist slot with a class 'blacklist-options' is only
+            // available when the user has the right to blacklist.
             var blacklist_slot = args.container.one('div.blacklist-options');
             if (blacklist_slot !== null) {
                 setup_blacklist_options(blacklist_slot, source_name, api_uri);
             }
+            // If the user has not the right to blacklist, we disable
+            // the blacklist slot.
+            var disabled_blacklist_slot = args.container.one(
+                'div.blacklist-options-disabled');
+            if (disabled_blacklist_slot !== null) {
+                disabled_blacklist_slot
+                    .all('input').set('disabled', 'disabled');
+            }
             // The add comment slot is only available when the user has the
             // right to add comments.
             var add_comment_placeholder =

=== modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
--- lib/lp/registry/templates/distroseriesdifference-listing-extra.pt	2011-06-17 11:39:04 +0000
+++ lib/lp/registry/templates/distroseriesdifference-listing-extra.pt	2011-06-21 16:02:39 +0000
@@ -20,16 +20,22 @@
     </metal:macro-parent>
   </metal:macros>
 
-  <tal:show_options condition="view/show_blacklist_options">
-    <div class="blacklist-options" style="float:right">
+  <tal:blacklist define="
+    disabled python:'' if view.enable_blacklist_options else '-disabled'">
+    <div
+      tal:attributes="class string:blacklist-options${disabled}"
+      style="float:right">
       <dl>
         <dt>Blacklisted:</dt>
         <dd>
-          <form><tal:replace replace="structure view/widgets/blacklist_options" /></form>
+          <form>
+            <tal:replace
+              replace="structure view/widgets/blacklist_options" />
+          </form>
         </dd>
       </dl>
-  </div>
-  </tal:show_options>
+    </div>
+  </tal:blacklist>
 
   <dl>
     <dt>Binary descriptions:</dt>