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

Proposed by Thiago F. Pappacena
Status: Merged
Approved by: Thiago F. Pappacena
Approved revision: bbca8aaa77eec65e94f9655d7259379828225a91
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~pappacena/launchpad:allow-mp-between-personal-repos
Merge into: launchpad:master
Diff against target: 163 lines (+60/-12)
5 files modified
lib/lp/code/browser/gitref.py (+21/-1)
lib/lp/code/browser/tests/test_gitref.py (+12/-1)
lib/lp/code/model/gitnamespace.py (+1/-1)
lib/lp/code/model/tests/test_gitnamespace.py (+19/-9)
lib/lp/code/templates/gitref-pending-merges.pt (+7/-0)
Reviewer Review Type Date Requested Status
Colin Watson Approve
Review via email: mp+387760@code.launchpad.net

Commit message

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.

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) wrote :

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

Coding style and better guard on merge proposal creation notes

Revision history for this message
Thiago F. Pappacena (pappacena) wrote :

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

Revision history for this message
Colin Watson (cjwatson) :
Revision history for this message
Thiago F. Pappacena (pappacena) :

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/code/browser/gitref.py b/lib/lp/code/browser/gitref.py
2index 25021bc..a945fa0 100644
3--- a/lib/lp/code/browser/gitref.py
4+++ b/lib/lp/code/browser/gitref.py
5@@ -1,4 +1,4 @@
6-# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
7+# Copyright 2015-2020 Canonical Ltd. This software is licensed under the
8 # GNU Affero General Public License version 3 (see the file LICENSE).
9
10 """Git reference views."""
11@@ -47,6 +47,7 @@ from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
12 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
13 from lp.code.interfaces.gitref import IGitRef
14 from lp.code.interfaces.gitrepository import IGitRepositorySet
15+from lp.registry.interfaces.person import IPerson
16 from lp.services.helpers import english_list
17 from lp.services.propertycache import cachedproperty
18 from lp.services.scripts import log
19@@ -135,6 +136,16 @@ class GitRefView(LaunchpadView, HasSnapsViewMixin):
20 """
21 if not self.context.namespace.supports_merge_proposals:
22 return False
23+ if IPerson.providedBy(self.context.namespace.target):
24+ # XXX pappacena 2020-07-21: For personal repositories, we enable
25+ # the link even if the user will only be allowed to merge
26+ # their personal repositories' branch into another personal repo
27+ # with the same name. But checking if there is another
28+ # repository with the same name might be a bit expensive query for
29+ # such a simple operation. Currently, we only have db index for
30+ # repo's name when searching together with owner.
31+ return True
32+
33 repositories = self.context.namespace.collection.getRepositories()
34 if repositories.count() > 1:
35 return True
36@@ -143,6 +154,15 @@ class GitRefView(LaunchpadView, HasSnapsViewMixin):
37 return False
38 return repository.refs.count() > 1
39
40+ @property
41+ def propose_merge_notes(self):
42+ messages = []
43+ if IPerson.providedBy(self.context.namespace.target):
44+ messages.append(
45+ "You will only be able to propose a merge to another personal "
46+ "repository with the same name.")
47+ return messages
48+
49 @cachedproperty
50 def landing_targets(self):
51 """Return a filtered list of landing targets."""
52diff --git a/lib/lp/code/browser/tests/test_gitref.py b/lib/lp/code/browser/tests/test_gitref.py
53index 5712d0a..0268ca4 100644
54--- a/lib/lp/code/browser/tests/test_gitref.py
55+++ b/lib/lp/code/browser/tests/test_gitref.py
56@@ -1,4 +1,4 @@
57-# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
58+# Copyright 2015-2020 Canonical Ltd. This software is licensed under the
59 # GNU Affero General Public License version 3 (see the file LICENSE).
60
61 """Unit tests for GitRefView."""
62@@ -275,6 +275,17 @@ class TestGitRefView(BrowserTestCase):
63 self.assertThat(
64 contents, Not(soupmatchers.HTMLContains(MissingCommitsNote())))
65
66+ def test_show_merge_link_for_personal_repo(self):
67+ person = self.factory.makePerson()
68+ repo = self.factory.makeGitRepository(
69+ owner=person, target=person)
70+ [ref] = self.factory.makeGitRefs(
71+ repository=repo, paths=["refs/heads/branch"])
72+
73+ view = create_initialized_view(ref, "+index")
74+ self.assertTrue(view.show_merge_links)
75+ self.assertEqual(1, len(view.propose_merge_notes))
76+
77 def _test_all_commits_link(self, branch_name, encoded_branch_name=None):
78 if encoded_branch_name is None:
79 encoded_branch_name = branch_name
80diff --git a/lib/lp/code/model/gitnamespace.py b/lib/lp/code/model/gitnamespace.py
81index 2a358f6..8a08a97 100644
82--- a/lib/lp/code/model/gitnamespace.py
83+++ b/lib/lp/code/model/gitnamespace.py
84@@ -330,7 +330,7 @@ class PersonalGitNamespace(_BaseGitNamespace):
85 if this.namespace != self:
86 raise AssertionError(
87 "Namespace of %s is not %s." % (this.unique_name, self.name))
88- return this == other
89+ return this.name == other.name
90
91 @property
92 def collection(self):
93diff --git a/lib/lp/code/model/tests/test_gitnamespace.py b/lib/lp/code/model/tests/test_gitnamespace.py
94index 5fed799..f873782 100644
95--- a/lib/lp/code/model/tests/test_gitnamespace.py
96+++ b/lib/lp/code/model/tests/test_gitnamespace.py
97@@ -1,4 +1,4 @@
98-# Copyright 2015-2019 Canonical Ltd. This software is licensed under the
99+# Copyright 2015-2020 Canonical Ltd. This software is licensed under the
100 # GNU Affero General Public License version 3 (see the file LICENSE).
101
102 """Tests for `IGitNamespace` implementations."""
103@@ -291,24 +291,34 @@ class TestPersonalGitNamespace(TestCaseWithFactory, NamespaceMixin):
104 repository.namespace.areRepositoriesMergeable(
105 repository, repository))
106
107- def test_areRepositoriesMergeable_same_namespace(self):
108+ def test_areRepositoriesMergeable_different_namespaces(self):
109+ # A personal repository is not mergeable if the origin namespace is
110+ # not the same as the namespacing checking the mergeability.
111+ owner = self.factory.makePerson()
112+ this = self.factory.makeGitRepository(owner=owner, target=owner)
113+ other = self.factory.makeGitRepository(owner=owner, target=owner)
114+ self.assertFalse(other.namespace.areRepositoriesMergeable(this, other))
115+
116+ def test_areRepositoriesMergeable_different_name(self):
117 # A personal repository is not mergeable into another personal
118- # repository, even if they are in the same namespace.
119+ # repository if they do not have the same name.
120 owner = self.factory.makePerson()
121 this = self.factory.makeGitRepository(owner=owner, target=owner)
122 other = self.factory.makeGitRepository(owner=owner, target=owner)
123 self.assertFalse(this.namespace.areRepositoriesMergeable(this, other))
124
125- def test_areRepositoriesMergeable_different_namespace(self):
126- # A personal repository is not mergeable into another personal
127- # repository with a different namespace.
128+ def test_areRepositoriesMergeable_same_name(self):
129+ # A personal repository is mergeable into another personal
130+ # repository if they have the same name, even if they target different
131+ # people.
132 this_owner = self.factory.makePerson()
133+ repo_name = "my-personal-repository"
134 this = self.factory.makeGitRepository(
135- owner=this_owner, target=this_owner)
136+ owner=this_owner, target=this_owner, name=repo_name)
137 other_owner = self.factory.makePerson()
138 other = self.factory.makeGitRepository(
139- owner=other_owner, target=other_owner)
140- self.assertFalse(this.namespace.areRepositoriesMergeable(this, other))
141+ owner=other_owner, target=other_owner, name=repo_name)
142+ self.assertTrue(this.namespace.areRepositoriesMergeable(this, other))
143
144 def test_areRepositoriesMergeable_project(self):
145 # Project repositories are not mergeable into personal repositories.
146diff --git a/lib/lp/code/templates/gitref-pending-merges.pt b/lib/lp/code/templates/gitref-pending-merges.pt
147index 9361354..5d41ce8 100644
148--- a/lib/lp/code/templates/gitref-pending-merges.pt
149+++ b/lib/lp/code/templates/gitref-pending-merges.pt
150@@ -43,6 +43,13 @@
151 tal:condition="link/enabled"
152 tal:replace="structure link/render"
153 />
154+ <div tal:condition="python: view.propose_merge_notes and context_menu['register_merge'].enabled">
155+ <ul>
156+ <li tal:repeat="message view/propose_merge_notes" class="registered">
157+ <spam tal:replace="message" />
158+ </li>
159+ </ul>
160+ </div>
161 </div>
162
163 </div>