launchpad-reviewers team mailing list archive

[Jürgen Gmach <mp+491799@xxxxxxxxxxxxxxxxxx>]

Although we have mixed behavior whether to include the date or not for "since" usage in the API, please update the behavior to include the date for "created_since" to choose the most logical approach.

Otherwise, this is great work. I left some comments for you to consider.

Diff comments:

> diff --git a/lib/lp/code/model/branchcollection.py b/lib/lp/code/model/branchcollection.py
> index 848383d..0e1704f 100644
> --- a/lib/lp/code/model/branchcollection.py
> +++ b/lib/lp/code/model/branchcollection.py
> @@ -400,6 +404,8 @@ class GenericBranchCollection:
>                  prerequisite_branch,
>                  merged_revnos,
>                  merged_revision,
> +                created_before=created_before,
> +                created_since=created_since,

Adding parameters in the middle of the list of parameters could break the callers, if they do not specify the parameter name.

>                  eager_load=eager_load,
>              )
>          else:
> diff --git a/lib/lp/code/model/tests/test_hasbranches.py b/lib/lp/code/model/tests/test_hasbranches.py
> index 64bfdbd..557e168 100644
> --- a/lib/lp/code/model/tests/test_hasbranches.py
> +++ b/lib/lp/code/model/tests/test_hasbranches.py
> @@ -56,7 +56,9 @@ class TestHasMergeProposalsWebservice(TestCaseWithFactory):
>          owner = self.factory.makePerson()
>          owner_url = api_url(owner)
>          webservice = webservice_for_person(
> -            owner, permission=OAuthPermission.READ_PRIVATE
> +            owner,
> +            permission=OAuthPermission.READ_PRIVATE,
> +            default_api_version="devel",

An inline comment with the reason why "devel" needs to be set would be useful.

>          )
>  
>          def create_merge_proposals():
> diff --git a/lib/lp/registry/interfaces/person.py b/lib/lp/registry/interfaces/person.py
> index a3cb45b..81c0164 100644
> --- a/lib/lp/registry/interfaces/person.py
> +++ b/lib/lp/registry/interfaces/person.py
> @@ -1930,6 +1931,40 @@ class IPersonViewRestricted(
>          If no orderby is provided, Person.sortingColumns is used.
>          """
>  
> +    @operation_parameters(
> +        status=List(
> +            title=_("A list of merge proposal statuses to filter by."),

This sentence sounds as it is not finished, as if there is missing something at the end.

> +            value_type=Choice(vocabulary=BranchMergeProposalStatus),
> +        ),
> +        created_before=Datetime(
> +            title=_(
> +                "Search for merge proposals that were created"
> +                "before the given date."
> +            ),
> +            required=False,
> +        ),
> +        created_since=Datetime(
> +            title=_(
> +                "Search for merge proposals that were created"
> +                "since the given date."
> +            ),
> +            required=False,
> +        ),
> +    )
> +    @call_with(visible_by_user=REQUEST_USER, eager_load=True)
> +    # Really IBranchMergeProposal
> +    @operation_returns_collection_of(Interface)
> +    @export_read_operation()
> +    @operation_for_version("devel")
> +    def getMergeProposals(
> +        status=None,
> +        visible_by_user=None,
> +        created_before=None,
> +        created_since=None,
> +        eager_load=False,
> +    ):
> +        """Return matching BranchMergeProposals."""
> +
>  
>  class IPersonEditRestricted(Interface):
>      """IPerson attributes that require launchpad.Edit permission."""
> diff --git a/lib/lp/registry/tests/test_person.py b/lib/lp/registry/tests/test_person.py
> index 2d60691..b4f8457 100644
> --- a/lib/lp/registry/tests/test_person.py
> +++ b/lib/lp/registry/tests/test_person.py
> @@ -908,6 +908,48 @@ class TestPerson(TestCaseWithFactory):
>          )
>  
>  
> +class TestPersonMergeProposals(TestCaseWithFactory):
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        super().setUp()
> +
> +    def _makeOwnedBranchMergeProposal(self):
> +        self.user = self.factory.makePerson()
> +        branch = self.factory.makeBranch(owner=self.user)
> +        self.today = datetime.now(timezone.utc)
> +        ten_days_ago = self.factory.makeBranchMergeProposal(
> +            registrant=self.user,
> +            source_branch=branch,
> +            date_created=self.today - timedelta(days=10),
> +        )
> +        nine_days_ago = self.factory.makeBranchMergeProposal(
> +            registrant=self.user,
> +            source_branch=branch,
> +            date_created=self.today - timedelta(days=9),
> +        )
> +        self.allMergeProposals = [nine_days_ago, ten_days_ago]
> +
> +    def test_all_merge_proposals(self):
> +        self._makeOwnedBranchMergeProposal()
> +        mp = self.user.getMergeProposals()
> +        self.assertEqual(self.allMergeProposals, list(mp))
> +
> +    def test_created_before(self):
> +        self._makeOwnedBranchMergeProposal()
> +        mp = self.user.getMergeProposals(
> +            created_before=self.today - timedelta(days=9)
> +        )
> +        self.assertEqual(self.allMergeProposals[1:], list(mp))
> +
> +    def test_created_since(self):

I understand and welcome that you sticked to the tests already existing, but I do not think they are following best practice.

I would love to have more self-explaining tests, where e.g. you have a helper function to create an MP where you just pass the date to it, and so everything is clearly visible in the scope of the test, and you do not need to dig into several functions.

> +        self._makeOwnedBranchMergeProposal()
> +        mp = self.user.getMergeProposals(
> +            created_since=self.today - timedelta(days=10)
> +        )
> +        self.assertEqual(self.allMergeProposals[:1], list(mp))
> +
> +
>  class TestPersonStates(TestCaseWithFactory):
>      layer = DatabaseFunctionalLayer
>  


-- 
https://code.launchpad.net/~finnrg/launchpad/+git/launchpad/+merge/491799
Your team Launchpad code reviewers is requested to review the proposed merge of ~finnrg/launchpad:feat/LP-2653-getMergeProposals-date-filtering into launchpad:master.