All set, thanks
Inline comments:
> --- lib/lp/app/javascript/comment.js 2014-04-04 05:23:48 +0000
> +++ lib/lp/app/javascript/comment.js 2014-05-15 03:58:24 +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,15 @@
> 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"]');
> + return inline_cb !== null && inline_cb.get('checked')
> + },
fixed.
> + /**
> * 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 +352,9 @@
> if (this.get_vote() !== null) {
> return true;
> }
> + if (this.has_inline()) {
> + return true
> + }
> return namespace.Comment.prototype.validate.apply(this);
> },
> /**
> @@ -357,9 +367,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 +390,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 +458,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);
> @@ -490,6 +494,11 @@
> this.review_type.on('keyup', this.syncUI, this);
> this.review_type.on('mouseup', this.syncUI, this);
> Y.all('a.menu-link-reply').on('click', this.reply_clicked, this);
> + Y.on('CodeReviewComment.SET_DISABLED', function(disabled) {
> + // this.set_disabled() unfortunately does too much for the
> + // current callsite (inlinecomment).
> + this.submit_button.set('disabled', disabled);
> + }, this);
> },
> /**
> * Implementation of Widget.syncUI: Update appearance according to state.
>
> === 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-15 03:58:24 +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-15 03:58:24 +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,68 @@
> };
>
>
> +var PublishDrafts = function () {
> + PublishDrafts.superclass.constructor.apply(this, arguments);
> +};
> +
> +Y.mix(PublishDrafts, {
> +
> + NAME: 'publishdrafts',
> +
> + ATTRS: {
> + }
> +
> +});
> +
> +Y.extend(PublishDrafts, Y.Widget, {
> +
> + /**
> + * syncUI implementation for PublishDrafts.
> + *
> + * Modifies the CodeReviewComment widget, by enable/disable it and
> + * adding/removing its 'publish_inline_comments' field according to
> + * the presence of draft inline comments to be published.
> + *
> + * @method syncUI
> + */
> + syncUI: function() {
> + this.get('srcNode').empty();
> + var n_drafts = Y.all('.draft').size();
Makes sense, thanks.
Most of our widgets are enhancers, but PublishDraft() surely isn't.
> + if (n_drafts == 0) {
> + Y.fire('CodeReviewComment.SET_DISABLED', true);
> + return;
> + }
> + var text = 'Publish ' + n_drafts + ' inline comment';
> + if (n_drafts > 1) text += 's';
> + var label = Y.Node.create('<a />')
> + .set('text', text);
> + var checkbox = Y.Node.create(
> + '<input id="field.publish_inline_comments"' +
> + 'name="field.publish_inline_comments"' +
> + 'type="checkbox" class="checkboxType"' +
> + 'checked=""></input>')
> + this.get('srcNode').append(checkbox);
> + this.get('srcNode').append(label);
> + Y.fire('CodeReviewComment.SET_DISABLED', false);
> + },
> +
> + /**
> + * bindUI implementation for PublishDrafts.
> + *
> + * Simply hook syncUI() to 'inlinecomment.UPDATE' events.
> + *
> + * @method bindUI
> + */
> + bindUI: function() {
> + Y.detach('inlinecomment.UPDATED');
> + Y.on('inlinecomment.UPDATED', this.syncUI, this);
> + }
> +
> +});
> +
> +namespace.PublishDrafts = PublishDrafts;
> +
> +
> var InlineCommentToggle = function () {
> InlineCommentToggle.superclass.constructor.apply(this, arguments);
> };
> @@ -383,7 +452,7 @@
> * @method cleanup_status
> */
> cleanup_status: function() {
> - this.get('srcNode').all('h2 img').remove();
> + this.get('srcNode').all('h2 img').remove(true);
> },
>
> /**
> @@ -395,6 +464,7 @@
> if (LP.cache.inline_diff_comments !== true) {
> return;
> }
> + namespace.cleanup_comments();
> namespace.populate_comments();
> this._connectScrollers();
> },
> @@ -408,10 +478,25 @@
> _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'));
> + Y.all('td[data-previewdiff-id]').each( function(td) {
> + // Comments from superseded MPs should be ignored.
> + if (td.getData('from-superseded') === 'True') {
> + return;
> + }
> + var previewdiff_id = td.getData('previewdiff-id');
> + var comment_id = td.getData('comment-id');
> + // We have to remove the old scrollers otherwise they will
> + // fire multiple 'click' handlers (and animations).
> + var scroller = td.one('.ic-scroller');
> + if (scroller !== null) {
> + scroller.remove(true);
> + }
> + scroller = Y.Node.create(
> + '<a href="" class="ic-scroller" style="float: right;">' +
> + 'View inline comments</a>');
> + td.append(scroller);
> + rc.link_scroller(scroller, '#review-diff', function() {
> + self._showPreviewDiff(previewdiff_id);
> });
> });
> },
> @@ -488,6 +573,14 @@
> self._updateDiff(preview_diff_uri);
> return;
> }
> +
> + var pub_drafts_container = Y.Node.create(
> + '<div class="publish_drafts_container">');
> + Y.one('[id="field.review_type"]').insert(
> + pub_drafts_container, 'after');
> + (new namespace.PublishDrafts(
> + {'srcNode': pub_drafts_container})).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-15 03:58:24 +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-15 03:58:24 +0000
> @@ -408,6 +408,31 @@
> 'table-row', Y.one('tr.ict-header').getStyle('display'));
> },
>
> + test_publish_drafts: function() {
> + // PublishDraft() widget modifies the add-comment form
> + // (adding/removing 'publish_inlines' checkbox) according
> + // to the existence 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 comment', container.get('text'));
> + Y.Assert.isNotNull(container.one('[type=checkbox]'));
> + // Adding another draft.
> + module.create_row({'line_number': '2', 'text': 'another'});
> + Y.fire('inlinecomment.UPDATED');
> + Y.Assert.areEqual(
> + 'Publish 2 inline comments', container.get('text'));
> + Y.Assert.isNotNull(container.one('[type=checkbox]'));
> + // Removing all drafts.
> + 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 +442,26 @@
> 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 comment = Y.one('[data-comment-id="2"]');
> + var scroller = comment.one('.ic-scroller')
> // 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 +487,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 = new_comment.one('.ic-scroller');
> 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-15
> 03:58:24 +0000
> @@ -189,3 +189,84 @@
> Testing commits in conversation
> 4. By ... on 2009-09-12
> and it works!
> +
> +
> +Inline Comments
> +---------------
> +
> +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....@canonical.com')
> + >>> 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-15
> 03:58:24 +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-15 03:58:24
> +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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
