launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18727
[Merge] lp:~cjwatson/launchpad/git-merged-revno into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/git-merged-revno into lp:launchpad.
Commit message:
Fix merge revision handling for Git.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1462436 in Launchpad itself: "Unable to set "merged at revision" using launchpad UI for git merge requests"
https://bugs.launchpad.net/launchpad/+bug/1462436
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-merged-revno/+merge/261258
Fix merge revision handling for Git. At the moment it successfully saves the revision ID you give, but the UI has some wonky text and doesn't know how to display the saved revision ID.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-merged-revno into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py 2015-05-13 16:56:06 +0000
+++ lib/lp/code/browser/branchmergeproposal.py 2015-06-05 18:00:45 +0000
@@ -207,6 +207,14 @@
formatter.displaydate())
return result
+ @property
+ def merged_revision(self):
+ """Return the merged revision identifier."""
+ if IBranch.providedBy(self.context.merge_target):
+ return self.context.merged_revno
+ else:
+ return self.context.merged_revision_id
+
class BranchMergeProposalMenuMixin:
"""Mixin class for merge proposal menus."""
@@ -276,7 +284,10 @@
@enabled_with_permission('launchpad.Edit')
def update_merge_revno(self):
- text = 'Update revision number'
+ if IBranch.providedBy(self.context.merge_target):
+ text = 'Update revision number'
+ else:
+ text = 'Update revision ID'
return Link('+merged', text)
@enabled_with_permission('launchpad.Edit')
@@ -396,6 +407,14 @@
return self._getRevisionNumberForRevisionId(
self.context.reviewed_revision_id)
+ @property
+ def merged_revision(self):
+ """Return the merged revision identifier."""
+ if IBranch.providedBy(self.context.merge_target):
+ return self.context.merged_revno
+ else:
+ return self.context.merged_revision_id
+
class BranchMergeProposalNavigation(Navigation):
"""Navigation from BranchMergeProposal to CodeReviewComment views."""
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2015-05-14 13:57:51 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2015-06-05 18:00:45 +0000
@@ -69,6 +69,7 @@
feature_flags,
login_person,
monkey_patch,
+ normalize_whitespace,
person_logged_in,
set_feature_flag,
TestCaseWithFactory,
@@ -79,7 +80,11 @@
DatabaseFunctionalLayer,
LaunchpadFunctionalLayer,
)
-from lp.testing.pages import find_tag_by_id
+from lp.testing.pages import (
+ extract_text,
+ find_tag_by_id,
+ get_feedback_messages,
+ )
from lp.testing.views import create_initialized_view
@@ -110,49 +115,78 @@
self.assertTrue(d.can_change_review)
-class TestBranchMergeProposalMergedViewBzr(TestCaseWithFactory):
+class TestBranchMergeProposalMergedViewMixin:
+ """Tests for `BranchMergeProposalMergedView`."""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_initial_values(self):
+ # The default merged_revno is the head revno of the target branch.
+ bmp = self.makeBranchMergeProposal()
+ login_person(bmp.registrant)
+ view = BranchMergeProposalMergedView(bmp, LaunchpadTestRequest())
+ self.setBranchRevision(bmp.merge_source, self.arbitrary_revisions[0])
+ self.setBranchRevision(bmp.merge_target, self.arbitrary_revisions[1])
+ self.assertEqual(
+ self.getBranchRevisionValues(bmp.merge_target),
+ view.initial_values)
+
+ def test_change_revision(self):
+ bmp = self.makeBranchMergeProposal()
+ login_person(bmp.registrant)
+ target_identity = bmp.merge_target.identity
+ bmp.markAsMerged(merge_reporter=bmp.registrant)
+ browser = self.getViewBrowser(bmp, '+merged', user=bmp.registrant)
+ browser.getControl(self.merged_revision_text).value = str(
+ self.arbitrary_revisions[2])
+ browser.getControl('Mark as Merged').click()
+ self.assertEqual(
+ ["The proposal's merged revision has been updated."],
+ get_feedback_messages(browser.contents))
+ self.assertIn(
+ 'Status:\nMerged\nMerged at revision:\n%s' % (
+ self.arbitrary_revisions[2]),
+ extract_text(find_tag_by_id(browser.contents, 'proposal-summary')))
+ browser = self.getViewBrowser(bmp.merge_source, '+index')
+ self.assertIn(
+ 'Merged into %s at revision %s' % (
+ target_identity, self.arbitrary_revisions[2]),
+ normalize_whitespace(extract_text(find_tag_by_id(
+ browser.contents, 'landing-targets'))))
+
+
+class TestBranchMergeProposalMergedViewBzr(
+ TestBranchMergeProposalMergedViewMixin, BrowserTestCase):
"""Tests for `BranchMergeProposalMergedView` for Bazaar."""
- layer = DatabaseFunctionalLayer
-
- def setUp(self):
- # Use an admin so we don't have to worry about launchpad.Edit
- # permissions on the merge proposals for adding comments, or
- # nominating reviewers.
- TestCaseWithFactory.setUp(self, user="admin@xxxxxxxxxxxxx")
- self.bmp = self.factory.makeBranchMergeProposal()
-
- def test_initial_values(self):
- # The default merged_revno is the head revno of the target branch.
- view = BranchMergeProposalMergedView(self.bmp, LaunchpadTestRequest())
- self.bmp.source_branch.revision_count = 1
- self.bmp.target_branch.revision_count = 2
- self.assertEqual(
- {'merged_revno': self.bmp.target_branch.revision_count},
- view.initial_values)
-
-
-class TestBranchMergeProposalMergedViewGit(TestCaseWithFactory):
+ arbitrary_revisions = (1, 2, 42)
+ merged_revision_text = 'Merged Revision Number'
+
+ def makeBranchMergeProposal(self):
+ return self.factory.makeBranchMergeProposal()
+
+ def setBranchRevision(self, branch, revision):
+ removeSecurityProxy(branch).revision_count = revision
+
+ def getBranchRevisionValues(self, branch):
+ return {'merged_revno': branch.revision_count}
+
+
+class TestBranchMergeProposalMergedViewGit(
+ TestBranchMergeProposalMergedViewMixin, BrowserTestCase):
"""Tests for `BranchMergeProposalMergedView` for Git."""
- layer = DatabaseFunctionalLayer
-
- def setUp(self):
- # Use an admin so we don't have to worry about launchpad.Edit
- # permissions on the merge proposals for adding comments, or
- # nominating reviewers.
- TestCaseWithFactory.setUp(self, user="admin@xxxxxxxxxxxxx")
- self.bmp = self.factory.makeBranchMergeProposalForGit()
-
- def test_initial_values(self):
- # The default merged_revision_id is the head commit SHA-1 of the
- # target ref.
- view = BranchMergeProposalMergedView(self.bmp, LaunchpadTestRequest())
- removeSecurityProxy(self.bmp.source_git_ref).commit_sha1 = "0" * 40
- removeSecurityProxy(self.bmp.target_git_ref).commit_sha1 = "1" * 40
- self.assertEqual(
- {'merged_revision_id': self.bmp.target_git_ref.commit_sha1},
- view.initial_values)
+ arbitrary_revisions = ("0" * 40, "1" * 40, "2" * 40)
+ merged_revision_text = 'Merged Revision ID'
+
+ def makeBranchMergeProposal(self):
+ return self.factory.makeBranchMergeProposalForGit()
+
+ def setBranchRevision(self, branch, revision):
+ removeSecurityProxy(branch).commit_sha1 = revision
+
+ def getBranchRevisionValues(self, branch):
+ return {'merged_revision_id': branch.commit_sha1}
class TestBranchMergeProposalAddVoteView(TestCaseWithFactory):
=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
--- lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2015-06-03 10:06:20 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2015-06-05 18:00:45 +0000
@@ -260,67 +260,6 @@
HWDB Team claimable ... ago Pending ...
-Marking as merged
------------------
-
-When a branch has been merged into the target branch, the proposal should
-be marked as merged.
-
- >>> nopriv_browser.open(klingon_proposal)
-
-The edit icon at the end of the status allows the user to edit the status.
-
- >>> nopriv_browser.getLink('Edit status').click()
-
-When marking a proposal as merged there is an optional revision number.
-If this is set then the when setting the merge proposal as merged the
-system looks for the revision in the history of the target branch,
-and if found uses the revision date as the merged date. If it cannot
-find one, then the current time is used as the date merged.
-
- >>> nopriv_browser.getControl(name='field.queue_status').displayValue = (
- ... ['Merged'])
- >>> nopriv_browser.getControl('Change Status').click()
-
-The proposal is now shown as have being merged.
-
- >>> print_summary(nopriv_browser)
- Status: Merged
- ...
-
-If the user subsequently wants to set the merged revision number,
-this can be updated by selecting the edit icon shown in the table
-next to the heading "Merged at revision".
-
- >>> sample_browser.open(
- ... 'http://code.launchpad.dev/~name12/gnome-terminal/klingon')
- >>> print extract_text(find_tag_by_id(
- ... sample_browser.contents, 'landing-targets'))
- Merged into lp://dev/~name12/gnome-terminal/main
- ...
- >>> sample_browser.getLink('Merged').click()
- >>> print_summary(sample_browser)
- Status: Merged
- ...
- Merged at revision: not available
- ...
- >>> sample_browser.getLink(url='+merged').click()
- >>> sample_browser.getControl('Merged Revision Number').value = '42'
- >>> sample_browser.getControl('Mark as Merged').click()
-
- >>> print_feedback_messages(sample_browser.contents)
- The proposal's merged revision has been updated.
- >>> print_summary(sample_browser)
- Status: Merged
- Merged at revision: 42
- ...
- >>> sample_browser.getLink('~name12/gnome-terminal/klingon').click()
- >>> print extract_text(find_tag_by_id(
- ... sample_browser.contents, 'landing-targets'))
- Merged into lp://dev/~name12/gnome-terminal/main at revision 42
- ...
-
-
Resubmitting proposals
----------------------
=== modified file 'lib/lp/code/templates/branch-macros.pt'
--- lib/lp/code/templates/branch-macros.pt 2013-02-21 05:43:21 +0000
+++ lib/lp/code/templates/branch-macros.pt 2015-06-05 18:00:45 +0000
@@ -65,9 +65,16 @@
<tal:merged replace="mergeproposal/date_merged/fmt:approximatedate">
2007-06-04
</tal:merged>
- <tal:have-revno condition="mergeproposal/merged_revno">
- at revision <tal:revno replace="mergeproposal/merged_revno"/>
- </tal:have-revno>
+ <tal:bzr condition="mergeproposal/target_branch">
+ <tal:have-revno condition="mergeproposal/merged_revno">
+ at revision <tal:revno replace="mergeproposal/merged_revno"/>
+ </tal:have-revno>
+ </tal:bzr>
+ <tal:git condition="mergeproposal/target_git_repository">
+ <tal:have-revision condition="mergeproposal/merged_revision_id">
+ at revision <tal:revision replace="mergeproposal/merged_revision_id"/>
+ </tal:have-revision>
+ </tal:git>
</div>
</tal:show-status>
<tal:show-links condition="show_associations|nothing">
=== modified file 'lib/lp/code/templates/branchmergeproposal-link-summary.pt'
--- lib/lp/code/templates/branchmergeproposal-link-summary.pt 2015-04-21 16:53:45 +0000
+++ lib/lp/code/templates/branchmergeproposal-link-summary.pt 2015-06-05 18:00:45 +0000
@@ -17,8 +17,8 @@
tal:content="branch/identity">lp:product/branch-name</a>
</tal:source-branch>
<tal:for-merging condition="context/queue_status/enumvalue:MERGED">
- <tal:have-revno condition="context/merged_revno">
- at revision <tal:revno replace="context/merged_revno"/>
- </tal:have-revno>
+ <tal:have-revision condition="view/merged_revision">
+ at revision <tal:revision replace="view/merged_revision"/>
+ </tal:have-revision>
</tal:for-merging>
</tal:root>
=== modified file 'lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt'
--- lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt 2015-04-22 16:11:40 +0000
+++ lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt 2015-06-05 18:00:45 +0000
@@ -86,11 +86,11 @@
<tr id="summary-row-7-merged-revision">
<th>Merged at revision:</th>
<td>
- <tal:not-available condition="not: context/merged_revno">
+ <tal:not-available condition="not: view/merged_revision">
not available
</tal:not-available>
- <tal:revision condition="context/merged_revno"
- content="context/merged_revno">
+ <tal:revision condition="view/merged_revision"
+ content="view/merged_revision">
1234
</tal:revision>
<a tal:define="link context_menu/update_merge_revno"
Follow ups
