launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:allow-mp-between-personal-repos into launchpad:master.

Commit message:
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.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/387760
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:allow-mp-between-personal-repos into launchpad:master.

diff --git a/lib/lp/code/browser/gitref.py b/lib/lp/code/browser/gitref.py
index 25021bc..68b1445 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 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..48bd0c9 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="view/propose_merge_notes">
+      <ul>
+        <li tal:repeat="message view/propose_merge_notes" class="registered">
+           * <p tal:replace="message" />
+        </li>
+      </ul>
+    </div>
   </div>
 
 </div>