launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18788
Re: [Merge] lp:~cjwatson/launchpad/git-default-branch into lp:launchpad
Review: Approve code
Diff comments:
> === modified file 'lib/lp/code/browser/gitrepository.py'
> --- lib/lp/code/browser/gitrepository.py 2015-06-12 08:04:42 +0000
> +++ lib/lp/code/browser/gitrepository.py 2015-06-13 01:58:23 +0000
> @@ -21,7 +21,10 @@
>
> from lazr.lifecycle.event import ObjectModifiedEvent
> from lazr.lifecycle.snapshot import Snapshot
> -from lazr.restful.interface import copy_field
> +from lazr.restful.interface import (
> + copy_field,
> + use_template,
> + )
> from storm.expr import Desc
> from zope.event import notify
> from zope.interface import (
> @@ -266,6 +269,7 @@
> This is necessary to make various fields editable that are not
> normally editable through the interface.
> """
> + use_template(IGitRepository, include=["default_branch"])
> information_type = copy_field(
> IGitRepository["information_type"], readonly=False,
> vocabulary=InformationTypeVocabulary(types=info_types))
> @@ -378,6 +382,7 @@
> "owner",
> "name",
> "information_type",
> + "default_branch",
> ]
>
> custom_widget("information_type", LaunchpadRadioWidgetWithDescription)
> @@ -416,6 +421,14 @@
> (owner.displayname, target.displayname))
> except GitRepositoryExists as e:
> self._setRepositoryExists(e.existing_repository)
> + if "default_branch" in data:
> + default_branch = data["default_branch"]
> + if (default_branch is not None and
> + self.context.getRefByPath(default_branch) is None):
> + self.setFieldError(
> + "default_branch",
> + "This repository does not contain a reference named "
> + "'%s'." % default_branch)
>
>
> class GitRepositoryDeletionView(LaunchpadFormView):
>
> === modified file 'lib/lp/code/browser/tests/test_gitrepository.py'
> --- lib/lp/code/browser/tests/test_gitrepository.py 2015-06-05 13:10:18 +0000
> +++ lib/lp/code/browser/tests/test_gitrepository.py 2015-06-13 01:58:23 +0000
> @@ -15,6 +15,7 @@
> DocTestMatches,
> Equals,
> )
> +import transaction
> from zope.component import getUtility
> from zope.publisher.interfaces import NotFound
> from zope.security.proxy import removeSecurityProxy
> @@ -22,6 +23,7 @@
> from lp.app.enums import InformationType
> from lp.app.interfaces.launchpad import ILaunchpadCelebrities
> from lp.app.interfaces.services import IService
> +from lp.code.interfaces.githosting import IGitHostingClient
> from lp.code.interfaces.revision import IRevisionSet
> from lp.registry.enums import BranchSharingPolicy
> from lp.registry.interfaces.person import PersonVisibility
> @@ -37,6 +39,8 @@
> record_two_runs,
> TestCaseWithFactory,
> )
> +from lp.testing.fakemethod import FakeMethod
> +from lp.testing.fixture import ZopeUtilityFixture
> from lp.testing.layers import DatabaseFunctionalLayer
> from lp.testing.matchers import HasQueryCount
> from lp.testing.pages import (
> @@ -343,6 +347,50 @@
> self.assertEqual(
> repository.information_type, InformationType.PUBLICSECURITY)
>
> + def test_change_default_branch(self):
> + # An authorised user can change the default branch to one that
> + # exists. They may omit "refs/heads/".
> + hosting_client = FakeMethod()
> + hosting_client.setProperties = FakeMethod()
> + self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
> + person = self.factory.makePerson()
> + repository = self.factory.makeGitRepository(owner=person)
> + master, new = self.factory.makeGitRefs(
> + repository=repository,
> + paths=[u"refs/heads/master", u"refs/heads/new"])
> + removeSecurityProxy(repository)._default_branch = u"refs/heads/master"
> + browser = self.getUserBrowser(
> + canonical_url(repository) + "/+edit", user=person)
> + browser.getControl(name="field.default_branch").value = u"new"
> + browser.getControl("Change Git Repository").click()
> + with person_logged_in(person):
> + self.assertEqual(
> + [((repository.getInternalPath(),),
> + {u"default_branch": u"refs/heads/new"})],
> + hosting_client.setProperties.calls)
> + self.assertEqual(u"refs/heads/new", repository.default_branch)
> +
> + def test_change_default_branch_nonexistent(self):
> + # Trying to change the default branch to one that doesn't exist
> + # displays an error.
> + person = self.factory.makePerson()
> + repository = self.factory.makeGitRepository(owner=person)
> + [master] = self.factory.makeGitRefs(
> + repository=repository, paths=[u"refs/heads/master"])
> + removeSecurityProxy(repository)._default_branch = u"refs/heads/master"
> + form = {
> + "field.default_branch": "refs/heads/new",
> + "field.actions.change": "Change Git Repository",
> + }
> + transaction.commit()
> + with person_logged_in(person):
> + view = create_initialized_view(repository, name="+edit", form=form)
> + self.assertEqual(
> + ["This repository does not contain a reference named "
> + "'refs/heads/new'."],
> + view.errors)
> + self.assertEqual(u"refs/heads/master", repository.default_branch)
> +
>
> class TestGitRepositoryEditViewInformationTypes(TestCaseWithFactory):
> """Tests for GitRepositoryEditView.getInformationTypesToShow."""
>
> === modified file 'lib/lp/code/configure.zcml'
> --- lib/lp/code/configure.zcml 2015-06-13 01:58:23 +0000
> +++ lib/lp/code/configure.zcml 2015-06-13 01:58:23 +0000
> @@ -787,7 +787,8 @@
> permission="launchpad.View"
> interface="lp.app.interfaces.launchpad.IPrivacy
> lp.code.interfaces.gitrepository.IGitRepositoryView
> - lp.code.interfaces.gitrepository.IGitRepositoryModerateAttributes" />
> + lp.code.interfaces.gitrepository.IGitRepositoryModerateAttributes
> + lp.code.interfaces.gitrepository.IGitRepositoryEditableAttributes" />
> <require
> permission="launchpad.Moderate"
> interface="lp.code.interfaces.gitrepository.IGitRepositoryModerate"
> @@ -795,7 +796,8 @@
> set_attributes="date_last_modified" />
> <require
> permission="launchpad.Edit"
> - interface="lp.code.interfaces.gitrepository.IGitRepositoryEdit" />
> + interface="lp.code.interfaces.gitrepository.IGitRepositoryEdit"
> + set_schema="lp.code.interfaces.gitrepository.IGitRepositoryEditableAttributes" />
> </class>
> <subscriber
> for="lp.code.interfaces.gitrepository.IGitRepository zope.lifecycleevent.interfaces.IObjectModifiedEvent"
>
> === modified file 'lib/lp/code/errors.py'
> --- lib/lp/code/errors.py 2015-05-19 11:29:24 +0000
> +++ lib/lp/code/errors.py 2015-06-13 01:58:23 +0000
> @@ -43,6 +43,7 @@
> 'InvalidNamespace',
> 'NoLinkedBranch',
> 'NoSuchBranch',
> + 'NoSuchGitReference',
> 'NoSuchGitRepository',
> 'PrivateBranchRecipe',
> 'ReviewNotPending',
> @@ -61,7 +62,10 @@
> from bzrlib.plugins.builder.recipe import RecipeParseError
> from lazr.restful.declarations import error_status
>
> -from lp.app.errors import NameLookupFailed
> +from lp.app.errors import (
> + NameLookupFailed,
> + NotFoundError,
> + )
>
> # Annotate the RecipeParseError's with a 400 webservice status.
> error_status(httplib.BAD_REQUEST)(RecipeParseError)
> @@ -402,6 +406,21 @@
> _message_prefix = "No such Git repository"
>
>
> +class NoSuchGitReference(NotFoundError):
> + """Raised when we try to look up a Git reference that does not exist."""
> +
> + def __init__(self, repository, path):
> + self.repository = repository
> + self.path = path
> + self.message = (
> + "The repository at %s does not contain a reference named '%s'." %
> + (repository.display_name, path))
> + NotFoundError.__init__(self, self.message)
> +
> + def __str__(self):
> + return self.message
> +
> +
> @error_status(httplib.CONFLICT)
> class GitDefaultConflict(Exception):
> """Raised when trying to set a Git repository as the default for
>
> === modified file 'lib/lp/code/interfaces/githosting.py'
> --- lib/lp/code/interfaces/githosting.py 2015-06-13 01:58:23 +0000
> +++ lib/lp/code/interfaces/githosting.py 2015-06-13 01:58:23 +0000
> @@ -23,6 +23,20 @@
> other physical path.
> """
>
> + def getProperties(path):
> + """Get properties of this repository.
> +
> + :param path: Physical path of the repository on the hosting service.
> + :return: A dict of properties.
> + """
> +
> + def setProperties(path, **props):
> + """Set properties of this repository.
> +
> + :param path: Physical path of the repository on the hosting service.
> + :param props: Properties to set.
> + """
> +
> def getRefs(path):
> """Get all refs in this repository.
>
>
> === modified file 'lib/lp/code/interfaces/gitrepository.py'
> --- lib/lp/code/interfaces/gitrepository.py 2015-06-13 01:58:23 +0000
> +++ lib/lp/code/interfaces/gitrepository.py 2015-06-13 01:58:23 +0000
> @@ -552,6 +552,19 @@
> """
>
>
> +class IGitRepositoryEditableAttributes(Interface):
> + """IGitRepository attributes that can be edited.
> +
> + These attributes need launchpad.View to see, and launchpad.Edit to change.
> + """
> +
> + default_branch = exported(TextLine(
> + title=_("Default branch"), required=False, readonly=False,
> + description=_(
> + "The full path to the default branch for this repository, e.g. "
> + "refs/heads/master.")))
> +
> +
> class IGitRepositoryEdit(Interface):
> """IGitRepository methods that require launchpad.Edit permission."""
>
> @@ -619,7 +632,8 @@
>
>
> class IGitRepository(IGitRepositoryView, IGitRepositoryModerateAttributes,
> - IGitRepositoryModerate, IGitRepositoryEdit):
> + IGitRepositoryModerate, IGitRepositoryEditableAttributes,
> + IGitRepositoryEdit):
> """A Git repository."""
>
> # Mark repositories as exported entries for the Launchpad API.
>
> === modified file 'lib/lp/code/model/githosting.py'
> --- lib/lp/code/model/githosting.py 2015-06-13 01:58:23 +0000
> +++ lib/lp/code/model/githosting.py 2015-06-13 01:58:23 +0000
> @@ -64,6 +64,9 @@
> def _post(self, path, **kwargs):
> return self._request("post", path, **kwargs)
>
> + def _patch(self, path, **kwargs):
> + return self._request("patch", path, **kwargs)
> +
> def _delete(self, path, **kwargs):
> return self._request("delete", path, **kwargs)
>
> @@ -78,6 +81,22 @@
> raise GitRepositoryCreationFault(
> "Failed to create Git repository: %s" % unicode(e))
>
> + def getProperties(self, path):
> + """See `IGitHostingClient`."""
> + try:
> + return self._get("/repo/%s" % path)
> + except Exception as e:
> + raise GitRepositoryScanFault(
> + "Failed to get properties of Git repository: %s" % unicode(e))
> +
> + def setProperties(self, path, **props):
> + """See `IGitHostingClient`."""
> + try:
> + self._patch("/repo/%s" % path, json_data=props)
> + except Exception as e:
> + raise GitRepositoryScanFault(
> + "Failed to set properties of Git repository: %s" % unicode(e))
> +
> def getRefs(self, path):
> try:
> return self._get("/repo/%s/refs" % path)
>
> === modified file 'lib/lp/code/model/gitjob.py'
> --- lib/lp/code/model/gitjob.py 2015-06-13 01:58:23 +0000
> +++ lib/lp/code/model/gitjob.py 2015-06-13 01:58:23 +0000
> @@ -28,6 +28,7 @@
> classProvides,
> implements,
> )
> +from zope.security.proxy import removeSecurityProxy
>
> from lp.app.errors import NotFoundError
> from lp.code.interfaces.githosting import IGitHostingClient
> @@ -212,6 +213,12 @@
> hosting_path, refs_to_upsert, logger=log)
> self.repository.synchroniseRefs(
> refs_to_upsert, refs_to_remove, logger=log)
> + props = getUtility(IGitHostingClient).getProperties(
> + hosting_path)
> + # We don't want ref canonicalisation, nor do we want to send
> + # this change back to the hosting service.
> + removeSecurityProxy(self.repository)._default_branch = (
> + props["default_branch"])
> except LostObjectError:
> log.info(
> "Skipping repository %s because it has been deleted." %
>
> === modified file 'lib/lp/code/model/gitrepository.py'
> --- lib/lp/code/model/gitrepository.py 2015-06-13 01:58:23 +0000
> +++ lib/lp/code/model/gitrepository.py 2015-06-13 01:58:23 +0000
> @@ -72,6 +72,7 @@
> CannotDeleteGitRepository,
> GitDefaultConflict,
> GitTargetError,
> + NoSuchGitReference,
> )
> from lp.code.event.git import GitRefsUpdatedEvent
> from lp.code.interfaces.branchmergeproposal import (
> @@ -206,6 +207,8 @@
> owner_default = Bool(name='owner_default', allow_none=False)
> target_default = Bool(name='target_default', allow_none=False)
>
> + _default_branch = Unicode(name='default_branch', allow_none=True)
> +
> def __init__(self, registrant, owner, target, name, information_type,
> date_created, reviewer=None, description=None):
> super(GitRepository, self).__init__()
> @@ -407,6 +410,21 @@
> GitRef.repository_id == self.id,
> GitRef.path.startswith(u"refs/heads/")).order_by(GitRef.path)
>
> + @property
> + def default_branch(self):
> + """See `IGitRepository`."""
> + return self._default_branch
> +
> + @default_branch.setter
> + def default_branch(self, value):
> + """See `IGitRepository`."""
> + ref = self.getRefByPath(value)
> + if ref is None:
> + raise NoSuchGitReference(self, value)
> + self._default_branch = ref.path
> + getUtility(IGitHostingClient).setProperties(
> + self.getInternalPath(), default_branch=ref.path)
Do we want to short-circuit if the value hasn't changed?
> +
> def getRefByPath(self, path):
> paths = [path]
> if not path.startswith(u"refs/heads/"):
>
> === modified file 'lib/lp/code/model/tests/test_gitjob.py'
> --- lib/lp/code/model/tests/test_gitjob.py 2015-06-13 01:58:23 +0000
> +++ lib/lp/code/model/tests/test_gitjob.py 2015-06-13 01:58:23 +0000
> @@ -148,9 +148,12 @@
> author_date_gen = time_counter(author_date_start, timedelta(days=1))
> hosting_client.getCommits = FakeMethod(
> result=self.makeFakeCommits(author, author_date_gen, paths))
> + hosting_client.getProperties = FakeMethod(
> + result={u"default_branch": u"refs/heads/master"})
> with dbuser("branchscanner"):
> JobRunner([job]).runAll()
> self.assertRefsMatch(repository.refs, repository, paths)
> + self.assertEqual(u"refs/heads/master", repository.default_branch)
>
> def test_logs_bad_ref_info(self):
> hosting_client = FakeGitHostingClient()
>
> === modified file 'lib/lp/code/model/tests/test_gitrepository.py'
> --- lib/lp/code/model/tests/test_gitrepository.py 2015-06-13 01:58:23 +0000
> +++ lib/lp/code/model/tests/test_gitrepository.py 2015-06-13 01:58:23 +0000
> @@ -1217,6 +1217,23 @@
> ) for path, sha1 in expected_sha1s]
> self.assertThat(repository.refs, MatchesSetwise(*matchers))
>
> + def test_set_default_branch(self):
> + hosting_client = FakeMethod()
> + hosting_client.setProperties = FakeMethod()
> + self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
> + repository = self.factory.makeGitRepository()
> + self.factory.makeGitRefs(
> + repository=repository,
> + paths=(u"refs/heads/master", u"refs/heads/new"))
> + removeSecurityProxy(repository)._default_branch = u"refs/heads/master"
> + with person_logged_in(repository.owner):
> + repository.default_branch = u"new"
> + self.assertEqual(
> + [((repository.getInternalPath(),),
> + {u"default_branch": u"refs/heads/new"})],
> + hosting_client.setProperties.calls)
> + self.assertEqual(u"refs/heads/new", repository.default_branch)
> +
>
> class TestGitRepositoryGetAllowedInformationTypes(TestCaseWithFactory):
> """Test `IGitRepository.getAllowedInformationTypes`."""
>
--
https://code.launchpad.net/~cjwatson/launchpad/git-default-branch/+merge/261892
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References
