Merge lp:~abentley/launchpad/mustache-bugs into lp:launchpad

Proposed by Aaron Bentley
Status: Merged
Merged at revision: 14193
Proposed branch: lp:~abentley/launchpad/mustache-bugs
Merge into: lp:launchpad
Prerequisite: lp:~abentley/launchpad/support-mustache
Diff against target: 450 lines (+328/-2)
8 files modified
lib/lp/bugs/browser/bugtask.py (+44/-0)
lib/lp/bugs/browser/tests/test_bugtask.py (+136/-1)
lib/lp/bugs/javascript/buglisting.js (+23/-0)
lib/lp/bugs/javascript/tests/test_buglisting.html (+32/-0)
lib/lp/bugs/javascript/tests/test_buglisting.js (+58/-0)
lib/lp/bugs/templates/buglisting-default.pt (+3/-1)
lib/lp/bugs/templates/buglisting.mustache (+25/-0)
lib/lp/bugs/templates/bugs-listing-table-without-navlinks.pt (+7/-0)
To merge this branch: bzr merge lp:~abentley/launchpad/mustache-bugs
Reviewer Review Type Date Requested Status
Benji York (community) code Approve
Review via email: mp+79441@code.launchpad.net

Commit message

Implement new bug listing style on server-side and client-side.

Description of the change

= Summary =
Fix bug 874595: bug listings need to follow new style

== Proposed fix ==
Use Mustache to render new bugs client-side and server-side.
Screenshot: http://people.canonical.com/~abentley/mustache-listings.png

== Pre-implementation notes ==
Discussed with deryck

== Implementation details ==
This is hidden behind a feature flag (bugs.dynamic_bug_listings.enabled). No change to behaviour or performance is expected while the flag is off.

The Mustache template is provided as a separate file, which is loaded by the view class, and provided in JSON form as LP.mustache_listings.

The data to be rendered is provided as LP.cache.mustache_model.

== Tests ==
bin/test -t buglisting -t test_bugtask

== Demo and Q/A ==
Enable the flag. Go to +bugs for a given project or source package. You should see a display similar to the screenshot above.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/javascript/tests/test_buglisting.html
  lib/lp/bugs/javascript/buglisting.js
  versions.cfg
  setup.py
  utilities/find-changed-files.sh
  lib/lp/bugs/templates/buglisting-default.pt
  lib/lp/bugs/browser/tests/test_bugtask.py
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/templates/buglisting.mustache
  utilities/sourcedeps.conf
  utilities/sourcedeps.cache
  lib/lp/bugs/templates/bugs-listing-table-without-navlinks.pt
  lib/lp/bugs/javascript/tests/test_buglisting.js

To post a comment you must log in.
Benji York (benji) wrote :

Overall this branch looks good. I look forward to using mustache.

I suspect you want to remove the "<em>Server-side mustache</em>" and
"<em>Client-side mustache</em>" bits as well as making the client/server
rendering conditional on the feature flag.

review: Approve (code)
Aaron Bentley (abentley) wrote :

The rendering is already conditional on the feature flag. When this feature is more complete, we will want to switch completely to one rendering, but doing that now would prevent us from QAing either the client-side or the server-side renderings.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/bugs/browser/bugtask.py'
2--- lib/lp/bugs/browser/bugtask.py 2011-10-19 14:54:07 +0000
3+++ lib/lp/bugs/browser/bugtask.py 2011-10-20 19:36:35 +0000
4@@ -53,6 +53,7 @@
5 log,
6 )
7 from operator import attrgetter
8+import os.path
9 import re
10 import transaction
11 import urllib
12@@ -74,6 +75,7 @@
13 )
14 from lazr.restful.utils import smartquote
15 from lazr.uri import URI
16+import pystache
17 from pytz import utc
18 from simplejson import dumps
19 from z3c.pt.pagetemplate import ViewPageTemplateFile
20@@ -2137,6 +2139,25 @@
21 """Returns the bug heat flames HTML."""
22 return bugtask_heat_html(self.bugtask, target=self.target_context)
23
24+ @property
25+ def model(self):
26+ """Provide flattened data about bugtask for simple templaters."""
27+ badges = getAdapter(self.bugtask, IPathAdapter, 'image').badges()
28+ target_image = getAdapter(self.target, IPathAdapter, 'image')
29+ return {
30+ 'importance': self.importance.title,
31+ 'importance_class': 'importance' + self.importance.name,
32+ 'status': self.status.title,
33+ 'status_class': 'status' + self.status.name,
34+ 'title': self.bug.title,
35+ 'id': self.bug.id,
36+ 'bug_url': canonical_url(self.bugtask),
37+ 'bugtarget': self.bugtargetdisplayname,
38+ 'bugtarget_css': target_image.sprite_css(),
39+ 'bug_heat_html': self.bug_heat_html,
40+ 'badges': badges,
41+ }
42+
43
44 class BugListingBatchNavigator(TableBatchNavigator):
45 """A specialised batch navigator to load smartly extra bug information."""
46@@ -2178,6 +2199,26 @@
47 """Return a decorated list of visible bug tasks."""
48 return [self._getListingItem(bugtask) for bugtask in self.batch]
49
50+ @cachedproperty
51+ def mustache_template(self):
52+ template_path = os.path.join(
53+ config.root, 'lib/lp/bugs/templates/buglisting.mustache')
54+ with open(template_path) as template_file:
55+ return template_file.read()
56+
57+ @property
58+ def mustache_listings(self):
59+ return 'LP.mustache_listings = %s;' % dumps(self.mustache_template)
60+
61+ @property
62+ def mustache(self):
63+ return pystache.render(self.mustache_template, self.model)
64+
65+ @property
66+ def model(self):
67+ bugtasks = [bugtask.model for bugtask in self.getBugListingItems()]
68+ return {'bugtasks': bugtasks}
69+
70
71 class NominatedBugReviewAction(EnumeratedType):
72 """Enumeration for nomination review actions"""
73@@ -2436,6 +2477,9 @@
74
75 expose_structural_subscription_data_to_js(
76 self.context, self.request, self.user)
77+ if getFeatureFlag('bugs.dynamic_bug_listings.enabled'):
78+ cache = IJSONRequestCache(self.request)
79+ cache.objects['mustache_model'] = self.search().model
80
81 @property
82 def columns_to_show(self):
83
84=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
85--- lib/lp/bugs/browser/tests/test_bugtask.py 2011-10-05 18:02:45 +0000
86+++ lib/lp/bugs/browser/tests/test_bugtask.py 2011-10-20 19:36:35 +0000
87@@ -3,13 +3,20 @@
88
89 __metaclass__ = type
90
91+from contextlib import contextmanager
92 from datetime import datetime
93+import re
94
95 from lazr.lifecycle.event import ObjectModifiedEvent
96+from lazr.restful.interfaces import IJSONRequestCache
97 from lazr.lifecycle.snapshot import Snapshot
98 from pytz import UTC
99+import soupmatchers
100 from storm.store import Store
101-from testtools.matchers import LessThan
102+from testtools.matchers import (
103+ LessThan,
104+ Not,
105+ )
106 import transaction
107 from zope.component import (
108 getMultiAdapter,
109@@ -38,7 +45,9 @@
110 from lp.bugs.browser.bugtask import (
111 BugActivityItem,
112 BugTaskEditView,
113+ BugTaskListingItem,
114 BugTasksAndNominationsView,
115+ BugTaskSearchListingView,
116 )
117 from lp.bugs.interfaces.bugactivity import IBugActivitySet
118 from lp.bugs.interfaces.bugnomination import IBugNomination
119@@ -54,8 +63,11 @@
120 from lp.services.propertycache import get_property_cache
121 from lp.soyuz.interfaces.component import IComponentSet
122 from lp.testing import (
123+ BrowserTestCase,
124 celebrity_logged_in,
125+ feature_flags,
126 person_logged_in,
127+ set_feature_flag,
128 TestCaseWithFactory,
129 )
130 from lp.testing._webservice import QueryCollector
131@@ -1194,3 +1206,126 @@
132 self._assertThatUnbatchedAndBatchedActivityMatch(
133 unbatched_view.activity_and_comments[4:],
134 batched_view.activity_and_comments)
135+
136+
137+def make_bug_task_listing_item(factory):
138+ owner = factory.makePerson()
139+ bug = factory.makeBug(
140+ owner=owner, private=True, security_related=True)
141+ bugtask = factory.makeBugTask(bug)
142+ bug_task_set = getUtility(IBugTaskSet)
143+ bug_badge_properties = bug_task_set.getBugTaskBadgeProperties(
144+ [bugtask])
145+ badge_property = bug_badge_properties[bugtask]
146+ return owner, BugTaskListingItem(
147+ bugtask,
148+ badge_property['has_branch'],
149+ badge_property['has_specification'],
150+ badge_property['has_patch'],
151+ target_context=bugtask.target)
152+
153+
154+class TestBugTaskSearchListingView(BrowserTestCase):
155+
156+ layer = DatabaseFunctionalLayer
157+
158+ server_listing = soupmatchers.Tag(
159+ 'Server', 'em', text='Server-side mustache')
160+
161+ client_listing = soupmatchers.Tag(
162+ 'client-listing', True, attrs={'id': 'client-listing'})
163+
164+ def makeView(self, bugtask=None):
165+ request = LaunchpadTestRequest()
166+ if bugtask is None:
167+ bugtask = self.factory.makeBugTask()
168+ view = BugTaskSearchListingView(bugtask.target, request)
169+ view.initialize()
170+ return view
171+
172+ @contextmanager
173+ def dynamic_listings(self):
174+ with feature_flags():
175+ set_feature_flag(u'bugs.dynamic_bug_listings.enabled', u'on')
176+ yield
177+
178+ def test_mustache_model_missing_if_no_flag(self):
179+ """The IJSONRequestCache should contain mustache_model."""
180+ view = self.makeView()
181+ cache = IJSONRequestCache(view.request)
182+ self.assertIs(None, cache.objects.get('mustache_model'))
183+
184+ def test_mustache_model_in_json(self):
185+ """The IJSONRequestCache should contain mustache_model.
186+
187+ mustache_model should contain bugtasks, the BugTaskListingItem.model
188+ for each BugTask.
189+ """
190+ owner, item = make_bug_task_listing_item(self.factory)
191+ self.useContext(person_logged_in(owner))
192+ with self.dynamic_listings():
193+ view = self.makeView(item.bugtask)
194+ cache = IJSONRequestCache(view.request)
195+ bugtasks = cache.objects['mustache_model']['bugtasks']
196+ self.assertEqual(1, len(bugtasks))
197+ self.assertEqual(item.model, bugtasks[0])
198+
199+ def getBugtaskBrowser(self):
200+ bugtask = self.factory.makeBugTask()
201+ with person_logged_in(bugtask.target.owner):
202+ bugtask.target.official_malone = True
203+ browser = self.getViewBrowser(
204+ bugtask.target, '+bugs', rootsite='bugs')
205+ return bugtask, browser
206+
207+ def assertHTML(self, browser, *tags, **kwargs):
208+ matcher = soupmatchers.HTMLContains(*tags)
209+ if kwargs.get('invert', False):
210+ matcher = Not(matcher)
211+ self.assertThat(browser.contents, matcher)
212+
213+ @staticmethod
214+ def getBugNumberTag(bug_task):
215+ """Bug numbers with a leading hash are unique to new rendering."""
216+ bug_number_re = re.compile(r'\#%d' % bug_task.bug.id)
217+ return soupmatchers.Tag('bug_number', 'td', text=bug_number_re)
218+
219+ def test_mustache_rendering_missing_if_no_flag(self):
220+ """If the flag is missing, then no mustache features appear."""
221+ bug_task, browser = self.getBugtaskBrowser()
222+ number_tag = self.getBugNumberTag(bug_task)
223+ self.assertHTML(browser, number_tag, invert=True)
224+ self.assertHTML(browser, self.client_listing, invert=True)
225+ self.assertHTML(browser, self.server_listing, invert=True)
226+
227+ def test_mustache_rendering(self):
228+ """If the flag is present, then all mustache features appear."""
229+ with self.dynamic_listings():
230+ bug_task, browser = self.getBugtaskBrowser()
231+ bug_number = self.getBugNumberTag(bug_task)
232+ self.assertHTML(
233+ browser, self.client_listing, self.server_listing, bug_number)
234+
235+
236+class TestBugTaskListingItem(TestCaseWithFactory):
237+
238+ layer = DatabaseFunctionalLayer
239+
240+ def test_model(self):
241+ """Model contains expected fields with expected values."""
242+ owner, item = make_bug_task_listing_item(self.factory)
243+ with person_logged_in(owner):
244+ model = item.model
245+ self.assertEqual('Undecided', model['importance'])
246+ self.assertEqual('importanceUNDECIDED', model['importance_class'])
247+ self.assertEqual('New', model['status'])
248+ self.assertEqual('statusNEW', model['status_class'])
249+ self.assertEqual(item.bug.title, model['title'])
250+ self.assertEqual(item.bug.id, model['id'])
251+ self.assertEqual(canonical_url(item.bugtask), model['bug_url'])
252+ self.assertEqual(item.bugtargetdisplayname, model['bugtarget'])
253+ self.assertEqual('sprite product', model['bugtarget_css'])
254+ self.assertEqual(item.bug_heat_html, model['bug_heat_html'])
255+ self.assertEqual(
256+ '<span alt="private" title="Private" class="sprite private">'
257+ '&nbsp;</span>', model['badges'])
258
259=== added file 'lib/lp/bugs/javascript/buglisting.js'
260--- lib/lp/bugs/javascript/buglisting.js 1970-01-01 00:00:00 +0000
261+++ lib/lp/bugs/javascript/buglisting.js 2011-10-20 19:36:35 +0000
262@@ -0,0 +1,23 @@
263+/* Copyright 2011 Canonical Ltd. This software is licensed under the
264+ * GNU Affero General Public License version 3 (see the file LICENSE).
265+ *
266+ * Client-side rendering of bug listings.
267+ *
268+ * @module bugs
269+ * @submodule buglisting
270+ */
271+
272+YUI.add('lp.bugs.buglisting', function(Y) {
273+
274+var namespace = Y.namespace('lp.bugs.buglisting');
275+
276+namespace.rendertable = function(){
277+ client_listing = Y.one('#client-listing');
278+ if (client_listing === null){
279+ return;
280+ }
281+ var txt = Mustache.to_html(LP.mustache_listings, LP.cache.mustache_model);
282+ client_listing.set('innerHTML', txt);
283+};
284+
285+}, "0.1", {"requires": ["node"]});
286
287=== added file 'lib/lp/bugs/javascript/tests/test_buglisting.html'
288--- lib/lp/bugs/javascript/tests/test_buglisting.html 1970-01-01 00:00:00 +0000
289+++ lib/lp/bugs/javascript/tests/test_buglisting.html 2011-10-20 19:36:35 +0000
290@@ -0,0 +1,32 @@
291+<html>
292+ <head>
293+ <title>Bug task listing</title>
294+
295+ <!-- YUI and test setup -->
296+ <script type="text/javascript"
297+ src="../../../../canonical/launchpad/icing/yui/yui/yui.js">
298+ </script>
299+ <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
300+ <script type="text/javascript"
301+ src="../../../app/javascript/testing/testrunner.js"></script>
302+
303+ <script type="text/javascript"
304+ src="../../../contrib/javascript/mustache.js"></script>
305+
306+ <!-- The module under test -->
307+ <script type="text/javascript"
308+ src="../buglisting.js"></script>
309+
310+ <!-- The test suite -->
311+ <script type="text/javascript"
312+ src="test_buglisting.js"></script>
313+
314+ <!-- Pretty up the sample html -->
315+ <style type="text/css">
316+ div#sample {margin:15px; width:200px; border:1px solid #999; padding:10px;}
317+ </style>
318+ </head>
319+ <body class="yui3-skin-sam">
320+ <div id="fixture"></div>
321+ </body>
322+</html>
323
324=== added file 'lib/lp/bugs/javascript/tests/test_buglisting.js'
325--- lib/lp/bugs/javascript/tests/test_buglisting.js 1970-01-01 00:00:00 +0000
326+++ lib/lp/bugs/javascript/tests/test_buglisting.js 2011-10-20 19:36:35 +0000
327@@ -0,0 +1,58 @@
328+YUI({
329+ base: '../../../../canonical/launchpad/icing/yui/',
330+ filter: 'raw', combine: false, fetchCSS: false
331+ }).use('test', 'console', 'lp.bugs.buglisting', function(Y) {
332+
333+var suite = new Y.Test.Suite("lp.bugs.buglisting Tests");
334+var module = Y.lp.bugs.buglisting;
335+
336+/**
337+ * Test is_notification_level_shown() for a given set of
338+ * conditions.
339+ */
340+suite.add(new Y.Test.Case({
341+ name: 'rendertable',
342+
343+ setUp: function () {
344+ this.MY_NAME = "ME";
345+ window.LP = { links: { me: "/~" + this.MY_NAME } };
346+ },
347+
348+ tearDown: function() {
349+ delete window.LP;
350+ this.set_fixture('');
351+ },
352+
353+ set_fixture: function(value) {
354+ var fixture = Y.one('#fixture');
355+ fixture.set('innerHTML', value);
356+ },
357+ test_rendertable_no_client_listing: function() {
358+ module.rendertable();
359+ },
360+ test_rendertable: function() {
361+ this.set_fixture('<div id="client-listing"></div>');
362+ window.LP.cache = {
363+ mustache_model: {
364+ foo: 'bar'
365+ }
366+ };
367+ window.LP.mustache_listings = "{{foo}}";
368+ module.rendertable();
369+ Y.Assert.areEqual('bar', Y.one('#client-listing').get('innerHTML'));
370+ }
371+}));
372+
373+var handle_complete = function(data) {
374+ window.status = '::::' + JSON.stringify(data);
375+ };
376+Y.Test.Runner.on('complete', handle_complete);
377+Y.Test.Runner.add(suite);
378+
379+var console = new Y.Console({newestOnTop: false});
380+console.render('#log');
381+
382+Y.on('domready', function() {
383+ Y.Test.Runner.run();
384+});
385+});
386
387=== modified file 'lib/lp/bugs/templates/buglisting-default.pt'
388--- lib/lp/bugs/templates/buglisting-default.pt 2011-06-22 14:09:43 +0000
389+++ lib/lp/bugs/templates/buglisting-default.pt 2011-10-20 19:36:35 +0000
390@@ -18,10 +18,12 @@
391 </tal:comment>
392 <script type="text/javascript"
393 tal:condition="not: view/shouldShowAdvancedForm">
394- LPS.use('lp.registry.structural_subscription', function(Y) {
395+ LPS.use('lp.registry.structural_subscription', 'lp.bugs.buglisting',
396+ function(Y) {
397 Y.on('domready', function() {
398 Y.lp.registry.structural_subscription.setup(
399 {content_box: "#structural-subscription-content-box"});
400+ Y.lp.bugs.buglisting.rendertable()
401 });
402 });
403 </script>
404
405=== added file 'lib/lp/bugs/templates/buglisting.mustache'
406--- lib/lp/bugs/templates/buglisting.mustache 1970-01-01 00:00:00 +0000
407+++ lib/lp/bugs/templates/buglisting.mustache 2011-10-20 19:36:35 +0000
408@@ -0,0 +1,25 @@
409+<table class="listing narrow-listing">
410+<tbody>
411+{{#bugtasks}}
412+ <tr>
413+ <td class={{importance_class}}>
414+ {{importance}}
415+ </td>
416+ <td>
417+ #{{id}} <a href="{{bug_url}}">{{title}}</a>
418+ </td>
419+ <td align="right">
420+ {{{badges}}}{{{bug_heat_html}}}
421+ </td>
422+ </tr>
423+ <tr >
424+ <td class={{status_class}} style="border-style: none">
425+ {{status}}
426+ </td>
427+ <td colspan="2" style="border-style: none">
428+ <span class="{{bugtarget_css}}">{{bugtarget}}</span>
429+ </td>
430+ </tr>
431+{{/bugtasks}}
432+</tbody>
433+</table>
434
435=== modified file 'lib/lp/bugs/templates/bugs-listing-table-without-navlinks.pt'
436--- lib/lp/bugs/templates/bugs-listing-table-without-navlinks.pt 2011-05-27 18:10:50 +0000
437+++ lib/lp/bugs/templates/bugs-listing-table-without-navlinks.pt 2011-10-20 19:36:35 +0000
438@@ -76,5 +76,12 @@
439 </tr>
440 </tbody>
441 </table>
442+ <tal:mustache condition="request/features/bugs.dynamic_bug_listings.enabled">
443+ <em>Server-side mustache</em>
444+ <span tal:replace="structure context/mustache" />
445+ <em>Client-side mustache</em>
446+ <span id="client-listing" />
447+ <script tal:content="structure context/mustache_listings" />
448+ </tal:mustache>
449 </tal:results>
450 </tal:root>