launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #16470
[Merge] lp:~cprov/launchpad/diff-navigator into lp:launchpad
Celso Providelo has proposed merging lp:~cprov/launchpad/diff-navigator into lp:launchpad with lp:~cprov/launchpad/previewdiff-promotion as a prerequisite.
Requested reviews:
William Grant (wgrant): code
For more details, see:
https://code.launchpad.net/~cprov/launchpad/diff-navigator/+merge/208186
Required changes/enhancements done in PreviewDiff realm were split in a new branch.
This patchset aims to create a widget for navigating and presenting available diffs (previewdiff really) along with their inline-comments.
Unresolved issues:
* Need of the ++diff-nav template fragment (could generate content only on client-side, depends on JS fmt:displaydate -like feature)
--
https://code.launchpad.net/~cprov/launchpad/diff-navigator/+merge/208186
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py 2014-02-25 17:00:05 +0000
+++ lib/lp/code/browser/branchmergeproposal.py 2014-02-25 17:00:06 +0000
@@ -638,17 +638,6 @@
self.context).event_key
if getFeatureFlag("code.inline_diff_comments.enabled"):
cache.objects['inline_diff_comments'] = True
- cache.objects['previewdiff_ids'] = [
- pd.id for pd in self.context.preview_diffs]
- if self.context.preview_diff:
- cache.objects['published_inline_comments'] = (
- self.context.getInlineComments(self.preview_diff.id))
- cache.objects['draft_inline_comments'] = (
- self.context.getDraftInlineComments(
- self.preview_diff.id, self.user))
- else:
- cache.objects['published_inline_comments'] = []
- cache.objects['draft_inline_comments'] = []
@action('Claim', name='claim')
def claim_action(self, action, data):
=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml 2014-02-25 17:00:05 +0000
+++ lib/lp/code/browser/configure.zcml 2014-02-25 17:00:06 +0000
@@ -165,6 +165,9 @@
name="++diff"
template="../templates/branchmergeproposal-diff.pt"/>
<browser:page
+ name="++diff-nav"
+ template="../templates/branchmergeproposal-diff-navigator.pt"/>
+ <browser:page
name="++diff-stats"
template="../templates/branchmergeproposal-diff-stats.pt"/>
<browser:page
=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-02-25 17:00:05 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-02-25 17:00:06 +0000
@@ -109,8 +109,8 @@
// the LP API (updates during the page life-span). This way
// the handling code can be unified.
if (Y.Lang.isUndefined(comment.person.get)) {
- var lp_client = new Y.lp.client.Launchpad();
- comment.person = lp_client.wrap_resource(null, comment.person);
+ comment.person = namespace.lp_client.wrap_resource(
+ null, comment.person);
}
var header_content = (
comment.person.get('display_name') +
@@ -144,42 +144,161 @@
line.remove();
});
- namespace.inlinecomments = {};
-
- // Display published inline comments.
- // [{'line_number': <lineno>, 'person': <IPerson> 'text': <comment>,
- // 'date': <timestamp>}, ...]
- LP.cache.published_inline_comments.forEach( function (comment) {
- namespace.create_row(comment);
- });
- // Display draft inline comments:
- // {'<line_number>':''<comment>', ...})
- if (LP.cache.draft_inline_comments !== null) {
- var drafts = LP.cache.draft_inline_comments;
- Object.keys(drafts).forEach( function (line_number) {
- var comment = {
- 'line_number': line_number,
- 'text': drafts[line_number],
- };
- namespace.create_row(comment);
- });
- }
-
+ var config_published = {
+ on: {
+ success: function (comments) {
+ // Display published inline comments.
+ // [{'line_number': <lineno>, 'person': <IPerson>,
+ // 'text': <comment>, 'date': <timestamp>}, ...]
+ comments.forEach( function (comment) {
+ namespace.create_row(comment);
+ });
+ },
+ },
+ parameters: {
+ previewdiff_id: namespace.current_previewdiff_id
+ }
+ };
+ namespace.lp_client.named_get(
+ LP.cache.context.self_link, 'getInlineComments', config_published);
+
+ var config_draft = {
+ on: {
+ success: function (drafts) {
+ namespace.inlinecomments = {};
+ if (drafts === null) {
+ return;
+ }
+ // Display draft inline comments:
+ // {'<line_number>':''<comment>', ...})
+ Object.keys(drafts).forEach( function (line_number) {
+ var comment = {
+ 'line_number': line_number,
+ 'text': drafts[line_number],
+ };
+ namespace.create_row(comment);
+ });
+ },
+ },
+ parameters: {
+ previewdiff_id: namespace.current_previewdiff_id
+ }
+ };
+ namespace.lp_client.named_get(
+ LP.cache.context.self_link, 'getDraftInlineComments', config_draft);
};
namespace.setup_inline_comments = function(previewdiff_id) {
if (LP.cache.inline_diff_comments !== true) {
- return;
+ return;
}
- // Store the current preview_diff ID locally.
+ // Store the current diff ID locally.
namespace.current_previewdiff_id = previewdiff_id;
// Add the double-click handler for each row in the diff. This needs
// to be done first since populating existing published and draft
// comments may add more rows.
namespace.add_doubleclick_handler();
namespace.populate_existing_comments();
-
};
+
+
+function DiffNav(config) {
+ DiffNav.superclass.constructor.apply(this, arguments);
+}
+
+Y.mix(DiffNav, {
+
+ NAME: 'diffnav',
+
+ ATTRS: {
+
+ /**
+ * The LP client to use. If none is provided, one will be
+ * created during initialization.
+ *
+ * @attribute lp_client
+ */
+ lp_client: {
+ value: null
+ },
+ }
+
+});
+
+
+Y.extend(DiffNav, Y.Widget, {
+
+ /*
+ * The initializer method that is called from the base Plugin class.
+ *
+ * @method initializer
+ * @protected
+ */
+ 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());
+ }
+ },
+
+ _connectNavigator: function(navigator) {
+ var self = this;
+ var rc = Y.lp.code.branchmergeproposal.reviewcomment;
+ (new rc.NumberToggle()).render();
+ namespace.setup_inline_comments(navigator.get('value'));
+ navigator.on('change', function() {
+ var previewdiff_id = this.get('value');
+ var container = self.get('srcNode').one('.diff-content');
+ var config = {
+ on: {
+ success: function (diff_text) {
+ container.set('innerHTML', diff_text);
+ (new rc.NumberToggle()).render();
+ namespace.setup_inline_comments(previewdiff_id);
+ },
+ failure: function(ignore, response, args) {
+ container.set('innerHTML', response.statusText);
+ Y.lp.anim.red_flash({node: diff_node}).run();
+ },
+ },
+ };
+ var mp_uri = LP.cache.context.web_link;
+ var previewdiff_path = (
+ '/+preview-diff/' + previewdiff_id + '/+diff');
+ self.get('lp_client').get(mp_uri + previewdiff_path, config);
+ });
+ },
+
+ renderUI: function() {
+ if (LP.cache.inline_diff_comments !== true) {
+ return
+ }
+ var self = this;
+ var container = self.get('srcNode').one('.diff-navigator')
+ var config = {
+ on: {
+ success: function(content) {
+ container.set('innerHTML', content);
+ var navigator = container.one('select')
+ Y.lp.anim.green_flash({node: navigator}).run();
+ self._connectNavigator(navigator);
+ },
+ failure: function(ignore, response, args) {
+ container.set('innerHTML', response.statusText);
+ Y.lp.anim.red_flash({node: container}).run();
+ },
+ }
+ };
+ var mp_uri = LP.cache.context.web_link;
+ this.get('lp_client').get(mp_uri + "/++diff-nav", config);
+ },
+
+});
+
+namespace.DiffNav = DiffNav;
+
}, '0.1', {requires: ['event', 'io', 'node', 'widget', 'lp.client',
'lp.ui.editor']});
=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html 2014-01-21 04:51:48 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html 2014-02-25 17:00:06 +0000
@@ -53,8 +53,10 @@
src="../../../../../build/js/lp/app/inlineedit/editor.js"></script>
<!-- The module under test. -->
+ <script type="text/javascript" src="../branchmergeproposal.reviewcomment.js"></script>
<script type="text/javascript" src="../branchmergeproposal.inlinecomments.js"></script>
+
<!-- The test suite -->
<script type="text/javascript" src="test_branchmergeproposal.inlinecomments.js"></script>
@@ -70,6 +72,22 @@
<ul id="suites">
<li>lp.code.branchmergeproposal.inlinecomments.test</li>
</ul>
+
+ <div id="review-diff">
+
+ <h2>Preview Diff</h2>
+
+ <div class="diff-navigator">
+ </div>
+
+ <div class="diff-content">
+
+ <div>
+ <div>
+ <ul class="horizontal">
+ </ul>
+ </div>
+
<table class="diff">
<tbody>
<tr id="diff-line-1">
@@ -86,5 +104,11 @@
</tr>
</tbody>
</table>
+
+ </div>
+ </div>
+
+ </div>
+
</body>
</html>
=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-02-25 17:00:05 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-02-25 17:00:06 +0000
@@ -12,11 +12,14 @@
setUp: function () {
// Loads testing values into LP.cache.
- LP.cache.published_inline_comments = [];
- LP.cache.draft_inline_comments = {};
- LP.cache.preview_diff_ids = [1];
+ LP.cache.context = {
+ web_link: "https://launchpad.dev/~foo/bar/foobr/+merge/1";,
+ self_link: "https://code.launchpad.dev/api/devel/~foo/bar/foobr/+merge/1";
+ };
+
LP.cache.inline_diff_comments = true;
module.current_previewdiff_id = 1;
+ module.inlinecomments = {};
},
tearDown: function () {},
@@ -33,20 +36,47 @@
"display_name": "Foo Bar",
"name": "name16",
"web_link": "http://launchpad.dev/~name16";,
- "resource_type_link": "http://launchpad.dev/api/devel/#person";,
+ "resource_type_link": "http://launchpad.dev/api/devel/#person";
};
- // Create a published and a draft inline comments.
- LP.cache.published_inline_comments = [
+ // Overrides module LP client by one using 'mockio'.
+ var mockio = new Y.lp.testing.mockio.MockIo();
+ module.lp_client = new Y.lp.client.Launchpad(
+ {io_provider: mockio});
+
+ // Populate the page.
+ module.populate_existing_comments();
+
+ // LP was hit twice for fetching published and draft inline
+ // comments
+ Y.Assert.areEqual(2, mockio.requests.length);
+
+ // Last request was for loading draft comments, let's
+ // respond it.
+ Y.Assert.areSame(
+ "ws.op=getDraftInlineComments&previewdiff_id=1",
+ mockio.requests[1].config.data);
+ draft_comments = {'3': 'Zoing!'};
+ mockio.success({
+ responseText: Y.JSON.stringify(draft_comments),
+ responseHeaders: {'Content-Type': 'application/json'}});
+
+ // First request was for loading published comments, let's
+ // respond it (mockio.last_request need a tweak to respond
+ // past requests)
+ Y.Assert.areSame(
+ "ws.op=getInlineComments&previewdiff_id=1",
+ mockio.requests[0].config.data);
+ mockio.last_request = mockio.requests[0];
+ published_comments = [
{'line_number': '2',
'person': person_obj,
'text': 'This is preloaded.',
'date': '2012-08-12 17:45'},
];
- LP.cache.draft_inline_comments = {'3': 'Zoing!'};
-
- // Populate the page.
- module.populate_existing_comments();
+ mockio.success({
+ responseText: Y.JSON.stringify(published_comments),
+ responseHeaders: {'Content-Type': 'application/json'}});
// Published comment is displayed.
var header = Y.one('#diff-line-2').next();
@@ -69,10 +99,8 @@
// Overrides module LP client by one using 'mockio'.
var mockio = new Y.lp.testing.mockio.MockIo();
- LP.cache.context = {'self_link': 'mock/'};
- var ns = Y.namespace(
- 'lp.code.branchmergeproposal.inlinecomments');
- ns.lp_client = new Y.lp.client.Launchpad({io_provider: mockio});
+ module.lp_client = new Y.lp.client.Launchpad(
+ {io_provider: mockio});
// No draft comment in line 1.
Y.Assert.isNull(Y.one('#ict-1-draft-header'));
@@ -124,7 +152,8 @@
module.current_previewdiff_id = null
LP.cache.inline_diff_comments = false;
- module.setup_inline_comments();
+ module.current_previewdiff_id = null
+ module.setup_inline_comments(1);
Y.Assert.isFalse(called);
Y.Assert.isNull(module.current_previewdiff_id);
@@ -142,12 +171,81 @@
Y.Assert.isTrue(called);
Y.Assert.areEqual(1, module.current_previewdiff_id);
+ },
+
+ test_diff_nav_feature_flag_disabled: function() {
+ // Disable feature flag.
+ LP.cache.inline_diff_comments = false;
+ // Overrides module LP client by one using 'mockio'.
+ var mockio = new Y.lp.testing.mockio.MockIo();
+ lp_client = new Y.lp.client.Launchpad({io_provider: mockio});
+ // Create a fake setup_inline_comments for testing purposes.
+ var called_diff_id = null;
+ fake_setup = function(diff_id) {
+ called_diff_id = diff_id;
+ };
+ module.setup_inline_comments = fake_setup;
+ // Render DiffNav widget..
+ (new module.DiffNav(
+ {srcNode: Y.one('#review-diff'), lp_client: lp_client}
+ )).render();
+ // Nothing actually happens.
+ Y.Assert.areEqual(0, mockio.requests.length);
+ Y.Assert.isNull(called_diff_id);
+ },
+
+ test_diff_nav: function() {
+ // Overrides module LP client by one using 'mockio'.
+ var mockio = new Y.lp.testing.mockio.MockIo();
+ lp_client = new Y.lp.client.Launchpad(
+ {io_provider: mockio});
+ // Create a fake setup_inline_comments for testing purposes.
+ var called_diff_id = null;
+ fake_setup = function(diff_id) {
+ called_diff_id = diff_id;
+ };
+ module.setup_inline_comments = fake_setup;
+ // Render DiffNav widget on the test HTML content.
+ (new module.DiffNav(
+ {srcNode: Y.one('#review-diff'), lp_client: lp_client}
+ )).render();
+ // diff-navigator section content was filled
+ Y.Assert.areEqual(1, mockio.requests.length);
+ Y.Assert.areSame(
+ "/~foo/bar/foobr/+merge/1/++diff-nav",
+ mockio.last_request.url);
+ mockio.success({
+ responseText: (
+ "<select>" +
+ "<option value=\"101\" selected=\"selected\">ONE</option>" +
+ "<option value=\"202\">TWO</option>" +
+ "</select>"),
+ responseHeaders: {'Content-Type': 'text/html'}});
+ // The local (fake) setup_inline_comments function
+ // was called with the selected diff_id value.
+ Y.Assert.areEqual(101, called_diff_id);
+ // Simulate a change in the diff navigator.
+ Y.one('select').set('value', 202);
+ Y.one('select').simulate('change');
+ // diff content was updated with the selected diff_id content.
+ Y.Assert.areEqual(2, mockio.requests.length);
+ Y.Assert.areSame(
+ "/~foo/bar/foobr/+merge/1/+preview-diff/202/+diff",
+ mockio.last_request.url);
+ // remove NumberToggle content before reusing 'diff-content'
+ // content, so we do not have it rendered twice.
+ Y.one('.diff-content div div ul.horizontal').empty();
+ mockio.success({
+ responseText: Y.one('.diff-content').get('innerHTML'),
+ responseHeaders: {'Content-Type': 'text/html'}});
+ Y.Assert.areEqual(202, called_diff_id);
}
}));
}, '0.1', {
requires: ['node-event-simulate', 'test', 'lp.testing.helpers',
- 'console', 'lp.client', 'lp.testing.mockio',
- 'lp.code.branchmergeproposal.inlinecomments']
+ 'console', 'lp.client', 'lp.testing.mockio', 'widget',
+ 'lp.code.branchmergeproposal.inlinecomments',
+ 'lp.code.branchmergeproposal.reviewcomment']
});
=== added file 'lib/lp/code/templates/branchmergeproposal-diff-navigator.pt'
--- lib/lp/code/templates/branchmergeproposal-diff-navigator.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/code/templates/branchmergeproposal-diff-navigator.pt 2014-02-25 17:00:06 +0000
@@ -0,0 +1,17 @@
+<tal:root
+ xmlns:tal="http://xml.zope.org/namespaces/tal";
+>
+
+ <select id="available_diffs_choice" name="available_diffs_choice"
+ tal:condition="context/preview_diffs">
+ <option tal:repeat="diff context/preview_diffs"
+ tal:attributes="value diff/id; selected
+ python:diff.id == view.preview_diff.id">
+ <tal:displayname replace="diff/displayname"
+ >r10 into r5</tal:displayname>
+ <tal:date replace="diff/date_created/fmt:displaydate"
+ >on 2014-01-25</tal:date>
+ </option>
+ </select>
+
+</tal:root>
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt 2014-02-25 17:00:05 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt 2014-02-25 17:00:06 +0000
@@ -184,6 +184,12 @@
</div>
<div id="review-diff" tal:condition="view/preview_diff">
<h2>Preview Diff </h2>
+
+ <div class="diff-navigator"
+ tal:condition="features/code.inline_diff_comments.enabled">
+ <div tal:replace="structure context/@@++diff-nav"/>
+ </div>
+
<div class="diff-content">
<div tal:replace="structure context/@@++diff"/>
</div>
@@ -197,38 +203,28 @@
LPJS.use('io-base', 'lp.code.branchmergeproposal.reviewcomment',
'lp.code.branchmergeproposal.status', 'lp.app.comment',
'lp.code.branchmergeproposal.updater', 'lp.app.widgets.expander',
- 'lp.code.branch.revisionexpander',
+ 'lp.code.branch.revisionexpander'',
'lp.code.branchmergeproposal.inlinecomments', function(Y) {
+ var ic = Y.lp.code.branchmergeproposal.inlinecomments;
+
Y.on('load', function() {
var logged_in = LP.links['me'] !== undefined;
var ic = Y.lp.code.branchmergeproposal.inlinecomments;
if (logged_in) {
var comment = new Y.lp.app.comment.CodeReviewComment();
comment.render();
-
comment._add_comment_success = function () {
- var lp_client = new Y.lp.client.Launchpad();
- var config = {
- on: {
- success: function (r) {
- LP.cache.published_inline_comments = r;
- LP.cache.draft_inline_comments = null;
- ic.populate_existing_comments();
- },
- },
- parameters: {
- previewdiff_id: ic.current_previewdiff_id,
- }
- };
- lp_client.named_get(
- LP.cache.context.self_link, 'getInlineComments', config);
+ if (LP.cache.inline_diff_comments === true) {
+ // No need to re-setup diff-line click handlers because
+ // the diff content did not change.
+ ic.populate_existing_comments();
+ }
}
Y.lp.code.branchmergeproposal.status.connect_status(conf);
}
Y.lp.code.branchmergeproposal.reviewcomment.connect_links();
- (new Y.lp.code.branchmergeproposal.reviewcomment.NumberToggle()
- ).render();
+ (new ic.DiffNav({srcNode: Y.one('#review-diff')})).render();
if (Y.Lang.isValue(LP.cache.merge_proposal_event_key)) {
var updater = new Y.lp.code.branchmergeproposal.updater.UpdaterWidget(
@@ -242,11 +238,9 @@
}
});
updater.on(updater.NAME + '.updated', function() {
- (new Y.lp.code.branchmergeproposal.reviewcomment.NumberToggle()
- ).render();
+ (new ic.DiffNav({srcNode: Y.one('#review-diff')})).render();
});
}
-
}, window);
Y.on('domready', function() {
@@ -256,9 +250,6 @@
'.expander-content',
false,
Y.lp.code.branch.revisionexpander.bmp_diff_loader);
- var previewdiff_id = LP.cache.previewdiff_ids.slice(-1)[0];
- var ic = Y.lp.code.branchmergeproposal.inlinecomments;
- ic.setup_inline_comments(previewdiff_id);
});
});
<tal:script replace="structure string:</script>" />
Follow ups
