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
=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2009-07-08 05:08:23 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2009-07-29 21:31:21 +0000
@@ -89,6 +89,8 @@
89 LAZR_WEBSERVICE_EXPORTED)['params']['parent'].schema = \89 LAZR_WEBSERVICE_EXPORTED)['params']['parent'].schema = \
90 ICodeReviewComment90 ICodeReviewComment
91IBranchMergeProposal['all_comments'].value_type.schema = ICodeReviewComment91IBranchMergeProposal['all_comments'].value_type.schema = ICodeReviewComment
92IBranchMergeProposal['nominateReviewer'].queryTaggedValue(
93 LAZR_WEBSERVICE_EXPORTED)['return_type'].schema = ICodeReviewVoteReference
92IBranchMergeProposal['votes'].value_type.schema = ICodeReviewVoteReference94IBranchMergeProposal['votes'].value_type.schema = ICodeReviewVoteReference
9395
94IHasBranches['getBranches'].queryTaggedValue(96IHasBranches['getBranches'].queryTaggedValue(
9597
=== added file 'lib/canonical/launchpad/javascript/code/codereview.js'
--- lib/canonical/launchpad/javascript/code/codereview.js 1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/javascript/code/codereview.js 2009-08-03 15:11:01 +0000
@@ -0,0 +1,114 @@
1/** Copyright (c) 2009, Canonical Ltd. All rights reserved.
2 *
3 * Library for code review javascript.
4 *
5 * @module CodeReview
6 * @requires base, lazr.anim, lazr.formoverlay
7 */
8
9YUI.add('code.codereview', function(Y) {
10
11Y.codereview = Y.namespace('code.codereview');
12
13var reviewer_picker; // The "Request a review" overlay
14var lp_client;
15
16Y.codereview.connect_links = function() {
17
18 var link = Y.get('#request-review');
19 link.addClass('js-action');
20 link.on('click', show_request_review_form);
21
22};
23
24function show_request_review_form(e) {
25
26 e.preventDefault();
27 var config = {
28 header: 'Request a review',
29 step_title: 'Search'
30 };
31
32 reviewer_picker = Y.lp.picker.create(
33 'ValidPersonOrTeam',
34 function(result) {
35 var review_type = Y.get("[id=field.review_type]").get('value');
36 request_reviewer(result, review_type);
37 },
38 config);
39 reviewer_picker.set('footer_slot', Y.Node.create([
40 '<div>',
41 '<div style="float: left; padding-right: 9px;">',
42 '<label for="field.review_type">Review type:</label><br />',
43 '<span class="fieldRequired">(Optional)</span>',
44 '</div>',
45 '<input class="textType" id="field.review_type" ',
46 'name="field.review_type" size="14" type="text" value="" /></div>'
47 ].join(' ')));
48 reviewer_picker.on('save', function() {});
49 reviewer_picker.on('cancel', function() {});
50 reviewer_picker.show();
51
52}
53
54function request_reviewer(person, reviewtype) {
55
56 // Add the temp "Requesting review..." text
57 var table_row = Y.Node.create([
58 '<tr><td colspan="4">',
59 '<img src="/@@/spinner" />',
60 'Requesting review...',
61 '</td></tr>'].join(""));
62 var last_element = Y.get('#email-review');
63 var reviewer_table = last_element.get('parentNode');
64 reviewer_table.insertBefore(table_row, last_element);
65
66
67 var context = LP.client.cache.context;
68 if (lp_client === undefined) {
69 lp_client = new LP.client.Launchpad();
70 }
71
72 var config = {
73 parameters: {
74 reviewer: LP.client.get_absolute_uri(person.api_uri),
75 review_type: reviewtype
76 },
77 on: {
78 success: function() {
79 var username = person.api_uri.substr(
80 2, person.api_uri.length);
81 add_reviewer_html(username);
82 },
83 failure: function(result) {
84 alert('An error has occurred. Unable to request review.');
85 Y.log(result);
86 }
87 }
88 };
89 lp_client.named_post(context.self_link,
90 'nominateReviewer', config);
91}
92
93
94function add_reviewer_html(username) {
95
96 var VOTES_TABLE_PATH = '+votes';
97 Y.io(VOTES_TABLE_PATH, {
98 on: {
99 success: function(id, response) {
100 var target = Y.get('#votes-target');
101 target.set('innerHTML', response.responseText);
102
103 Y.codereview.connect_links();
104 var new_reviewer = Y.get('#review-' + username);
105 var anim = Y.lazr.anim.green_flash({node: new_reviewer});
106 anim.run();
107 },
108 failure: function() {}
109 }
110 });
111}
112
113
114}, '0.1', {requires: ['base', 'lazr.anim', 'lazr.formoverlay', 'lp.picker']});
0115
=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py 2009-06-15 06:46:17 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py 2009-07-29 21:31:21 +0000
@@ -406,6 +406,7 @@
406 title=_("A reviewer."), schema=IPerson),406 title=_("A reviewer."), schema=IPerson),
407 review_type=Text())407 review_type=Text())
408 @call_with(registrant=REQUEST_USER)408 @call_with(registrant=REQUEST_USER)
409 @operation_returns_entry(Interface) # Really ICodeReviewVoteReference
409 @export_write_operation()410 @export_write_operation()
410 def nominateReviewer(reviewer, registrant, review_type=None):411 def nominateReviewer(reviewer, registrant, review_type=None):
411 """Set the specified person as a reviewer.412 """Set the specified person as a reviewer.
412413
=== modified file 'lib/lp/code/templates/branch-index.pt'
--- lib/lp/code/templates/branch-index.pt 2009-07-14 05:58:56 +0000
+++ lib/lp/code/templates/branch-index.pt 2009-07-16 19:08:45 +0000
@@ -20,10 +20,12 @@
20 }20 }
21 </style>21 </style>
22 <script type="text/javascript"22 <script type="text/javascript"
23 tal:condition="devmode"
23 tal:define="lp_js string:${icingroot}/build"24 tal:define="lp_js string:${icingroot}/build"
24 tal:attributes="src string:${lp_js}/code/branchsubscription.js">25 tal:attributes="src string:${lp_js}/code/branchsubscription.js">
25 </script>26 </script>
26 <script type="text/javascript"27 <script type="text/javascript"
28 tal:condition="devmode"
27 tal:define="lp_js string:${icingroot}/build"29 tal:define="lp_js string:${icingroot}/build"
28 tal:attributes="src string:${lp_js}/code/branchlinks.js">30 tal:attributes="src string:${lp_js}/code/branchlinks.js">
29 </script>31 </script>
3032
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt 2009-07-01 13:16:44 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt 2009-07-30 00:01:58 +0000
@@ -12,11 +12,19 @@
1212
13<body>13<body>
1414
15<style type="text/css" metal:fill-slot="head_epilogue">15
16 #code-review-votes {16<metal:block fill-slot="head_epilogue">
17 margin: 2em 0;17 <style type="text/css">
18 }18 #code-review-votes {
19</style>19 margin: 2em 0;
20 }
21 </style>
22 <script type="text/javascript"
23 tal:define="lp_js string:${icingroot}/build"
24 tal:attributes="src string:${lp_js}/code/codereview.js">
25 </script>
26</metal:block>
27
2028
21<div metal:fill-slot="main"29<div metal:fill-slot="main"
22 tal:define="context_menu context/menu:context">30 tal:define="context_menu context/menu:context">
@@ -73,7 +81,8 @@
73 </div>81 </div>
74 </tal:whiteboard>82 </tal:whiteboard>
7583
76 <tal:votes replace="structure context/@@+votes"/>84 <div id="votes-target"
85 tal:content="structure context/@@+votes" />
7786
78 <tal:comments condition="view/conversation"87 <tal:comments condition="view/conversation"
79 replace="structure view/conversation/@@+render"/>88 replace="structure view/conversation/@@+render"/>
@@ -139,6 +148,25 @@
139 </div>148 </div>
140 </div>149 </div>
141150
151<tal:script
152 replace="structure
153 string:&lt;script id='codereview-script' type='text/javascript'&gt;" />
154 <!--
155 YUI().use('io-base', 'code.codereview', function(Y) {
156
157
158 if(Y.UA.ie) {
159 return;
160 }
161
162 Y.on('domready', function() {
163 Y.code.codereview.connect_links();
164 });
165
166 });
167 -->
168<tal:script replace="structure string:&lt;/script&gt;" />
169
142</div>170</div>
143</body>171</body>
144</html>172</html>
145173
=== modified file 'lib/lp/code/templates/branchmergeproposal-votes.pt'
--- lib/lp/code/templates/branchmergeproposal-votes.pt 2009-07-01 13:16:44 +0000
+++ lib/lp/code/templates/branchmergeproposal-votes.pt 2009-07-30 00:01:58 +0000
@@ -15,7 +15,8 @@
15 </thead>15 </thead>
16 <tbody>16 <tbody>
17 <!-- Current reviews -->17 <!-- Current reviews -->
18 <tr tal:repeat="review view/current_reviews">18 <tr tal:repeat="review view/current_reviews"
19 tal:attributes="id string:review-${review/reviewer/name}">
19 <td>20 <td>
20 <tal:reviewer replace="structure review/reviewer/fmt:link:mainsite">21 <tal:reviewer replace="structure review/reviewer/fmt:link:mainsite">
21 Eric the Reviewer</tal:reviewer>22 Eric the Reviewer</tal:reviewer>
@@ -41,7 +42,8 @@
41 </td>42 </td>
42 </tr>43 </tr>
43 <!-- Pending reviews -->44 <!-- Pending reviews -->
44 <tr tal:repeat="review view/requested_reviews">45 <tr tal:repeat="review view/requested_reviews"
46 tal:attributes="id string:review-${review/reviewer/name}">
45 <td tal:content="structure review/reviewer/fmt:link:mainsite" />47 <td tal:content="structure review/reviewer/fmt:link:mainsite" />
46 <td tal:content="review/review_type" />48 <td tal:content="review/review_type" />
47 <td>49 <td>
@@ -58,7 +60,7 @@
58 </tal:vote-link>60 </tal:vote-link>
59 </td>61 </td>
60 </tr>62 </tr>
61 <tr>63 <tr id="email-review">
62 <td colspan="4" style="padding-top: 1em; patting-bottom: 0.5em">64 <td colspan="4" style="padding-top: 1em; patting-bottom: 0.5em">
63 Review <a href="https://help.launchpad.net/Code/Review"65 Review <a href="https://help.launchpad.net/Code/Review"
64 class="help">via email</a>:66 class="help">via email</a>:
@@ -72,8 +74,11 @@
72 condition="link/enabled">74 condition="link/enabled">
73 <tr>75 <tr>
74 <td colspan="4" style="text-align:right">76 <td colspan="4" style="text-align:right">
75 <div tal:content="structure link/render">77 <div>
76 Request review78 <a id="request-review"
79 class="sprite add"
80 tal:attributes="href link/url"
81 tal:content="link/text">Request a review</a>
77 </div>82 </div>
78 </td>83 </td>
79 </tr>84 </tr>
8085
=== added file 'lib/lp/code/windmill/test_code_review.py'
--- lib/lp/code/windmill/test_code_review.py 1970-01-01 00:00:00 +0000
+++ lib/lp/code/windmill/test_code_review.py 2009-07-30 20:41:48 +0000
@@ -0,0 +1,39 @@
1# Copyright 2009 Canonical Ltd. All rights reserved.
2
3"""Test for code review."""
4
5__metaclass__ = type
6__all__ = []
7
8import windmill
9from windmill.authoring import WindmillTestClient
10
11from canonical.launchpad.windmill.testing import lpuser
12from canonical.launchpad.windmill.testing.widgets import (
13 _search_picker_widget)
14
15
16MERGE_LINK = '%s/~name12/gnome-terminal/klingon/+register-merge' % (
17 windmill.settings['TEST_URL'])
18
19def test_inline_request_a_reviewer():
20 """Test inline request a reviewer."""
21 client = WindmillTestClient("Code review")
22
23 lpuser.FOO_BAR.ensure_login(client)
24
25 client.open(
26 url=windmill.settings['TEST_URL'] + '/~name12/gnome-terminal/klingon/')
27 client.waits.forPageLoad(timeout=u'10000')
28
29 client.click(xpath=u'//a[@href="%s"]' % MERGE_LINK)
30 client.type(text=u'~name12/gnome-terminal/main',
31 id=u'field.target_branch.target_branch')
32 client.click(id=u'field.actions.register')
33
34 client.waits.forPageLoad(timeout=u'10000')
35 client.click(id=u'request-review')
36
37 _search_picker_widget(client, u'sabdfl', 1)
38
39 client.waits.forElement(id=u'review-sabdfl', timeout=u'10000')