Merge ~pappacena/launchpad:allow-mp-between-personal-repos into launchpad:master

[Thiago F. Pappacena]

LP: #1888295 Allowing users to propose merge between personal git repositories

We are also adding a note on the UI, below the "create MP" button, to warn users about restrictions that may apply to the MP creation, in order to avoid misunderstanding and reduce support requests.

[Colin Watson (cjwatson)]

Thanks! A few minor nits, but this seems mostly good.

I guess this MP should be explicitly linked to bug 1888295. (Ideally, do this in the commit message as described in https://help.launchpad.net/Code/Git#Linking_to_bugs, but failing that you can do it manually in the MP web UI.)

review: Approve

bbca8aa... by Thiago F. Pappacena on 2020-07-21

Coding style and better guard on merge proposal creation notes

[Thiago F. Pappacena (pappacena)]

Linked to the LP bug, and pushed the requested changes. I'll top-approve this.

diff --git a/lib/lp/code/browser/gitref.py b/lib/lp/code/browser/gitref.py
index 25021bc..a945fa0 100644
--- a/lib/lp/code/browser/gitref.py
+++ b/lib/lp/code/browser/gitref.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Git reference views."""
@@ -47,6 +47,7 @@ from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
 from lp.code.interfaces.gitref import IGitRef
 from lp.code.interfaces.gitrepository import IGitRepositorySet
+from lp.registry.interfaces.person import IPerson
 from lp.services.helpers import english_list
 from lp.services.propertycache import cachedproperty
 from lp.services.scripts import log
@@ -135,6 +136,16 @@ class GitRefView(LaunchpadView, HasSnapsViewMixin):
         """
         if not self.context.namespace.supports_merge_proposals:
             return False
+        if IPerson.providedBy(self.context.namespace.target):
+            # XXX pappacena 2020-07-21: For personal repositories, we enable
+            # the link even if the user will only be allowed to merge
+            # their personal repositories' branch into another personal repo
+            # with the same name. But checking if there is another
+            # repository with the same name might be a bit expensive query for
+            # such a simple operation. Currently, we only have db index for
+            # repo's name when searching together with owner.
+            return True
+
         repositories = self.context.namespace.collection.getRepositories()
         if repositories.count() > 1:
             return True
@@ -143,6 +154,15 @@ class GitRefView(LaunchpadView, HasSnapsViewMixin):
             return False
         return repository.refs.count() > 1
 
+    @property
+    def propose_merge_notes(self):
+        messages = []
+        if IPerson.providedBy(self.context.namespace.target):
+            messages.append(
+                "You will only be able to propose a merge to another personal "
+                "repository with the same name.")
+        return messages
+
     @cachedproperty
     def landing_targets(self):
         """Return a filtered list of landing targets."""
diff --git a/lib/lp/code/browser/tests/test_gitref.py b/lib/lp/code/browser/tests/test_gitref.py
index 5712d0a..0268ca4 100644
--- a/lib/lp/code/browser/tests/test_gitref.py
+++ b/lib/lp/code/browser/tests/test_gitref.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for GitRefView."""
@@ -275,6 +275,17 @@ class TestGitRefView(BrowserTestCase):
         self.assertThat(
             contents, Not(soupmatchers.HTMLContains(MissingCommitsNote())))
 
+    def test_show_merge_link_for_personal_repo(self):
+        person = self.factory.makePerson()
+        repo = self.factory.makeGitRepository(
+            owner=person, target=person)
+        [ref] = self.factory.makeGitRefs(
+            repository=repo, paths=["refs/heads/branch"])
+
+        view = create_initialized_view(ref, "+index")
+        self.assertTrue(view.show_merge_links)
+        self.assertEqual(1, len(view.propose_merge_notes))
+
     def _test_all_commits_link(self, branch_name, encoded_branch_name=None):
         if encoded_branch_name is None:
             encoded_branch_name = branch_name
diff --git a/lib/lp/code/model/gitnamespace.py b/lib/lp/code/model/gitnamespace.py
index 2a358f6..8a08a97 100644
--- a/lib/lp/code/model/gitnamespace.py
+++ b/lib/lp/code/model/gitnamespace.py
@@ -330,7 +330,7 @@ class PersonalGitNamespace(_BaseGitNamespace):
         if this.namespace != self:
             raise AssertionError(
                 "Namespace of %s is not %s." % (this.unique_name, self.name))
-        return this == other
+        return this.name == other.name
 
     @property
     def collection(self):
diff --git a/lib/lp/code/model/tests/test_gitnamespace.py b/lib/lp/code/model/tests/test_gitnamespace.py
index 5fed799..f873782 100644
--- a/lib/lp/code/model/tests/test_gitnamespace.py
+++ b/lib/lp/code/model/tests/test_gitnamespace.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `IGitNamespace` implementations."""
@@ -291,24 +291,34 @@ class TestPersonalGitNamespace(TestCaseWithFactory, NamespaceMixin):
             repository.namespace.areRepositoriesMergeable(
                 repository, repository))
 
-    def test_areRepositoriesMergeable_same_namespace(self):
+    def test_areRepositoriesMergeable_different_namespaces(self):
+        # A personal repository is not mergeable if the origin namespace is
+        # not the same as the namespacing checking the mergeability.
+        owner = self.factory.makePerson()
+        this = self.factory.makeGitRepository(owner=owner, target=owner)
+        other = self.factory.makeGitRepository(owner=owner, target=owner)
+        self.assertFalse(other.namespace.areRepositoriesMergeable(this, other))
+
+    def test_areRepositoriesMergeable_different_name(self):
         # A personal repository is not mergeable into another personal
-        # repository, even if they are in the same namespace.
+        # repository if they do not have the same name.
         owner = self.factory.makePerson()
         this = self.factory.makeGitRepository(owner=owner, target=owner)
         other = self.factory.makeGitRepository(owner=owner, target=owner)
         self.assertFalse(this.namespace.areRepositoriesMergeable(this, other))
 
-    def test_areRepositoriesMergeable_different_namespace(self):
-        # A personal repository is not mergeable into another personal
-        # repository with a different namespace.
+    def test_areRepositoriesMergeable_same_name(self):
+        # A personal repository is mergeable into another personal
+        # repository if they have the same name, even if they target different
+        # people.
         this_owner = self.factory.makePerson()
+        repo_name = "my-personal-repository"
         this = self.factory.makeGitRepository(
-            owner=this_owner, target=this_owner)
+            owner=this_owner, target=this_owner, name=repo_name)
         other_owner = self.factory.makePerson()
         other = self.factory.makeGitRepository(
-            owner=other_owner, target=other_owner)
-        self.assertFalse(this.namespace.areRepositoriesMergeable(this, other))
+            owner=other_owner, target=other_owner, name=repo_name)
+        self.assertTrue(this.namespace.areRepositoriesMergeable(this, other))
 
     def test_areRepositoriesMergeable_project(self):
         # Project repositories are not mergeable into personal repositories.
diff --git a/lib/lp/code/templates/gitref-pending-merges.pt b/lib/lp/code/templates/gitref-pending-merges.pt
index 9361354..5d41ce8 100644
--- a/lib/lp/code/templates/gitref-pending-merges.pt
+++ b/lib/lp/code/templates/gitref-pending-merges.pt
@@ -43,6 +43,13 @@
       tal:condition="link/enabled"
       tal:replace="structure link/render"
       />
+    <div tal:condition="python: view.propose_merge_notes and context_menu['register_merge'].enabled">
+      <ul>
+        <li tal:repeat="message view/propose_merge_notes" class="registered">
+           <spam tal:replace="message" />
+        </li>
+      </ul>
+    </div>
   </div>
 
 </div>