Launchpad itself

Code review comment for lp:~allenap/launchpad/show-me-too-counts

  1. show-me-too-counts
  2. Merge into devel
Revision history for this message
Gavin Panella (allenap) wrote :

Thanks for the UI review Michael! I've implemented that. It now says
"This bug affects 1 person" or "This bug affects x people" when x > 1.
If no one is affected, nothing is shown and there's no extraneous
white-space left in its wake.

I've attached a diff of the changes, though perhaps that's more for
the benefit of the code and js reviewer.

post-ui-review.diff
1=== modified file 'lib/canonical/launchpad/javascript/bugs/bugtask-index.js'
2--- lib/canonical/launchpad/javascript/bugs/bugtask-index.js 2009-12-10 14:23:23 +0000
3+++ lib/canonical/launchpad/javascript/bugs/bugtask-index.js 2009-12-10 16:36:41 +0000
4@@ -1791,7 +1791,7 @@
5 */
6 function setup_add_attachment() {
7 // Find zero or more links to modify.
8- var attachment_link = Y.get('.menu-link-addcomment');
9+ var attachment_link = Y.all('.menu-link-addcomment');
10 attachment_link.on('click', function(e) {
11 var comment_input = Y.one('[id="field.comment"]');
12 if (comment_input.get('value') !== '') {
13
14=== modified file 'lib/lp/bugs/browser/bugtask.py'
15--- lib/lp/bugs/browser/bugtask.py 2009-12-10 11:51:24 +0000
16+++ lib/lp/bugs/browser/bugtask.py 2009-12-10 15:41:16 +0000
17@@ -3134,6 +3134,17 @@
18 else:
19 return "This bug doesn't affect you"
20
21+ @property
22+ def anon_affected_statement(self):
23+ """The "this bug affects" statement to show to anonymous users."""
24+ if self.context.users_affected_count == 1:
25+ return "This bug affects 1 person"
26+ elif self.context.users_affected_count > 1:
27+ return "This bug affects %d people" % (
28+ self.context.users_affected_count)
29+ else:
30+ return None
31+
32
33 class BugTaskTableRowView(LaunchpadView):
34 """Browser class for rendering a bugtask row on the bug page."""
35
36=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
37--- lib/lp/bugs/browser/tests/test_bugtask.py 2009-12-10 11:51:24 +0000
38+++ lib/lp/bugs/browser/tests/test_bugtask.py 2009-12-10 15:46:47 +0000
39@@ -186,6 +186,24 @@
40 "This bug affects 2 people, but not you",
41 self.view.affected_statement)
42
43+ def test_anon_affected_statement_no_one_affected(self):
44+ self.bug.markUserAffected(self.bug.owner, False)
45+ self.failUnlessEqual(0, self.bug.users_affected_count)
46+ self.assertIs(None, self.view.anon_affected_statement)
47+
48+ def test_anon_affected_statement_1_user_affected(self):
49+ self.failUnlessEqual(1, self.bug.users_affected_count)
50+ self.failUnlessEqual(
51+ "This bug affects 1 person",
52+ self.view.anon_affected_statement)
53+
54+ def test_anon_affected_statement_2_users_affected(self):
55+ self.view.context.markUserAffected(self.view.user, True)
56+ self.failUnlessEqual(2, self.bug.users_affected_count)
57+ self.failUnlessEqual(
58+ "This bug affects 2 people",
59+ self.view.anon_affected_statement)
60+
61
62 def test_suite():
63 suite = unittest.TestSuite()
64
65=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-affects-me-too.txt'
66--- lib/lp/bugs/stories/bugs/xx-bug-affects-me-too.txt 2009-12-10 11:54:48 +0000
67+++ lib/lp/bugs/stories/bugs/xx-bug-affects-me-too.txt 2009-12-10 16:12:43 +0000
68@@ -73,6 +73,27 @@
69 This bug affects 1 person, but not you
70
71
72+== Anonymous users ==
73+
74+Anonymous users just see the number of affected users.
75+
76+ >>> anon_browser.open(test_bug_url)
77+ >>> print extract_text(find_tag_by_id(
78+ ... anon_browser.contents, 'affectsmetoo'))
79+ This bug affects 1 person
80+
81+If no one is marked as affected by the bug, the message does not
82+appear at all to anonymous users.
83+
84+ >>> login('test@canonical.com')
85+ >>> test_bug.markUserAffected(test_bug.owner, False)
86+ >>> logout()
87+
88+ >>> anon_browser.open(test_bug_url)
89+ >>> print find_tag_by_id(anon_browser.contents, 'affectsmetoo')
90+ None
91+
92+
93 == Static and dynamic support ==
94
95 A bug page contains markup to support both static (no Javascript) and
96
97=== modified file 'lib/lp/bugs/templates/bugtasks-and-nominations-table.pt'
98--- lib/lp/bugs/templates/bugtasks-and-nominations-table.pt 2009-12-10 13:20:50 +0000
99+++ lib/lp/bugs/templates/bugtasks-and-nominations-table.pt 2009-12-10 16:04:44 +0000
100@@ -4,53 +4,60 @@
101 define="context_menu context/menu:context"
102 omit-tag="">
103
104-<div class="actions">
105- <span id="affectsmetoo" style="display: inline"
106- tal:condition="link/enabled"
107- tal:define="link context_menu/affectsmetoo;
108- affected view/current_user_affected_status">
109-
110- <tal:comment condition="nothing">
111- This .static section is shown in browsers with javascript
112- enabled, and before setup_me_too is run.
113- </tal:comment>
114- <span class="static">
115- <tal:affected condition="affected">
116- <img width="14" height="14" src="/@@/flame-icon" alt="" />
117- <tal:statement replace="view/affected_statement" />
118- </tal:affected>
119- <tal:not-affected condition="not:affected">
120- <tal:statement replace="view/affected_statement" />
121- </tal:not-affected>
122- <a href="+affectsmetoo">
123+<tal:affects-editable condition="context_menu/affectsmetoo/enabled">
124+ <div class="actions">
125+ <span id="affectsmetoo" style="display: inline"
126+ tal:define="affected view/current_user_affected_status">
127+
128+ <tal:comment condition="nothing">
129+ This .static section is shown in browsers with javascript
130+ enabled, and before setup_me_too is run.
131+ </tal:comment>
132+ <span class="static">
133+ <tal:affected condition="affected">
134+ <img width="14" height="14" src="/@@/flame-icon" alt="" />
135+ <tal:statement replace="view/affected_statement" />
136+ </tal:affected>
137+ <tal:not-affected condition="not:affected">
138+ <tal:statement replace="view/affected_statement" />
139+ </tal:not-affected>
140+ <a href="+affectsmetoo">
141+ <img class="editicon" src="/@@/edit" alt="Edit" />
142+ </a>
143+ </span>
144+
145+ <tal:comment condition="nothing">
146+ This .dynamic section is used by setup_me_too to display
147+ controls and information in the correct places.
148+ </tal:comment>
149+ <span class="dynamic unseen">
150+ <img src="/@@/flame-icon" alt=""/>
151+ <a href="+affectsmetoo" class="js-action"
152+ ><span class="value" tal:content="view/affected_statement" /></a>
153 <img class="editicon" src="/@@/edit" alt="Edit" />
154- </a>
155- </span>
156-
157- <tal:comment condition="nothing">
158- This .dynamic section is used by setup_me_too to display
159- controls and information in the correct places.
160- </tal:comment>
161- <span class="dynamic unseen">
162- <img src="/@@/flame-icon" alt=""/>
163- <a href="+affectsmetoo" class="js-action"
164- ><span class="value" tal:content="view/affected_statement" /></a>
165- <img class="editicon" src="/@@/edit" alt="Edit" />
166- </span>
167-
168- <script type="text/javascript" tal:content="string:
169- LPS.use('event', 'bugs.bugtask_index', function(Y) {
170- Y.on('load', function(e) {
171- Y.bugs.setup_me_too(
172- ${view/current_user_affected_js_status},
173- ${view/other_users_affected_count});
174- }, window);
175- });
176- ">
177- </script>
178-
179- </span>
180-</div>
181+ </span>
182+
183+ <script type="text/javascript" tal:content="string:
184+ LPS.use('event', 'bugs.bugtask_index', function(Y) {
185+ Y.on('load', function(e) {
186+ Y.bugs.setup_me_too(
187+ ${view/current_user_affected_js_status},
188+ ${view/other_users_affected_count});
189+ }, window);
190+ });
191+ ">
192+ </script>
193+
194+ </span>
195+ </div>
196+</tal:affects-editable>
197+<tal:affects-not-editable condition="not:context_menu/affectsmetoo/enabled">
198+ <div class="actions"
199+ tal:define="statement view/anon_affected_statement"
200+ tal:condition="statement">
201+ <span id="affectsmetoo" style="display: inline" tal:content="statement" />
202+ </div>
203+</tal:affects-not-editable>
204
205 <script type="text/javascript">
206 function toggleFormVisibility(row_id) {

« Back to merge proposal