launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #16460
[Merge] lp:~cprov/launchpad/diff-navigator into lp:launchpad
Celso Providelo has proposed merging lp:~cprov/launchpad/diff-navigator into lp:launchpad.
Requested reviews:
William Grant (wgrant)
For more details, see:
https://code.launchpad.net/~cprov/launchpad/diff-navigator/+merge/205689
--
https://code.launchpad.net/~cprov/launchpad/diff-navigator/+merge/205689
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-01-17 04:10:34 +0000
+++ lib/lp/app/javascript/comment.js 2014-02-24 17:23:56 +0000
@@ -387,8 +387,7 @@
this.publish_inline_comments.get('checked')) {
var ns = Y.lp.code.branchmergeproposal.inlinecomments;
config.parameters.inline_comments = ns.inlinecomments;
- config.parameters.diff_timestamp = (
- LP.cache.preview_diff_timestamps.slice(-1)[0]);
+ config.parameters.diff_id = ns.current_diff_id;
}
this.get('lp_client').named_post(
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py 2014-01-17 17:01:53 +0000
+++ lib/lp/code/browser/branchmergeproposal.py 2014-02-24 17:23:56 +0000
@@ -461,10 +461,17 @@
except WrongBranchMergeProposal:
return None
- @stepto("+preview-diff")
- def preview_diff(self):
- """Step to the preview diff."""
- return self.context.preview_diff
+ @stepthrough("+preview-diff")
+ def traverse_preview_diff(self, id):
+ """Navigate to a PreviewDiff through its BMP."""
+ try:
+ id = int(id)
+ except ValueError:
+ return None
+ try:
+ return self.context.getPreviewDiff(id)
+ except WrongBranchMergeProposal:
+ return None
@stepthrough('+review')
def review(self, id):
@@ -631,18 +638,6 @@
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.preview_diff.date_created))
- cache.objects['draft_inline_comments'] = (
- self.context.getDraftInlineComments(
- self.preview_diff.date_created, self.user))
- else:
- cache.objects['published_inline_comments'] = []
- cache.objects['draft_inline_comments'] = []
@action('Claim', name='claim')
def claim_action(self, action, data):
=== modified file 'lib/lp/code/browser/codereviewcomment.py'
--- lib/lp/code/browser/codereviewcomment.py 2014-01-21 14:58:22 +0000
+++ lib/lp/code/browser/codereviewcomment.py 2014-02-24 17:23:56 +0000
@@ -285,17 +285,16 @@
vote = data.get('vote')
review_type = data.get('review_type')
inline_comments = {}
- diff_timestamp = None
+ diff_id = None
if (getFeatureFlag('code.inline_diff_comments.enabled') and
data.get('publish_inline_comments')):
- diff_timestamp = self.previewdiff.date_created
+ diff_id = self.previewdiff.id
inline_comments = (
- self.branch_merge_proposal.getDraftInlineComments(
- diff_timestamp))
+ self.branch_merge_proposal.getDraftInlineComments(diff_id))
comment = self.branch_merge_proposal.createComment(
self.user, subject=None, content=data['comment'],
parent=self.reply_to, vote=vote, review_type=review_type,
- diff_timestamp=diff_timestamp, inline_comments=inline_comments)
+ diff_id=diff_id, inline_comments=inline_comments)
@property
def next_url(self):
=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml 2014-02-24 06:50:46 +0000
+++ lib/lp/code/browser/configure.zcml 2014-02-24 17:23:56 +0000
@@ -165,6 +165,9 @@
name="++diff"
template="../templates/branchmergeproposal-diff.pt"/>
<browser:page
+ name="++diff-nav"
+ template="../templates/branchmergeproposal-diff-navigator.pt"/>
+ <browser:page
name="++diff-stats"
template="../templates/branchmergeproposal-diff-stats.pt"/>
<browser:page
@@ -827,7 +830,7 @@
<browser:url
for="lp.code.interfaces.diff.IPreviewDiff"
- path_expression="string:+preview-diff"
+ path_expression="string:+preview-diff/${id}"
attribute_to_parent="branch_merge_proposal"
rootsite="code" />
=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py 2014-01-18 04:52:02 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py 2014-02-24 17:23:56 +0000
@@ -174,7 +174,11 @@
next_preview_diff_job = Attribute(
'The next BranchMergeProposalJob that will update a preview diff.')
- preview_diffs = Attribute('All preview diffs for this merge proposal.')
+ preview_diffs = exported(
+ CollectionField(
+ title=_("All preview diffs for this merge proposal."),
+ value_type=Reference(schema=IPreviewDiff),
+ readonly=True))
preview_diff = exported(
Reference(
@@ -545,14 +549,14 @@
subject=Text(), content=Text(),
vote=Choice(vocabulary=CodeReviewVote), review_type=Text(),
parent=Reference(schema=Interface),
- diff_timestamp=Datetime(),
+ diff_id=Int(),
inline_comments=Dict(key_type=TextLine(), value_type=Text()))
@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,
- diff_timestamp=None, inline_comments=None):
+ diff_id=None, inline_comments=None):
"""Create an ICodeReviewComment associated with this merge proposal.
:param owner: The person who the message is from.
@@ -561,8 +565,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 diff_timestamp: the context diff creation timestamp which
- will be used to retrive the actual `PreviewDiff` register.
+ :param diff_id: the context diff creation ID which will be used to
+ retrive the actual `PreviewDiff` register.
:param inline_comments: a dictionary containing the draft inline
comments keyed by the diff line number.
"""
@@ -579,42 +583,50 @@
@export_read_operation()
@operation_parameters(
- diff_timestamp=Datetime())
+ diff_id=Int())
@operation_for_version('devel')
- def getInlineComments(diff_timestamp):
+ def getInlineComments(diff_id):
"""Return a list of inline comments related to this MP.
- The return value is a list of 4-tuples representing published and
- draft inline comments.
+ The return value is an list of dictionaries (objects), each one
+ representing a comment with 'line_number', 'person', 'text' and
+ 'date' attributes.
- :param diff_timestamp: The timestamp of the target `PreviewDiff`.
+ :param diff_id: The ID of the target `PreviewDiff`.
"""
@export_read_operation()
@operation_parameters(
- diff_timestamp=Datetime())
+ diff_id=Int())
@call_with(person=REQUEST_USER)
@operation_for_version('devel')
- def getDraftInlineComments(diff_timestamp, person):
- """Return a list of draft inline comments related to this MP.
-
- The return value is a list of 4-tuples representing published and
- draft inline comments.
-
- :param diff_timestamp: The timestamp of the target `PreviewDiff`.
+ def getDraftInlineComments(diff_id, person):
+ """Return the draft inline comments related to this MP.
+
+ The return value is a dictionary (object) where the keys are the
+ diff lines and their values are the actual draft comment created
+ by the given person.
+
+ :param diff_id: The ID of the target `PreviewDiff`.
:param person: The `IPerson` owner of the draft comments.
"""
+ def getPreviewDiff(id):
+ """Return the preview diff with the given id.
+
+ :param id: The id of the target `PreviewDiff`.
+ """
+
@export_write_operation()
@operation_parameters(
- diff_timestamp=Datetime(),
+ diff_id=Int(),
comments=Dict(key_type=TextLine(), value_type=Text()))
@call_with(person=REQUEST_USER)
@operation_for_version('devel')
- def saveDraftInlineComment(diff_timestamp, person, comments):
+ def saveDraftInlineComment(diff_id, person, comments):
"""Save `ICodeReviewInlineCommentDraft`
- :param diff_timestamp: The timestamp of the target `PreviewDiff`.
+ :param diff_id: The ID of the target `PreviewDiff`.
:param person: The `IPerson` making the comments.
:param comments: The comments.
"""
=== modified file 'lib/lp/code/interfaces/diff.py'
--- lib/lp/code/interfaces/diff.py 2013-05-08 05:08:51 +0000
+++ lib/lp/code/interfaces/diff.py 2014-02-24 17:23:56 +0000
@@ -34,6 +34,11 @@
class IDiff(Interface):
"""A diff that is stored in the Library."""
+ id = exported(
+ Int(
+ title=_('DB ID'), required=True, readonly=True,
+ description=_("The tracking number for this diff.")))
+
text = Text(
title=_('Textual contents of a diff.'), readonly=True,
description=_("The text may be cut off at a defined maximum size."))
@@ -86,7 +91,7 @@
trying to determine the effective changes of landing the source branch on
the target branch.
"""
- export_as_webservice_entry(publish_web_link=False)
+ export_as_webservice_entry()
source_revision_id = exported(
TextLine(
@@ -124,8 +129,10 @@
Interface, readonly=True,
title=_('The branch merge proposal that diff relates to.')))
- date_created = Datetime(
- title=_("When this diff was created."), readonly=True)
+ date_created = exported(
+ Datetime(
+ title=_('Date Created'), required=False, readonly=True,
+ description=_("When this diff was created.")))
stale = exported(
Bool(readonly=True, description=_(
=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-01-21 04:51:48 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-02-24 17:23:56 +0000
@@ -12,7 +12,9 @@
// Grab the namespace in order to be able to expose the connect methods.
var namespace = Y.namespace('lp.code.branchmergeproposal.inlinecomments');
-namespace.inlinecomments = {};
+namespace.inlinecomments = null;
+
+namespace.current_diff_id = null;
namespace.lp_client = new Y.lp.client.Launchpad();
@@ -46,6 +48,9 @@
handle_widget_button = function(saved, comment) {
if (saved === false) {
handling_request = false;
+ if (namespace.inlinecomments[line_number] === undefined) {
+ header_row.remove(true);
+ }
return;
}
@@ -55,10 +60,9 @@
header_row.remove(true);
content_row.remove(true);
}
- var diff_timestamp = LP.cache.preview_diff_timestamps.slice(-1)[0];
var config = {
parameters: {
- diff_timestamp: diff_timestamp,
+ diff_id: namespace.current_diff_id,
comments: namespace.inlinecomments
}
};
@@ -105,8 +109,8 @@
// the LP API (updates during the page life-span). This way
// the handling code can be unified.
if (Y.Lang.isUndefined(comment.person.get)) {
- var lp_client = new Y.lp.client.Launchpad();
- comment.person = lp_client.wrap_resource(null, comment.person);
+ comment.person = namespace.lp_client.wrap_resource(
+ null, comment.person);
}
var header_content = (
comment.person.get('display_name') +
@@ -139,29 +143,55 @@
line.next().remove();
line.remove();
});
- // Display published inline comments.
- // [{'line_number': <lineno>, 'person': <IPerson> 'text': <comment>,
- // 'date': <timestamp>}, ...]
- LP.cache.published_inline_comments.forEach( function (comment) {
- namespace.create_row(comment);
- });
- // Display draft inline comments:
- // {'<line_number>':''<comment>', ...})
- if (LP.cache.draft_inline_comments !== null) {
- var drafts = LP.cache.draft_inline_comments;
- Object.keys(drafts).forEach( function (line_number) {
- var comment = {
- 'line_number': line_number,
- 'text': drafts[line_number],
- };
- namespace.create_row(comment);
- });
- }
-
+
+ var config_published = {
+ on: {
+ success: function (comments) {
+ // Display published inline comments.
+ // [{'line_number': <lineno>, 'person': <IPerson>,
+ // 'text': <comment>, 'date': <timestamp>}, ...]
+ comments.forEach( function (comment) {
+ namespace.create_row(comment);
+ });
+ },
+ },
+ parameters: {
+ diff_id: namespace.current_diff_id,
+ }
+ };
+ namespace.lp_client.named_get(
+ LP.cache.context.self_link, 'getInlineComments', config_published);
+
+ var config_draft = {
+ on: {
+ success: function (drafts) {
+ namespace.inlinecomments = {};
+ if (drafts === null) {
+ return;
+ }
+ // Display draft inline comments:
+ // {'<line_number>':''<comment>', ...})
+ Object.keys(drafts).forEach( function (line_number) {
+ var comment = {
+ 'line_number': line_number,
+ 'text': drafts[line_number],
+ };
+ namespace.create_row(comment);
+ });
+ },
+ },
+ parameters: {
+ diff_id: namespace.current_diff_id,
+ }
+ };
+ namespace.lp_client.named_get(
+ LP.cache.context.self_link, 'getDraftInlineComments', config_draft);
};
-namespace.setup_inline_comments = function() {
+namespace.setup_inline_comments = function(diff_id) {
if (LP.cache.inline_diff_comments === true) {
+ // Store the current diff ID locally.
+ namespace.current_diff_id = diff_id;
// Add the double-click handler for each row in the diff. This needs
// to be done first since populating existing published and draft
// comments may add more rows.
@@ -170,5 +200,104 @@
}
};
+
+
+function DiffNav(config) {
+ DiffNav.superclass.constructor.apply(this, arguments);
+}
+
+Y.mix(DiffNav, {
+
+ NAME: 'diffnav',
+
+ ATTRS: {
+
+ /**
+ * The LP client to use. If none is provided, one will be
+ * created during initialization.
+ *
+ * @attribute lp_client
+ */
+ lp_client: {
+ value: null
+ },
+ }
+
+});
+
+
+Y.extend(DiffNav, Y.Widget, {
+
+ /*
+ * The initializer method that is called from the base Plugin class.
+ *
+ * @method initializer
+ * @protected
+ */
+ initializer: function(cfg){
+ // If we have not been provided with a Launchpad Client, then
+ // create one now:
+ if (null === this.get("lp_client")){
+ // Create our own instance of the LP client.
+ this.set("lp_client", new Y.lp.client.Launchpad());
+ }
+ },
+
+ _connectNavigator: function(navigator) {
+ var self = this;
+ var rc = Y.lp.code.branchmergeproposal.reviewcomment;
+ (new rc.NumberToggle()).render();
+ namespace.setup_inline_comments(navigator.get('value'));
+ navigator.on('change', function() {
+ var diff_id = this.get('value');
+ var container = self.get('srcNode').one('.diff-content');
+ var config = {
+ on: {
+ success: function (diff_text) {
+ container.set('innerHTML', diff_text);
+ (new rc.NumberToggle()).render();
+ namespace.setup_inline_comments(diff_id);
+ },
+ failure: function(ignore, response, args) {
+ container.set('innerHTML', response.statusText);
+ Y.lp.anim.red_flash({node: diff_node}).run();
+ },
+ },
+ };
+ var mp_uri = LP.cache.context.web_link;
+ var diff_path = '/+preview-diff/' + diff_id + '/+diff';
+ self.get('lp_client').get(mp_uri + diff_path, config);
+ });
+ },
+
+ renderUI: function() {
+ if (LP.cache.inline_diff_comments === true) {
+ return
+ }
+ var self = this;
+ var container = self.get('srcNode').one('.diff-navigator')
+ var config = {
+ on: {
+ success: function(content) {
+ container.set('innerHTML', content);
+ var navigator = container.one('select')
+ Y.lp.anim.green_flash({node: navigator}).run();
+ self._connectNavigator(navigator);
+ },
+ failure: function(ignore, response, args) {
+ container.set('innerHTML', response.statusText);
+ Y.lp.anim.red_flash({node: container}).run();
+ },
+ }
+ };
+ var mp_uri = LP.cache.context.web_link;
+ this.get('lp_client').get(mp_uri + "/++diff-nav", config);
+ },
+
+});
+
+namespace.DiffNav = DiffNav;
+
+
}, '0.1', {requires: ['event', 'io', 'node', 'widget', 'lp.client',
'lp.ui.editor']});
=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-01-21 04:51:48 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-02-24 17:23:56 +0000
@@ -12,10 +12,14 @@
setUp: function () {
// Loads testing values into LP.cache.
- LP.cache.published_inline_comments = [];
- LP.cache.draft_inline_comments = {};
- LP.cache.preview_diff_timestamps = ['2014-01-01 10:00'];
+ LP.cache.context = {
+ web_link: "https://launchpad.dev/~foo/bar/foobr/+merge/1";,
+ self_link: "https://code.launchpad.dev/api/devel/~foo/bar/foobr/+merge/1";,
+ };
+
LP.cache.inline_diff_comments = true;
+ module.current_diff_id = 1;
+ module.inlinecomments = {};
},
tearDown: function () {},
@@ -35,17 +39,45 @@
"resource_type_link": "http://launchpad.dev/api/devel/#person";,
};
- // Create a published and a draft inline comments.
- LP.cache.published_inline_comments = [
+ // Overrides module LP client by one using 'mockio'.
+ var mockio = new Y.lp.testing.mockio.MockIo();
+ var ns = Y.namespace(
+ 'lp.code.branchmergeproposal.inlinecomments');
+ ns.lp_client = new Y.lp.client.Launchpad({io_provider: mockio});
+
+ // Populate the page.
+ module.populate_existing_comments();
+
+ // LP was hit twice for fetching published and draft inline
+ // comments
+ Y.Assert.areEqual(2, mockio.requests.length);
+
+ // Last request was for loading draft comments, let's
+ // respond it.
+ Y.Assert.areSame(
+ "ws.op=getDraftInlineComments&diff_id=1",
+ mockio.requests[1].config.data);
+ draft_comments = {'3': 'Zoing!'};
+ mockio.success({
+ responseText: Y.JSON.stringify(draft_comments),
+ responseHeaders: {'Content-Type': 'application/json'}});
+
+ // First request was for loading published comments, let's
+ // respond it (mockio.last_request need a tweak to respond
+ // past requests)
+ Y.Assert.areSame(
+ "ws.op=getInlineComments&diff_id=1",
+ mockio.requests[0].config.data);
+ mockio.last_request = mockio.requests[0];
+ published_comments = [
{'line_number': '2',
'person': person_obj,
'text': 'This is preloaded.',
'date': '2012-08-12 17:45'},
];
- LP.cache.draft_inline_comments = {'3': 'Zoing!'};
-
- // Populate the page.
- module.populate_existing_comments();
+ mockio.success({
+ responseText: Y.JSON.stringify(published_comments),
+ responseHeaders: {'Content-Type': 'application/json'}});
// Published comment is displayed.
var header = Y.one('#diff-line-2').next();
@@ -68,7 +100,6 @@
// Overrides module LP client by one using 'mockio'.
var mockio = new Y.lp.testing.mockio.MockIo();
- LP.cache.context = {'self_link': 'mock/'};
var ns = Y.namespace(
'lp.code.branchmergeproposal.inlinecomments');
ns.lp_client = new Y.lp.client.Launchpad({io_provider: mockio});
@@ -122,8 +153,11 @@
module.add_doubleclick_handler = add_doubleclick_handler;
LP.cache.inline_diff_comments = false;
- module.setup_inline_comments();
+ module.current_diff_id = null
+ module.setup_inline_comments(1);
+
Y.Assert.isFalse(called);
+ Y.Assert.isNull(module.current_diff_id);
},
test_feature_flag: function() {
@@ -133,8 +167,11 @@
};
module.add_doubleclick_handler = add_doubleclick_handler;
- module.setup_inline_comments();
+ module.current_diff_id = null
+ module.setup_inline_comments(1);
+
Y.Assert.isTrue(called);
+ Y.Assert.areEqual(1, module.current_diff_id);
}
}));
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2014-01-17 16:10:34 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2014-02-24 17:23:56 +0000
@@ -752,7 +752,7 @@
def createComment(self, owner, subject, content=None, vote=None,
review_type=None, parent=None, _date_created=DEFAULT,
- diff_timestamp=None, inline_comments=None,
+ diff_id=None, inline_comments=None,
_notify_listeners=True):
"""See `IBranchMergeProposal`."""
#:param _date_created: The date the message was created. Provided
@@ -788,10 +788,10 @@
if getFeatureFlag("code.inline_diff_comments.enabled"):
if inline_comments:
- assert diff_timestamp is not None, (
- 'Inline comments must be associated with a previewdiff '
- 'timestamp.')
- previewdiff = self._getPreviewDiffByTimestamp(diff_timestamp)
+ assert diff_id is not None, (
+ 'Inline comments must be associated with a '
+ 'previewdiff ID.')
+ previewdiff = self.getPreviewDiff(diff_id)
getUtility(ICodeReviewInlineCommentSet).ensureDraft(
previewdiff, owner, inline_comments)
getUtility(ICodeReviewInlineCommentSet).publishDraft(
@@ -799,24 +799,6 @@
return comment
- def _getPreviewDiffByTimestamp(self, diff_timestamp):
- """Return a `PreviewDiff` created on the given timestamp.
-
- Looks for a `PreviewDiff` for this merge proposal created exactly
- on the given timestamp. If it could not be found `DiffNotFound`
- is raised.
- """
- 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))
-
- return previewdiff
-
def getUsersVoteReference(self, user, review_type=None):
"""Get the existing vote reference for the given user."""
# Lower case the review type.
@@ -906,23 +888,31 @@
code_review_message, original_email))
return code_review_message
- def getInlineComments(self, diff_timestamp):
+ def getInlineComments(self, diff_id):
"""See `IBranchMergeProposal`."""
- previewdiff = self._getPreviewDiffByTimestamp(diff_timestamp)
+ previewdiff = self.getPreviewDiff(diff_id)
return getUtility(ICodeReviewInlineCommentSet).getPublished(
previewdiff)
- def getDraftInlineComments(self, diff_timestamp, person):
+ def getDraftInlineComments(self, diff_id, person):
"""See `IBranchMergeProposal`."""
- previewdiff = self._getPreviewDiffByTimestamp(diff_timestamp)
+ previewdiff = self.getPreviewDiff(diff_id)
return getUtility(ICodeReviewInlineCommentSet).getDraft(
previewdiff, person)
- def saveDraftInlineComment(self, diff_timestamp, person, comments):
+ def getPreviewDiff(self, id):
+ """See `IBranchMergeProposal`."""
+ previewdiff = IStore(self).find(
+ PreviewDiff, PreviewDiff.id == id).one()
+ if previewdiff.branch_merge_proposal != self:
+ raise WrongBranchMergeProposal
+ return previewdiff
+
+ def saveDraftInlineComment(self, diff_id, person, comments):
"""See `IBranchMergeProposal`."""
if not getFeatureFlag("code.inline_diff_comments.enabled"):
return
- previewdiff = self._getPreviewDiffByTimestamp(diff_timestamp)
+ previewdiff = self.getPreviewDiff(diff_id)
getUtility(ICodeReviewInlineCommentSet).ensureDraft(
previewdiff, person, comments)
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py 2014-01-30 15:04:06 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py 2014-02-24 17:23:56 +0000
@@ -717,6 +717,32 @@
self.vote.id)
+class TestMergeProposalGetPreviewDiff(TestCaseWithFactory):
+ """Tests for `BranchMergeProposal.getPreviewDiff`."""
+
+ layer = LaunchpadFunctionalLayer
+
+ def setUp(self):
+ TestCaseWithFactory.setUp(self, user='foo.bar@xxxxxxxxxxxxx')
+ self.mp_one = self.factory.makeBranchMergeProposal()
+ self.mp_two = self.factory.makeBranchMergeProposal()
+ self.preview_diff = self.mp_one.updatePreviewDiff(
+ u'Some diff', u"source_id", u"target_id")
+ transaction.commit()
+
+ def test_getPreviewDiff(self):
+ """We can get a preview-diff."""
+ self.assertEqual(
+ self.preview_diff,
+ self.mp_one.getPreviewDiff(self.preview_diff.id))
+
+ def test_getPreviewDiffWrongBranchMergeProposal(self):
+ """An error is raised if the given id does not match the MP."""
+ self.assertRaises(
+ WrongBranchMergeProposal,
+ self.mp_two.getPreviewDiff, self.preview_diff.id)
+
+
class TestMergeProposalNotification(TestCaseWithFactory):
"""Test that events are created when merge proposals are manipulated"""
=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py 2012-12-26 01:32:19 +0000
+++ lib/lp/code/model/tests/test_diff.py 2014-02-24 17:23:56 +0000
@@ -359,7 +359,8 @@
# canonical_url of the merge proposal itself.
mp = self._createProposalWithPreviewDiff()
self.assertEqual(
- canonical_url(mp) + '/+preview-diff',
+ '{0}/+preview-diff/{1}'.format(
+ canonical_url(mp), mp.preview_diff.id),
canonical_url(mp.preview_diff))
def test_empty_diff(self):
=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
--- lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2013-05-09 01:26:05 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2014-02-24 17:23:56 +0000
@@ -625,15 +625,21 @@
u'Preview Diff\nDownload diff\n1\n...Fake Diff\u1010'
There is also a link to the diff URL, which is the preview diff URL plus
-"+files/preview.diff". It redirects to the file in the restricted librarian.
+"+files/preview.diff". It redirects logged in users to the file in the
+restricted librarian.
- >>> from lazr.uri import URI
>>> link = get_review_diff().find('a')
+ >>> print extract_text(link)
+ Download diff
+
>>> print link['href']
- http.../+preview-diff/+files/preview.diff
- >>> print http(
- ... """GET %s HTTP/1.1
- ... """ % URI(link['href']).path)
+ http://.../+preview-diff/.../+files/preview.diff
+
+ >>> from lazr.uri import URI
+ >>> print http(r"""
+ ... GET %s HTTP/1.1
+ ... Authorization: Basic no-priv@xxxxxxxxxxxxx:test
+ ... """ % URI(link['href']).path)
HTTP/1.1 303 See Other
...
Location: https://...restricted.../...txt?token=...
=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2013-02-05 22:56:33 +0000
+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2014-02-24 17:23:56 +0000
@@ -58,6 +58,7 @@
merged_revno: None
prerequisite_branch_link: u'http://api.launchpad.dev/devel/~...'
preview_diff_link: None
+ preview_diffs_collection_link: u'http://.../preview_diffs'
private: False
queue_position: None
queue_status: u'Needs review'
@@ -156,6 +157,7 @@
merged_revno: None
prerequisite_branch_link: None
preview_diff_link: None
+ preview_diffs_collection_link: u'http://.../preview_diffs'
private: False
queue_position: None
queue_status: u'Work in progress'
=== added file 'lib/lp/code/templates/branchmergeproposal-diff-navigator.pt'
--- lib/lp/code/templates/branchmergeproposal-diff-navigator.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/code/templates/branchmergeproposal-diff-navigator.pt 2014-02-24 17:23:56 +0000
@@ -0,0 +1,14 @@
+<tal:root
+ xmlns:tal="http://xml.zope.org/namespaces/tal";
+>
+
+ <select id="available_diffs_choice" name="available_diffs_choice"
+ tal:condition="context/preview_diffs">
+ <option tal:repeat="available_diff context/preview_diffs"
+ tal:attributes="value available_diff/id; selected
+ python:available_diff.id == view.preview_diff.id"
+ tal:content="available_diff/date_created/fmt:datetime"
+ >Diff date</option>
+ </select>
+
+</tal:root>
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt 2014-01-18 04:52:02 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt 2014-02-24 17:23:56 +0000
@@ -184,6 +184,12 @@
</div>
<div id="review-diff" tal:condition="view/preview_diff">
<h2>Preview Diff </h2>
+
+ <div class="diff-navigator">
+ <div tal:condition="features/code.inline_diff_comments.enabled"
+ tal:replace="structure context/@@++diff-nav"/>
+ </div>
+
<div class="diff-content">
<div tal:replace="structure context/@@++diff"/>
</div>
@@ -197,41 +203,28 @@
LPJS.use('io-base', 'lp.code.branchmergeproposal.reviewcomment',
'lp.code.branchmergeproposal.status', 'lp.app.comment',
'lp.code.branchmergeproposal.updater', 'lp.app.widgets.expander',
- 'lp.code.branch.revisionexpander',
+ 'lp.code.branch.revisionexpander', 'lp.anin',
'lp.code.branchmergeproposal.inlinecomments', function(Y) {
+ var ic = Y.lp.code.branchmergeproposal.inlinecomments;
+
Y.on('load', function() {
var logged_in = LP.links['me'] !== undefined;
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 diff_timestamp = (
- LP.cache.preview_diff_timestamps.slice(-1)[0]);
- var config = {
- on: {
- success: function (r) {
- LP.cache.published_inline_comments = r;
- LP.cache.draft_inline_comments = null;
- ns.populate_existing_comments();
- },
- },
- parameters: {
- diff_timestamp: diff_timestamp,
- }
- };
- lp_client.named_get(
- LP.cache.context.self_link, 'getInlineComments', config);
+ if (LP.cache.inline_diff_comments === true) {
+ // No need to re-setup diff-line click handlers because
+ // the diff content did not change.
+ ic.populate_existing_comments();
+ }
}
Y.lp.code.branchmergeproposal.status.connect_status(conf);
}
Y.lp.code.branchmergeproposal.reviewcomment.connect_links();
- (new Y.lp.code.branchmergeproposal.reviewcomment.NumberToggle()
- ).render();
+ (new ic.DiffNav({srcNode: Y.one('#review-diff')})).render();
if (Y.Lang.isValue(LP.cache.merge_proposal_event_key)) {
var updater = new Y.lp.code.branchmergeproposal.updater.UpdaterWidget(
@@ -245,11 +238,9 @@
}
});
updater.on(updater.NAME + '.updated', function() {
- (new Y.lp.code.branchmergeproposal.reviewcomment.NumberToggle()
- ).render();
+ (new ic.DiffNav({srcNode: Y.one('#review-diff')})).render();
});
}
-
}, window);
Y.on('domready', function() {
@@ -259,7 +250,6 @@
'.expander-content',
false,
Y.lp.code.branch.revisionexpander.bmp_diff_loader);
- Y.lp.code.branchmergeproposal.inlinecomments.setup_inline_comments();
});
});
<tal:script replace="structure string:</script>" />
Follow ups
