Launchpad itself

Merge lp:~rockstar/launchpad/codereview-js into lp:launchpad

  1. codereview-js
  2. Merge into devel
Proposed by Paul Hummer
Status: Merged
Merged at revision: not available
Proposed branch: lp:~rockstar/launchpad/codereview-js
Merge into: lp:launchpad
Diff against target: None lines
To merge this branch: bzr merge lp:~rockstar/launchpad/codereview-js
Reviewer Review Type Date Requested Status
Martin Albisetti (community) ui Approve
Eleanor Berger (community) code Approve
Review via email: mp+9587@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Paul Hummer (rockstar) wrote :

This branch adds the ability to request a review from someone without having
to do multiple page requests. I used a page template fragment to do the
updates to the template, and I find this means I have less duplicated html in
the javascript.

To test, run the test_code_review.py windmill test

 reviewer beuno ui
 reviewer intellectronica code

Cheers,
Paul

Revision history for this message
Eleanor Berger (intellectronica) :
review: Approve (code)
Revision history for this message
Martin Albisetti (beuno) wrote :

Hi Paul,

This branch looks great, it feels quite polished.

There are two issues I wanted to ask you about:

- The escape key doesn't seem to work for me on this overlay. Any idea why?
- The optional type field feels odd. I think it is because it's not clear what to click to accept, and since the person is above that field, then it's even weirder to have to click on top to save something on the bottom. My feeling is that this should have an accept button come up once you've clicked on a person, to confirm it, and give you time to fill-in the review type.
What do you think?

review: Needs Information (ui)
Revision history for this message
Paul Hummer (rockstar) wrote :

On Mon, 03 Aug 2009 18:41:09 -0000, Martin Albisetti <email address hidden>
wrote:
> - The escape key doesn't seem to work for me on this overlay. Any idea why?

So, this overlay doesn't seem to act like our other overlays, which leads me to
the next point...

> - The optional type field feels odd. I think it is because it's not clear
> what to click to accept, and since the person is above that field, then
> it's even weirder to have to click on top to save something on the bottom.
> My feeling is that this should have an accept button come up once you've
> clicked on a person, to confirm it, and give you time to fill-in the review
> type. What do you think?

So, I've also thought about this. I'd like to land it as-is, but I've spent
all weekend thinking about how it needs to change. The biggest thing, I think,
is that in all other overlays, there's an ok and a cancel, but in this one, on
click of a person, it automatically submits. This means we can't use the
person picker in other situations that also require context (like subscribing
a person to a branch). This branch's work still won't go in vain though,
because we've discovered this issue, and have been putting thought into how we
fix this.

The ultimate plan is to make the person picker itself it's own widget, and then
make the person picker overlay just use that new widget. Then I can have other
overlays that use that widget as well.

Revision history for this message
Martin Albisetti (beuno) wrote :

I understand the fix is way beyond the scope of this branch then. In that scenario, I'm happy for this to land as long as:

- There's a bug filed for this specific interaction to be fixed
- There's bugs filed to fix the underlying issues
- The above gets nailed before 3.0

Does that sound reasonable?

review: Approve (ui)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
2--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2009-07-08 05:08:23 +0000
3+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2009-07-29 21:31:21 +0000
4@@ -89,6 +89,8 @@
5 LAZR_WEBSERVICE_EXPORTED)['params']['parent'].schema = \
6 ICodeReviewComment
7 IBranchMergeProposal['all_comments'].value_type.schema = ICodeReviewComment
8+IBranchMergeProposal['nominateReviewer'].queryTaggedValue(
9+ LAZR_WEBSERVICE_EXPORTED)['return_type'].schema = ICodeReviewVoteReference
10 IBranchMergeProposal['votes'].value_type.schema = ICodeReviewVoteReference
11
12 IHasBranches['getBranches'].queryTaggedValue(
13
14=== added file 'lib/canonical/launchpad/javascript/code/codereview.js'
15--- lib/canonical/launchpad/javascript/code/codereview.js 1970-01-01 00:00:00 +0000
16+++ lib/canonical/launchpad/javascript/code/codereview.js 2009-08-03 15:11:01 +0000
17@@ -0,0 +1,114 @@
18+/** Copyright (c) 2009, Canonical Ltd. All rights reserved.
19+ *
20+ * Library for code review javascript.
21+ *
22+ * @module CodeReview
23+ * @requires base, lazr.anim, lazr.formoverlay
24+ */
25+
26+YUI.add('code.codereview', function(Y) {
27+
28+Y.codereview = Y.namespace('code.codereview');
29+
30+var reviewer_picker; // The "Request a review" overlay
31+var lp_client;
32+
33+Y.codereview.connect_links = function() {
34+
35+ var link = Y.get('#request-review');
36+ link.addClass('js-action');
37+ link.on('click', show_request_review_form);
38+
39+};
40+
41+function show_request_review_form(e) {
42+
43+ e.preventDefault();
44+ var config = {
45+ header: 'Request a review',
46+ step_title: 'Search'
47+ };
48+
49+ reviewer_picker = Y.lp.picker.create(
50+ 'ValidPersonOrTeam',
51+ function(result) {
52+ var review_type = Y.get("[id=field.review_type]").get('value');
53+ request_reviewer(result, review_type);
54+ },
55+ config);
56+ reviewer_picker.set('footer_slot', Y.Node.create([
57+ '<div>',
58+ '<div style="float: left; padding-right: 9px;">',
59+ '<label for="field.review_type">Review type:</label><br />',
60+ '<span class="fieldRequired">(Optional)</span>',
61+ '</div>',
62+ '<input class="textType" id="field.review_type" ',
63+ 'name="field.review_type" size="14" type="text" value="" /></div>'
64+ ].join(' ')));
65+ reviewer_picker.on('save', function() {});
66+ reviewer_picker.on('cancel', function() {});
67+ reviewer_picker.show();
68+
69+}
70+
71+function request_reviewer(person, reviewtype) {
72+
73+ // Add the temp "Requesting review..." text
74+ var table_row = Y.Node.create([
75+ '<tr><td colspan="4">',
76+ '<img src="/@@/spinner" />',
77+ 'Requesting review...',
78+ '</td></tr>'].join(""));
79+ var last_element = Y.get('#email-review');
80+ var reviewer_table = last_element.get('parentNode');
81+ reviewer_table.insertBefore(table_row, last_element);
82+
83+
84+ var context = LP.client.cache.context;
85+ if (lp_client === undefined) {
86+ lp_client = new LP.client.Launchpad();
87+ }
88+
89+ var config = {
90+ parameters: {
91+ reviewer: LP.client.get_absolute_uri(person.api_uri),
92+ review_type: reviewtype
93+ },
94+ on: {
95+ success: function() {
96+ var username = person.api_uri.substr(
97+ 2, person.api_uri.length);
98+ add_reviewer_html(username);
99+ },
100+ failure: function(result) {
101+ alert('An error has occurred. Unable to request review.');
102+ Y.log(result);
103+ }
104+ }
105+ };
106+ lp_client.named_post(context.self_link,
107+ 'nominateReviewer', config);
108+}
109+
110+
111+function add_reviewer_html(username) {
112+
113+ var VOTES_TABLE_PATH = '+votes';
114+ Y.io(VOTES_TABLE_PATH, {
115+ on: {
116+ success: function(id, response) {
117+ var target = Y.get('#votes-target');
118+ target.set('innerHTML', response.responseText);
119+
120+ Y.codereview.connect_links();
121+ var new_reviewer = Y.get('#review-' + username);
122+ var anim = Y.lazr.anim.green_flash({node: new_reviewer});
123+ anim.run();
124+ },
125+ failure: function() {}
126+ }
127+ });
128+}
129+
130+
131+}, '0.1', {requires: ['base', 'lazr.anim', 'lazr.formoverlay', 'lp.picker']});
132
133=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
134--- lib/lp/code/interfaces/branchmergeproposal.py 2009-06-15 06:46:17 +0000
135+++ lib/lp/code/interfaces/branchmergeproposal.py 2009-07-29 21:31:21 +0000
136@@ -406,6 +406,7 @@
137 title=_("A reviewer."), schema=IPerson),
138 review_type=Text())
139 @call_with(registrant=REQUEST_USER)
140+ @operation_returns_entry(Interface) # Really ICodeReviewVoteReference
141 @export_write_operation()
142 def nominateReviewer(reviewer, registrant, review_type=None):
143 """Set the specified person as a reviewer.
144
145=== modified file 'lib/lp/code/templates/branch-index.pt'
146--- lib/lp/code/templates/branch-index.pt 2009-07-14 05:58:56 +0000
147+++ lib/lp/code/templates/branch-index.pt 2009-07-16 19:08:45 +0000
148@@ -20,10 +20,12 @@
149 }
150 </style>
151 <script type="text/javascript"
152+ tal:condition="devmode"
153 tal:define="lp_js string:${icingroot}/build"
154 tal:attributes="src string:${lp_js}/code/branchsubscription.js">
155 </script>
156 <script type="text/javascript"
157+ tal:condition="devmode"
158 tal:define="lp_js string:${icingroot}/build"
159 tal:attributes="src string:${lp_js}/code/branchlinks.js">
160 </script>
161
162=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
163--- lib/lp/code/templates/branchmergeproposal-index.pt 2009-07-01 13:16:44 +0000
164+++ lib/lp/code/templates/branchmergeproposal-index.pt 2009-07-30 00:01:58 +0000
165@@ -12,11 +12,19 @@
166
167 <body>
168
169-<style type="text/css" metal:fill-slot="head_epilogue">
170- #code-review-votes {
171- margin: 2em 0;
172- }
173-</style>
174+
175+<metal:block fill-slot="head_epilogue">
176+ <style type="text/css">
177+ #code-review-votes {
178+ margin: 2em 0;
179+ }
180+ </style>
181+ <script type="text/javascript"
182+ tal:define="lp_js string:${icingroot}/build"
183+ tal:attributes="src string:${lp_js}/code/codereview.js">
184+ </script>
185+</metal:block>
186+
187
188 <div metal:fill-slot="main"
189 tal:define="context_menu context/menu:context">
190@@ -73,7 +81,8 @@
191 </div>
192 </tal:whiteboard>
193
194- <tal:votes replace="structure context/@@+votes"/>
195+ <div id="votes-target"
196+ tal:content="structure context/@@+votes" />
197
198 <tal:comments condition="view/conversation"
199 replace="structure view/conversation/@@+render"/>
200@@ -139,6 +148,25 @@
201 </div>
202 </div>
203
204+<tal:script
205+ replace="structure
206+ string:&lt;script id='codereview-script' type='text/javascript'&gt;" />
207+ <!--
208+ YUI().use('io-base', 'code.codereview', function(Y) {
209+
210+
211+ if(Y.UA.ie) {
212+ return;
213+ }
214+
215+ Y.on('domready', function() {
216+ Y.code.codereview.connect_links();
217+ });
218+
219+ });
220+ -->
221+<tal:script replace="structure string:&lt;/script&gt;" />
222+
223 </div>
224 </body>
225 </html>
226
227=== modified file 'lib/lp/code/templates/branchmergeproposal-votes.pt'
228--- lib/lp/code/templates/branchmergeproposal-votes.pt 2009-07-01 13:16:44 +0000
229+++ lib/lp/code/templates/branchmergeproposal-votes.pt 2009-07-30 00:01:58 +0000
230@@ -15,7 +15,8 @@
231 </thead>
232 <tbody>
233 <!-- Current reviews -->
234- <tr tal:repeat="review view/current_reviews">
235+ <tr tal:repeat="review view/current_reviews"
236+ tal:attributes="id string:review-${review/reviewer/name}">
237 <td>
238 <tal:reviewer replace="structure review/reviewer/fmt:link:mainsite">
239 Eric the Reviewer</tal:reviewer>
240@@ -41,7 +42,8 @@
241 </td>
242 </tr>
243 <!-- Pending reviews -->
244- <tr tal:repeat="review view/requested_reviews">
245+ <tr tal:repeat="review view/requested_reviews"
246+ tal:attributes="id string:review-${review/reviewer/name}">
247 <td tal:content="structure review/reviewer/fmt:link:mainsite" />
248 <td tal:content="review/review_type" />
249 <td>
250@@ -58,7 +60,7 @@
251 </tal:vote-link>
252 </td>
253 </tr>
254- <tr>
255+ <tr id="email-review">
256 <td colspan="4" style="padding-top: 1em; patting-bottom: 0.5em">
257 Review <a href="https://help.launchpad.net/Code/Review"
258 class="help">via email</a>:
259@@ -72,8 +74,11 @@
260 condition="link/enabled">
261 <tr>
262 <td colspan="4" style="text-align:right">
263- <div tal:content="structure link/render">
264- Request review
265+ <div>
266+ <a id="request-review"
267+ class="sprite add"
268+ tal:attributes="href link/url"
269+ tal:content="link/text">Request a review</a>
270 </div>
271 </td>
272 </tr>
273
274=== added file 'lib/lp/code/windmill/test_code_review.py'
275--- lib/lp/code/windmill/test_code_review.py 1970-01-01 00:00:00 +0000
276+++ lib/lp/code/windmill/test_code_review.py 2009-07-30 20:41:48 +0000
277@@ -0,0 +1,39 @@
278+# Copyright 2009 Canonical Ltd. All rights reserved.
279+
280+"""Test for code review."""
281+
282+__metaclass__ = type
283+__all__ = []
284+
285+import windmill
286+from windmill.authoring import WindmillTestClient
287+
288+from canonical.launchpad.windmill.testing import lpuser
289+from canonical.launchpad.windmill.testing.widgets import (
290+ _search_picker_widget)
291+
292+
293+MERGE_LINK = '%s/~name12/gnome-terminal/klingon/+register-merge' % (
294+ windmill.settings['TEST_URL'])
295+
296+def test_inline_request_a_reviewer():
297+ """Test inline request a reviewer."""
298+ client = WindmillTestClient("Code review")
299+
300+ lpuser.FOO_BAR.ensure_login(client)
301+
302+ client.open(
303+ url=windmill.settings['TEST_URL'] + '/~name12/gnome-terminal/klingon/')
304+ client.waits.forPageLoad(timeout=u'10000')
305+
306+ client.click(xpath=u'//a[@href="%s"]' % MERGE_LINK)
307+ client.type(text=u'~name12/gnome-terminal/main',
308+ id=u'field.target_branch.target_branch')
309+ client.click(id=u'field.actions.register')
310+
311+ client.waits.forPageLoad(timeout=u'10000')
312+ client.click(id=u'request-review')
313+
314+ _search_picker_widget(client, u'sabdfl', 1)
315+
316+ client.waits.forElement(id=u'review-sabdfl', timeout=u'10000')