launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #16587
[Merge] lp:~cprov/launchpad/ic-ui-stab-two into lp:launchpad
Celso Providelo has proposed merging lp:~cprov/launchpad/ic-ui-stab-two into lp:launchpad.
Requested reviews:
William Grant (wgrant): code
For more details, see:
https://code.launchpad.net/~cprov/launchpad/ic-ui-stab-two/+merge/214158
Creating *scroller* links to navigate from review comments to their related inline-comments.
It uses the existing link_scroller() implementation to change the page focus to the review-diff area and uses the DIffNav() to update the diff and comments to the related contents.
Also incorporating previous changes (ic-ui-stab-one) removing diff-content handling from Updater() widget and delegating to DiffNav() even when inline-comments are not enabled.
--
https://code.launchpad.net/~cprov/launchpad/ic-ui-stab-two/+merge/214158
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
=== modified file 'lib/lp/app/javascript/comment.js'
--- lib/lp/app/javascript/comment.js 2014-02-25 16:53:24 +0000
+++ lib/lp/app/javascript/comment.js 2014-04-04 05:33:32 +0000
@@ -291,7 +291,7 @@
}, {
ATTRS: {
lp_client: {
- valueFn: function() { return new Y.lp.client.Launchpad() }
+ valueFn: function() { return new Y.lp.client.Launchpad(); }
},
animate: { value: true }
}
@@ -459,7 +459,8 @@
namespace.Comment.prototype.reset_contents.apply(this);
},
/**
- * Insert the specified HTML into the page.
+ * Insert the specified HTML into the page and fires
+ * 'CodeReviewComment.appended' event.
*
* @method insert_comment_HTML
* @param message_html The HTML of the comment to insert.
@@ -469,6 +470,7 @@
var comment = Y.Node.create(message_html);
conversation.appendChild(comment);
this.reset_contents();
+ this.fire('CodeReviewComment.appended');
Y.lp.anim.green_flash({node: comment}).run();
},
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py 2014-02-25 19:40:56 +0000
+++ lib/lp/code/browser/branchmergeproposal.py 2014-04-04 05:33:32 +0000
@@ -61,7 +61,6 @@
SimpleTerm,
SimpleVocabulary,
)
-from zope.security.proxy import removeSecurityProxy
from lp import _
from lp.app.browser.launchpadform import (
@@ -112,7 +111,10 @@
)
from lp.services.librarian.interfaces.client import LibrarianServerError
from lp.services.messages.interfaces.message import IMessageSet
-from lp.services.propertycache import cachedproperty
+from lp.services.propertycache import (
+ cachedproperty,
+ get_property_cache,
+ )
from lp.services.webapp import (
canonical_url,
ContextMenu,
@@ -121,7 +123,6 @@
Link,
Navigation,
stepthrough,
- stepto,
)
from lp.services.webapp.authorization import check_permission
from lp.services.webapp.breadcrumb import Breadcrumb
@@ -684,8 +685,22 @@
for comment in merge_proposal.all_comments)
merge_proposal = merge_proposal.supersedes
comments = sorted(comments, key=operator.attrgetter('date'))
+ self._populate_previewdiffs(comments)
return CodeReviewConversation(comments)
+ def _populate_previewdiffs(self, comments):
+ """Lookup and populate caches for 'previewdiff_id'.
+
+ Only operated on objects providing `ICodeReviewComment`.
+ """
+ comments = [comment for comment in comments
+ if ICodeReviewComment.providedBy(comment)]
+ cric_set = getUtility(ICodeReviewInlineCommentSet)
+ relations = cric_set.getPreviewDiffsForComments(comments)
+ for comment in comments:
+ get_property_cache(
+ comment).previewdiff_id = relations.get(comment.id)
+
@property
def comments(self):
"""Return comments associated with this proposal, plus styling info.
=== modified file 'lib/lp/code/browser/codereviewcomment.py'
--- lib/lp/code/browser/codereviewcomment.py 2014-02-25 16:53:24 +0000
+++ lib/lp/code/browser/codereviewcomment.py 2014-04-04 05:33:32 +0000
@@ -98,6 +98,14 @@
return ''
@cachedproperty
+ def previewdiff_id(self):
+ inline_comment = getUtility(
+ ICodeReviewInlineCommentSet).getByReviewComment(self.comment)
+ if inline_comment is not None:
+ return inline_comment.previewdiff_id
+ return None
+
+ @cachedproperty
def body_text(self):
"""Get the body text for the message."""
return self.comment.message_body
@@ -290,8 +298,9 @@
data.get('publish_inline_comments')):
previewdiff_id = self.previewdiff.id
inline_comments = (
- self.branch_merge_proposal.getDraftInlineComments(diff_id))
- comment = self.branch_merge_proposal.createComment(
+ self.branch_merge_proposal.getDraftInlineComments(
+ previewdiff_id))
+ self.branch_merge_proposal.createComment(
self.user, subject=None, content=data['comment'],
parent=self.reply_to, vote=vote, review_type=review_type,
previewdiff_id=previewdiff_id, inline_comments=inline_comments)
=== modified file 'lib/lp/code/browser/tests/test_codereviewcomment.py'
--- lib/lp/code/browser/tests/test_codereviewcomment.py 2012-12-26 01:32:19 +0000
+++ lib/lp/code/browser/tests/test_codereviewcomment.py 2014-04-04 05:33:32 +0000
@@ -9,21 +9,33 @@
HTMLContains,
Tag,
)
-from testtools.matchers import Not
+from testtools.matchers import (
+ Equals,
+ Not,
+ )
+from zope.component import getUtility
from lp.code.browser.codereviewcomment import (
CodeReviewDisplayComment,
ICodeReviewDisplayComment,
)
+from lp.code.interfaces.codereviewinlinecomment import (
+ ICodeReviewInlineCommentSet,
+ )
from lp.services.webapp import canonical_url
from lp.services.webapp.interfaces import IPrimaryContext
from lp.testing import (
BrowserTestCase,
person_logged_in,
+ StormStatementRecorder,
TestCaseWithFactory,
verifyObject,
)
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import (
+ DatabaseFunctionalLayer,
+ LaunchpadFunctionalLayer,
+ )
+from lp.testing.matchers import HasQueryCount
class TestCodeReviewComments(TestCaseWithFactory):
@@ -54,6 +66,65 @@
verifyObject(ICodeReviewDisplayComment, display_comment)
+class TestCodeReviewCommentInlineComments(TestCaseWithFactory):
+ """Test `CodeReviewDisplayComment` integration with inline-comments."""
+
+ layer = LaunchpadFunctionalLayer
+
+ def makeInlineComment(self, person, comment, previewdiff=None,
+ comments=None):
+ # Test helper for creating inline comments.
+ if previewdiff is None:
+ previewdiff = self.factory.makePreviewDiff()
+ if comments is None:
+ comments = {'1': 'Foo'}
+ getUtility(ICodeReviewInlineCommentSet).ensureDraft(
+ previewdiff, person, comments)
+ cric = getUtility(ICodeReviewInlineCommentSet).publishDraft(
+ previewdiff, person, comment)
+ return cric
+
+ def test_display_comment_inline_comment(self):
+ # The CodeReviewDisplayComment links to related inline comments
+ # when they exist.
+ person = self.factory.makePerson()
+ with person_logged_in(person):
+ comment = self.factory.makeCodeReviewComment()
+ # `CodeReviewDisplayComment.previewdiff_id` is None if there
+ # is no related inline-comments.
+ display_comment = CodeReviewDisplayComment(comment)
+ self.assertIsNone(display_comment.previewdiff_id)
+ # Create a `PreviewDiff` and add inline-comments in
+ # the context of this review comment.
+ with person_logged_in(person):
+ previewdiff = self.factory.makePreviewDiff()
+ self.makeInlineComment(person, comment, previewdiff)
+ # 'previewdiff_id' property is cached, so its value did not
+ # change on the existing object.
+ self.assertIsNone(display_comment.previewdiff_id)
+ # On a new object, it successfully returns the `PreviewDiff.id`
+ # containing inline-comments related with this review comment.
+ display_comment = CodeReviewDisplayComment(comment)
+ self.assertEqual(previewdiff.id, display_comment.previewdiff_id)
+
+ def test_conversation_with_previewdiffs_populated(self):
+ # `CodeReviewConversation` comments have 'previewdiff_id'
+ # property pre-populated in view.
+ person = self.factory.makePerson()
+ merge_proposal = self.factory.makeBranchMergeProposal()
+ with person_logged_in(person):
+ for i in range(5):
+ comment = self.factory.makeCodeReviewComment(
+ merge_proposal=merge_proposal)
+ self.makeInlineComment(person, comment)
+ from lp.testing.views import create_initialized_view
+ view = create_initialized_view(merge_proposal, '+index')
+ conversation = view.conversation
+ with StormStatementRecorder() as recorder:
+ [c.previewdiff_id for c in conversation.comments]
+ self.assertThat(recorder, HasQueryCount(Equals(0)))
+
+
class TestCodeReviewCommentHtml(BrowserTestCase):
layer = DatabaseFunctionalLayer
=== modified file 'lib/lp/code/interfaces/codereviewinlinecomment.py'
--- lib/lp/code/interfaces/codereviewinlinecomment.py 2014-03-23 18:20:31 +0000
+++ lib/lp/code/interfaces/codereviewinlinecomment.py 2014-04-04 05:33:32 +0000
@@ -14,10 +14,7 @@
Attribute,
Interface,
)
-from zope.schema import (
- Datetime,
- TextLine,
- )
+from zope.schema import Datetime
from lp import _
from lp.code.interfaces.codereviewcomment import ICodeReviewComment
@@ -26,6 +23,8 @@
class ICodeReviewInlineComment(Interface):
+
+ previewdiff_id = Attribute(_('The preview diff ID'))
previewdiff = Reference(
title=_('The preview diff'), schema=IPreviewDiff, required=True,
readonly=True)
@@ -76,5 +75,17 @@
"""Return published comments for a given `CodeReviewComment`.
:param comment: The `CodeReviewComment` for linked to the inline
- comments.
+ comments.
+ """
+
+ def getPreviewDiffsForComments(comments):
+ """Return a dictionary container related comments and diffs.
+
+ Used for prepopulating `BranchMergeProposal` view caches.
+ `CodeReviewComment` and `PreviewDiff` are related by the existence
+ of `CodeReviewInlineComment`.
+
+ :param comments: a list of `CodeReviewComment`s
+ :return: a dictionary containing the given `CodeReviewComment.id`s
+ and the corresponding `PreviewDiff.id` or None.
"""
=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-02-28 01:19:39 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-04-04 05:33:32 +0000
@@ -35,7 +35,7 @@
header_row = namespace.create_row(
{'line_number': line_number});
}
- var content_row = header_row.next()
+ var content_row = header_row.next();
var widget = new Y.EditableText({
contentBox: content_row.one(
'#inlinecomment-' + line_number + '-draft'),
@@ -48,9 +48,9 @@
handle_widget_button = function(saved, comment) {
if (saved === false) {
handling_request = false;
- if (namespace.inlinecomments[line_number] === undefined) {
- header_row.remove(true);
- }
+ if (namespace.inlinecomments[line_number] === undefined) {
+ header_row.remove(true);
+ }
return;
}
@@ -119,7 +119,7 @@
'text', header_content);
// XXX cprov 20140226: the date should be formatted according to
// the user locale/timezone similar to fmt:displaydate.
- // We should leverage Y.Intl features.
+ // We should leverage Y.Intl features.
var fmt_date = 'on ' + Y.Date.format(
new Date(comment.date), {format: '%Y-%m-%d'});
headerspan.one('span').set('text', fmt_date);
@@ -129,7 +129,7 @@
headerrow.one('td').appendChild(headerspan);
// We want to have the comments in order after the line.
- var tr = Y.one('#diff-line-' + (parseInt(comment.line_number) + 1));
+ var tr = Y.one('#diff-line-' + (parseInt(comment.line_number, 10) + 1));
if (tr !== null) {
tr.insert(headerrow, 'before');
} else {
@@ -158,7 +158,7 @@
comments.forEach( function (comment) {
namespace.create_row(comment);
});
- },
+ }
},
parameters: {
previewdiff_id: namespace.current_previewdiff_id
@@ -170,7 +170,7 @@
var config_draft = {
on: {
success: function (drafts) {
- namespace.inlinecomments = {};
+ namespace.inlinecomments = {};
if (drafts === null) {
return;
}
@@ -179,11 +179,11 @@
Object.keys(drafts).forEach( function (line_number) {
var comment = {
'line_number': line_number,
- 'text': drafts[line_number],
+ 'text': drafts[line_number]
};
namespace.create_row(comment);
});
- },
+ }
},
parameters: {
previewdiff_id: namespace.current_previewdiff_id
@@ -227,6 +227,25 @@
lp_client: {
value: null
},
+
+ previewdiff_id: {
+ value: null
+ },
+
+ navigator: {
+ getter: function() {return this.get('srcNode').one('select');},
+ setter: function(navigator) {
+ var container = this.get('srcNode').one('.diff-navigator');
+ container.append(navigator);
+ var self = this;
+ navigator.on('change', function() {
+ self._showPreviewDiff(this.get('value'), false);
+ });
+ this._connectScrollers();
+ this._showPreviewDiff(navigator.get('value'), true);
+ }
+ }
+
}
});
@@ -234,7 +253,7 @@
Y.extend(DiffNav, Y.Widget, {
- /*
+ /**
* The initializer method that is called from the base Plugin class.
*
* @method initializer
@@ -249,47 +268,143 @@
}
},
- _connectNavigator: function(navigator) {
- var self = this;
+ /**
+ * Add the spinner image to the diff section title.
+ *
+ * @method set_status_updating
+ */
+ set_status_updating: function() {
+ var image = Y.Node.create('<img />')
+ .set('src', '/@@/spinner')
+ .set('title', 'Updating diff ...');
+ this.get('srcNode').one('h2').append(image);
+ },
+
+ /**
+ * Run finishing tasks after the diff content is updated.
+ *
+ * @method updated
+ */
+ set_status_updated: function() {
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 preview_diff_uri = (
- LP.cache.context.web_link +
- '/+preview-diff/' + previewdiff_id + '/+diff');
- var container = self.get('srcNode').one('.diff-content');
- var config = {
- on: {
- success: function (diff_html) {
- container.set('innerHTML', diff_html);
- (new rc.NumberToggle()).render();
- namespace.setup_inline_comments(previewdiff_id);
- },
- failure: function(ignore, response, args) {
- Y.log('DiffNav: ' + preview_diff_uri +
- ' - ' + response.statusText);
- var error_note = Y.Node.create('<p />')
- .addClass('exception')
- .addClass('lowlight')
- .set('text', 'Failed to fetch diff content.');
- container.empty();
- container.append(error_note);
- },
- },
- };
- self.get('lp_client').get(preview_diff_uri, config);
+ if (this.get('previewdiff_id')) {
+ namespace.setup_inline_comments(this.get('previewdiff_id'));
+ }
+ },
+
+ /**
+ * Remove the spinner image from the diff section title.
+ *
+ * @method cleanup_status
+ */
+ cleanup_status: function() {
+ this.get('srcNode').all('h2 img').remove();
+ },
+
+ /**
+ * Update diff status on new review comments
+ *
+ * @method update_on_new_comment
+ */
+ update_on_new_comment: function() {
+ if (LP.cache.inline_diff_comments !== true) {
+ return;
+ }
+ namespace.populate_existing_comments();
+ this._connectScrollers();
+ },
+
+ /**
+ * Helper method to connect all inline-comment-scroller links to the
+ * to the diff navigator.
+ *
+ * @method _connectScrollers
+ */
+ _connectScrollers: function() {
+ var self = this;
+ var rc = Y.lp.code.branchmergeproposal.reviewcomment;
+ Y.all('.inline-comment-scroller').each( function(node) {
+ rc.link_scroller(node, '#review-diff', function() {
+ self._showPreviewDiff(
+ node.getAttribute('data-previewdiff-id'), false);
+ });
});
},
+ /**
+ * Helper method to update diff-content area according to given
+ * diff content uri
+ *
+ * @method _updateDiff
+ */
+ _updateDiff: function(preview_diff_uri) {
+ var self = this;
+ var container = this.get('srcNode').one('.diff-content');
+ var config = {
+ on: {
+ success: function (diff_html) {
+ container.set('innerHTML', diff_html);
+ self.set_status_updated();
+ },
+ failure: function(ignore, response, args) {
+ Y.log('DiffNav: ' + preview_diff_uri +
+ ' - ' + response.statusText);
+ var error_note = Y.Node.create('<p />')
+ .addClass('exception')
+ .addClass('lowlight')
+ .set('text', 'Failed to fetch diff content.');
+ container.empty();
+ container.append(error_note);
+ },
+ start: function() {
+ self.set_status_updating();
+ },
+ end: function() {
+ self.cleanup_status();
+ }
+ }
+ };
+ this.get('lp_client').get(preview_diff_uri, config);
+ },
+
+ /**
+ * Helper method to show the previewdiff for the given id.
+ * Do nothing if the current content is already displayed.
+ *
+ * @method _showPreviewDiff
+ */
+ _showPreviewDiff: function(previewdiff_id, force) {
+ var navigator = this.get('navigator');
+ if (!Y.Lang.isValue(navigator)) {
+ // DiffNav was not properly initialised.
+ return;
+ }
+ if (this.get('previewdiff_id') === previewdiff_id) {
+ // The requested diff is already presented.
+ return;
+ }
+ navigator.set('value', previewdiff_id);
+ this.set('previewdiff_id', previewdiff_id);
+ var preview_diff_uri = (
+ LP.cache.context.web_link +
+ '/+preview-diff/' + previewdiff_id + '/+diff');
+ this._updateDiff(preview_diff_uri);
+ },
+
+ /**
+ * Render diff navigator contents (navigator and diff area).
+ *
+ * @method renderUI
+ */
renderUI: function() {
+ var self = this;
if (LP.cache.inline_diff_comments !== true) {
- return
+ var preview_diff_uri = (LP.cache.context.web_link + '/++diff');
+ self._updateDiff(preview_diff_uri);
+ return;
}
- var self = this;
- var container = self.get('srcNode').one('.diff-navigator');
+ var container = this.get('srcNode').one('.diff-navigator');
container.empty();
var preview_diffs_uri = LP.cache.context.preview_diffs_collection_link;
var config = {
@@ -301,7 +416,7 @@
// XXX cprov 20140226: the date should be formatted
// according to the user locale/timezone similar to
// fmt:displaydate. We should leverage Y.Intl
- // features.
+ // features.
var fmt_date = 'on ' + Y.Date.format(
new Date(pd.get('date_created')),
{format: '%Y-%m-%d'});
@@ -309,29 +424,33 @@
var option = Y.Node.create('<option />')
.set('value', pd.get('id'))
.set('text', text);
- if (LP.cache.context.preview_diff_link ==
+ if (LP.cache.context.preview_diff_link ===
pd.get('self_link')) {
option.set('selected', 'selected');
}
navigator.append(option);
});
- container.append(navigator)
- self._connectNavigator(navigator);
+ self.set('navigator', navigator);
},
failure: function(ignore, response, args) {
Y.log('DiffNav: ' + preview_diffs_uri +
- ' - ' + response.statusText)
+ ' - ' + response.statusText);
var error_note = Y.Node.create('<p />')
.addClass('exception')
.addClass('lowlight')
.set('text', 'Failed to fetch available diffs.');
- container.empty();
container.append(error_note);
},
+ start: function() {
+ self.set_status_updating();
+ },
+ end: function() {
+ self.cleanup_status();
+ }
}
};
this.get('lp_client').get(preview_diffs_uri, config);
- },
+ }
});
=== modified file 'lib/lp/code/javascript/branchmergeproposal.reviewcomment.js'
--- lib/lp/code/javascript/branchmergeproposal.reviewcomment.js 2014-01-14 20:11:37 +0000
+++ lib/lp/code/javascript/branchmergeproposal.reviewcomment.js 2014-04-04 05:33:32 +0000
@@ -19,6 +19,9 @@
duration: 1,
easing: Y.Easing.easeOut
});
+// XXX cprov 20140402: only exposed so tests can fire its 'end' event
+// for testing hooked actions.
+namespace.window_scroll_anim = window_scroll_anim;
/*
* Connect all the links to their given actions.
=== modified file 'lib/lp/code/javascript/branchmergeproposal.updater.js'
--- lib/lp/code/javascript/branchmergeproposal.updater.js 2014-02-27 19:26:31 +0000
+++ lib/lp/code/javascript/branchmergeproposal.updater.js 2014-04-04 05:33:32 +0000
@@ -77,28 +77,6 @@
'summary_node').one('#summary-row-b-diff').one('td');
container.set('innerHTML', value);
}
- },
-
- /**
- * The HTML code for the diff.
- *
- * @attribute diff
- */
- diff: {
- getter: function() {
- if (this.get('pending')) {
- return '';
- }
- else {
- return this.get(
- 'srcNode').one('.diff-content').get('innerHTML');
- }
- },
- setter: function(value) {
- this._setup_diff_container();
- this.get(
- 'srcNode').one('.diff-content').set('innerHTML', value);
- }
}
}
@@ -214,7 +192,7 @@
.append(Y.Node.create('<h2 />')
.set("text", "Preview Diff "))
.append(Y.Node.create('<div />')
- .addClass("diff-content"))
+ .addClass("diff-navigator"))
.append(Y.Node.create('<div />')
.addClass("diff-content"));
this.get('srcNode').append(review_diff);
@@ -222,14 +200,12 @@
},
/*
- * Update the page with the last version of the diff and update the
- * stats.
+ * Update the page diff stats.
*
* @method update
*/
update: function() {
this.update_stats();
- this.update_diff();
},
/*
@@ -248,35 +224,10 @@
'#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: {
- success: function(diff) {
- self.set('diff', diff);
- var node = self.get('srcNode').one('.diff-content');
- Y.lp.anim.green_flash({node: node}).run();
self.fire(self.NAME + '.updated');
},
failure: function() {
- var node = self.get('srcNode').one('.diff-content');
+ var node = self.get('summary_node');
Y.lp.anim.red_flash({node: node}).run();
},
start: function() {
@@ -288,7 +239,7 @@
}
};
var mp_uri = LP.cache.context.web_link;
- this.get('lp_client').get(mp_uri + "/++diff", config);
+ this.get('lp_client').get(mp_uri + "/++diff-stats", config);
}
});
=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html 2014-02-27 19:26:31 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html 2014-04-04 05:33:32 +0000
@@ -72,6 +72,12 @@
<li>lp.code.branchmergeproposal.inlinecomments.test</li>
</ul>
+ <a id="scroll-one" class="inline-comment-scroller" href=""
+ data-previewdiff-id="101">Go to One</a>
+
+ <a id="scroll-two" class="inline-comment-scroller" href=""
+ data-previewdiff-id="202">Go to Two</a>
+
<div id="review-diff">
<h2>Preview Diff</h2>
=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-02-28 03:32:53 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-04-04 05:33:32 +0000
@@ -8,20 +8,16 @@
tests.suite = new Y.Test.Suite('branchmergeproposal.inlinecomments Tests');
tests.suite.add(new Y.Test.Case({
- name: 'code.branchmergeproposal.inlinecomments_tests',
+ name: 'code.branchmergeproposal.inlinecomments_comments_tests',
setUp: function () {
// Loads testing values into LP.cache.
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";,
- preview_diff_link: "https://code.launchpad.dev/api/devel/~foo/bar/foobr/+merge/1/preview_diff";,
- preview_diffs_collection_link: "https://code.launchpad.dev/api/devel/~foo/bar/foobr/+merge/1/preview_diffs";
+ 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 () {},
@@ -47,6 +43,7 @@
{io_provider: mockio});
// Populate the page.
+ module.current_previewdiff_id = 1;
module.populate_existing_comments();
// LP was hit twice for fetching published and draft inline
@@ -74,7 +71,7 @@
{'line_number': '2',
'person': person_obj,
'text': 'This is preloaded.',
- 'date': '2012-08-12T10:00:00.00001+00:00'},
+ 'date': '2012-08-12T10:00:00.00001+00:00'}
];
mockio.success({
responseText: Y.JSON.stringify(published_comments),
@@ -92,9 +89,9 @@
Y.Assert.areEqual('This is preloaded.', text.get('text'));
// Draft comment is displayed.
- var header = Y.one('#diff-line-3').next();
+ header = Y.one('#diff-line-3').next();
Y.Assert.areEqual('Draft comment.', header.get('text'));
- var text = header.next();
+ text = header.next();
Y.Assert.areEqual('Zoing!', text.get('text'));
},
@@ -126,7 +123,7 @@
// Cancelling a draft comment attempt ...
line.simulate('dblclick');
- var ic_area = line.next().next();
+ ic_area = line.next().next();
ic_area.one('.yui3-ieditor-input>textarea').set('value', 'No!');
ic_area.one('.lazr-neg').simulate('click');
@@ -137,7 +134,7 @@
// Removing a draft comment by submitting an emtpy text.
line.simulate('dblclick');
- var ic_area = line.next().next();
+ ic_area = line.next().next();
ic_area.one('.yui3-ieditor-input>textarea').set('value', '');
ic_area.one('.lazr-pos').simulate('click');
@@ -154,10 +151,10 @@
called = true;
};
module.add_doubleclick_handler = add_doubleclick_handler;
- module.current_previewdiff_id = null
+ module.current_previewdiff_id = null;
LP.cache.inline_diff_comments = false;
- module.current_previewdiff_id = null
+ module.current_previewdiff_id = null;
module.setup_inline_comments(1);
Y.Assert.isFalse(called);
@@ -170,56 +167,53 @@
called = true;
};
module.add_doubleclick_handler = add_doubleclick_handler;
- module.current_previewdiff_id = null
+ module.current_previewdiff_id = null;
module.setup_inline_comments(1);
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();
+ }
+
+ }));
+
+ tests.suite.add(new Y.Test.Case({
+ name: 'code.branchmergeproposal.inlinecomments_diffnav_tests',
+
+ setUp: function () {
+ // Loads testing values into LP.cache.
+ 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"),
+ preview_diff_link: ("https://code.launchpad.dev/api/devel/"; +
+ "~foo/bar/foobr/+merge/1/preview_diff"),
+ preview_diffs_collection_link: (
+ "https://code.launchpad.dev/api/devel/"; +
+ "~foo/bar/foobr/+merge/1/preview_diffs")
+ };
+
+ LP.cache.inline_diff_comments = true;
+ // Disable/Instrument inlinecomments hook.
+ var self = this;
+ self.inline_comments_requested_id = null;
+ module.setup_inline_comments = function(previewdiff_id) {
+ self.inline_comments_requested_id = previewdiff_id;
+ };
+ // Create an instrument DiffNav instance.
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(
+ {io_provider: new Y.lp.testing.mockio.MockIo()});
+ this.diffnav = new module.DiffNav(
{srcNode: Y.one('#review-diff'), lp_client: lp_client}
- )).render();
- // diff-navigator section content is rendered based on the
- // preview_diffs API object.
- Y.Assert.areEqual(1, mockio.requests.length);
- Y.Assert.areSame(
- "/api/devel/~foo/bar/foobr/+merge/1/preview_diffs",
- mockio.last_request.url);
+ );
+
+ },
+
+ tearDown: function () {},
+
+ reply_previewdiffs: function() {
+ // test helper to reply 'available_diffs' collection via mockio.
+ var mockio = this.diffnav.get('lp_client').io_provider;
var preview_diffs = {
total_size: 2,
start: 0,
@@ -231,13 +225,70 @@
date_created: '2014-02-20T18:07:38.678947+00:00',
self_link: LP.cache.context.preview_diff_link}
]
- }
+ };
mockio.success({
responseText: Y.JSON.stringify(preview_diffs),
responseHeaders: {'Content-Type': 'application/json'}});
+ },
+
+ reply_diffcontent: function() {
+ // test helper to reply 'diff' content via mockio.
+ // it reuses the existing test template contents, but
+ // empties the page before replying it.
+ Y.one('.diff-content div div ul.horizontal').empty();
+ var diff_content = Y.one('.diff-content').get('innerHTML');
+ var mockio = this.diffnav.get('lp_client').io_provider;
+ Y.one('.diff-content').empty();
+ mockio.success({
+ responseText: diff_content,
+ responseHeaders: {'Content-Type': 'text/html'}});
+ },
+
+ test_diff_nav_feature_flag_disabled: function() {
+ // When rendered with the corresponding feature-flag disabled,
+ // Diff Navigator only updates the diff content with the latest
+ // previewdiff and does not create the navigator (<select>) nor
+ // fetches the inline comments.
+ LP.cache.inline_diff_comments = false;
+ this.diffnav.render();
+ mockio = this.diffnav.get('lp_client').io_provider;
+ Y.Assert.areEqual(1, mockio.requests.length);
+ Y.Assert.areSame(
+ "/~foo/bar/foobr/+merge/1/++diff",
+ mockio.last_request.url);
+ this.reply_diffcontent();
+ Y.Assert.isNull(this.diffnav.get('previewdiff_id'));
+ },
+
+ test_diff_nav_rendering: function() {
+ // When rendered the Diff Navigator fetches the available diffs
+ // collection and builds the selector, fetches and displays
+ // the diff contents for the most recent previewdiff and also
+ // fetches and displays the related inline comments.
+ this.diffnav.render();
+ // diff-navigator section content is rendered based on the
+ // preview_diffs API object.
+ var mockio = this.diffnav.get('lp_client').io_provider;
+ Y.Assert.areEqual(1, mockio.requests.length);
+ Y.Assert.areSame(
+ "/api/devel/~foo/bar/foobr/+merge/1/preview_diffs",
+ mockio.last_request.url);
+ this.reply_previewdiffs();
+ // The selected preview_diff content is retrieved.
+ Y.Assert.areEqual(2, mockio.requests.length);
+ Y.Assert.areSame(
+ "/~foo/bar/foobr/+merge/1/+preview-diff/101/+diff",
+ mockio.last_request.url);
+ this.reply_diffcontent();
// The local (fake) setup_inline_comments function
// was called with the selected diff_id value.
- Y.Assert.areEqual(101, called_diff_id);
+ Y.Assert.areEqual(101, this.inline_comments_requested_id);
+ // NumberToggle widget is rendered
+ Y.Assert.isNotNull(Y.one('.diff-content').one('#show-no'));
+ // Diff content is rendered/updated.
+ Y.Assert.areSame(
+ "foo bar",
+ Y.one('.diff-content table tbody tr td').next().get('text'));
// The option corresponding to the current 'preview_diff'
// is selected and contains the expected text (title and
// formatted date_created).
@@ -248,46 +299,107 @@
//Y.Assert.areEqual(
// 'r1 into r1 on 2014-02-20',
// Y.one('select').get('options').item(1).get('text'));
- // Simulate a change in the diff navigator.
+ },
+
+ test_diff_nav_changes: function() {
+ // Changes on the DiffNav selector result in diff content
+ // and inline comments updates.
+ this.diffnav.render();
+ this.reply_previewdiffs();
+ this.reply_diffcontent();
+ var mockio = this.diffnav.get('lp_client').io_provider;
+ Y.Assert.areEqual(2, mockio.requests.length);
+
Y.one('select').set('value', 202);
Y.one('select').simulate('change');
- // diff content was updated with the selected diff_id content.
+ Y.Assert.areEqual(3, mockio.requests.length);
+ Y.Assert.areSame(
+ "/~foo/bar/foobr/+merge/1/+preview-diff/202/+diff",
+ mockio.last_request.url);
+ this.reply_diffcontent();
+ Y.Assert.areEqual(202, this.diffnav.get('previewdiff_id'));
+ Y.Assert.areEqual(202, this.inline_comments_requested_id);
+ },
+
+ test_diff_nav_scrollers: function() {
+ // The Diff Navigator review comment *scrollers* are connected
+ // upon widget rendering and when clicked trigger the diff
+ // content update.
+ this.diffnav.render();
+ this.reply_previewdiffs();
+ this.reply_diffcontent();
+ var mockio = this.diffnav.get('lp_client').io_provider;
Y.Assert.areEqual(2, mockio.requests.length);
+ // We need to re-instrument the scroller in other to
+ // instantly fire the 'end' event (which runs the code
+ // that matters to us).
+ scroller = Y.one('#scroll-two');
+ scroller.on('click', function() {
+ var rc = Y.lp.code.branchmergeproposal.reviewcomment;
+ rc.window_scroll_anim.fire('end');
+ });
+ // Click the scroller results in a diff update.
+ scroller.simulate('click');
+ Y.Assert.areEqual(3, 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);
+ this.reply_diffcontent();
+ Y.Assert.areEqual(202, this.diffnav.get('previewdiff_id'));
+ Y.Assert.areEqual(202, this.inline_comments_requested_id);
+ // If the scroller target diff is already displayed, the diff
+ // content is not refreshed.
+ scroller.simulate('click');
+ Y.Assert.areEqual(3, mockio.requests.length);
+ },
+
+ test_update_on_new_comment: function() {
+ // When a new comment is added to the page, it triggers
+ // a Diff Navigator action to update the inline comments and
+ // hook new inline-comment scroller links.
+ this.diffnav.set('navigator', Y.one('select'));
+ // Let's create add a new scroller to the page.
+ var new_scroller = Y.Node.create(
+ '<a id="scroll-three" class="inline-comment-scroller" ' +
+ 'href="" data-previewdiff-id="202">Another scroller</a>');
+ Y.one('#scroll-two').insert(new_scroller, 'after');
+ // and instrument the inline-comment updated function.
+ var populated = false;
+ module.populate_existing_comments = function () {
+ populated = true;
+ };
+ // Once called, update_on_new_comment() integrates the
+ // just-added scroller and updates the inline comments.
+ this.diffnav.update_on_new_comment();
+ Y.Assert.isTrue(new_scroller.hasClass('js-action'));
+ Y.Assert.isTrue(populated);
+ // Instrument the new scroller, so it will instantly fire
+ // the diff navigator hook and register the requested diff.
+ new_scroller.on('click', function() {
+ var rc = Y.lp.code.branchmergeproposal.reviewcomment;
+ rc.window_scroll_anim.fire('end');
+ });
+ var called_pd_id = null;
+ var called_force = null;
+ this.diffnav._showPreviewDiff = function (pd_id, force) {
+ called_pd_id = pd_id;
+ called_force = force;
+ };
+ // the new scroller works, it has requested and diff update
+ // to the encoded data-previewdiff-id.
+ new_scroller.simulate('click');
+ Y.Assert.areEqual(202, called_pd_id);
+ Y.Assert.isFalse(called_force);
},
test_diff_nav_failed_diff_content: function() {
- var mockio = new Y.lp.testing.mockio.MockIo();
- lp_client = new Y.lp.client.Launchpad(
- {io_provider: mockio});
- module.setup_inline_comments = function(diff_id) {};
- (new module.DiffNav(
- {srcNode: Y.one('#review-diff'), lp_client: lp_client}
- )).render();
- var preview_diffs = {
- total_size: 1,
- start: 0,
- entries: [
- {title: 'boing', id: 1,
- date_created: '2014-02-22T10:00:00.00001+00:00',
- self_link: 'something-else'}
- ]
- }
- mockio.success({
- responseText: Y.JSON.stringify(preview_diffs),
- responseHeaders: {'Content-Type': 'application/json'}});
- // Simulate a change in the diff navigator that will fail
- // to fetch the diff contents.
+ // An error message is presented when the Diff Navigator
+ // fails to fetch the selected diff contents.
+ this.diffnav.render();
+ this.reply_previewdiffs();
+ Y.one('select').set('value', 2);
Y.one('select').simulate('change');
+ var mockio = this.diffnav.get('lp_client').io_provider;
mockio.failure();
Y.Assert.areEqual(
'Failed to fetch diff content.',
@@ -295,17 +407,27 @@
},
test_diff_nav_failed_available_diffs: function() {
- var mockio = new Y.lp.testing.mockio.MockIo();
- lp_client = new Y.lp.client.Launchpad(
- {io_provider: mockio});
- // Simulate a failure when fetching available diffs.
- (new module.DiffNav(
- {srcNode: Y.one('#review-diff'), lp_client: lp_client}
- )).render();
+ // An error message is presented with the Diff Navigator
+ // fails to fetch the collection of available diffs.
+ this.diffnav.render();
+ var mockio = this.diffnav.get('lp_client').io_provider;
mockio.failure();
Y.Assert.areEqual(
'Failed to fetch available diffs.',
Y.one('.diff-navigator').get('text'));
+ },
+
+ test_status_indicators: function() {
+ // Indicating progress with the spinner image.
+ this.diffnav.set_status_updating();
+ Y.Assert.areEqual(
+ '/@@/spinner',
+ Y.one('h2').one('img').getAttribute('src'));
+ // Remove the spinner when the work is done.
+ this.diffnav.cleanup_status();
+ Y.Assert.areEqual(
+ 'Preview Diff',
+ Y.one('h2').get('innerHTML'));
}
}));
@@ -313,6 +435,6 @@
}, '0.1', {
requires: ['node-event-simulate', 'test', 'lp.testing.helpers',
'console', 'lp.client', 'lp.testing.mockio', 'widget',
- 'lp.code.branchmergeproposal.inlinecomments',
+ 'lp.code.branchmergeproposal.inlinecomments', 'lp.anim',
'lp.code.branchmergeproposal.reviewcomment']
});
=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.updater.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.updater.js 2012-10-26 10:00:20 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.updater.js 2014-04-04 05:33:32 +0000
@@ -43,9 +43,7 @@
test_default_values: function() {
Y.Assert.isTrue(this.updater.get('pending'));
- Y.Assert.areEqual(
- '',
- this.updater.get('diff'));
+ Y.Assert.isNull(this.updater.get('diff_stats'));
},
test__setup_diff_container: function() {
@@ -104,38 +102,6 @@
Y.one('h2').get('innerHTML'));
},
- test_get_diff: function() {
- this.updater._setup_diff_container();
- Y.one('.diff-content').set(
- 'innerHTML', 'this is a <span>diff</span>');
- Y.Assert.areEqual(
- 'this is a <span>diff</span>',
- this.updater.get('diff'));
- },
-
- test_set_diff: function() {
- this.updater.set('diff', 'this is a <span>diff</span>');
- Y.Assert.areEqual(
- 'this is a <span>diff</span>',
- Y.one('.diff-content').get('innerHTML'));
- },
-
- 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_diff();
- mockio.success({
- responseText: 'New <span>diff</span>',
- responseHeaders: {'Content-Type': 'text/html'}});
-
- Y.Assert.areEqual(
- 'New <span>diff</span>',
- this.updater.get('diff'));
- },
-
test_update_stats_success: function() {
var mockio = new Y.lp.testing.mockio.MockIo();
this.updater.get('lp_client').io_provider = mockio;
@@ -150,61 +116,22 @@
this.updater.get('diff_stats'));
},
- test_update_diff_fires_event: function() {
+ test_update_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_diff();
+ this.updater.update();
mockio.success({
- responseText: 'New <span>diff</span>',
+ responseText: '13 lines (+4/-0) 1 file modified',
responseHeaders: {'Content-Type': 'text/html'}});
-
Y.Assert.isTrue(fired);
}
}));
-/*
- * Tests for when the updater is built on top of an existing diff.
- *
- */
-var current_mp = Y.one('#current-mp').getContent();
-
-tests.suite.add(new Y.Test.Case({
-
- name: 'branchmergeproposal-updater-refresh-tests',
-
- setUp: function() {
- Y.one("#placeholder")
- .empty()
- .append(Y.Node.create(current_mp));
- var diff_area = Y.one('#diff-area');
- this.updater = new UpdaterWidget({srcNode: diff_area});
- },
-
- tearDown: function() {
- this.updater.destroy();
- },
-
- test_default_values: function() {
- Y.Assert.isFalse(this.updater.get('pending'));
- Y.Assert.areEqual(
- 'Example diff',
- this.updater.get('diff'));
- },
-
- test_get_diff: function() {
- Y.one('.diff-content').set(
- 'innerHTML', 'this is a <span>diff</span>');
- Y.Assert.areEqual(
- 'this is a <span>diff</span>',
- this.updater.get('diff'));
- }
-
-}));
tests.suite.add(new Y.Test.Case({
=== modified file 'lib/lp/code/model/codereviewinlinecomment.py'
--- lib/lp/code/model/codereviewinlinecomment.py 2014-03-23 18:20:31 +0000
+++ lib/lp/code/model/codereviewinlinecomment.py 2014-04-04 05:33:32 +0000
@@ -10,6 +10,7 @@
'CodeReviewInlineCommentSet',
]
+from storm.expr import LeftJoin
from storm.locals import (
Int,
Reference,
@@ -128,4 +129,17 @@
return IStore(CodeReviewInlineComment).find(
CodeReviewInlineComment,
CodeReviewInlineComment.comment_id == comment.id).one()
-
+
+ def getPreviewDiffsForComments(self, comments):
+ """See `ICodeReviewInlineCommentSet`."""
+ origin = [
+ CodeReviewComment,
+ LeftJoin(CodeReviewInlineComment,
+ CodeReviewComment.id ==
+ CodeReviewInlineComment.comment_id),
+ ]
+ relations = IStore(CodeReviewInlineComment).using(*origin).find(
+ (CodeReviewComment.id,
+ CodeReviewInlineComment.previewdiff_id),
+ CodeReviewComment.id.is_in(c.id for c in comments))
+ return dict(relations)
=== modified file 'lib/lp/code/model/tests/test_codereviewinlinecomment.py'
--- lib/lp/code/model/tests/test_codereviewinlinecomment.py 2014-03-23 18:20:31 +0000
+++ lib/lp/code/model/tests/test_codereviewinlinecomment.py 2014-04-04 05:33:32 +0000
@@ -55,7 +55,7 @@
previewdiff, person, {'2': 'foobar'})
drafts = getUtility(ICodeReviewInlineCommentSet).getDraft(
previewdiff, person)
- self.assertEqual({'2':'foobar'}, drafts)
+ self.assertEqual({'2': 'foobar'}, drafts)
def test_ensure_deletes(self):
# ICodeReviewInlineCommentSet.ensureDraft() will delete a draft if
@@ -86,7 +86,7 @@
previewdiff, person, {'1': 'bar'})
drafts = getUtility(ICodeReviewInlineCommentSet).getDraft(
previewdiff, person)
- self.assertEqual({'1':'bar'}, drafts)
+ self.assertEqual({'1': 'bar'}, drafts)
def test_publishDraft(self):
# ICodeReviewInlineCommentSet.publishDraft() will publish draft
@@ -117,7 +117,7 @@
previewdiff, person = self.makeCodeReviewInlineCommentDraft()
drafts = getUtility(ICodeReviewInlineCommentSet).getDraft(
previewdiff, person)
- self.assertEqual({'2':'foobar'}, drafts)
+ self.assertEqual({'2': 'foobar'}, drafts)
def test_get_published_sorted(self):
# ICodeReviewInlineCommentSet.findByPreviewDiff() will return a sorted
@@ -149,7 +149,7 @@
previewdiff=previewdiff, person=person, comment=comment_one,
comments={'1': 'one'})
comment_two = self.factory.makeCodeReviewComment()
- inline_two =self.makeCodeReviewInlineComment(
+ inline_two = self.makeCodeReviewInlineComment(
previewdiff=previewdiff, person=person, comment=comment_two,
comments={'2': 'two'})
cric_set = getUtility(ICodeReviewInlineCommentSet)
@@ -164,3 +164,27 @@
# Lookups for comments with no inline comments return an empty list.
comment_empty = self.factory.makeCodeReviewComment()
self.assertIsNone(cric_set.getByReviewComment(comment_empty))
+
+ def test_get_previewdiff_for_comments(self):
+ # For facilitating view cache population, all `PreviewDiffs`
+ # related with a set of `CodeReviewComment` (by having inline
+ # comments), can be retrieved as a dictionary from a single query.
+ expected_relations = {}
+ comments = []
+ person = self.factory.makePerson()
+ for i in range(5):
+ comment = self.factory.makeCodeReviewComment()
+ comments.append(comment)
+ inline_comment = self.makeCodeReviewInlineComment(
+ person=person, comment=comment)
+ expected_relations[comment.id] = inline_comment.previewdiff_id
+ # `CodeReviewComment` without inline comments have no corresponding
+ # `Previewdiff`.
+ comment = self.factory.makeCodeReviewComment()
+ comments.append(comment)
+ expected_relations[comment.id] = None
+ # The constructed relations match the ones returned by
+ # getPreviewDiffsForComments().
+ cric_set = getUtility(ICodeReviewInlineCommentSet)
+ found_relations = cric_set.getPreviewDiffsForComments(comments)
+ self.assertEqual(expected_relations, found_relations)
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt 2014-02-27 17:45:09 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt 2014-04-04 05:33:32 +0000
@@ -207,37 +207,35 @@
Y.on('load', function() {
var logged_in = LP.links['me'] !== undefined;
var ic = Y.lp.code.branchmergeproposal.inlinecomments;
+ var diffnav = new ic.DiffNav({srcNode: Y.one('#review-diff')});
if (logged_in) {
var comment = new Y.lp.app.comment.CodeReviewComment();
comment.render();
- comment._add_comment_success = function () {
- 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();
- }
- }
+ comment.on('CodeReviewComment.appended', function () {
+ diffnav.update_on_new_comment();
+ });
Y.lp.code.branchmergeproposal.status.connect_status(conf);
}
Y.lp.code.branchmergeproposal.reviewcomment.connect_links();
- (new ic.DiffNav({srcNode: Y.one('#review-diff')})).render();
+ diffnav.render();
if (Y.Lang.isValue(LP.cache.merge_proposal_event_key)) {
- var updater = new Y.lp.code.branchmergeproposal.updater.UpdaterWidget(
- {
+ var upt = Y.lp.code.branchmergeproposal.updater;
+ var cfg = {
srcNode: Y.one('#diff-area'),
summary_node: Y.one('#proposal-summary')
- });
+ };
+ var updater = new upt.UpdaterWidget(cfg);
Y.on(LP.cache.merge_proposal_event_key, function(data) {
- if (Y.lp.code.branchmergeproposal.updater.is_mp_diff_updated(data)) {
+ if (upt.is_mp_diff_updated(data)) {
updater.update();
}
});
updater.on(updater.NAME + '.updated', function() {
- (new ic.DiffNav({srcNode: Y.one('#review-diff')})).render();
+ diffnav.render();
});
}
- }, window);
+ }, window);
Y.on('domready', function() {
Y.lp.app.widgets.expander.createByCSS(
=== modified file 'lib/lp/code/templates/codereviewcomment-header.pt'
--- lib/lp/code/templates/codereviewcomment-header.pt 2011-12-22 09:05:46 +0000
+++ lib/lp/code/templates/codereviewcomment-header.pt 2014-04-04 05:33:32 +0000
@@ -26,6 +26,11 @@
tal:attributes="href context/branch_merge_proposal/fmt:url">a
previous version</a>
of this proposal</span>
+ <span style="float: right;" tal:condition="context/previewdiff_id">
+ <a class="inline-comment-scroller" href=""
+ tal:attributes="data-previewdiff-id context/previewdiff_id"
+ >View inline comments</a>
+ </span>
</td>
<td class="bug-comment-index">
Follow ups
