launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #16365
[Merge] lp:~cprov/launchpad/cric-api-take2 into lp:launchpad
Celso Providelo has proposed merging lp:~cprov/launchpad/cric-api-take2 into lp:launchpad.
Requested reviews:
William Grant (wgrant)
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cprov/launchpad/cric-api-take2/+merge/200563
--
https://code.launchpad.net/~cprov/launchpad/cric-api-take2/+merge/200563
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cprov/launchpad/cric-api-take2 into lp:launchpad.
=== modified file 'lib/lp/app/javascript/client.js'
--- lib/lp/app/javascript/client.js 2013-12-12 04:47:30 +0000
+++ lib/lp/app/javascript/client.js 2014-01-14 20:14:32 +0000
@@ -183,8 +183,9 @@
for (index = 0; index < value.length; index++) {
elems.push(enc(key) + "=" + enc(value[index]));
}
- }
- else {
+ } else if (Y.Lang.isObject(value)) {
+ elems.push(enc(key) + "=" + Y.JSON.stringify(value));
+ } else {
elems.push(enc(key) + "=" + enc(value));
}
return elems.join("&");
@@ -1256,5 +1257,5 @@
}
});
}, "0.1", {
- requires: ["base", "plugin", "dump", "lp.client"]
+ requires: ["base", "plugin", "dump", "json", "lp.client"]
});
=== modified file 'lib/lp/app/javascript/comment.js'
--- lib/lp/app/javascript/comment.js 2012-07-25 21:49:22 +0000
+++ lib/lp/app/javascript/comment.js 2014-01-14 20:14:32 +0000
@@ -308,6 +308,8 @@
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;
},
/**
@@ -355,6 +357,7 @@
namespace.Comment.prototype.set_disabled.call(this, disabled);
this.vote_input.set('disabled', disabled);
this.review_type.set('disabled', disabled);
+ this.publish_inline_comments.set('disabled', disabled);
},
/**
* Post the comment to the Launchpad API
@@ -372,6 +375,8 @@
content: this.comment_input.get('value'),
subject: '',
review_type: this.review_type.get('value'),
+ publish_inline_comments: this.publish_inline_comments.get(
+ 'checked'),
vote: this.get_vote()
}
};
@@ -439,6 +444,7 @@
*/
reset_contents: function() {
this.review_type.set('value', '');
+ this.publish_inline_comments.set('value', '');
this.vote_input.set('selectedIndex', 0);
this.in_reply_to = null;
namespace.Comment.prototype.reset_contents.apply(this);
@@ -456,7 +462,7 @@
this.reset_contents();
Y.lp.anim.green_flash({node: comment}).run();
},
-
+
/**
* Implementation of Widget.bindUI: Bind events to methods.
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py 2013-09-05 03:57:28 +0000
+++ lib/lp/code/browser/branchmergeproposal.py 2014-01-14 20:14:32 +0000
@@ -61,6 +61,7 @@
SimpleTerm,
SimpleVocabulary,
)
+from zope.security.proxy import removeSecurityProxy
from lp import _
from lp.app.browser.launchpadform import (
@@ -92,6 +93,9 @@
)
from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
from lp.code.interfaces.codereviewcomment import ICodeReviewComment
+from lp.code.interfaces.codereviewinlinecomment import (
+ ICodeReviewInlineCommentSet,
+ )
from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
from lp.code.interfaces.diff import IPreviewDiff
from lp.registry.interfaces.person import IPersonSet
@@ -627,6 +631,13 @@
self.context).event_key
if getFeatureFlag("code.inline_diff_comments.enabled"):
cache.objects['inline_diff_comments'] = True
+ cache.objects['preview_diff_timestamps'] = [
+ pd.date_created for pd in self.context.preview_diffs]
+ if self.context.preview_diff:
+ cache.objects['published_inline_comments'] = (
+ self.context.getInlineComments(self.user))
+ else:
+ cache.objects['published_inline_comments'] = []
@action('Claim', name='claim')
def claim_action(self, action, data):
@@ -713,9 +724,7 @@
If no preview is available, try using the review diff.
"""
- if self.context.preview_diff is not None:
- return self.context.preview_diff
- return None
+ return self.context.preview_diff
@property
def has_bug_or_spec(self):
@@ -1462,8 +1471,7 @@
else:
review_type = self.users_vote_ref.review_type
# We'll be positive here and default the vote to approve.
- return {'vote': CodeReviewVote.APPROVE,
- 'review_type': review_type}
+ return {'vote': CodeReviewVote.APPROVE, 'review_type': review_type}
def initialize(self):
"""Get the users existing vote reference."""
@@ -1478,10 +1486,10 @@
if self.user is None:
# Anonymous users are not valid voters.
raise AssertionError('Invalid voter')
- LaunchpadFormView.initialize(self)
+ super(BranchMergeProposalAddVoteView, self).initialize()
def setUpFields(self):
- LaunchpadFormView.setUpFields(self)
+ super(BranchMergeProposalAddVoteView, self).setUpFields()
self.reviewer = self.user.name
# claim_review is set in situations where a user is reviewing on
# behalf of a team.
@@ -1512,8 +1520,7 @@
# the data dict. If this is the case, get the review_type from the
# hidden field that we so cunningly added to the form.
review_type = data.get(
- 'review_type',
- self.request.form.get('review_type'))
+ 'review_type', self.request.form.get('review_type'))
# Translate the request parameter back into what our object model
# needs.
if review_type == '':
=== modified file 'lib/lp/code/browser/codereviewcomment.py'
--- lib/lp/code/browser/codereviewcomment.py 2013-04-11 04:54:04 +0000
+++ lib/lp/code/browser/codereviewcomment.py 2014-01-14 20:14:32 +0000
@@ -13,6 +13,7 @@
from lazr.delegates import delegates
from lazr.restful.interface import copy_field
+from zope.component import getUtility
from zope.formlib.widgets import (
DropdownWidget,
TextAreaWidget,
@@ -21,7 +22,10 @@
implements,
Interface,
)
-from zope.schema import Text
+from zope.schema import (
+ Bool,
+ Text,
+ )
from lp import _
from lp.app.browser.launchpadform import (
@@ -30,11 +34,15 @@
LaunchpadFormView,
)
from lp.code.interfaces.codereviewcomment import ICodeReviewComment
+from lp.code.interfaces.codereviewinlinecomment import (
+ ICodeReviewInlineCommentSet,
+ )
from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
from lp.services.comments.browser.comment import download_body
from lp.services.comments.browser.messagecomment import MessageComment
from lp.services.comments.interfaces.conversation import IComment
from lp.services.config import config
+from lp.services.features import getFeatureFlag
from lp.services.librarian.interfaces import ILibraryFileAlias
from lp.services.propertycache import (
cachedproperty,
@@ -214,6 +222,9 @@
comment = Text(title=_('Comment'), required=False)
+ publish_inline_comments = Bool(
+ title=_("Publish draft inline comments"), required=False)
+
class CodeReviewCommentAddView(LaunchpadFormView):
"""View for adding a CodeReviewComment."""
@@ -242,6 +253,11 @@
comment = ''
return {'comment': comment}
+ def setUpFields(self):
+ super(CodeReviewCommentAddView, self).setUpFields()
+ if not getFeatureFlag('code.inline_diff_comments.enabled'):
+ self.form_fields.omit('publish_inline_comments')
+
@property
def is_reply(self):
"""True if this comment is a reply to another comment, else False."""
@@ -268,9 +284,14 @@
"""Create the comment..."""
vote = data.get('vote')
review_type = data.get('review_type')
- self.branch_merge_proposal.createComment(
+ publish_inline_comments = False
+ if (getFeatureFlag('code.inline_diff_comments.enabled') and
+ data.get('publish_inline_comments')):
+ publish_inline_comments = True
+ comment = self.branch_merge_proposal.createComment(
self.user, subject=None, content=data['comment'],
- parent=self.reply_to, vote=vote, review_type=review_type)
+ parent=self.reply_to, vote=vote, review_type=review_type,
+ publish_inline_comments=publish_inline_comments)
@property
def next_url(self):
=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml 2013-11-21 03:42:38 +0000
+++ lib/lp/code/configure.zcml 2014-01-14 20:14:32 +0000
@@ -586,6 +586,23 @@
provides="lp.services.webapp.interfaces.IPrimaryContext"
factory="lp.code.browser.codereviewcomment.CodeReviewCommentPrimaryContext"/>
+ <!-- CodeReviewInlineComment -->
+
+ <class class="lp.code.model.codereviewinlinecomment.CodeReviewInlineComment">
+ <allow interface="lp.code.interfaces.codereviewinlinecomment.ICodeReviewInlineComment"/>
+ </class>
+
+ <!-- CodeReviewInlineCommentSet -->
+
+ <securedutility
+ class="lp.code.model.codereviewinlinecomment.CodeReviewInlineCommentSet"
+ provides="lp.code.interfaces.codereviewinlinecomment.ICodeReviewInlineCommentSet">
+ <allow interface="lp.code.interfaces.codereviewinlinecomment.ICodeReviewInlineCommentSet"/>
+ </securedutility>
+ <class class="lp.code.model.codereviewinlinecomment.CodeReviewInlineCommentSet">
+ <allow interface="lp.code.interfaces.codereviewinlinecomment.ICodeReviewInlineCommentSet"/>
+ </class>
+
<!-- hierarchy -->
<class class="lp.code.model.branchjob.BranchJob">
=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py 2012-08-28 00:07:47 +0000
+++ lib/lp/code/errors.py 2014-01-14 20:14:32 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Errors used in the lp/code modules."""
@@ -27,6 +27,7 @@
'CodeImportAlreadyRunning',
'CodeImportNotInReviewedState',
'ClaimReviewFailed',
+ 'DiffNotFound',
'InvalidBranchMergeProposal',
'InvalidMergeQueueConfig',
'InvalidNamespace',
@@ -384,3 +385,8 @@
def __init__(self):
message = ('The configuration specified is not a valid JSON string.')
Exception.__init__(self, message)
+
+
+@error_status(httplib.BAD_REQUEST)
+class DiffNotFound(Exception):
+ """A `IPreviewDiff` with the timestamp was not found."""
=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py 2013-12-16 05:04:43 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py 2014-01-14 20:14:32 +0000
@@ -56,6 +56,7 @@
Bool,
Choice,
Datetime,
+ Dict,
Int,
Object,
Text,
@@ -543,12 +544,14 @@
@operation_parameters(
subject=Text(), content=Text(),
vote=Choice(vocabulary=CodeReviewVote), review_type=Text(),
- parent=Reference(schema=Interface))
+ parent=Reference(schema=Interface),
+ publish_inline_comments=Bool())
@call_with(owner=REQUEST_USER)
# ICodeReviewComment supplied as Interface to avoid circular imports.
@export_factory_operation(Interface, [])
def createComment(owner, subject, content=None, vote=None,
- review_type=None, parent=None):
+ review_type=None, parent=None,
+ publish_inline_comments=False):
"""Create an ICodeReviewComment associated with this merge proposal.
:param owner: The person who the message is from.
@@ -557,6 +560,8 @@
unspecified, the text of the merge proposal is used.
:param parent: The previous CodeReviewComment in the thread. If
unspecified, the root message is used.
+ :param publish_inline_comments: whether or not to publish the existing
+ (draft) code review inline comments.
"""
def createCommentFromMessage(message, vote, review_type,
@@ -569,6 +574,32 @@
:param original_email: Original email message.
"""
+ @export_read_operation()
+ @call_with(person=REQUEST_USER)
+ @operation_for_version('devel')
+ def getInlineComments(person):
+ """Return the set of inline comments related to this MP.
+
+ The return value is a list of 4-tuples representing published and
+ draft inline comments.
+
+ :param person: The `IPerson` owner of the draft comments.
+ """
+
+ @export_write_operation()
+ @operation_parameters(
+ diff_timestamp=Datetime(), comments=Dict(key_type=TextLine()))
+ @call_with(person=REQUEST_USER)
+ @operation_for_version('devel')
+ def saveDraftInlineComment(diff_timestamp, person, comments):
+ """Save `ICodeReviewInlineCommentDraft`
+
+ :param diff_timestamp: The timestamp of the `PreviewDiff` the comments
+ are on.
+ :param person: The `IPerson` making the comments.
+ :param comments: The comments.
+ """
+
class IBranchMergeProposal(IBranchMergeProposalPublic,
IBranchMergeProposalView, IBranchMergeProposalEdit,
=== added file 'lib/lp/code/interfaces/codereviewinlinecomment.py'
--- lib/lp/code/interfaces/codereviewinlinecomment.py 1970-01-01 00:00:00 +0000
+++ lib/lp/code/interfaces/codereviewinlinecomment.py 2014-01-14 20:14:32 +0000
@@ -0,0 +1,67 @@
+# Copyright 2013 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Interfaces for inline comments with preview diffs."""
+
+__metaclass__ = type
+__all__ = [
+ 'ICodeReviewInlineComment',
+ 'ICodeReviewInlineCommentSet',
+ ]
+
+from lazr.restful.fields import Reference
+from zope.interface import Interface
+from zope.schema import (
+ Datetime,
+ TextLine,
+ )
+
+from lp import _
+from lp.code.interfaces.codereviewcomment import ICodeReviewComment
+from lp.code.interfaces.diff import IPreviewDiff
+from lp.registry.interfaces.person import IPerson
+
+
+class ICodeReviewInlineComment(Interface):
+ previewdiff = Reference(
+ title=_('The preview diff'), schema=IPreviewDiff, required=True,
+ readonly=True)
+ person = Reference(
+ title=_('Person'), schema=IPerson, required=True, readonly=True)
+ comment = Reference(
+ title=_('The branch merge proposal comment'),
+ schema=ICodeReviewComment, required=True, readonly=True)
+ date_created = Datetime(
+ title=_('The date on which the comments were published'),
+ required=True, readonly=True)
+ comments = TextLine(
+ title=_('Inline comments'), required=True, readonly=True)
+
+
+class ICodeReviewInlineCommentSet(Interface):
+ def findDraft(previewdiff, person):
+ """Find the draft comments for a given diff and person.
+
+ :param previewdiff: The `PreviewDiff` these comments are for.
+ :param person: The `Person` making the comments.
+ """
+
+ def ensureDraft(previewdiff, person, comments):
+ """Ensure a `ICodeReviewInlineCommentDraft` is up to date. This method
+ will also delete an existing draft if the comments are empty.
+
+ :param previewdiff: The `PreviewDiff` these comments are for.
+ :param person: The `Person` making the comments.
+ :param comments: The comments themselves.
+ """
+
+ def publishDraft(previewdiff, person, comment):
+ """Publish code review inline comments so other people can view them.
+
+ :param previewdiff: The `PreviewDiff` these comments are for.
+ :param person: The `Person` making the comments.
+ :param comment: The `CodeReviewComment` linked to the comments.
+ """
+
+ def findByPreviewDiff(previewdiff):
+ """Find all comments for a given `PreviewDiff`."""
=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2013-09-17 06:14:49 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-01-14 20:14:32 +0000
@@ -11,9 +11,9 @@
// Grab the namespace in order to be able to expose the connect methods.
var namespace = Y.namespace('lp.code.branchmergeproposal.inlinecomments');
+var inlinecomments = {};
namespace.add_doubleclick_handler = function() {
- var inlinecomments = {};
var rows = Y.one('.diff').all('tr');
var handling_request = false;
handler = function(e) {
@@ -39,9 +39,20 @@
inlinecomments[rownumber] = comment;
}
if (comment === '') {
+ delete inlinecomments[rownumber];
headerrow.remove(true);
newrow.remove(true);
}
+ var diff_timestamp = LP.cache.preview_diff_timestamps.slice(-1)[0];
+ var config = {
+ parameters: {
+ diff_timestamp: diff_timestamp,
+ comments: inlinecomments
+ }
+ };
+ lp_client = new Y.lp.client.Launchpad();
+ lp_client.named_post(
+ LP.cache.context.self_link, 'saveDraftInlineComment', config);
handling_request = false;
};
widget.editor.on('save', function() {
@@ -72,7 +83,7 @@
headerrow = Y.Node.create(
'<tr><td colspan="2"></td></tr>').addClass('ict-header');
var headerspan;
- if (person !== null && date !== null) {
+ if (person != null && date != null) {
headerspan = Y.Node.create(
'<span>Comment by <a></a> on <span></span></span>');
headerspan.one('a').set('href', person.web_link).set(
@@ -90,6 +101,10 @@
'<div class="yui3-editable_text-trigger"></div>' +
'</div></td></tr>').set('id', 'ict-' + middle);
newrow.one('td>div').set('id', 'inlinecomment-' + middle);
+ if (comment !== null) {
+ inlinecomments[rownumber] = comment;
+ newrow.one('span').set('text', comment);
+ }
} else {
newrow = Y.Node.create('<tr><td></td><td><span></span></td></tr>');
newrow.one('span').set('text', comment);
@@ -111,11 +126,15 @@
};
namespace.populate_existing_comments = function() {
- var i;
- for (i = 0; i < LP.cache.published_inline_comments.length; i++) {
+ // Cleanup previous inline review comments.
+ Y.all('.ict-header').each( function(line) {
+ line.next().remove();
+ line.remove();
+ });
+ for (var i = 0; i < LP.cache.published_inline_comments.length; i++) {
var row = LP.cache.published_inline_comments[i];
namespace.create_or_return_row(
- row.line, row.person, row.comment, row.date);
+ row[0], row[1], row[2], row[3]);
}
};
=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2013-09-17 06:14:49 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-01-14 20:14:32 +0000
@@ -21,9 +21,9 @@
test_populatation: function () {
var published_inline_comments = [
- {'line': 2, 'person': {'display_name': 'Sample Person',
- 'name': 'name.12', 'web_link': 'http://launchpad.dev/~name12'},
- 'comment': 'This is preloaded.', 'date': '2012-08-12 17:45'}];
+ [2, {'display_name': 'Sample Person', 'name': 'name.12',
+ 'web_link': 'http://launchpad.dev/~name12'},
+ 'This is preloaded.', '2012-08-12 17:45']];
LP.cache.published_inline_comments = published_inline_comments;
module.populate_existing_comments();
var row = Y.one('#diff-line-2').next().next();
@@ -51,7 +51,7 @@
Y.one('.yui3-ieditor-input>textarea').set('value', 'Foobar!');
Y.one('.lazr-neg').simulate('click');
Y.Assert.areEqual(
- comment, Y.one('.yui3-editable_text-text').get('text'));
+ '', Y.one('.yui3-editable_text-text').get('text'));
},
test_handler_cancel_immediately: function() {
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2013-06-20 05:50:00 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2014-01-14 20:14:32 +0000
@@ -45,6 +45,7 @@
BadBranchMergeProposalSearchContext,
BadStateTransition,
BranchMergeProposalExists,
+ DiffNotFound,
UserNotBranchReviewer,
WrongBranchMergeProposal,
)
@@ -63,6 +64,9 @@
)
from lp.code.interfaces.branchrevision import IBranchRevision
from lp.code.interfaces.branchtarget import IHasBranchTarget
+from lp.code.interfaces.codereviewinlinecomment import (
+ ICodeReviewInlineCommentSet,
+ )
from lp.code.mail.branch import RecipientReason
from lp.code.model.branchrevision import BranchRevision
from lp.code.model.codereviewcomment import CodeReviewComment
@@ -72,6 +76,7 @@
IncrementalDiff,
PreviewDiff,
)
+from lp.services.features import getFeatureFlag
from lp.registry.interfaces.person import (
IPerson,
IPersonSet,
@@ -747,7 +752,7 @@
def createComment(self, owner, subject, content=None, vote=None,
review_type=None, parent=None, _date_created=DEFAULT,
- _notify_listeners=True):
+ publish_inline_comments=False, _notify_listeners=True):
"""See `IBranchMergeProposal`."""
#:param _date_created: The date the message was created. Provided
# only for testing purposes, as it can break
@@ -776,10 +781,17 @@
parent=parent_message, owner=owner, rfc822msgid=msgid,
subject=subject, datecreated=_date_created)
MessageChunk(message=message, content=content, sequence=1)
- return self.createCommentFromMessage(
+ comment = self.createCommentFromMessage(
message, vote, review_type, original_email=None,
_notify_listeners=_notify_listeners, _validate=False)
+ if (getFeatureFlag("code.inline_diff_comments.enabled") and
+ publish_inline_comments):
+ getUtility(ICodeReviewInlineCommentSet).publishDraft(
+ self.preview_diff, owner, comment)
+
+ return comment
+
def getUsersVoteReference(self, user, review_type=None):
"""Get the existing vote reference for the given user."""
# Lower case the review type.
@@ -869,6 +881,24 @@
code_review_message, original_email))
return code_review_message
+ def getInlineComments(self, person):
+ """See `IBranchMergeProposal`."""
+ return getUtility(ICodeReviewInlineCommentSet).findByPreviewDiff(
+ self.preview_diff, person)
+
+ def saveDraftInlineComment(self, diff_timestamp, person, comments):
+ """See `IBranchMergeProposal`."""
+ previewdiff = IStore(PreviewDiff).find(
+ PreviewDiff,
+ PreviewDiff.branch_merge_proposal_id == self.id,
+ PreviewDiff.date_created == diff_timestamp).one()
+ if not previewdiff:
+ raise DiffNotFound(
+ "Could not locate a preview diff with a timestamp of %s" % (
+ diff_timestamp))
+ getUtility(ICodeReviewInlineCommentSet).ensureDraft(
+ previewdiff, person, comments)
+
def updatePreviewDiff(self, diff_content, source_revision_id,
target_revision_id, prerequisite_revision_id=None,
conflicts=None):
=== added file 'lib/lp/code/model/codereviewinlinecomment.py'
--- lib/lp/code/model/codereviewinlinecomment.py 1970-01-01 00:00:00 +0000
+++ lib/lp/code/model/codereviewinlinecomment.py 2014-01-14 20:14:32 +0000
@@ -0,0 +1,125 @@
+# Copyright 2013 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Model classes for inline comments for preview diffs."""
+
+__metaclass__ = type
+__all__ = [
+ 'CodeReviewInlineComment',
+ 'CodeReviewInlineCommentDraft',
+ 'CodeReviewInlineCommentSet',
+ ]
+
+import simplejson
+from storm.locals import (
+ Int,
+ Reference,
+ Unicode,
+ )
+from zope.component import getUtility
+from zope.interface import implements
+
+from lp.code.interfaces.codereviewinlinecomment import (
+ ICodeReviewInlineComment,
+ ICodeReviewInlineCommentSet,
+ )
+from lp.code.model.codereviewcomment import CodeReviewComment
+from lp.registry.interfaces.person import IPersonSet
+from lp.services.database.bulk import load_related
+from lp.services.database.interfaces import IStore
+from lp.services.database.stormbase import StormBase
+
+
+class CodeReviewInlineComment(StormBase):
+ __storm_table__ = 'CodeReviewInlineComment'
+
+ implements(ICodeReviewInlineComment)
+
+ previewdiff_id = Int(name='previewdiff')
+ previewdiff = Reference(previewdiff_id, 'PreviewDiff.id')
+ person_id = Int(name='person')
+ person = Reference(person_id, 'Person.id')
+ comment_id = Int(name='comment', primary=True)
+ comment = Reference(comment_id, 'CodeReviewComment.id')
+ comments = Unicode()
+
+
+class CodeReviewInlineCommentDraft(StormBase):
+ __storm_table__ = 'CodeReviewInlineCommentDraft'
+ __storm_primary__ = ('previewdiff_id', 'person_id')
+
+ previewdiff_id = Int(name='previewdiff')
+ previewdiff = Reference(previewdiff_id, 'PreviewDiff.id')
+ person_id = Int(name='person')
+ person = Reference(person_id, 'Person.id')
+ comments = Unicode()
+
+
+class CodeReviewInlineCommentSet:
+
+ implements(ICodeReviewInlineCommentSet)
+
+ def _findDraftObject(self, previewdiff, person):
+ return IStore(CodeReviewInlineCommentDraft).find(
+ CodeReviewInlineCommentDraft,
+ CodeReviewInlineCommentDraft.previewdiff_id == previewdiff.id,
+ CodeReviewInlineCommentDraft.person_id == person.id).one()
+
+ def findDraft(self, previewdiff, person):
+ cricd = self._findDraftObject(previewdiff, person)
+ if cricd:
+ return simplejson.loads(cricd.comments)
+ return {}
+
+ def ensureDraft(self, previewdiff, person, comments):
+ cricd = self._findDraftObject(previewdiff, person)
+ if not comments:
+ if cricd:
+ IStore(CodeReviewInlineCommentDraft).remove(cricd)
+ else:
+ if type(comments) == dict:
+ comments = simplejson.dumps(comments).decode('utf-8')
+ if cricd:
+ cricd.comments = comments
+ else:
+ cricd = CodeReviewInlineCommentDraft()
+ cricd.previewdiff = previewdiff
+ cricd.person = person
+ cricd.comments = comments
+ IStore(CodeReviewInlineCommentDraft).add(cricd)
+
+ def publishDraft(self, previewdiff, person, comment):
+ cricd = self._findDraftObject(previewdiff, person)
+ if cricd is None:
+ return
+ cric = CodeReviewInlineComment()
+ cric.previewdiff = cricd.previewdiff
+ # XXX cprov 20140114: why the hell cricd.person is empty here ?
+ cric.person = person
+ cric.comment = comment
+ cric.comments = cricd.comments
+ IStore(CodeReviewInlineComment).add(cric)
+ IStore(CodeReviewInlineComment).remove(cricd)
+ return cric
+
+ def findByPreviewDiff(self, previewdiff, person):
+ crics = IStore(CodeReviewInlineComment).find(
+ CodeReviewInlineComment,
+ CodeReviewInlineComment.previewdiff_id == previewdiff.id)
+ getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+ [cric.person_id for cric in crics])
+ load_related(CodeReviewComment, crics, ['comment_id'])
+ sorted_crics = sorted(
+ list(crics), key=lambda c: c.comment.date_created)
+ inline_comments = []
+ for cric in sorted_crics:
+ comments = simplejson.loads(cric.comments)
+ for lineno in comments:
+ inline_comments.append(
+ [lineno, cric.person, comments[lineno],
+ cric.comment.date_created])
+ draft_comments = self.findDraft(previewdiff, person)
+ for draft_lineno in draft_comments:
+ inline_comments.append(
+ [draft_lineno, None, draft_comments[draft_lineno], None])
+ return inline_comments
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py 2013-05-14 14:30:09 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py 2014-01-14 20:14:32 +0000
@@ -47,6 +47,9 @@
IBranchMergeProposalGetter,
notify_modified,
)
+from lp.code.interfaces.codereviewinlinecomment import (
+ ICodeReviewInlineCommentSet,
+ )
from lp.code.model.branchmergeproposal import (
BranchMergeProposalGetter,
is_valid_transition,
@@ -2038,6 +2041,75 @@
self.assertNotIn(r1, partial_revisions)
+class TestBranchMergeProposalInlineComments(TestCaseWithFactory):
+
+ layer = LaunchpadFunctionalLayer
+
+ def setUp(self):
+ super(TestBranchMergeProposalInlineComments, self).setUp()
+ # Create a testing IPerson, IPreviewDiff and IBranchMergeProposal
+ # for tests. Log in as the testing IPerson.
+ self.person = self.factory.makePerson()
+ self.bmp = self.factory.makeBranchMergeProposal(
+ registrant=self.person)
+ self.previewdiff = self.factory.makePreviewDiff(
+ merge_proposal=self.bmp)
+ login_person(self.person)
+
+ def test_save_draft(self):
+ # Arbitrary comments, passed as a dictionary keyed by diff line
+ # number, can be saved as draft using 'saveDraftInlineComment'.
+ comments = {'10': 'DrAfT', '15': 'CoMmEnTs'}
+ self.bmp.saveDraftInlineComment(
+ diff_timestamp=self.previewdiff.date_created,
+ person=self.person,
+ comments=comments)
+ self.assertEqual(2, len(self.bmp.getInlineComments(self.person)))
+
+ def test_publish_draft(self):
+ # Existing draft comments can be published associated with an
+ # `ICodeReviewComment` using
+ # 'ICodeReviewInlineCommentSet.publishDraft'.
+ self.factory.makeCodeReviewInlineCommentDraft(
+ previewdiff=self.previewdiff, person=self.person)
+ review_comment = self.factory.makeCodeReviewComment(
+ merge_proposal=self.bmp)
+ getUtility(ICodeReviewInlineCommentSet).publishDraft(
+ self.previewdiff, self.person, review_comment)
+ self.assertEqual(1, len(self.bmp.getInlineComments(self.person)))
+
+ def test_get_published_and_draft(self):
+ # Published and draft comments can be retrieved via
+ # 'getInlineComments'.
+ self.factory.makeCodeReviewInlineComment(
+ previewdiff=self.previewdiff, person=self.person,
+ comments={'11': 'eleven'})
+ self.factory.makeCodeReviewInlineCommentDraft(
+ previewdiff=self.previewdiff, person=self.person,
+ comments={'10': 'ten'})
+ transaction.commit()
+
+ inline_comments = self.bmp.getInlineComments(self.person)
+ self.assertEqual(2, len(inline_comments))
+ [published, draft] = inline_comments
+
+ # The 'published' inline comment is represented by a list of 4
+ # elements (line_number, author, comment, timestamp).
+ [line_number, author, content, timestamp] = published
+ self.assertEqual('11', line_number)
+ self.assertEqual(self.person.displayname, author.displayname)
+ self.assertEqual('eleven', content)
+ self.assertIsNotNone(timestamp)
+
+ # The 'draft' comment is similar but 'author' and 'timestamp' are
+ # not set.
+ [line_number, author, content, timestamp] = draft
+ self.assertEqual('10', line_number)
+ self.assertIsNone(author)
+ self.assertEqual('ten', content)
+ self.assertIsNone(timestamp)
+
+
class TestWebservice(WebServiceTestCase):
"""Tests for the webservice."""
@@ -2087,3 +2159,53 @@
user = previewdiff.branch_merge_proposal.target_branch.owner
ws_previewdiff = self.wsObject(previewdiff, user=user)
self.assertIsNone(ws_previewdiff.diffstat)
+
+ def test_saveDraftInlineComment_with_no_previewdiff(self):
+ bmp = self.factory.makeBranchMergeProposal()
+ ws_bmp = self.wsObject(bmp, user=bmp.target_branch.owner)
+ self.assertRaises(
+ BadRequest, ws_bmp.saveDraftInlineComment,
+ diff_timestamp=datetime(2001, 1, 1, 12, tzinfo=UTC), comments={})
+
+ def test_saveDraftInlineComment(self):
+ previewdiff = self.factory.makePreviewDiff()
+ user = previewdiff.branch_merge_proposal.target_branch.owner
+ ws_bmp = self.wsObject(previewdiff.branch_merge_proposal, user=user)
+ comments = {u'2': u'foo'}
+ ws_bmp.saveDraftInlineComment(
+ diff_timestamp=previewdiff.date_created, comments=comments)
+ transaction.commit()
+ draft_comments = getUtility(ICodeReviewInlineCommentSet).findDraft(
+ previewdiff, user)
+ self.assertEqual(comments, draft_comments)
+
+ def test_getInlineComments(self):
+ # IBranchMergeProposal.getInlineComments returns all inline comments
+ # (draft or published) for the *current* diff.
+
+ # Let's create a merge proposal with one draft and one published
+ # inline comment.
+ person = self.factory.makePerson()
+ previewdiff = self.factory.makePreviewDiff()
+ cric = self.factory.makeCodeReviewInlineComment(
+ previewdiff=previewdiff, person=person,
+ comments={'1': 'published'})
+ cricd = self.factory.makeCodeReviewInlineCommentDraft(
+ previewdiff=previewdiff, person=person,
+ comments={'2': 'draft'})
+ transaction.commit()
+
+ # Fetch the 'IBranchMergeProposal' webservice object in question for
+ # calling 'getInlineComments' ('person' argument is implicitly passed
+ # as the request user).
+ ws_bmp = self.wsObject(previewdiff.branch_merge_proposal, user=person)
+ inline_comments = ws_bmp.getInlineComments()
+
+ # The result is a list with 2 elements, the inline comments. See
+ # IBranchMergeProposal.getInlineComments() for more details.
+ self.assertEqual(2, len(inline_comments))
+
+ # XXX cprov 20140114: the person reference is returned as
+ # a dictionary. is this correct ?
+ [published, draft] = inline_comments
+ self.assertEqual(person.displayname, published[1]['display_name'])
=== added file 'lib/lp/code/model/tests/test_codereviewinlinecomment.py'
--- lib/lp/code/model/tests/test_codereviewinlinecomment.py 1970-01-01 00:00:00 +0000
+++ lib/lp/code/model/tests/test_codereviewinlinecomment.py 2014-01-14 20:14:32 +0000
@@ -0,0 +1,110 @@
+# Copyright 2013 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for CodeReviewInlineComment{,Draft,Set}"""
+
+__metaclass__ = type
+
+from datetime import datetime
+
+from pytz import UTC
+from zope.component import getUtility
+
+from lp.code.interfaces.codereviewinlinecomment import (
+ ICodeReviewInlineCommentSet,
+ )
+from lp.testing import TestCaseWithFactory
+from lp.testing.layers import LaunchpadFunctionalLayer
+
+
+class TestCodeReviewInlineComment(TestCaseWithFactory):
+ layer = LaunchpadFunctionalLayer
+
+ def _makeCRICD(self):
+ previewdiff = self.factory.makePreviewDiff()
+ person = self.factory.makePerson()
+ self.factory.makeCodeReviewInlineCommentDraft(
+ previewdiff=previewdiff, person=person)
+ return previewdiff, person
+
+ def test_ensure_creates(self):
+ # ICodeReviewInlineCommentSet.ensureDraft() will create one if it
+ # does not exist.
+ interface = getUtility(ICodeReviewInlineCommentSet)
+ previewdiff, person = self._makeCRICD()
+ self.assertEqual(
+ {'2': 'foobar'}, interface.findDraft(previewdiff, person))
+
+ def test_ensure_deletes(self):
+ # ICodeReviewInlineCommentSet.ensureDraft() will delete a draft if
+ # no comments are provided.
+ interface = getUtility(ICodeReviewInlineCommentSet)
+ previewdiff, person = self._makeCRICD()
+ interface.ensureDraft(previewdiff, person, {})
+ self.assertEqual({}, interface.findDraft(previewdiff, person))
+
+ def test_ensure_deletes_with_no_draft(self):
+ # ICodeReviewInlineCommentSet.ensureDraft() will cope with a draft
+ # that does not exist when called with no comments.
+ interface = getUtility(ICodeReviewInlineCommentSet)
+ previewdiff = self.factory.makePreviewDiff()
+ person = self.factory.makePerson()
+ interface.ensureDraft(previewdiff, person, {})
+ self.assertEqual({}, interface.findDraft(previewdiff, person))
+
+ def test_ensure_updates(self):
+ # ICodeReviewInlineCommentSet.ensureDraft() will update the draft when
+ # the comments change.
+ interface = getUtility(ICodeReviewInlineCommentSet)
+ previewdiff, person = self._makeCRICD()
+ interface.ensureDraft(previewdiff, person, {'1': 'bar'})
+ comment = interface.findDraft(previewdiff, person)
+ self.assertEqual({'1': 'bar'}, comment)
+
+ def test_publishDraft(self):
+ # ICodeReviewInlineCommentSet.publishDraft() will publish draft
+ # comments.
+ interface = getUtility(ICodeReviewInlineCommentSet)
+ previewdiff, person = self._makeCRICD()
+ comment = self.factory.makeCodeReviewComment(
+ merge_proposal=previewdiff.branch_merge_proposal, sender=person)
+ cric = interface.publishDraft(previewdiff, person, comment)
+ self.assertIsNot(None, cric)
+ self.assertEqual({}, interface.findDraft(previewdiff, person))
+
+ def test_publishDraft_without_comments(self):
+ # ICodeReviewInlineCommentSet.publishDraft() will not choke if
+ # there are no draft comments to publish.
+ comment = self.factory.makeCodeReviewComment()
+ previewdiff = self.factory.makePreviewDiff(
+ merge_proposal=comment.branch_merge_proposal)
+ cric = getUtility(ICodeReviewInlineCommentSet).publishDraft(
+ previewdiff, comment.author, comment)
+ self.assertIs(None, cric)
+
+ def test_findByPreviewDiff_draft(self):
+ # ICodeReviewInlineCommentSet.findByPreviewDiff() will return a draft
+ # so it can be rendered.
+ interface = getUtility(ICodeReviewInlineCommentSet)
+ previewdiff, person = self._makeCRICD()
+ results = interface.findByPreviewDiff(previewdiff, person)
+ self.assertEqual([[u'2', None, u'foobar', None]], results)
+
+ def test_findByPreviewDiff_sorted(self):
+ # ICodeReviewInlineCommentSet.findByPreviewDiff() will return a sorted
+ # list.
+ interface = getUtility(ICodeReviewInlineCommentSet)
+ previewdiff = self.factory.makePreviewDiff()
+ person = self.factory.makePerson()
+ comment = self.factory.makeCodeReviewComment()
+ self.factory.makeCodeReviewInlineComment(
+ previewdiff=previewdiff, person=person, comment=comment)
+ old_comment = self.factory.makeCodeReviewComment(
+ date_created=datetime(2001, 1, 1, 12, tzinfo=UTC))
+ self.factory.makeCodeReviewInlineComment(
+ previewdiff=previewdiff, person=person, comment=old_comment,
+ comments={'8': 'baz'})
+ self.assertEqual(
+ [[u'8', person, u'baz', old_comment.date_created],
+ [u'2', person, u'foobar', comment.date_created]],
+ interface.findByPreviewDiff(previewdiff, person))
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt 2013-09-05 03:57:28 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt 2014-01-14 20:14:32 +0000
@@ -144,6 +144,7 @@
<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>
@@ -193,7 +194,6 @@
replace="structure
string:<script id='codereview-script' type='text/javascript'>" />
conf = <tal:status-config replace="view/status_config" />
- <!--
LPJS.use('io-base', 'lp.code.branchmergeproposal.reviewcomment',
'lp.code.branchmergeproposal.status', 'lp.app.comment',
'lp.code.branchmergeproposal.updater', 'lp.app.widgets.expander',
@@ -206,6 +206,23 @@
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 ns = Y.lp.code.branchmergeproposal.inlinecomments;
+ var config = {
+ on: {
+ success: function (r) {
+ console.log(r);
+ console.log(LP.cache.published_inline_comments);
+ LP.cache.published_inline_comments = r;
+ ns.populate_existing_comments();
+ },
+ },
+ };
+ lp_client.named_get(
+ LP.cache.context.self_link, 'getInlineComments', config);
+ }
Y.lp.code.branchmergeproposal.status.connect_status(conf);
}
Y.lp.code.branchmergeproposal.reviewcomment.connect_links();
@@ -241,7 +258,6 @@
Y.lp.code.branchmergeproposal.inlinecomments.setup_inline_comments();
});
});
- -->
<tal:script replace="structure string:</script>" />
</div>
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2013-11-30 00:12:15 +0000
+++ lib/lp/testing/factory.py 2014-01-14 20:14:32 +0000
@@ -116,6 +116,9 @@
from lp.code.interfaces.codeimportevent import ICodeImportEventSet
from lp.code.interfaces.codeimportmachine import ICodeImportMachineSet
from lp.code.interfaces.codeimportresult import ICodeImportResultSet
+from lp.code.interfaces.codereviewinlinecomment import (
+ ICodeReviewInlineCommentSet,
+ )
from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
from lp.code.interfaces.revision import IRevisionSet
from lp.code.interfaces.sourcepackagerecipe import (
@@ -4316,6 +4319,27 @@
product.redeemSubscriptionVoucher(
self.getUniqueString(), person, person, months)
+ def makeCodeReviewInlineCommentDraft(self, previewdiff=None, person=None,
+ comments={'2': 'foobar'}):
+ if previewdiff is None:
+ previewdiff = self.makePreviewDiff()
+ if person is None:
+ person = self.makePerson()
+ getUtility(ICodeReviewInlineCommentSet).ensureDraft(
+ previewdiff, person, comments)
+
+ def makeCodeReviewInlineComment(self, previewdiff=None, person=None,
+ comment=None, comments={'2': 'foobar'}):
+ if previewdiff is None:
+ previewdiff = self.makePreviewDiff()
+ if person is None:
+ person = self.makePerson()
+ if comment is None:
+ comment = self.makeCodeReviewComment()
+ self.makeCodeReviewInlineCommentDraft(previewdiff, person, comments)
+ return getUtility(ICodeReviewInlineCommentSet).publishDraft(
+ previewdiff, person, comment)
+
# Some factory methods return simple Python types. We don't add
# security wrappers for them, as well as for objects created by
Follow ups
