launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-1524316 into lp:launchpad.

Commit message:
Fix setDefaultRepository(ForOwner) to cope with replacing an existing default.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1524316 in Launchpad itself: "GitDefaultConflict: The default repository for 'project' is already set"
  https://bugs.launchpad.net/launchpad/+bug/1524316

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1524316/+merge/280096

Fix setDefaultRepository(ForOwner) to cope with replacing an existing default.

The owner variant also didn't handle no-ops, though this would never have been hit on production.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1524316 into lp:launchpad.

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2015-10-09 16:56:45 +0000
+++ lib/lp/code/model/gitrepository.py	2015-12-10 00:33:48 +0000
@@ -1241,16 +1241,16 @@
             raise GitTargetError(
                 "Cannot set a default Git repository for a person, only "
                 "for a project or a package.")
-        if repository is not None:
-            if repository.target != target:
-                raise GitTargetError(
-                    "Cannot set default Git repository to one attached to "
-                    "another target.")
-            repository.setTargetDefault(True)
-        else:
-            previous = self.getDefaultRepository(target)
+        if repository is not None and repository.target != target:
+            raise GitTargetError(
+                "Cannot set default Git repository to one attached to "
+                "another target.")
+        previous = self.getDefaultRepository(target)
+        if previous != repository:
             if previous is not None:
                 previous.setTargetDefault(False)
+            if repository is not None:
+                repository.setTargetDefault(True)
 
     def setDefaultRepositoryForOwner(self, owner, target, repository, user):
         """See `IGitRepositorySet`."""
@@ -1276,11 +1276,12 @@
                 raise GitTargetError(
                     "Cannot set a person's default Git repository to one "
                     "owned by somebody else.")
-            repository.setOwnerDefault(True)
-        else:
-            previous = self.getDefaultRepositoryForOwner(owner, target)
+        previous = self.getDefaultRepositoryForOwner(owner, target)
+        if previous != repository:
             if previous is not None:
                 previous.setOwnerDefault(False)
+            if repository is not None:
+                repository.setOwnerDefault(True)
 
     def empty_list(self):
         """See `IGitRepositorySet`."""

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2015-11-02 15:31:39 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2015-12-10 00:33:48 +0000
@@ -2074,16 +2074,81 @@
                 self.repository_set.setDefaultRepositoryForOwner,
                 person, person, repository, user)
 
-    def test_setDefaultRepository_for_same_targetdefault_noops(self):
-        # If a repository is already the target default,
-        # setting the defaultRepository again should no-op.
+    def test_setDefaultRepositoryForOwner_noop(self):
+        # If a repository is already the target owner default, setting
+        # the default again should no-op.
+        owner = self.factory.makePerson()
+        target = self.factory.makeProduct()
+        repo = self.factory.makeGitRepository(owner=owner, target=target)
+        with person_logged_in(owner):
+            self.repository_set.setDefaultRepositoryForOwner(
+                owner, target, repo, owner)
+            self.assertEqual(
+                repo,
+                self.repository_set.getDefaultRepositoryForOwner(
+                    owner, target))
+            self.repository_set.setDefaultRepositoryForOwner(
+                owner, target, repo, owner)
+            self.assertEqual(
+                repo,
+                self.repository_set.getDefaultRepositoryForOwner(
+                    owner, target))
+
+    def test_setDefaultRepositoryForOwner_replaces_old(self):
+        # If another repository is already the target owner default,
+        # setting it overwrites.
+        owner = self.factory.makePerson()
+        target = self.factory.makeProduct()
+        repo1 = self.factory.makeGitRepository(owner=owner, target=target)
+        repo2 = self.factory.makeGitRepository(owner=owner, target=target)
+        with person_logged_in(owner):
+            self.repository_set.setDefaultRepositoryForOwner(
+                owner, target, repo1, owner)
+            self.assertEqual(
+                repo1,
+                self.repository_set.getDefaultRepositoryForOwner(
+                    owner, target))
+            self.assertTrue(repo1.owner_default)
+            self.assertFalse(repo2.owner_default)
+            self.repository_set.setDefaultRepositoryForOwner(
+                owner, target, repo2, owner)
+            self.assertEqual(
+                repo2,
+                self.repository_set.getDefaultRepositoryForOwner(
+                    owner, target))
+            self.assertFalse(repo1.owner_default)
+            self.assertTrue(repo2.owner_default)
+
+    def test_setDefaultRepository_noop(self):
+        # If a repository is already the target default, setting the
+        # default again should no-op.
         project = self.factory.makeProduct()
         repository = self.factory.makeGitRepository(target=project)
         with person_logged_in(project.owner):
             self.repository_set.setDefaultRepository(project, repository)
-            result = self.repository_set.setDefaultRepository(
-                project, repository)
-        self.assertEqual(None, result)
+            self.assertEqual(
+                repository, self.repository_set.getDefaultRepository(project))
+            self.repository_set.setDefaultRepository(project, repository)
+            self.assertEqual(
+                repository, self.repository_set.getDefaultRepository(project))
+
+    def test_setDefaultRepository_replaces_old(self):
+        # If another repository is already the target default, setting
+        # it overwrites.
+        project = self.factory.makeProduct()
+        repo1 = self.factory.makeGitRepository(target=project)
+        repo2 = self.factory.makeGitRepository(target=project)
+        with person_logged_in(project.owner):
+            self.repository_set.setDefaultRepository(project, repo1)
+            self.assertEqual(
+                repo1, self.repository_set.getDefaultRepository(project))
+            self.assertTrue(repo1.target_default)
+            self.assertFalse(repo2.target_default)
+            self.repository_set.setDefaultRepository(project, repo2)
+            self.assertEqual(
+                repo2, self.repository_set.getDefaultRepository(project))
+            self.assertFalse(repo1.target_default)
+            self.assertTrue(repo2.target_default)
 
 
 class TestGitRepositorySetDefaultsMixin: