launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/bug-787765 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #787765 in Launchpad itself: "ProductSeries:+index timeouts"
  https://bugs.launchpad.net/launchpad/+bug/787765

For more details, see:
https://code.launchpad.net/~lifeless/launchpad/bug-787765/+merge/62425

expose_user_administered_teams_to_js is both slow and used many times. Its not meant to be slow, so I've slightly tuned it (set intersection is faster than list intersection - way faster with storm objects), and its not meant to be used many times so I've worked around that by short-circuiting it for now as a pragmatic tradeoff. I've filed a techdebt bug about the fact we're using it inappropriately, but fixing that is high, not critical - and likely to be seriously fiddly so I'm not attempting that now. I've run this past Gary as a preimp.
-- 
https://code.launchpad.net/~lifeless/launchpad/bug-787765/+merge/62425
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-787765 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/structuralsubscription.py'
--- lib/lp/bugs/browser/structuralsubscription.py	2011-05-17 15:41:09 +0000
+++ lib/lp/bugs/browser/structuralsubscription.py	2011-05-26 08:27:38 +0000
@@ -431,33 +431,41 @@
 def expose_user_administered_teams_to_js(request, user, context,
         absoluteURL=absoluteURL):
     """Make the list of teams the user administers available to JavaScript."""
+    # XXX: Robert Collins workaround multiple calls making this cause timeouts:
+    # see bug 788510.
+    objects = IJSONRequestCache(request).objects
+    if 'administratedTeams' in objects:
+        return
     info = []
     api_request = IWebServiceClientRequest(request)
     is_distro = IDistribution.providedBy(context)
+    if is_distro:
+        # If the context is a distro AND a bug supervisor is set then we only
+        # allow subscriptions from members of the bug supervisor team.
+        bug_supervisor = context.bug_supervisor
+    else:
+        bug_supervisor = None
     if user is not None:
-        administrated_teams = user.administrated_teams
+        administrated_teams = set(user.administrated_teams)
         if administrated_teams:
             # Get this only if we need to.
-            membership = list(user.teams_participated_in)
-            for team in administrated_teams:
-                # If the user is not a member of the team itself, then
-                # skip it, because structural subscriptions and their
-                # filters can only be edited by the subscriber.
-                # This can happen if the user is an owner but not a member.
-                if not team in membership:
-                    continue
-                # If the context is a distro AND a bug supervisor is set
-                # AND the admininistered team is not a member of the bug
-                # supervisor team THEN skip it.
-                if (is_distro and context.bug_supervisor is not None and
-                    not team.inTeam(context.bug_supervisor)):
+            membership = set(user.teams_participated_in)
+            # Only consider teams the user is both in and administers:
+            #  If the user is not a member of the team itself, then
+            # skip it, because structural subscriptions and their
+            # filters can only be edited by the subscriber.
+            # This can happen if the user is an owner but not a member.
+            administers_and_in = membership.intersection(administrated_teams)
+            for team in administers_and_in:
+                if (bug_supervisor is not None and
+                    not team.inTeam(bug_supervisor)):
                     continue
                 info.append({
                     'link': absoluteURL(team, api_request),
                     'title': team.title,
                     'url': canonical_url(team),
                 })
-    IJSONRequestCache(request).objects['administratedTeams'] = info
+    objects['administratedTeams'] = info
 
 
 def expose_user_subscriptions_to_js(user, subscriptions, request,