launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #16743
Re: [Merge] lp:~cprov/launchpad/ic-superseded-mps into lp:launchpad
Most issues addressed.
I've proposed to postpone the event-bubbling experiment (since the change would be in the link_scroller, not IC) and the pagetest-rewrite.
Inline comments:
> --- lib/lp/app/javascript/comment.js 2014-04-04 05:23:48 +0000
> +++ lib/lp/app/javascript/comment.js 2014-05-14 20:06:59 +0000
> @@ -308,8 +308,6 @@
> initializer: function() {
> this.vote_input = Y.one('[id="field.vote"]');
> this.review_type = Y.one('[id="field.review_type"]');
> - this.publish_inline_comments = Y.one(
> - '[id="field.publish_inline_comments"]');
> this.in_reply_to = null;
> },
> /**
> @@ -334,6 +332,20 @@
> return selected.get('innerHTML');
> },
> /**
> + * Whether or not the review comment has inline comments to be published
> + *
> + * @method has_inline
> + */
> + has_inline: function() {
> + var inline_cb = Y.one('[id="field.publish_inline_comments"]');
> + if (inline_cb !== null) {
> + if (inline_cb.get('checked')) {
> + return true;
> + }
> + }
> + return false;
> + },
RIght, I had the impression the evaluation shortcut was not available in JS, but it is. Thanks for point it.
> + /**
> * Ensure that the widget's values are suitable for submission.
> *
> * This allows the vote to be submitted, even when no text is specified
> @@ -345,6 +357,9 @@
> if (this.get_vote() !== null) {
> return true;
> }
> + if (this.has_inline()) {
> + return true
> + }
> return namespace.Comment.prototype.validate.apply(this);
> },
> /**
> @@ -357,9 +372,6 @@
> namespace.Comment.prototype.set_disabled.call(this, disabled);
> this.vote_input.set('disabled', disabled);
> this.review_type.set('disabled', disabled);
> - if (this.publish_inline_comments) {
> - this.publish_inline_comments.set('disabled', disabled);
> - }
> },
> /**
> * Post the comment to the Launchpad API
> @@ -383,8 +395,8 @@
> if (this.in_reply_to !== null) {
> config.parameters.parent = this.in_reply_to.get('self_link');
> }
> - if (this.publish_inline_comments &&
> - this.publish_inline_comments.get('checked')) {
> +
> + if (this.has_inline()) {
> var ic = Y.lp.code.branchmergeproposal.inlinecomments;
> config.parameters.inline_comments = ic.inlinecomments;
> config.parameters.previewdiff_id = ic.current_previewdiff_id;
> @@ -451,9 +463,6 @@
> */
> reset_contents: function() {
> this.review_type.set('value', '');
> - if (this.publish_inline_comments) {
> - this.publish_inline_comments.set('value', '');
> - }
> this.vote_input.set('selectedIndex', 0);
> this.in_reply_to = null;
> namespace.Comment.prototype.reset_contents.apply(this);
>
> === modified file 'lib/lp/code/browser/codereviewcomment.py'
> --- lib/lp/code/browser/codereviewcomment.py 2014-04-04 05:23:48 +0000
> +++ lib/lp/code/browser/codereviewcomment.py 2014-05-14 20:06:59 +0000
> @@ -292,18 +292,9 @@
> """Create the comment..."""
> vote = data.get('vote')
> review_type = data.get('review_type')
> - inline_comments = {}
> - previewdiff_id = None
> - if (getFeatureFlag('code.inline_diff_comments.enabled') and
> - data.get('publish_inline_comments')):
> - previewdiff_id = self.previewdiff.id
> - inline_comments = (
> - 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)
> + parent=self.reply_to, vote=vote, review_type=review_type)
>
> @property
> def next_url(self):
>
> === modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
> --- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-05-07 06:16:12 +0000
> +++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-05-14 20:06:59 +0000
> @@ -62,6 +62,11 @@
> content_row.remove(true);
> }
> var config = {
> + on: {
> + success: function () {
> + Y.fire('inlinecomment.UPDATED');
> + }
> + },
> parameters: {
> previewdiff_id: namespace.current_previewdiff_id,
> comments: namespace.inlinecomments
> @@ -161,8 +166,8 @@
> namespace.cleanup_comments = function() {
> // Cleanup previous inline review comments.
> Y.all('.ict-header').each( function(line) {
> - line.next().remove();
> - line.remove();
> + line.next().remove(true);
> + line.remove(true);
> });
> };
>
> @@ -176,6 +181,7 @@
> comments.forEach( function (comment) {
> namespace.create_row(comment);
> });
> + Y.fire('inlinecomment.UPDATED');
> }
> },
> parameters: {
> @@ -203,6 +209,7 @@
> };
> namespace.create_row(comment);
> });
> + Y.fire('inlinecomment.UPDATED');
> }
> },
> parameters: {
> @@ -237,6 +244,63 @@
> };
>
>
> +var PublishDrafts = function () {
> + PublishDrafts.superclass.constructor.apply(this, arguments);
> +};
> +
> +Y.mix(PublishDrafts, {
> +
> + NAME: 'publishdrafts',
> +
> + ATTRS: {
> + }
> +
> +});
> +
> +Y.extend(PublishDrafts, Y.Widget, {
> +
> + renderUI: function() {
> + if (Y.one('#publish_drafts_container')) {
> + return;
I could not tell the difference between 'srcNode' and 'contentBox' from the YUI3 docs. Since 'srcNode' seems to be the choice of other existing widget, I am using it.
Does it sound correct ?
> + }
> + var container = Y.Node.create('<div />')
> + .set('id', 'publish_drafts_container');
> + this.get('srcNode').insert(container, 'after');
> + },
> +
> + updateUI: function() {
> + var container = Y.one('#publish_drafts_container')
> + container.empty();
'srcNode', ditto
> +
> + var n_drafts = Y.all('.draft').size();
> + Y.log(n_drafts + ' drafts');
> + if (n_drafts == 0) {
> + return;
> + }
> + var label = Y.Node.create('<a />')
> + .set('text', 'Publish ' + n_drafts + ' inline comments');
> + var checkbox = Y.Node.create('<input />')
> + .set('id', 'field.publish_inline_comments')
> + .set('name', 'field.publish_inline_comments')
> + .set('type', 'checkbox')
> + .set('checked', true)
> + .addClass('checkboxType');
> + container.append(checkbox);
fixed.
> + container.append(label);
> + Y.one('[id="field.actions.add"]').set('disabled', false)
> + },
Right, now we have 'CodeReviewComment.SET_DISABLED', but unfortunately it could not use this.set_disabled() implementation, since it also disables the textarea, so the user cannot recover from it.
> +
> + bindUI: function() {
> + var self = this;
> + Y.detach('inlinecomment.UPDATED');
it's used now since the updateUI() method uses this.get('srcNode').
> + Y.on('inlinecomment.UPDATED', this.updateUI);
> + }
> +
> +});
> +
> +namespace.PublishDrafts = PublishDrafts;
> +
> +
> var InlineCommentToggle = function () {
> InlineCommentToggle.superclass.constructor.apply(this, arguments);
> };
> @@ -383,7 +447,7 @@
> * @method cleanup_status
> */
> cleanup_status: function() {
> - this.get('srcNode').all('h2 img').remove();
> + this.get('srcNode').all('h2 img').remove(true);
> },
>
> /**
> @@ -395,6 +459,7 @@
> if (LP.cache.inline_diff_comments !== true) {
> return;
> }
> + namespace.cleanup_comments();
> namespace.populate_comments();
> this._connectScrollers();
> },
> @@ -408,10 +473,29 @@
> _connectScrollers: function() {
> var self = this;
> var rc = Y.lp.code.branchmergeproposal.reviewcomment;
> - Y.all('.inline-comment-scroller').each( function(node) {
> + Y.all('td[data-previewdiff-id]').each( function(td) {
> + // Comments from superseded MPs should be ignored.
> + if (td.getAttribute('data-from-superseded') === 'True') {
> + return;
Cool! Thanks
> + }
> + var previewdiff_id = td.getAttribute('data-previewdiff-id');
> + var comment_id = td.getAttribute('data-comment-id');
> + var scroller_id = (
> + 'ic-scroller-' + previewdiff_id + '-' + comment_id);
> + // We have to remove the old scrollers otherwise they will
> + // fire multiple 'click' handlers (and animations).
> + var node = Y.one('#' + scroller_id)
Right, event bubbling would solve my problem if I was using the original click handler directly, but I am using it via the link_scroller() ...
I propose we carry on with they problem in this branch and attack it more aggressively on the JS-refactoring iteration (hopefully post release).
What do you think ?
> + if (node !== null) {
> + node.remove(true);
> + }
> + node = Y.Node.create('<a />')
> + .set('id', scroller_id)
> + .set('href', '')
> + .set('text', 'View inline comments')
> + .setStyle('float', 'right');
> + td.append(node);
> rc.link_scroller(node, '#review-diff', function() {
> - self._showPreviewDiff(
> - node.getAttribute('data-previewdiff-id'));
> + self._showPreviewDiff(previewdiff_id);
> });
> });
> },
> @@ -488,6 +572,8 @@
> self._updateDiff(preview_diff_uri);
> return;
> }
> + (new namespace.PublishDrafts(
> + {'srcNode': Y.one('[id="field.review_type"]')})).render();
> var container = this.get('srcNode').one('.diff-navigator');
> container.empty();
> var preview_diffs_uri = LP.cache.context.preview_diffs_collection_link;
>
> === modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html'
> --- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html 2014-04-03 20:33:08 +0000
> +++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html 2014-05-14 20:06:59 +0000
> @@ -72,11 +72,30 @@
> <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>
> + <div>
> + <input id="field.review_type" type="text"></input>
> + <input id="field.actions.add" type="submit" value="Save"></input>
> + </div>
>
> - <a id="scroll-two" class="inline-comment-scroller" href=""
> - data-previewdiff-id="202">Go to Two</a>
> + <table class="conversation">
> + <tr>
> + <td data-previewdiff-id="102" data-from-superseded="True"
> + data-comment-id="10">
> + Comment from superseded </td>
> + </tr><tr>
> + <td data-previewdiff-id="101" data-from-superseded="False"
> + data-comment-id="1">
> + Comment One </td>
> + </tr><tr>
> + <td data-previewdiff-id="202" data-from-superseded="False"
> + data-comment-id="2">
> + Comment Two </td>
> + </tr><tr>
> + <td data-from-superseded="False"
> + data-comment-id="3">
> + No inlines </td>
> + </tr>
> + </table>
>
> <div id="review-diff">
>
>
> === modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
> --- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-05-07 06:16:12 +0000
> +++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-05-14 20:06:59 +0000
> @@ -408,6 +408,26 @@
> 'table-row', Y.one('tr.ict-header').getStyle('display'));
> },
>
> + test_publish_drafts: function() {
> + // PublishDraft() widget modified the add-comment form
> + // (adding/removing 'publish_inlines' checkbox) according
> + // to the existance of draft inline comments.
> + var container = Y.one('#publish_drafts_container')
> + Y.Assert.areEqual('', container.get('text'));
> + Y.Assert.isNull(container.one('[type=checkbox]'));
> +
> + // A event is fired when the inlinecomments set changes.
> + Y.fire('inlinecomment.UPDATED');
> + Y.Assert.areEqual(
> + 'Publish 1 inline comments', container.get('text'));
> + Y.Assert.isNotNull(container.one('[type=checkbox]'));
> +
> + module.cleanup_comments();
> + Y.fire('inlinecomment.UPDATED');
> + Y.Assert.areEqual('', container.get('text'));
> + Y.Assert.isNull(container.one('[type=checkbox]'));
> + },
> +
> test_diff_nav_scrollers: function() {
> // The Diff Navigator review comment *scrollers* are connected
> // upon widget rendering and when clicked trigger the diff
> @@ -417,10 +437,25 @@
> this.reply_diffcontent();
> var mockio = this.diffnav.get('lp_client').io_provider;
> Y.Assert.areEqual(2, mockio.requests.length);
> + // The needed review-comment scrollers (View inline comments)
> + // are created. Only 'current' (non-superseded) comments
> + // associated with inline comments get a scroller.
> + var lines = []
> + Y.one('.conversation').get('text').split('\n').forEach(
> + function(line) { if (line.trim()) lines.push(line.trim()); }
> + );
> + Y.ArrayAssert.itemsAreEqual(
> + ['Comment from superseded',
> + 'Comment One View inline comments',
> + 'Comment Two View inline comments',
> + 'No inlines'],
> + lines);
> + // Scroller are identified as:
> + // 'ic-scroller-<previewdiff_id>-<comment_id>'.
> + var scroller = Y.one('#ic-scroller-202-2');
> // 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');
> @@ -446,20 +481,27 @@
> // 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 new_comment = Y.Node.create(
> + '<tr><td data-previewdiff-id="202" data-comment-id="3">' +
> + 'Comment Three</td></tr>');
> + Y.one('.conversation').append(new_comment);
> + // and instrument the inline-comment functions to remove
> + // and populate comments.
> + var cleaned = false;
> var populated = false;
> + module.cleanup_comments = function () {
> + cleaned = true;
> + };
> module.populate_comments = function () {
> populated = true;
> };
> - // Once called, update_on_new_comment() integrates the
> - // just-added scroller and updates the inline comments.
> + // Once called, update_on_new_comment() creates the scroller
> + // for the just-added comment and refreshes the inline comments.
> this.diffnav.update_on_new_comment();
> + var new_scroller = Y.one('#ic-scroller-202-3');
> Y.Assert.isTrue(new_scroller.hasClass('js-action'));
> Y.Assert.isTrue(populated);
> + Y.Assert.isTrue(cleaned);
> // Instrument the new scroller, so it will instantly fire
> // the diff navigator hook and register the requested diff.
> new_scroller.on('click', function() {
>
> === modified file 'lib/lp/code/stories/branches/xx-code-review-comments.txt'
> --- lib/lp/code/stories/branches/xx-code-review-comments.txt 2012-01-15 13:32:27 +0000
> +++ lib/lp/code/stories/branches/xx-code-review-comments.txt 2014-05-14 20:06:59 +0000
> @@ -189,3 +189,84 @@
> Testing commits in conversation
> 4. By ... on 2009-09-12
> and it works!
> +
> +
> +Inline Comments
> +---------------
> +
Can I postpone this move too ?
> +The code review inline comments support is entirely implemented in
> +Javascript. The current implementation relies on comments being
> +rendered with the following 'data-' attributes:
> +
> + # Created a new review comment with associated inline comments
> + # on the superseded and on the new MP.
> + >>> from lp.services.features.testing import FeatureFixture
> + >>> fixture = FeatureFixture(
> + ... {'code.inline_diff_comments.enabled': 'enabled'})
> +
> + >>> login('foo.bar@xxxxxxxxxxxxx')
> + >>> fixture.setUp()
> + >>> previewdiff = factory.makePreviewDiff(merge_proposal=merge_proposal)
> + >>> new_previewdiff = factory.makePreviewDiff(
> + ... merge_proposal=new_merge_proposal)
> + >>> transaction.commit()
> + >>> comment = merge_proposal.createComment(
> + ... eric, None, content='a_content',
> + ... previewdiff_id=previewdiff.id,
> + ... inline_comments={'1': 'No!'}
> + ... )
> + >>> transaction.commit()
> + >>> comment = new_merge_proposal.createComment(
> + ... eric, None, content='a_content',
> + ... previewdiff_id=new_previewdiff.id,
> + ... inline_comments={'1': 'Yes!'}
> + ... )
> + >>> fixture.cleanUp()
> + >>> logout()
> +
> + # helper for printing review comment 'data-' attributes.
> + >>> def print_comment_attributes(contents):
> + ... comments = find_tags_by_class(contents, 'boardCommentDetails')
> + ... print 'Text', 'Comment', 'Diff', 'Superseded'
> + ... for comment in comments:
> + ... tds = comment.findAll('td')
> + ... if len(tds) == 0:
> + ... continue
> + ... td = tds[0]
> + ... print ' '.join(
> + ... (extract_text(td),
> + ... td['data-comment-id'],
> + ... td.get('data-previewdiff-id', '-'),
> + ... td.get('data-from-superseded', '-')))
> +
> +The 'data-' attributes:
> +
> + * 'comment-id': The review comment ID, used to highlight related
> + * inline comments.
> + * 'previewdiff-id': used to load the corresponding `PreviewDiff`.
> + * 'from_superseded': 'True' or 'False' whether the context MP is
> + superseded by another. Used to supress context
> + handlers on superseded comments.
> +
> +They are always available in `BranchMergeProposal` pages.
> +
> + >>> anon_browser.open(merge_proposal_url)
> + >>> print_comment_attributes(anon_browser.contents)
> + Text Comment Diff Superseded
> + Eric... 1 - False
> + Eric... 2 - False
> + Eric... 3 - False
> + Eric... 4 1 False
> +
> +When visualized in the new merge proposal the comments from the original
> +merge proposal are marked as 'superseded' and there is a new and
> +non-superseded local comment.
> +
> + >>> anon_browser.open(new_merge_proposal_url)
> + >>> print_comment_attributes(anon_browser.contents)
> + Text Comment Diff Superseded
> + Eric... 1 - True
> + Eric... 2 - True
> + Eric... 3 - True
> + Eric... 4 1 True
> + Eric... 5 2 False
>
> === modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
> --- lib/lp/code/templates/branchmergeproposal-index.pt 2014-04-04 04:47:15 +0000
> +++ lib/lp/code/templates/branchmergeproposal-index.pt 2014-05-14 20:06:59 +0000
> @@ -144,7 +144,6 @@
> <div id="add-comment-review-fields">
> Review: <tal:review replace="structure comment_form/widgets/vote"/>
> Review type: <tal:review replace="structure comment_form/widgets/review_type"/>
> - <tal:publish_inline_comments condition="features/code.inline_diff_comments.enabled">Publish inline comments: <tal:review replace="structure comment_form/widgets/publish_inline_comments" tal:condition="features/code.inline_diff_comments.enabled"/></tal:publish_inline_comments>
> <div class="actions"
> tal:content="structure comment_form/actions/field.actions.add/render" />
> </div>
>
> === modified file 'lib/lp/code/templates/codereviewcomment-header.pt'
> --- lib/lp/code/templates/codereviewcomment-header.pt 2014-04-03 20:33:08 +0000
> +++ lib/lp/code/templates/codereviewcomment-header.pt 2014-05-14 20:06:59 +0000
> @@ -6,7 +6,9 @@
> <table>
> <tbody>
> <tr>
> - <td>
> + <td tal:attributes="data-previewdiff-id context/previewdiff_id;
> + data-from-superseded context/from_superseded;
> + data-comment-id context/id">
> <span
> itemprop="creator"
> tal:content="structure context/comment_author/fmt:link-display-name-id"/>
> @@ -26,11 +28,6 @@
> 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">
>
--
https://code.launchpad.net/~cprov/launchpad/ic-superseded-mps/+merge/219431
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References
