launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #16445
[Merge] lp:~cprov/launchpad/diff-navigator into lp:launchpad
Celso Providelo has proposed merging lp:~cprov/launchpad/diff-navigator into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
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 requested to review the proposed merge of lp:~cprov/launchpad/diff-navigator into lp:launchpad.
=== 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-11 04:29:38 +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):
=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml 2012-10-08 01:59:53 +0000
+++ lib/lp/code/browser/configure.zcml 2014-02-11 04:29:38 +0000
@@ -922,7 +922,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-11 04:29:38 +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(
@@ -584,8 +588,9 @@
def getInlineComments(diff_timestamp):
"""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`.
"""
@@ -596,15 +601,22 @@
@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.
+ """Return the draft 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 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_timestamp: The timestamp 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(),
=== 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-11 04:29:38 +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/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2014-01-17 16:10:34 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2014-02-11 04:29:38 +0000
@@ -918,6 +918,14 @@
return getUtility(ICodeReviewInlineCommentSet).getDraft(
previewdiff, person)
+ 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_timestamp, person, comments):
"""See `IBranchMergeProposal`."""
if not getFeatureFlag("code.inline_diff_comments.enabled"):
=== 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-11 04:29:38 +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-11 04:29:38 +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/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-11 04:29:38 +0000
@@ -184,6 +184,15 @@
</div>
<div id="review-diff" tal:condition="view/preview_diff">
<h2>Preview Diff </h2>
+
+ <select id="available_diffs_choice" name="available_diffs_choice">
+ <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>
+
<div class="diff-content">
<div tal:replace="structure context/@@++diff"/>
</div>
@@ -197,7 +206,7 @@
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) {
Y.on('load', function() {
@@ -260,6 +269,31 @@
false,
Y.lp.code.branch.revisionexpander.bmp_diff_loader);
Y.lp.code.branchmergeproposal.inlinecomments.setup_inline_comments();
+
+ // Evil hack ...
+ var diff_choice = Y.one('#available_diffs_choice');
+ diff_choice.on('click', function() {
+ var lp_client = new Y.lp.client.Launchpad();
+ var diff_node = Y.one('.diff-content');
+ var config = {
+ on: {
+ success: function (diff_text) {
+ diff_node.set('innerHTML', diff_text);
+ var rc = Y.lp.code.branchmergeproposal.reviewcomment;
+ (new rc.NumberToggle()).render();
+ },
+ failure: function(ignore, response, args) {
+ diff_node.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_choice.get('value') + '/+diff');
+ lp_client.get(mp_uri + diff_path, config);
+ });
+
});
});
<tal:script replace="structure string:</script>" />
Follow ups
