Merge ~twom/launchpad:git-branch-picker-unpicked-with-merge-pickers into launchpad:master

[Tom Wardill]

Add autocomplete branch picker to git merge proposal page

[Tom Wardill (twom)]

Screenshots:
https://people.canonical.com/~tomwardill/git-branch-picker/Screenshot-20201119163719-650x173.png
https://people.canonical.com/~tomwardill/git-branch-picker/Screenshot-20201119163742-610x156.png

[Colin Watson (cjwatson)]

review: Approve

[Otto Co-Pilot (otto-copilot)]

Merging failed
https://jenkins.ols.canonical.com/online-services/job/launchpad/967/

diff --git a/lib/lp/code/browser/gitref.py b/lib/lp/code/browser/gitref.py
index f17207e..5edce2c 100644
--- a/lib/lp/code/browser/gitref.py
+++ b/lib/lp/code/browser/gitref.py
@@ -27,9 +27,7 @@ from zope.interface import Interface
 from zope.publisher.interfaces import NotFound
 from zope.schema import (
     Bool,
-    Choice,
     Text,
-    TextLine,
     )
 
 from lp import _
@@ -37,11 +35,11 @@ from lp.app.browser.launchpadform import (
     action,
     LaunchpadFormView,
     )
-from lp.app.widgets.suggestion import TargetGitRepositoryWidget
 from lp.code.browser.branchmergeproposal import (
     latest_proposals_for_each_branch,
     )
 from lp.code.browser.sourcepackagerecipelisting import HasRecipesMenuMixin
+from lp.code.browser.widgets.gitref import GitRefWidget
 from lp.code.enums import GitRepositoryType
 from lp.code.errors import InvalidBranchMergeProposal
 from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
@@ -247,30 +245,15 @@ class GitRefView(LaunchpadView, HasSnapsViewMixin):
 class GitRefRegisterMergeProposalSchema(Interface):
     """The schema to define the form for registering a new merge proposal."""
 
-    target_git_repository = Choice(
-        title=_("Target repository"),
-        vocabulary="GitRepository", required=True, readonly=True,
-        description=_("The repository that the source will be merged into."))
+    target_git_ref = copy_field(
+        IBranchMergeProposal['target_git_ref'], required=True)
 
-    target_git_path = TextLine(
-        title=_("Target branch"), required=True, readonly=True,
-        description=_(
-            "The branch within the target repository that the source will "
-            "be merged into."))
-
-    prerequisite_git_repository = Choice(
-        title=_("Prerequisite repository"),
-        vocabulary="GitRepository", required=False, readonly=True,
-        description=_(
-            "A repository containing a branch that should be merged before "
-            "this one.  (Its changes will not be shown in the diff.)"))
-
-    prerequisite_git_path = TextLine(
-        title=_("Prerequisite branch"), required=False, readonly=True,
-        description=_(
-            "A branch within the prerequisite repository that should be "
-            "merged before this one.  (Its changes will not be shown in the "
-            "diff.)"))
+    prerequisite_git_ref = copy_field(
+        IBranchMergeProposal['prerequisite_git_ref'], required=False,
+        description=_("If the target branch is based on a different branch, "
+                      "you can add this as a prerequisite. "
+                      "The changes from that branch will not show "
+                      "in the diff."))
 
     comment = Text(
         title=_('Description of the change'), required=False,
@@ -302,7 +285,10 @@ class GitRefRegisterMergeProposalView(LaunchpadFormView):
     schema = GitRefRegisterMergeProposalSchema
     for_input = True
 
-    custom_widget_target_git_repository = TargetGitRepositoryWidget
+    custom_widget_target_git_ref = CustomWidgetFactory(
+        GitRefWidget, require_branch=True)
+    custom_widget_prerequisite_git_ref = CustomWidgetFactory(
+        GitRefWidget, require_branch=True)
     custom_widget_commit_message = CustomWidgetFactory(
         TextAreaWidget, cssClass='comment-text')
     custom_widget_comment = CustomWidgetFactory(
@@ -323,15 +309,25 @@ class GitRefRegisterMergeProposalView(LaunchpadFormView):
     def setUpWidgets(self, context=None):
         super(GitRefRegisterMergeProposalView, self).setUpWidgets(
             context=context)
-        term = next(
-            iter(self.widgets['target_git_repository'].vocabulary),
-            None)
-        # If we have a target, and the user hasn't entered a value.
-        if term and not self.widgets['target_git_path'].hasInput():
-            branch_display = term.value.default_branch
-            if branch_display and branch_display.startswith("refs/heads/"):
-                branch_display = branch_display[len("refs/heads/"):]
-            self.widgets['target_git_path'].setRenderedValue(branch_display)
+
+        if not self.widgets['target_git_ref'].hasInput():
+            if self.context.repository.namespace.has_defaults:
+                repo_set = getUtility(IGitRepositorySet)
+                default_repo = repo_set.getDefaultRepository(
+                    self.context.repository.target)
+            else:
+                default_repo = None
+            if not default_repo:
+                default_repo = self.context.repository
+            if default_repo.default_branch:
+                default_ref = default_repo.getRefByPath(
+                    default_repo.default_branch)
+                with_path = True
+            else:
+                default_ref = self.context
+                with_path = False
+            self.widgets["target_git_ref"].setRenderedValue(
+                default_ref, with_path=with_path)
 
     @action('Propose Merge', name='register',
             failure=LaunchpadFormView.ajax_failure_handler)
@@ -340,15 +336,8 @@ class GitRefRegisterMergeProposalView(LaunchpadFormView):
 
         registrant = self.user
         source_ref = self.context
-        target_ref = data['target_git_repository'].getRefByPath(
-            data['target_git_path'])
-        if (data.get('prerequisite_git_repository') is not None and
-                data.get('prerequisite_git_path') is not None):
-            prerequisite_ref = (
-                data['prerequisite_git_repository'].getRefByPath(
-                    data['prerequisite_git_path']))
-        else:
-            prerequisite_ref = None
+        target_ref = data['target_git_ref']
+        prerequisite_ref = data.get('prerequisite_git_ref')
 
         review_requests = []
         reviewer = data.get('reviewer')
@@ -403,41 +392,31 @@ class GitRefRegisterMergeProposalView(LaunchpadFormView):
             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:
+        ref = data['{}_git_ref'.format(name)]
+        if ref == self.context:
             self.setFieldError(
-                '%s_git_path' % name,
+                '%s_git_ref' % name,
                 "The %s repository and path together cannot be the same "
                 "as the source repository and path." % name)
-        return repository
+        return ref.repository
 
     def validate(self, data):
         source_ref = self.context
         # The existence of target_git_repository is handled by the form
         # machinery.
-        if data.get('target_git_repository') is not None:
+        if data.get('target_git_ref') is not None:
             target_repository = self._validateRef(data, 'target')
             if not target_repository.isRepositoryMergeable(
                     source_ref.repository):
                 self.setFieldError(
-                    'target_git_repository',
+                    'target_git_ref',
                     "%s is not mergeable into this repository." %
                     source_ref.repository.identity)
-        if data.get('prerequisite_git_repository') is not None:
+        if data.get('prerequisite_git_ref') is not None:
             prerequisite_repository = self._validateRef(data, 'prerequisite')
             if not target_repository.isRepositoryMergeable(
                     prerequisite_repository):
                 self.setFieldError(
-                    'prerequisite_git_repository',
+                    'prerequisite_git_ref',
                     "This repository is not mergeable into %s." %
                     target_repository.identity)
diff --git a/lib/lp/code/browser/tests/test_branchmergeproposal.py b/lib/lp/code/browser/tests/test_branchmergeproposal.py
index a9ac39b..fedb00d 100644
--- a/lib/lp/code/browser/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/browser/tests/test_branchmergeproposal.py
@@ -821,6 +821,7 @@ class TestRegisterBranchMergeProposalViewBzr(
                         target_branch.unique_name},
                 **extra)
             request.setPrincipal(owner)
+            transaction.commit()
             view = create_initialized_view(
                 target_branch,
                 name='+register-merge',
@@ -858,10 +859,7 @@ class TestRegisterBranchMergeProposalViewGit(
 
     @staticmethod
     def _getFormValues(target_branch, extras):
-        values = {
-            'target_git_repository': target_branch.repository,
-            'target_git_path': target_branch.path,
-            }
+        values = {'target_git_ref': target_branch}
         values.update(extras)
         return values
 
@@ -873,17 +871,19 @@ class TestRegisterBranchMergeProposalViewGit(
         view = self._createView()
         self.assertEqual(
             target_branch.repository.default_branch.split('/')[-1],
-            view.widgets['target_git_path']._getCurrentValue())
+            view.widgets['target_git_ref'].path_widget._getCurrentValue())
 
     def test_default_branch_no_default_set(self):
         with admin_logged_in():
             self._makeTargetBranch(target_default=True)
         view = self._createView()
-        self.assertIsNone(view.widgets['target_git_path']._getCurrentValue())
+        self.assertIsNone(
+            view.widgets['target_git_ref'].path_widget._getCurrentValue())
 
     def test_default_branch_no_target(self):
         view = self._createView()
-        self.assertIsNone(view.widgets['target_git_path']._getCurrentValue())
+        self.assertIsNone(
+            view.widgets['target_git_ref'].path_widget._getCurrentValue())
 
     def test_register_ajax_request_with_confirmation(self):
         # Ajax submits return json data containing info about what the visible
@@ -929,9 +929,9 @@ class TestRegisterBranchMergeProposalViewGit(
                 method='POST',
                 form={
                     'field.actions.register': 'Propose Merge',
-                    'field.target_git_repository.target_git_repository':
+                    'field.target_git_ref.repository':
                         target_branch.repository.unique_name,
-                    'field.target_git_path': target_branch.path,
+                    'field.target_git_ref.path': target_branch.path,
                     },
                 **extra)
             request.setPrincipal(owner)
@@ -944,7 +944,7 @@ class TestRegisterBranchMergeProposalViewGit(
         self.assertEqual(
             {'error_summary': 'There is 1 error.',
             'errors': {
-                'field.target_git_path':
+                'field.target_git_ref':
                     ('The target repository and path together cannot be the '
                      'same as the source repository and path.')},
             'form_wide_errors': []},
@@ -959,9 +959,9 @@ class TestRegisterBranchMergeProposalViewGit(
                 method='POST',
                 form={
                     'field.actions.register': 'Propose Merge',
-                    'field.target_git_repository.target_git_repository': '',
-                    'field.target_git_repository-empty-marker': '1',
-                    'field.target_git_path': 'master',
+                    'field.target_git_ref.repository': '',
+                    'field.target_git_ref.repository-empty-marker': '1',
+                    'field.target_git_ref.path': 'master',
                     },
                 **extra)
             request.setPrincipal(owner)
@@ -974,7 +974,7 @@ class TestRegisterBranchMergeProposalViewGit(
         self.assertEqual(
             {'error_summary': 'There is 1 error.',
             'errors': {
-                'field.target_git_repository': 'Required input is missing.',
+                'field.target_git_ref': 'Required input is missing.',
                 },
             'form_wide_errors': []},
             simplejson.loads(view.form_result))
@@ -984,13 +984,14 @@ class TestRegisterBranchMergeProposalViewGit(
         owner = self.factory.makePerson()
         target_branch = self._makeTargetBranch(
             owner=owner, information_type=InformationType.USERDATA)
+        transaction.commit()
         extra = {'HTTP_X_REQUESTED_WITH': 'XMLHttpRequest'}
         with person_logged_in(owner):
             request = LaunchpadTestRequest(
                 method='POST',
                 form={
                     'field.actions.register': 'Propose Merge',
-                    'field.target_git_repository.target_git_repository':
+                    'field.target_git_ref.repository':
                         target_branch.repository.unique_name,
                     },
                 **extra)
@@ -1004,9 +1005,8 @@ class TestRegisterBranchMergeProposalViewGit(
         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.')},
+                'field.target_git_ref':
+                    'Please enter a Git branch path.'},
             'form_wide_errors': []},
             simplejson.loads(view.form_result))
 
@@ -1018,16 +1018,17 @@ class TestRegisterBranchMergeProposalViewGit(
             owner=owner, information_type=InformationType.USERDATA)
         prerequisite_branch = self._makeTargetBranch(
             owner=owner, information_type=InformationType.USERDATA)
+        transaction.commit()
         extra = {'HTTP_X_REQUESTED_WITH': 'XMLHttpRequest'}
         with person_logged_in(owner):
             request = LaunchpadTestRequest(
                 method='POST',
                 form={
                     'field.actions.register': 'Propose Merge',
-                    'field.target_git_repository.target_git_repository':
+                    'field.target_git_ref.repository':
                         target_branch.repository.unique_name,
-                    'field.target_git_path': target_branch.path,
-                    'field.prerequisite_git_repository':
+                    'field.target_git_ref.path': target_branch.path,
+                    'field.prerequisite_git_ref.repository':
                         prerequisite_branch.repository.unique_name,
                     },
                 **extra)
@@ -1041,9 +1042,8 @@ class TestRegisterBranchMergeProposalViewGit(
         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.')},
+                'field.prerequisite_git_ref':
+                    'Please enter a Git branch path.'},
             'form_wide_errors': []},
             simplejson.loads(view.form_result))
 
diff --git a/lib/lp/code/interfaces/branchmergeproposal.py b/lib/lp/code/interfaces/branchmergeproposal.py
index bf481c9..820b7b0 100644
--- a/lib/lp/code/interfaces/branchmergeproposal.py
+++ b/lib/lp/code/interfaces/branchmergeproposal.py
@@ -174,7 +174,7 @@ class IBranchMergeProposalPublic(IPrivacy):
     target_git_commit_sha1 = TextLine(
         title=_('Target Git commit SHA-1'), required=False, readonly=True)
     target_git_ref = Reference(
-        title=_('Target Git reference'),
+        title=_('Target Git branch'),
         schema=IGitRef, required=False, readonly=True)
 
     prerequisite_branch = exported(
@@ -205,7 +205,7 @@ class IBranchMergeProposalPublic(IPrivacy):
         title=_('Prerequisite Git commit SHA-1'),
         required=False, readonly=True)
     prerequisite_git_ref = Reference(
-        title=_('Prerequisite Git reference'),
+        title=_('Prerequisite Git branch'),
         schema=IGitRef, required=False, readonly=True)
 
     merge_source = Attribute(
diff --git a/lib/lp/code/templates/gitref-register-merge.pt b/lib/lp/code/templates/gitref-register-merge.pt
index 91a06b9..0d9df63 100644
--- a/lib/lp/code/templates/gitref-register-merge.pt
+++ b/lib/lp/code/templates/gitref-register-merge.pt
@@ -12,11 +12,7 @@
         <metal:formbody fill-slot="widgets">
           <table class="form">
 
-            <tal:widget define="widget nocall:view/widgets/target_git_repository">
-              <metal:block use-macro="context/@@launchpad_form/widget_row" />
-            </tal:widget>
-
-            <tal:widget define="widget nocall:view/widgets/target_git_path">
+            <tal:widget define="widget nocall:view/widgets/target_git_ref">
               <metal:block use-macro="context/@@launchpad_form/widget_row" />
             </tal:widget>
 
@@ -40,11 +36,7 @@
               <metal:block use-macro="context/@@launchpad_form/widget_row" />
             </tal:widget>
 
-            <tal:widget define="widget nocall:view/widgets/prerequisite_git_repository">
-              <metal:block use-macro="context/@@launchpad_form/widget_row" />
-            </tal:widget>
-
-            <tal:widget define="widget nocall:view/widgets/prerequisite_git_path">
+            <tal:widget define="widget nocall:view/widgets/prerequisite_git_ref">
               <metal:block use-macro="context/@@launchpad_form/widget_row" />
             </tal:widget>