launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

I'd be tempted to put the property in the model for simplicity, but this works.

Diff comments:

> === 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

Could this just inherit BranchMergeProposalRevisionIdMixin?

> +
>  
>  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"
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-merged-revno/+merge/261258
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.