Merge lp:~cjwatson/launchpad/git-register-merge-missing-path into lp:launchpad

[Colin Watson]

Fix GitRefRegisterMergeProposalView.validate to stop assuming that all required fields are present.

Fix GitRefRegisterMergeProposalView.validate to stop assuming that all required fields are present.

[William Grant (wgrant)]

Probably worth factoring the target/prereq validate bits out.

review: Approve (code)

=== 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-05 10:16:00 +0000
@@ -293,36 +293,36 @@
         except InvalidBranchMergeProposal as error:
             self.addError(str(error))
 
+    def _validateRef(self, data, name):
+        repository = data.get('%s_git_repository' % name)
+        path = data.get('%s_git_path' % name)
+        if path:
+            ref = repository.getRefByPath(path)
+        else:
+            ref = None
+        if ref is None:
+            self.setFieldError(
+                '%s_git_path' % name,
+                "The %s path must be the path of a reference in the "
+                "%s repository." % (name, name))
+        elif ref == self.context:
+            self.setFieldError(
+                '%s_git_path' % name,
+                "The %s repository and path together cannot be the same "
+                "as the source repository and path." % name)
+        return repository
+
     def validate(self, data):
         source_ref = self.context
-        target_repository = data['target_git_repository']
-        target_ref = target_repository.getRefByPath(data['target_git_path'])
-        if target_ref is None:
-            self.setFieldError(
-                'target_git_path',
-                "The target path must be the path of a reference in the "
-                "target repository.")
-        elif source_ref == target_ref:
-            self.setFieldError(
-                '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):
+        target_repository = self._validateRef(data, 'target')
+        if not target_repository.isRepositoryMergeable(source_ref.repository):
             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):
-            prerequisite_repository = data['prerequisite_git_repository']
-            prerequisite_ref = prerequisite_repository.getRefByPath(
-                data['prerequisite_git_path'])
-            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(
+                "%s is not mergeable into this repository." %
+                source_ref.repository.identity)
+        if data.get('prerequisite_git_repository') is not None:
+            prerequisite_repository = self._validateRef(data, 'prerequisite')
+            if not target_repository.isRepositoryMergeable(
                     prerequisite_repository):
                 self.setFieldError(
                     'prerequisite_git_repository',

=== 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-05 10:16:00 +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."""