launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Данило Шеган has proposed merging lp:~danilo/launchpad/anon-subscribers-list into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #798622 in Launchpad itself: "JS failure for anonymous users on bug page"
  https://bugs.launchpad.net/launchpad/+bug/798622

For more details, see:
https://code.launchpad.net/~danilo/launchpad/anon-subscribers-list/+merge/64964

= Bug 778622 =

When trying to load the bug page, there is a JS error "No bug specified in `config' or bug.web_link is invalid." when loading the "other subscribers" list. This was introduced in the fix for bug 772754.

The problem is that we are using LP.cache.bug for the bug, but that's not available when users are not logged in. LP.cache.context instead is a bugtask (because the bug page forwards to the bug task page), and the +bug-portlet-subscribers-details is only registered on the bug, so we can't trivially use LP.cache.context.

== Proposed fix ==

Provide +bug-portlet-subscribers-details view on the bug task as well, and use LP.cache.context exclusively.

(Alternative would have been to fetch the bug from the API using LP.cache.context.bug_link, then pass that in, but that'd be more expensive [one more round-trip to the server], so I decided on this approach instead)

== Tests ==

lib/lp/bugs/javascript/tests/test_subscribers_list.html

bin/test -cvvt BugPortletSubscribersWithDetailsTests

== Demo and Q/A ==

As anonymous user, try to load any bug page.  Subscribers list should show up, and 'subscribe someone else' link should stay blue.  No JS errors should show up in the console.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/bugsubscription.py
  lib/lp/bugs/browser/configure.zcml
  lib/lp/bugs/browser/tests/test_bugsubscription_views.py
  lib/lp/bugs/javascript/subscribers_list.js
  lib/lp/bugs/javascript/tests/test_subscribers_list.js
  lib/lp/bugs/templates/bugtask-index.pt
-- 
https://code.launchpad.net/~danilo/launchpad/anon-subscribers-list/+merge/64964
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/anon-subscribers-list into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py	2011-06-16 13:50:58 +0000
+++ lib/lp/bugs/browser/bugsubscription.py	2011-06-17 10:46:31 +0000
@@ -43,12 +43,13 @@
     action,
     LaunchpadFormView,
     )
-from lp.bugs.browser.bug import BugViewMixin
 from lp.bugs.browser.structuralsubscription import (
     expose_structural_subscription_data_to_js,
     )
 from lp.bugs.enum import BugNotificationLevel
+from lp.bugs.interfaces.bug import IBug
 from lp.bugs.interfaces.bugsubscription import IBugSubscription
+from lp.bugs.interfaces.bugtask import IBugTask
 from lp.bugs.model.personsubscriptioninfo import PersonSubscriptions
 from lp.bugs.model.structuralsubscription import (
     get_structural_subscriptions_for_bug,
@@ -532,7 +533,11 @@
     def subscriber_data_js(self):
         """Return subscriber_ids in a form suitable for JavaScript use."""
         data = []
-        details = list(self.context.getDirectSubscribersWithDetails())
+        if IBug.providedBy(self.context):
+            bug = self.context
+        elif IBugTask.providedBy(self.context):
+            bug = self.context.bug
+        details = list(bug.getDirectSubscribersWithDetails())
         api_request = IWebServiceClientRequest(self.request)
         for person, subscription in details:
             can_edit = self.user is not None and self.user.inTeam(person)
@@ -556,7 +561,7 @@
                 }
             data.append(record)
 
-        others = list(self.context.getIndirectSubscribers())
+        others = list(bug.getIndirectSubscribers())
         for person in others:
             if person == self.user:
                 # Skip the current user viewing the page.

=== modified file 'lib/lp/bugs/browser/configure.zcml'
--- lib/lp/bugs/browser/configure.zcml	2011-06-16 13:50:58 +0000
+++ lib/lp/bugs/browser/configure.zcml	2011-06-17 10:46:31 +0000
@@ -1089,6 +1089,12 @@
             class="
               lp.bugs.browser.bugsubscription.BugPortletSubscribersWithDetails"
             permission="zope.Public"/>
+        <browser:page
+            for="lp.bugs.interfaces.bugtask.IBugTask"
+            name="+bug-portlet-subscribers-details"
+            class="
+              lp.bugs.browser.bugsubscription.BugPortletSubscribersWithDetails"
+            permission="zope.Public"/>
         <browser:navigation
             module="lp.bugs.browser.bug"
             classes="

=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2011-06-16 13:50:58 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2011-06-17 10:46:31 +0000
@@ -434,6 +434,23 @@
             harness.request.response.getHeader('content-type'),
             'application/json')
 
+    def test_bugtask(self):
+        # This view works for the bug-task as well, when it renders
+        # the same data as for the bug.
+        bug = self.factory.makeBug()
+
+        # It works even for anonymous users, so no log-in is needed.
+        harness_bug = LaunchpadFormHarness(
+            bug, BugPortletSubscribersWithDetails)
+        bug_content = harness_bug.view.render()
+
+        # It works even for anonymous users, so no log-in is needed.
+        harness_bugtask = LaunchpadFormHarness(
+            bug.default_bugtask, BugPortletSubscribersWithDetails)
+        bugtask_content = harness_bugtask.view.render()
+
+        self.assertEqual(bug_content, bugtask_content)
+
     def _makeBugWithNoSubscribers(self):
         bug = self.factory.makeBug()
         with person_logged_in(bug.owner):

=== modified file 'lib/lp/bugs/javascript/subscribers_list.js'
--- lib/lp/bugs/javascript/subscribers_list.js	2011-06-16 13:50:58 +0000
+++ lib/lp/bugs/javascript/subscribers_list.js	2011-06-17 10:46:31 +0000
@@ -275,6 +275,10 @@
         step_title: 'Search',
         picker_activator: this.subscribe_someone_else_link
     };
+    if (!Y.Lang.isValue(LP.links.me)) {
+        // No-op for anonymous users.
+        return;
+    }
     if (Y.one(this.subscribe_someone_else_link) === null) {
         Y.error("No link matching CSS selector '" +
                 this.subscribe_someone_else_link +

=== modified file 'lib/lp/bugs/javascript/tests/test_subscribers_list.js'
--- lib/lp/bugs/javascript/tests/test_subscribers_list.js	2011-06-16 13:50:58 +0000
+++ lib/lp/bugs/javascript/tests/test_subscribers_list.js	2011-06-17 10:46:31 +0000
@@ -1288,6 +1288,7 @@
     } else {
         config = container_config;
     }
+    window.LP = { links: { me : "/~viewer" } };
     return new module.BugSubscribersLoader(config);
 }
 
@@ -1800,6 +1801,18 @@
         loader._setupSubscribeSomeoneElse();
     },
 
+    test_setupSubscribeSomeoneElse_not_logged_in: function() {
+        // When user is not logged in (LP.links.me undefined),
+        // it silently does nothing.
+
+        // Initialize the loader and make sure the user is not logged in.
+        var loader = setUpLoader(this.root);
+        window.LP.links.me = undefined;
+        loader._setupSubscribeSomeoneElse();
+        // Nothing happens, not even a person picker is set up.
+        Y.Assert.isUndefined(loader._picker);
+    },
+
     test_setupSubscribeSomeoneElse: function() {
         // _setupSubscribeSomeoneElse ties in a link with
         // the appropriate person picker and with the save

=== modified file 'lib/lp/bugs/templates/bugtask-index.pt'
--- lib/lp/bugs/templates/bugtask-index.pt	2011-06-16 13:50:58 +0000
+++ lib/lp/bugs/templates/bugtask-index.pt	2011-06-17 10:46:31 +0000
@@ -40,11 +40,13 @@
                 Y.lp.code.branchmergeproposal.diff.connect_diff_links();
             }, window);
             Y.on('domready', function() {
+                var bug = { self_link: LP.cache.context.bug_link,
+                            web_link: LP.cache.context.web_link };
                 LP.cache.comment_context = LP.cache.bug;
                 Y.lp.comments.hide.setup_hide_controls();
                 var sl = new Y.lp.bugs.subscribers_list.BugSubscribersLoader({
                     container_box: '#other-bug-subscribers',
-                    bug: LP.cache.bug,
+                    bug: bug,
                     subscribers_details_view:
                         '/+bug-portlet-subscribers-details',
                     subscribe_someone_else_link: '.menu-link-addsubscriber'