launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/code/browser/gitrepository.py'
> --- lib/lp/code/browser/gitrepository.py	2015-06-04 16:57:58 +0000
> +++ lib/lp/code/browser/gitrepository.py	2015-06-05 13:21:54 +0000
> @@ -24,12 +24,19 @@
>  from lazr.restful.interface import copy_field
>  from storm.expr import Desc
>  from zope.event import notify
> +from zope.formlib import form
>  from zope.interface import (
>      implements,
>      Interface,
>      providedBy,
>      )
> +from zope.schema import Choice
> +from zope.schema.vocabulary import (
> +    SimpleTerm,
> +    SimpleVocabulary,
> +    )
>  
> +from lp import _
>  from lp.app.browser.informationtype import InformationTypePortletMixin
>  from lp.app.browser.launchpadform import (
>      action,
> @@ -40,9 +47,14 @@
>  from lp.app.errors import NotFoundError
>  from lp.app.vocabularies import InformationTypeVocabulary
>  from lp.app.widgets.itemswidgets import LaunchpadRadioWidgetWithDescription
> -from lp.code.errors import GitRepositoryExists
> +from lp.code.errors import (
> +    GitRepositoryCreationForbidden,
> +    GitRepositoryExists,
> +    )
> +from lp.code.interfaces.gitnamespace import get_git_namespace
>  from lp.code.interfaces.gitref import IGitRefBatchNavigator
>  from lp.code.interfaces.gitrepository import IGitRepository
> +from lp.registry.vocabularies import UserTeamsParticipationPlusSelfVocabulary
>  from lp.services.config import config
>  from lp.services.database.constants import UTC_NOW
>  from lp.services.propertycache import cachedproperty
> @@ -102,7 +114,7 @@
>              ref_segments.append(segments.pop())
>              ref = self.context.getRefByPath("/".join(ref_segments))
>              if ref is not None:
> -                for _ in range(len(ref_segments)):
> +                for unused in range(len(ref_segments)):
>                      self.request.stepstogo.consume()
>                  return ref
>          raise NotFoundError
> @@ -264,6 +276,7 @@
>                  IGitRepository["information_type"], readonly=False,
>                  vocabulary=InformationTypeVocabulary(types=info_types))
>              name = copy_field(IGitRepository["name"], readonly=False)
> +            owner = copy_field(IGitRepository["owner"], readonly=False)
>              reviewer = copy_field(IGitRepository["reviewer"], required=True)
>  
>          return GitRepositoryEditSchema
> @@ -297,6 +310,14 @@
>              if name != self.context.name:
>                  self.context.setName(name, self.user)
>                  changed = True
> +        if "owner" in data:
> +            owner = data.pop("owner")
> +            if owner != self.context.owner:
> +                self.context.setOwner(owner, self.user)
> +                changed = True
> +                self.request.response.addNotification(
> +                    "The repository owner has been changed to %s (%s)" %
> +                    (owner.displayname, owner.name))
>          if "information_type" in data:
>              information_type = data.pop("information_type")
>              self.context.transitionToInformationType(
> @@ -360,12 +381,54 @@
>      """The main view for editing repository attributes."""
>  
>      field_names = [
> +        "owner",
>          "name",
>          "information_type",
>          ]
>  
>      custom_widget("information_type", LaunchpadRadioWidgetWithDescription)
>  
> +    def setUpFields(self):
> +        super(GitRepositoryEditView, self).setUpFields()
> +        repository = self.context
> +        # If the user can administer repositories, then they should be able
> +        # to assign the ownership of the repository to any valid person or
> +        # team.
> +        if check_permission("launchpad.Admin", repository):
> +            owner_field = self.schema["owner"]
> +            any_owner_choice = Choice(
> +                __name__="owner", title=owner_field.title,
> +                description=_(
> +                    "As an administrator you are able to assign this "
> +                    "repository to any person or team."),
> +                required=True, vocabulary="ValidPersonOrTeam")
> +            any_owner_field = form.Fields(
> +                any_owner_choice, render_context=self.render_context)
> +            # Replace the normal owner field with a more permissive vocab.
> +            self.form_fields = self.form_fields.omit("owner")
> +            self.form_fields = any_owner_field + self.form_fields
> +        else:
> +            # For normal users, there are some cases (e.g. package
> +            # repositories) where the editor may not be in the team of the
> +            # repository owner.  In these cases we need to extend the
> +            # vocabulary connected to the owner field.
> +            if not self.user.inTeam(self.context.owner):
> +                vocab = UserTeamsParticipationPlusSelfVocabulary()
> +                owner = self.context.owner
> +                terms = [SimpleTerm(
> +                    owner, owner.name, owner.unique_displayname)]
> +                terms.extend([term for term in vocab])
> +                owner_field = self.schema["owner"]
> +                owner_choice = Choice(
> +                    __name__="owner", title=owner_field.title,
> +                    description=owner_field.description,
> +                    required=True, vocabulary=SimpleVocabulary(terms))
> +                new_owner_field = form.Fields(
> +                    owner_choice, render_context=self.render_context)
> +                # Replace the normal owner field with a more permissive vocab.
> +                self.form_fields = self.form_fields.omit("owner")
> +                self.form_fields = new_owner_field + self.form_fields

Can you factor this out so it can be shared with the Branch equivalent?

> +
>      def _setRepositoryExists(self, existing_repository, field_name="name"):
>          owner = existing_repository.owner
>          if owner == self.user:
> @@ -379,12 +442,21 @@
>          self.setFieldError(field_name, message)
>  
>      def validate(self, data):
> -        if "name" in data:
> +        if "name" in data and "owner" in data:
>              name = data["name"]
> -            if name != self.context.name:
> -                namespace = self.context.namespace
> +            owner = data["owner"]
> +            if name != self.context.name or owner != self.context.owner:
> +                if self.context.owner == self.context.target:
> +                    target = owner
> +                else:
> +                    target = self.context.target
> +                namespace = get_git_namespace(target, owner)
>                  try:
>                      namespace.validateMove(self.context, self.user, name=name)
> +                except GitRepositoryCreationForbidden:
> +                    self.addError(
> +                        "%s is not allowed to own Git repositories in %s." %
> +                        (owner.displayname, target.displayname))
>                  except GitRepositoryExists as e:
>                      self._setRepositoryExists(e.existing_repository)
>  
> 
> === modified file 'lib/lp/code/browser/tests/test_gitrepository.py'
> --- lib/lp/code/browser/tests/test_gitrepository.py	2015-06-04 16:57:58 +0000
> +++ lib/lp/code/browser/tests/test_gitrepository.py	2015-06-05 13:21:54 +0000
> @@ -267,6 +267,45 @@
>          with person_logged_in(person):
>              self.assertEqual(u"bar", repository.name)
>  
> +    def test_change_owner(self):
> +        # An authorised user can change the owner to a team they're a member
> +        # of.
> +        person = self.factory.makePerson()
> +        team = self.factory.makeTeam(name="newowner", members=[person])
> +        repository = self.factory.makeGitRepository(owner=person)
> +        browser = self.getUserBrowser(
> +            canonical_url(repository) + "/+edit", user=person)
> +        browser.getControl(name="field.owner").value = ["newowner"]
> +        browser.getControl("Change Git Repository").click()
> +        with person_logged_in(person):
> +            self.assertEqual(team, repository.owner)
> +
> +    def test_change_owner_personal(self):
> +        # An authorised user can change the owner of a personal repository
> +        # to a team they're a member of.
> +        person = self.factory.makePerson()
> +        team = self.factory.makeTeam(name="newowner", members=[person])
> +        repository = self.factory.makeGitRepository(
> +            owner=person, target=person)
> +        browser = self.getUserBrowser(
> +            canonical_url(repository) + "/+edit", user=person)
> +        browser.getControl(name="field.owner").value = ["newowner"]
> +        browser.getControl("Change Git Repository").click()
> +        with person_logged_in(person):
> +            self.assertEqual(team, repository.owner)
> +            self.assertEqual(team, repository.target)
> +
> +    def test_cannot_change_owner_to_foreign_team(self):
> +        # A user cannot change the owner of their repository to a team
> +        # they're not a member of.
> +        person = self.factory.makePerson()
> +        self.factory.makeTeam(name="newowner")
> +        repository = self.factory.makeGitRepository(owner=person)
> +        browser = self.getUserBrowser(
> +            canonical_url(repository) + "/+edit", user=person)
> +        self.assertNotIn(
> +            "newowner", browser.getControl(name="field.owner").options)
> +
>      def test_information_type_in_ui(self):
>          # The information_type of a repository can be changed via the UI by
>          # an authorised user.
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-repository-ui-edit-owner/+merge/261221
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.