Merge lp:~bac/launchpad/bug-436986-team-map into lp:launchpad

[Brad Crittenden]

Fix timeouts on the team index page caused by the bounds method not limiting data.

[Brad Crittenden (bac)]

= Summary =

The bounds method for the map portlet on the team index page times out as it uses all
participants to calculate the bounds. (Bug 436986)

== Proposed fix ==

1) Pass the limit to the model method that calculates the bounds.
2) Create new views for the portlet rather than relying on the ?preview parameter.
The ?preview happens only in the JS call but the view to calculate the bounds was
being used without preview and no limits.

== Pre-implementation notes ==

None

== Implementation details ==

As above.

== Tests ==

bin/test -vv -t team-views.txt -t team-map.txt

== Demo and Q/A ==

Create a huge team (e.g., big-team)and set the location for each member and then
visit https://launchpad.dev/~big-team. Verify that a subset of the members are shown
and the bounds are set appropriately for that subset.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/browser/configure.zcml
  lib/lp/registry/browser/team.py
  lib/canonical/launchpad/javascript/lp/mapping.js
  lib/lp/registry/browser/tests/team-views.txt
  lib/lp/registry/model/person.py
  lib/lp/registry/stories/location/team-map.txt

== JSLint notices ==
jslint: Lint found in
'/home/bac/canonical/lp-branches/bug-436986-team-map/lib/canonical/launchpad/javascript/bugs/bugtask-index.js':
Line 1382 character 33: Missing semicolon.
        assignee_picker.render()

jslint: No problem found in
'/home/bac/canonical/lp-branches/bug-436986-team-map/lib/canonical/launchpad/javascript/lp/mapping.js'.

jslint: 2 files to lint.

I'll fix this problem before landing.

[Abel Deuring (adeuring)]

thanks for your explanations on IRC for my quite stupid questions...
r=me

review: Approve

[Brad Crittenden (bac)]

> thanks for your explanations on IRC for my quite stupid questions...
> r=me

Good questions and good ideas on making the tests clearer and reducing the propagation of magic numbers.

An incremental diff is at http://pastebin.ubuntu.com/294744/

[Abel Deuring (adeuring)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 16.10.2009 16:24, Brad Crittenden wrote:
>> thanks for your explanations on IRC for my quite stupid questions...
>> r=me
>
> Good questions and good ideas on making the tests clearer and reducing the propagation of magic numbers.
>
> An incremental diff is at http://pastebin.ubuntu.com/294744/

looks good

Abel
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFK2KtRekBPhm8NrtARAqVuAJ9pGKQDc8VIzsBrrFRto7W5ukdFcgCfXaMz
ppWQrb3knAx6QJtc9ESFSDk=
=tDYn
-----END PGP SIGNATURE-----

=== modified file 'lib/canonical/launchpad/javascript/lp/mapping.js'
--- lib/canonical/launchpad/javascript/lp/mapping.js	2009-07-12 15:40:23 +0000
+++ lib/canonical/launchpad/javascript/lp/mapping.js	2009-10-16 14:41:12 +0000
@@ -225,7 +225,7 @@
         }
         var team_map = mapping.getSmallMap(
             'team_map_div', center_lat, center_lng);
-        gDownloadUrl("+mapdata?preview=true", function(data) {
+        gDownloadUrl("+mapdataltd", function(data) {
             mapping.setMarkersInfoWindowForSmallMap(data, team_map);
             });
     };

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2009-10-09 17:18:38 +0000
+++ lib/lp/registry/browser/configure.zcml	2009-10-16 14:41:12 +0000
@@ -1079,6 +1079,12 @@
         <browser:page
             for="lp.registry.interfaces.person.ITeam"
             permission="zope.Public"
+            class="lp.registry.browser.team.TeamMapLtdData"
+            template="../templates/team-map-data.pt"
+            name="+mapdataltd"/>
+        <browser:page
+            for="lp.registry.interfaces.person.ITeam"
+            permission="zope.Public"
             class="lp.registry.browser.person.TeamEditLocationView"
             name="+editlocation"/>
         <browser:page
@@ -1094,7 +1100,7 @@
             class="lp.registry.browser.person.TeamMugshotView"/>
         <browser:page
             for="lp.registry.interfaces.person.ITeam"
-            class="lp.registry.browser.team.TeamMapView"
+            class="lp.registry.browser.team.TeamMapLtdView"
             permission="zope.Public"
             name="+portlet-map"
             template="../templates/team-portlet-map.pt"/>

=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py	2009-09-15 22:55:30 +0000
+++ lib/lp/registry/browser/team.py	2009-10-16 14:41:12 +0000
@@ -15,7 +15,9 @@
     'TeamMailingListModerationView',
     'TeamMailingListSubscribersView',
     'TeamMapData',
+    'TeamMapLtdData',
     'TeamMapView',
+    'TeamMapLtdView',
     'TeamMemberAddView',
     'TeamPrivacyAdapter',
     ]
@@ -1012,14 +1014,7 @@
     """
 
     label = "Team member locations"
-
-    def __init__(self, context, request):
-        """Accept the 'preview' parameter to limit mapped participants."""
-        super(TeamMapView, self).__init__(context, request)
-        if 'preview' in self.request.form:
-            self.limit = 24
-        else:
-            self.limit = None
+    limit = None
 
     def initialize(self):
         # Tell our main-template to include Google's gmap2 javascript so that
@@ -1067,7 +1062,7 @@
     def bounds(self):
         """A dictionary with the bounds and center of the map, or None"""
         if self.has_mapped_participants:
-            return self.context.getMappedParticipantsBounds()
+            return self.context.getMappedParticipantsBounds(self.limit)
         return None
 
     @property
@@ -1110,6 +1105,19 @@
         return body.encode('utf-8')
 
 
+class TeamMapLtdMixin:
+    """A mixin for team views with limited participants."""
+    limit = 24
+
+
+class TeamMapLtdView(TeamMapLtdMixin, TeamMapView):
+    """Team map view with limited participants."""
+
+
+class TeamMapLtdData(TeamMapLtdMixin, TeamMapData):
+    """An XML dump of the locations of limited number of team members."""
+
+
 class TeamHierarchyView(LaunchpadView):
     """View for ~team/+teamhierarchy page."""
 

=== modified file 'lib/lp/registry/browser/tests/team-views.txt'
--- lib/lp/registry/browser/tests/team-views.txt	2009-10-12 18:59:54 +0000
+++ lib/lp/registry/browser/tests/team-views.txt	2009-10-16 14:41:12 +0000
@@ -160,19 +160,19 @@
     >>> len(view.times)
     2
 
-The number of persons returned my mapped_participants is governed by the
-view's limit attribute. the default value is None, meaning there is no
-limit.
+The number of persons returned by mapped_participants is governed by the
+view's limit attribute. The default value is None, meaning there is no
+limit.  This view is used for displaying the full map.
 
     >>> print view.limit
     None
     >>> len(view.mapped_participants)
     3
 
-When the preview parameter is sent to the view, the number of
-mapped_participants is limited to 24.
+A portlet for the map also exists which is used for displaying the map
+inside the person or team page.  It has the number of participants limited.
 
-    >>> view = create_initialized_view(context, '+map', form={'preview': 'anything'})
+    >>> view = create_initialized_view(context, '+portlet-map')
     >>> view.limit
     24
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2009-10-12 11:48:21 +0000
+++ lib/lp/registry/model/person.py	2009-10-16 14:41:12 +0000
@@ -1615,13 +1615,13 @@
         """See `IPersonViewRestricted`."""
         return self._getMappedParticipantsLocations().count()
 
-    def getMappedParticipantsBounds(self):
+    def getMappedParticipantsBounds(self, limit=None):
         """See `IPersonViewRestricted`."""
         max_lat = -90.0
         min_lat = 90.0
         max_lng = -180.0
         min_lng = 180.0
-        locations = self._getMappedParticipantsLocations()
+        locations = self._getMappedParticipantsLocations(limit)
         if self.mapped_participants_count == 0:
             raise AssertionError, (
                 'This method cannot be called when '

=== modified file 'lib/lp/registry/stories/location/team-map.txt'
--- lib/lp/registry/stories/location/team-map.txt	2009-09-14 21:01:18 +0000
+++ lib/lp/registry/stories/location/team-map.txt	2009-10-16 14:41:12 +0000
@@ -56,6 +56,31 @@
     </participants>
     <BLANKLINE>
 
+The team index page has a map with data limited to 24 members and uses
+a slightly different URL.  We can see the URL works though there isn't
+enough test data to demonstrate the limit but that is done in the view
+test.
+
+    >>> anon_browser.open('http://launchpad.dev/~guadamen/+mapdataltd')
+    >>> print anon_browser.contents
+    <?xml version="1.0"...
+    <participants>
+      <!--
+        This collection of team location data is (c) Canonical Ltd and
+        contributors, and the format may be changed at any time.
+      -->
+      <participant
+        displayname="Colin Watson"
+        name="kamion"
+        logo_html="&lt;img alt=&quot;&quot; width=&quot;64&quot; height=&quot;64&quot; src=&quot;/@@/person-logo&quot; /&gt;"
+        url="/~kamion"
+        local_time="..."
+        lat="52.2"
+        lng="0.3"/>
+    </participants>
+    <BLANKLINE>
+
+
 It doesn't make sense to edit the location of the team itself, not even
 if we are an admin, so a team's +editlocation page will simply redirect
 to +map.