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

Proposed by Brad Crittenden on 2009-10-16
Status: Merged
Merged at revision: not available
Proposed branch: lp:~bac/launchpad/bug-436986-team-map
Merge into: lp:launchpad
Diff against target: 183 lines
6 files modified
lib/canonical/launchpad/javascript/lp/mapping.js (+1/-1)
lib/lp/registry/browser/configure.zcml (+7/-1)
lib/lp/registry/browser/team.py (+17/-9)
lib/lp/registry/browser/tests/team-views.txt (+6/-6)
lib/lp/registry/model/person.py (+2/-2)
lib/lp/registry/stories/location/team-map.txt (+25/-0)
To merge this branch: bzr merge lp:~bac/launchpad/bug-436986-team-map
Reviewer Review Type Date Requested Status
Abel Deuring (community) 2009-10-16 Approve on 2009-10-16
Review via email: mp+13471@code.launchpad.net

Commit message

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

To post a comment you must log in.
Brad Crittenden (bac) wrote :

= 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) wrote :

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

review: Approve
Brad Crittenden (bac) 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/

Abel Deuring (adeuring) wrote :

-----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-----

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/javascript/lp/mapping.js'
2--- lib/canonical/launchpad/javascript/lp/mapping.js 2009-07-12 15:40:23 +0000
3+++ lib/canonical/launchpad/javascript/lp/mapping.js 2009-10-16 14:41:12 +0000
4@@ -225,7 +225,7 @@
5 }
6 var team_map = mapping.getSmallMap(
7 'team_map_div', center_lat, center_lng);
8- gDownloadUrl("+mapdata?preview=true", function(data) {
9+ gDownloadUrl("+mapdataltd", function(data) {
10 mapping.setMarkersInfoWindowForSmallMap(data, team_map);
11 });
12 };
13
14=== modified file 'lib/lp/registry/browser/configure.zcml'
15--- lib/lp/registry/browser/configure.zcml 2009-10-09 17:18:38 +0000
16+++ lib/lp/registry/browser/configure.zcml 2009-10-16 14:41:12 +0000
17@@ -1079,6 +1079,12 @@
18 <browser:page
19 for="lp.registry.interfaces.person.ITeam"
20 permission="zope.Public"
21+ class="lp.registry.browser.team.TeamMapLtdData"
22+ template="../templates/team-map-data.pt"
23+ name="+mapdataltd"/>
24+ <browser:page
25+ for="lp.registry.interfaces.person.ITeam"
26+ permission="zope.Public"
27 class="lp.registry.browser.person.TeamEditLocationView"
28 name="+editlocation"/>
29 <browser:page
30@@ -1094,7 +1100,7 @@
31 class="lp.registry.browser.person.TeamMugshotView"/>
32 <browser:page
33 for="lp.registry.interfaces.person.ITeam"
34- class="lp.registry.browser.team.TeamMapView"
35+ class="lp.registry.browser.team.TeamMapLtdView"
36 permission="zope.Public"
37 name="+portlet-map"
38 template="../templates/team-portlet-map.pt"/>
39
40=== modified file 'lib/lp/registry/browser/team.py'
41--- lib/lp/registry/browser/team.py 2009-09-15 22:55:30 +0000
42+++ lib/lp/registry/browser/team.py 2009-10-16 14:41:12 +0000
43@@ -15,7 +15,9 @@
44 'TeamMailingListModerationView',
45 'TeamMailingListSubscribersView',
46 'TeamMapData',
47+ 'TeamMapLtdData',
48 'TeamMapView',
49+ 'TeamMapLtdView',
50 'TeamMemberAddView',
51 'TeamPrivacyAdapter',
52 ]
53@@ -1012,14 +1014,7 @@
54 """
55
56 label = "Team member locations"
57-
58- def __init__(self, context, request):
59- """Accept the 'preview' parameter to limit mapped participants."""
60- super(TeamMapView, self).__init__(context, request)
61- if 'preview' in self.request.form:
62- self.limit = 24
63- else:
64- self.limit = None
65+ limit = None
66
67 def initialize(self):
68 # Tell our main-template to include Google's gmap2 javascript so that
69@@ -1067,7 +1062,7 @@
70 def bounds(self):
71 """A dictionary with the bounds and center of the map, or None"""
72 if self.has_mapped_participants:
73- return self.context.getMappedParticipantsBounds()
74+ return self.context.getMappedParticipantsBounds(self.limit)
75 return None
76
77 @property
78@@ -1110,6 +1105,19 @@
79 return body.encode('utf-8')
80
81
82+class TeamMapLtdMixin:
83+ """A mixin for team views with limited participants."""
84+ limit = 24
85+
86+
87+class TeamMapLtdView(TeamMapLtdMixin, TeamMapView):
88+ """Team map view with limited participants."""
89+
90+
91+class TeamMapLtdData(TeamMapLtdMixin, TeamMapData):
92+ """An XML dump of the locations of limited number of team members."""
93+
94+
95 class TeamHierarchyView(LaunchpadView):
96 """View for ~team/+teamhierarchy page."""
97
98
99=== modified file 'lib/lp/registry/browser/tests/team-views.txt'
100--- lib/lp/registry/browser/tests/team-views.txt 2009-10-12 18:59:54 +0000
101+++ lib/lp/registry/browser/tests/team-views.txt 2009-10-16 14:41:12 +0000
102@@ -160,19 +160,19 @@
103 >>> len(view.times)
104 2
105
106-The number of persons returned my mapped_participants is governed by the
107-view's limit attribute. the default value is None, meaning there is no
108-limit.
109+The number of persons returned by mapped_participants is governed by the
110+view's limit attribute. The default value is None, meaning there is no
111+limit. This view is used for displaying the full map.
112
113 >>> print view.limit
114 None
115 >>> len(view.mapped_participants)
116 3
117
118-When the preview parameter is sent to the view, the number of
119-mapped_participants is limited to 24.
120+A portlet for the map also exists which is used for displaying the map
121+inside the person or team page. It has the number of participants limited.
122
123- >>> view = create_initialized_view(context, '+map', form={'preview': 'anything'})
124+ >>> view = create_initialized_view(context, '+portlet-map')
125 >>> view.limit
126 24
127
128
129=== modified file 'lib/lp/registry/model/person.py'
130--- lib/lp/registry/model/person.py 2009-10-12 11:48:21 +0000
131+++ lib/lp/registry/model/person.py 2009-10-16 14:41:12 +0000
132@@ -1615,13 +1615,13 @@
133 """See `IPersonViewRestricted`."""
134 return self._getMappedParticipantsLocations().count()
135
136- def getMappedParticipantsBounds(self):
137+ def getMappedParticipantsBounds(self, limit=None):
138 """See `IPersonViewRestricted`."""
139 max_lat = -90.0
140 min_lat = 90.0
141 max_lng = -180.0
142 min_lng = 180.0
143- locations = self._getMappedParticipantsLocations()
144+ locations = self._getMappedParticipantsLocations(limit)
145 if self.mapped_participants_count == 0:
146 raise AssertionError, (
147 'This method cannot be called when '
148
149=== modified file 'lib/lp/registry/stories/location/team-map.txt'
150--- lib/lp/registry/stories/location/team-map.txt 2009-09-14 21:01:18 +0000
151+++ lib/lp/registry/stories/location/team-map.txt 2009-10-16 14:41:12 +0000
152@@ -56,6 +56,31 @@
153 </participants>
154 <BLANKLINE>
155
156+The team index page has a map with data limited to 24 members and uses
157+a slightly different URL. We can see the URL works though there isn't
158+enough test data to demonstrate the limit but that is done in the view
159+test.
160+
161+ >>> anon_browser.open('http://launchpad.dev/~guadamen/+mapdataltd')
162+ >>> print anon_browser.contents
163+ <?xml version="1.0"...
164+ <participants>
165+ <!--
166+ This collection of team location data is (c) Canonical Ltd and
167+ contributors, and the format may be changed at any time.
168+ -->
169+ <participant
170+ displayname="Colin Watson"
171+ name="kamion"
172+ logo_html="&lt;img alt=&quot;&quot; width=&quot;64&quot; height=&quot;64&quot; src=&quot;/@@/person-logo&quot; /&gt;"
173+ url="/~kamion"
174+ local_time="..."
175+ lat="52.2"
176+ lng="0.3"/>
177+ </participants>
178+ <BLANKLINE>
179+
180+
181 It doesn't make sense to edit the location of the team itself, not even
182 if we are an admin, so a team's +editlocation page will simply redirect
183 to +map.