launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06023
[Merge] lp:~rvb/launchpad/longpoll-stats-903586 into lp:launchpad
Raphaël Badin has proposed merging lp:~rvb/launchpad/longpoll-stats-903586 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #903586 in Launchpad itself: "Diff against target text is not longpoll updated"
https://bugs.launchpad.net/launchpad/+bug/903586
For more details, see:
https://code.launchpad.net/~rvb/launchpad/longpoll-stats-903586/+merge/87372
This branch makes it so that ,on a merge proposal page, when a new diff is available the stats (number of lines changed, conflicts) are updated. This rely on the existing longpoll mechanism that already updates the diff itself when a new version is available.
= Implementation details =
This branch does 2 things:
- Extract the template bit that generates the stats and expose it mp/++diff-stats (it is done the same way for the diff itself: mp/++diff)
- Change the JavaScript code used by the MP page to update the diff stats when a new version of the diff is available.
= Tests =
firefox lib/lp/code/javascript/tests/test_branchmergeproposal.updater.html
(note that the change to the stats display is already tested in the code)
= QA =
Create a MP and then add a new revision to the branch, wait for the branches to be scanned and make sure that the stats are updated along with the diff itself when the longpoll event kicks in.
--
https://code.launchpad.net/~rvb/launchpad/longpoll-stats-903586/+merge/87372
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/longpoll-stats-903586 into lp:launchpad.
=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml 2011-12-24 17:49:30 +0000
+++ lib/lp/code/browser/configure.zcml 2012-01-03 15:48:33 +0000
@@ -193,6 +193,9 @@
name="++diff"
template="../templates/branchmergeproposal-diff.pt"/>
<browser:page
+ name="++diff-stats"
+ template="../templates/branchmergeproposal-diff-stats.pt"/>
+ <browser:page
name="+pagelet-summary"
template="../templates/branchmergeproposal-pagelet-summary.pt"/>
</browser:pages>
=== modified file 'lib/lp/code/javascript/branchmergeproposal.reviewcomment.js'
--- lib/lp/code/javascript/branchmergeproposal.reviewcomment.js 2011-09-20 15:27:04 +0000
+++ lib/lp/code/javascript/branchmergeproposal.reviewcomment.js 2012-01-03 15:48:33 +0000
@@ -71,6 +71,8 @@
});
}
+namespace.link_scroller = link_scroller;
+
/*
* Make the edit link a javascript link (green).
@@ -245,7 +247,7 @@
var NumberToggle = function () {
- NumberToggle.superclass.constructor.apply(this, arguments);
+ NumberToggle.superclass.constructor.apply(this, arguments);
};
=== modified file 'lib/lp/code/javascript/branchmergeproposal.updater.js'
--- lib/lp/code/javascript/branchmergeproposal.updater.js 2011-09-30 09:15:21 +0000
+++ lib/lp/code/javascript/branchmergeproposal.updater.js 2012-01-03 15:48:33 +0000
@@ -32,6 +32,16 @@
},
/**
+ * The summary node.
+ *
+ * @attribute summary_node
+ */
+ summary_node: {
+ value: null,
+ writeOnce: "initOnly"
+ },
+
+ /**
* Whether or not this MP is still 'pending'.
*
* @attribute pending
@@ -46,6 +56,30 @@
},
/**
+ * The HTML code for the stats diff.
+ *
+ * @attribute diff_stats
+ */
+ diff_stats: {
+ getter: function() {
+ var summary_node = this.get('summary_node');
+ if (!Y.Lang.isValue(summary_node) ||
+ !Y.Lang.isValue(summary_node.one(
+ '#summary-row-b-diff'))) {
+ return null;
+ }
+ return summary_node.one(
+ '#summary-row-b-diff').one('td').get('innerHTML');
+ },
+ setter: function(value) {
+ this._setup_diff_stats_container();
+ var container = this.get(
+ 'summary_node').one('#summary-row-b-diff').one('td');
+ container.set('innerHTML', value);
+ }
+ },
+
+ /**
* The HTML code for the diff.
*
* @attribute diff
@@ -78,14 +112,14 @@
* @method initializer
* @protected
*/
- initializer: function(){
+ initializer: function(cfg){
// If we have not been provided with a Launchpad Client, then
// create one now:
if (null === this.get("lp_client")){
// Create our own instance of the LP client.
this.set("lp_client", new Y.lp.client.Launchpad());
}
- var self = this;
+ this.set('summary_node', cfg.summary_node);
},
/*
@@ -146,6 +180,25 @@
},
/*
+ * Add a row in the page summary table to display the diff stats
+ * if needed.
+ *
+ * @method _setup_diff_stats_container
+ */
+ _setup_diff_stats_container: function() {
+ if (!Y.Lang.isValue(this.get('diff_stats'))) {
+ var summary_node = this.get('summary_node');
+ var diff_stats = Y.Node.create('<tr />')
+ .set('id', 'summary-row-b-diff')
+ .append(Y.Node.create('<th />')
+ .set("text", "Diff against target:"))
+ .append(Y.Node.create('<td />'));
+ summary_node.one(
+ '#summary-row-9-target-branch').insert(diff_stats, 'after');
+ }
+ },
+
+ /*
* Populate the widget with the required nodes to display the diff
* if needed.
*
@@ -167,11 +220,50 @@
},
/*
- * Update the diff content with the last version.
+ * Update the page with the last version of the diff and update the
+ * stats.
*
* @method update
*/
update: function() {
+ this.update_stats();
+ this.update_diff();
+ },
+
+ /*
+ * Update the diff stats with the last version.
+ *
+ * @method update_stats
+ */
+ update_stats: function() {
+ var self = this;
+ var config = {
+ on: {
+ success: function(diff_stats) {
+ self.set('diff_stats', diff_stats);
+ // (re)connect the js scroller link.
+ Y.lp.code.branchmergeproposal.reviewcomment.link_scroller(
+ '#proposal-summary a.diff-link', '#review-diff');
+ var node = self.get('summary_node');
+ Y.lp.anim.green_flash({node: node}).run();
+ },
+ failure: function() {
+ var node = self.get('summary_node');
+ Y.lp.anim.red_flash({node: node}).run();
+ }
+ }
+ };
+ var mp_uri = LP.cache.context.web_link;
+ this.get('lp_client').get(mp_uri + "/++diff-stats", config);
+ },
+
+
+ /*
+ * Update the diff content with the last version.
+ *
+ * @method update_diff
+ */
+ update_diff: function() {
var self = this;
var config = {
on: {
@@ -214,4 +306,5 @@
event_data.edited_fields.indexOf("preview_diff") >= 0);
};
-}, '0.1', {requires: ['node', 'lp.client', 'lp.anim']});
+}, '0.1', {requires: ['node', 'lp.client', 'lp.anim',
+ 'lp.code.branchmergeproposal.reviewcomment']});
=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.updater.html'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.updater.html 2011-09-20 15:30:37 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.updater.html 2012-01-03 15:48:33 +0000
@@ -17,6 +17,9 @@
<script type="text/javascript" src="../../../app/javascript/anim/anim.js"></script>
<script type="text/javascript"
src="../../../app/javascript/extras/extras.js"></script>
+ <script type="text/javascript"
+ src="../branchmergeproposal.reviewcomment.js"></script>
+
<!-- expected variable -->
<script type="text/javascript">
@@ -38,6 +41,15 @@
</div>
<script type="text/x-template" id="pending-mp">
+ <table id="proposal-summary">
+ <tr id="summary-row-9-target-branch">
+ <th>Merge into:</th>
+ <td>
+ <a href="/~me/project/branch"
+ class="sprite branch">lp://dev/~me/project/branch</a>
+ </td>
+ </tr>
+ </table>
<div id="diff-area">
<div class="pending-update" id="diff-pending-update">
<h3>Updating diff...</h3>
=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.updater.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.updater.js 2011-09-30 09:14:49 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.updater.js 2012-01-03 15:48:33 +0000
@@ -30,7 +30,9 @@
.empty()
.append(Y.Node.create(pending_mp));
var diff_area = Y.one('#diff-area');
- this.updater = new UpdaterWidget({srcNode: diff_area});
+ var summary_node = Y.one('#proposal-summary');
+ this.updater = new UpdaterWidget(
+ {srcNode: diff_area, summary_node: summary_node});
LP.cache.context = {
web_link: "https://code.launchpad.dev/~foo/bar/foobr/+merge/123"};
@@ -61,6 +63,19 @@
'srcNode').one('.diff-content').get('text'));
},
+ test__setup_diff_stats_container: function() {
+ Y.Assert.isNull(this.updater.get('diff_stats'));
+ this.updater._setup_diff_stats_container();
+ Y.Assert.areEqual('', this.updater.get('diff_stats'));
+ },
+
+ test_set_diff_stats: function() {
+ this.updater.set('diff_stats', '13 lines (+4/-0) 1 file modified');
+ Y.Assert.areEqual(
+ '13 lines (+4/-0) 1 file modified',
+ this.updater.get('diff_stats'));
+ },
+
test_set_status_updating: function() {
this.updater.set_status_updating();
Y.Assert.areEqual(
@@ -107,13 +122,13 @@
Y.one('.diff-content').get('innerHTML'));
},
- test_update_success: function() {
+ test_update_diff_success: function() {
var mockio = new Y.lp.testing.mockio.MockIo();
this.updater.get('lp_client').io_provider = mockio;
Y.Assert.areEqual(
'',
this.updater.get('diff'));
- this.updater.update();
+ this.updater.update_diff();
mockio.success({
responseText: 'New <span>diff</span>',
responseHeaders: {'Content-Type': 'text/html'}});
@@ -123,14 +138,28 @@
this.updater.get('diff'));
},
- test_update_fires_event: function() {
+ test_update_stats_success: function() {
+ var mockio = new Y.lp.testing.mockio.MockIo();
+ this.updater.get('lp_client').io_provider = mockio;
+ Y.Assert.isNull(this.updater.get('diff_stats'));
+ this.updater.update_stats();
+ mockio.success({
+ responseText: '13 lines (+4/-0) 1 file modified',
+ responseHeaders: {'Content-Type': 'text/html'}});
+
+ Y.Assert.areEqual(
+ '13 lines (+4/-0) 1 file modified',
+ this.updater.get('diff_stats'));
+ },
+
+ test_update_diff_fires_event: function() {
var fired = false;
var mockio = new Y.lp.testing.mockio.MockIo();
this.updater.get('lp_client').io_provider = mockio;
this.updater.on(this.updater.NAME + '.updated', function() {
fired = true;
});
- this.updater.update();
+ this.updater.update_diff();
mockio.success({
responseText: 'New <span>diff</span>',
responseHeaders: {'Content-Type': 'text/html'}});
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt 2011-12-08 12:40:18 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt 2012-01-03 15:48:33 +0000
@@ -230,7 +230,10 @@
if (Y.Lang.isValue(LP.cache.merge_proposal_event_key)) {
var updater = new Y.lp.code.branchmergeproposal.updater.UpdaterWidget(
- {srcNode: Y.one('#diff-area')});
+ {
+ srcNode: Y.one('#diff-area'),
+ summary_node: Y.one('#proposal-summary')
+ });
Y.on(LP.cache.merge_proposal_event_key, function(data) {
if (Y.lp.code.branchmergeproposal.updater.is_mp_diff_updated(data)) {
updater.update();
=== modified file 'lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt'
--- lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt 2010-12-30 19:37:29 +0000
+++ lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt 2012-01-03 15:48:33 +0000
@@ -136,9 +136,7 @@
tal:condition="context/preview_diff">
<th>Diff against target:</th>
<td>
- <tal:diff replace="structure context/preview_diff/fmt:link"/>
- <pre tal:condition="context/preview_diff/has_conflicts"
- tal:content="context/preview_diff/conflicts"/>
+ <div tal:replace="structure context/@@++diff-stats" />
</td>
</tr>
<tr id="summary-row-merge-instruction">
