launchpad-reviewers team mailing list archive

[Ioana Lasc <ioana.lasc@xxxxxxxxxxxxx>]

Ioana Lasc has proposed merging ~ilasc/launchpad:bug-1775148 into launchpad:master.

Commit message:
default_branch should be required when editing a Git repo in LP.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1775148 in Launchpad itself: "Can't unset default branch for a git repository"
  https://bugs.launchpad.net/launchpad/+bug/1775148

For more details, see:
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/374662

We are not generating and Oops anymore when unsetting the deafult branch.

Testing of the Get call on the webservice while the Git repo still has default_branch = null in DB  and scan job hasn't run yet passes.

The second part of the solution for this bug (marking of the default_branch as required on the webservice) is not done yet.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/launchpad:bug-1775148 into launchpad:master.

diff --git a/lib/lp/code/browser/tests/test_gitrepository.py b/lib/lp/code/browser/tests/test_gitrepository.py
index 246d48a..f82cd53 100644
--- a/lib/lp/code/browser/tests/test_gitrepository.py
+++ b/lib/lp/code/browser/tests/test_gitrepository.py
@@ -98,7 +98,7 @@ from lp.testing.views import (
     create_initialized_view,
     create_view,
     )
-
+from lp.code.errors import NoSuchGitReference
 
 class TestGitRepositoryNavigation(TestCaseWithFactory):
 
@@ -1001,6 +1001,22 @@ class TestGitRepositoryEditView(TestCaseWithFactory):
                 hosting_fixture.setProperties.calls)
             self.assertEqual("refs/heads/new", repository.default_branch)
 
+    def unset_default_branch(self, newBranch):
+        hosting_fixture = self.useFixture(GitHostingFixture())
+        person = self.factory.makePerson()
+        repository = self.factory.makeGitRepository(owner=person)
+        master, new = self.factory.makeGitRefs(
+            repository = repository,
+            paths = ["refs/heads/master", "refs/heads/other"])
+        removeSecurityProxy(repository)._default_branch = "refs/heads/master"
+        browser = self.getUserBrowser(
+            canonical_url(repository) + "/+edit", user=person)
+        browser.getControl(name="field.default_branch").value = newBranch
+        browser.getControl("Change Git Repository").click()
+
+    def test_exception_unset_default_branch(self):
+        self.assertRaises(NoSuchGitReference, self.unset_default_branch, '')
+
     def test_change_default_branch_nonexistent(self):
         # Trying to change the default branch to one that doesn't exist
         # displays an error.
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 7c7d451..4344a54 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -558,13 +558,14 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
         """See `IGitRepository`."""
         if self.repository_type != GitRepositoryType.HOSTED:
             raise CannotModifyNonHostedGitRepository(self)
-        ref = self.getRefByPath(value)
-        if ref is None:
+        if value is None:
             raise NoSuchGitReference(self, value)
-        if self._default_branch != ref.path:
-            self._default_branch = ref.path
-            getUtility(IGitHostingClient).setProperties(
-                self.getInternalPath(), default_branch=ref.path)
+        else:
+            ref = self.getRefByPath(value)
+            if self._default_branch != ref.path:
+                self._default_branch = ref.path
+                getUtility(IGitHostingClient).setProperties(
+                    self.getInternalPath(), default_branch=ref.path)
 
     def getRefByPath(self, path):
         if path == u"HEAD":
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index da75ad9..7c48511 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -158,6 +158,7 @@ from lp.testing import (
     admin_logged_in,
     ANONYMOUS,
     api_url,
+    time_counter,
     celebrity_logged_in,
     login_person,
     logout,
@@ -3400,6 +3401,34 @@ class TestGitRepositoryWebservice(TestCaseWithFactory):
             "git+ssh://git.launchpad.test/~person/project/+git/repository",
             repository["git_ssh_url"])
 
+    def test_getRepoWorksForDefaultBranchNone(self):
+        # Ensure we're not getting an error when calling
+        # GET on the Webservice when a Git Repo exists in the DB
+        # with a NULL default branch
+
+        hosting_fixture = self.useFixture(GitHostingFixture())
+        owner = self.factory.makePerson(name="testowner")
+        repository = self.factory.makeGitRepository(
+            owner=owner, name="empty_default_branch_repo"
+        )
+        webservice = webservice_for_person(
+            repository.owner, permission=OAuthPermission.READ_PUBLIC)
+        webservice.default_api_version = "devel"
+        with person_logged_in(ANONYMOUS):
+            repository_url = api_url(repository)
+        response = webservice.get(repository_url).jsonBody()
+        self.assertEqual(
+            None,
+            response["default_branch"])
+
+        removeSecurityProxy(repository)._default_branch = "refs/heads/master"
+        with person_logged_in(ANONYMOUS):
+            repository_url = api_url(repository)
+        response = webservice.get(repository_url).jsonBody()
+        self.assertEqual(
+            "refs/heads/master",
+            response["default_branch"])
+
     def assertNewWorks(self, target_db):
         hosting_fixture = self.useFixture(GitHostingFixture())
         if IPerson.providedBy(target_db):