launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-register-merge-missing-path into lp:launchpad.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1451068 in Launchpad itself: "OOPS while attempting to create a git merge request"
  https://bugs.launchpad.net/launchpad/+bug/1451068

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-register-merge-missing-path/+merge/258121

Fix GitRefRegisterMergeProposalView.validate to stop assuming that all required fields are present.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-register-merge-missing-path into lp:launchpad.

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