launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

Probably worth factoring the target/prereq validate bits out.

Diff comments:

> === modified file 'lib/lp/code/browser/gitref.py'
> --- lib/lp/code/browser/gitref.py	2015-04-28 16:48:22 +0000
> +++ lib/lp/code/browser/gitref.py	2015-05-03 11:42:57 +0000
> @@ -296,7 +296,16 @@
>      def validate(self, data):
>          source_ref = self.context
>          target_repository = data['target_git_repository']
> -        target_ref = target_repository.getRefByPath(data['target_git_path'])
> +        if not target_repository.isRepositoryMergeable(self.context):
> +            self.setFieldError(
> +                'target_git_repository',
> +                "This repository is not mergeable into %s." %
> +                target_repository.identity)
> +        target_path = data.get('target_git_path')
> +        if target_path:
> +            target_ref = target_repository.getRefByPath(target_path)
> +        else:
> +            target_ref = None
>          if target_ref is None:
>              self.setFieldError(
>                  'target_git_path',
> @@ -307,24 +316,22 @@
>                  'target_git_path',
>                  "The target repository and path together cannot be the same "
>                  "as the source repository and path.")
> -        elif not target_repository.isRepositoryMergeable(self.context):
> -            self.setFieldError(
> -                'target_git_repository',
> -                "This repository is not mergeable into %s." %
> -                target_repository.identity)
> -        elif (data.get('prerequisite_git_repository') is not None and
> -                data.get('prerequisite_git_path') is not None):
> +        if data.get('prerequisite_git_repository') is not None:
>              prerequisite_repository = data['prerequisite_git_repository']
> -            prerequisite_ref = prerequisite_repository.getRefByPath(
> -                data['prerequisite_git_path'])
> +            if not target_repository.isRepositoryMergeable(
> +                    prerequisite_repository):
> +                self.setFieldError(
> +                    'prerequisite_git_repository',
> +                    "This repository is not mergeable into %s." %
> +                    target_repository.identity)
> +            prerequisite_path = data.get('prerequisite_git_path')
> +            if prerequisite_path:
> +                prerequisite_ref = prerequisite_repository.getRefByPath(
> +                    prerequisite_path)
> +            else:
> +                prerequisite_ref = None

This and the target case share a lot of code. Worth factoring out?

>              if prerequisite_ref is None:
>                  self.setFieldError(
>                      'prerequisite_git_path',
>                      "The prerequisite path must be the path of a reference in "
>                      "the prerequisite repository.")
> -            elif not target_repository.isRepositoryMergeable(
> -                    prerequisite_repository):
> -                self.setFieldError(
> -                    'prerequisite_git_repository',
> -                    "This repository is not mergeable into %s." %
> -                    target_repository.identity)
> 
> === modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
> --- lib/lp/code/browser/tests/test_branchmergeproposal.py	2015-04-28 16:48:22 +0000
> +++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2015-05-03 11:42:57 +0000
> @@ -808,6 +808,72 @@
>              'form_wide_errors': []},
>              simplejson.loads(view.form_result))
>  
> +    def test_register_ajax_request_with_missing_target_git_path(self):
> +        # A missing target_git_path is a validation error.
> +        owner = self.factory.makePerson()
> +        target_branch = self._makeTargetBranch(
> +            owner=owner, information_type=InformationType.USERDATA)
> +        extra = {'HTTP_X_REQUESTED_WITH': 'XMLHttpRequest'}
> +        with person_logged_in(owner):
> +            request = LaunchpadTestRequest(
> +                method='POST', principal=owner,
> +                form={
> +                    'field.actions.register': 'Propose Merge',
> +                    'field.target_git_repository.target_git_repository':
> +                        target_branch.repository.unique_name,
> +                    },
> +                **extra)
> +            view = create_initialized_view(
> +                self.source_branch,
> +                name='+register-merge',
> +                request=request)
> +        self.assertEqual(
> +            '400 Validation', view.request.response.getStatusString())
> +        self.assertEqual(
> +            {'error_summary': 'There is 1 error.',
> +            'errors': {
> +                'field.target_git_path':
> +                    ('The target path must be the path of a reference in the '
> +                     'target repository.')},
> +            'form_wide_errors': []},
> +            simplejson.loads(view.form_result))
> +
> +    def test_register_ajax_request_with_missing_prerequisite_git_path(self):
> +        # A missing prerequisite_git_path is a validation error if
> +        # prerequisite_git_repository is present.
> +        owner = self.factory.makePerson()
> +        target_branch = self._makeTargetBranch(
> +            owner=owner, information_type=InformationType.USERDATA)
> +        prerequisite_branch = self._makeTargetBranch(
> +            owner=owner, information_type=InformationType.USERDATA)
> +        extra = {'HTTP_X_REQUESTED_WITH': 'XMLHttpRequest'}
> +        with person_logged_in(owner):
> +            request = LaunchpadTestRequest(
> +                method='POST', principal=owner,
> +                form={
> +                    'field.actions.register': 'Propose Merge',
> +                    'field.target_git_repository.target_git_repository':
> +                        target_branch.repository.unique_name,
> +                    'field.target_git_path': target_branch.path,
> +                    'field.prerequisite_git_repository':
> +                        prerequisite_branch.repository.unique_name,
> +                    },
> +                **extra)
> +            view = create_initialized_view(
> +                self.source_branch,
> +                name='+register-merge',
> +                request=request)
> +        self.assertEqual(
> +            '400 Validation', view.request.response.getStatusString())
> +        self.assertEqual(
> +            {'error_summary': 'There is 1 error.',
> +            'errors': {
> +                'field.prerequisite_git_path':
> +                    ('The prerequisite path must be the path of a reference '
> +                     'in the prerequisite repository.')},
> +            'form_wide_errors': []},
> +            simplejson.loads(view.form_result))
> +
>  
>  class TestBranchMergeProposalResubmitViewMixin:
>      """Test BranchMergeProposalResubmitView."""
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-register-merge-missing-path/+merge/258121
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.