launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'database/schema/security.cfg'
> --- database/schema/security.cfg	2016-06-14 12:40:30 +0000
> +++ database/schema/security.cfg	2016-06-25 09:46:10 +0000
> @@ -2082,6 +2084,7 @@
>  public.teammembership                   = SELECT
>  public.teamparticipation                = SELECT
>  public.validpersoncache                 = SELECT
> +public.xref                             = SELECT

send-branch-mail doesn't presently have access to bugbranch, so it seems weird that it would require xref without further changes.

>  type=user
>  
>  [reclaim-branch-space]
> 
> === modified file 'lib/lp/bugs/interfaces/bug.py'
> --- lib/lp/bugs/interfaces/bug.py	2016-02-04 12:21:45 +0000
> +++ lib/lp/bugs/interfaces/bug.py	2016-06-25 09:46:10 +0000
> @@ -1009,8 +1016,23 @@
>      @export_read_operation()
>      @operation_for_version('beta')
>      def getVisibleLinkedBranches(user):
> -        """Return the branches linked to this bug that are visible by
> -        `user`."""
> +        """Return all the branches linked to the bug that `user` can see."""
> +
> +    linked_merge_proposals = exported(
> +        CollectionField(
> +            title=_("Merge proposals associated with this bug."),

Should we document that Bazaar is weird?

> +            # Really IBranchMergeProposal, patched in
> +            # _schema_circular_imports.py.
> +            value_type=Reference(schema=Interface),
> +            readonly=True),
> +        as_of='devel')
> +
> +    @accessor_for(linked_merge_proposals)
> +    @call_with(user=REQUEST_USER)
> +    @export_read_operation()
> +    @operation_for_version('devel')
> +    def getVisibleLinkedMergeProposals(user):
> +        """Return all the MPs linked to the bug that `user` can see."""
>  
>  
>  # We are forced to define these now to avoid circular import problems.
> 
> === modified file 'lib/lp/bugs/model/bugtasksearch.py'
> --- lib/lp/bugs/model/bugtasksearch.py	2015-09-28 12:36:14 +0000
> +++ lib/lp/bugs/model/bugtasksearch.py	2016-06-25 09:46:10 +0000
> @@ -671,16 +671,38 @@
>                      BugBranch.branchID, branches))
>          return Exists(Select(1, tables=[BugBranch], where=And(*where)))
>  
> +    def make_merge_proposal_clause(merge_proposals=None):
> +        where = [
> +            XRef.from_type == u'bug',
> +            XRef.from_id_int == BugTaskFlat.bug_id,
> +            XRef.to_type == u'merge_proposal',
> +            ]
> +        if merge_proposals is not None:
> +            where.append(
> +                search_value_to_storm_where_condition(
> +                    XRef.to_id_int, merge_proposals))
> +        return Exists(Select(1, tables=[XRef], where=And(*where)))
> +
>      if zope_isinstance(params.linked_branches, BaseItem):
>          if params.linked_branches == BugBranchSearch.BUGS_WITH_BRANCHES:
> -            extra_clauses.append(make_branch_clause())
> +            extra_clauses.append(
> +                Or(make_branch_clause(), make_merge_proposal_clause()))
>          elif (params.linked_branches ==
>                  BugBranchSearch.BUGS_WITHOUT_BRANCHES):
>              extra_clauses.append(Not(make_branch_clause()))
> +            extra_clauses.append(Not(make_merge_proposal_clause()))
>      elif zope_isinstance(params.linked_branches, (any, all, int)):
> -        # A specific search term has been supplied.
> +        # A specific search term has been supplied.  Note that this only
> +        # works with branches, not merge proposals, as it takes integer
> +        # branch IDs.
>          extra_clauses.append(make_branch_clause(params.linked_branches))

This section really deserves a comment describing that linked_branches with BUGS_WITH(OUT)_BRANCHES will also find things with linked Git merge proposals.

This also has potential to break some API clients, if they assume that they can get at least one branch from every task found by a linked_branches=BUGS_WITH_BRANCHES search, but I think the change makes sense regardless.

>  
> +    if zope_isinstance(params.linked_merge_proposals, (any, all, int)):
> +        # This is normally only used internally by
> +        # BranchMergeProposal.getRelatedBugTasks.
> +        extra_clauses.append(
> +            make_merge_proposal_clause(params.linked_merge_proposals))

Should the guard just be "is not None"? Otherwise passing in something weird will just result in the criterion being ignored.

> +
>      linked_blueprints_clause = _build_blueprint_related_clause(params)
>      if linked_blueprints_clause is not None:
>          extra_clauses.append(linked_blueprints_clause)
> 
> === modified file 'lib/lp/code/model/branchmergeproposal.py'
> --- lib/lp/code/model/branchmergeproposal.py	2016-06-25 09:46:10 +0000
> +++ lib/lp/code/model/branchmergeproposal.py	2016-06-25 09:46:10 +0000
> @@ -399,8 +424,9 @@
>              # this would require a complicated data migration.
>              return self.source_branch.unlinkBug(bug, user)
>          else:
> -            # XXX cjwatson 2016-06-24: Implement for Git.
> -            raise NotImplementedError
> +            # Otherwise, link the bug to the merge proposal directly.

unlink from

> +            return super(BranchMergeProposal, self).unlinkBug(
> +                bug, user=user, check_permissions=check_permissions)
>  
>      @property
>      def address(self):
> 
> === modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
> --- lib/lp/code/model/tests/test_branchmergeproposal.py	2016-06-25 09:46:10 +0000
> +++ lib/lp/code/model/tests/test_branchmergeproposal.py	2016-06-25 09:46:10 +0000
> @@ -1448,6 +1448,22 @@
>          self.assertEqual([], list(bmp.getRelatedBugTasks(self.user)))
>  
>  
> +class TestBranchMergeProposalBugsGit(
> +    TestBranchMergeProposalBugsMixin, TestCaseWithFactory):
> +
> +    def setUp(self):
> +        super(TestBranchMergeProposalBugsGit, self).setUp()
> +        # Disable GitRef._getLog's use of memcache; we don't need it here,
> +        # it requires more time-consuming test setup, and it makes it harder
> +        # to repeatedly run updateRelatedBugsFromSource with different log
> +        # responses.
> +        self.useFixture(FeatureFixture(
> +            {u"code.git.log.disable_memcache": u"on"}))

And so the illusion that this series was perfectly split from the start is destroyed. I'm shattered.

> +
> +    def _makeBranchMergeProposal(self):
> +        return self.factory.makeBranchMergeProposalForGit()
> +
> +
>  class TestNotifyModified(TestCaseWithFactory):
>  
>      layer = DatabaseFunctionalLayer


-- 
https://code.launchpad.net/~cjwatson/launchpad/bmp-buglinktarget-git/+merge/298366
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.