launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/code/interfaces/gitrepository.py'
> --- lib/lp/code/interfaces/gitrepository.py	2016-10-05 09:54:42 +0000
> +++ lib/lp/code/interfaces/gitrepository.py	2016-10-13 12:29:37 +0000
> @@ -946,14 +951,24 @@
>          return identities
>  
>  
> -def user_has_special_git_repository_access(user):
> +def user_has_special_git_repository_access(user, repository=None):
>      """Admins have special access.
>  
>      :param user: An `IPerson` or None.
> +    :param repository: An `IGitRepository` or None when checking collection
> +        access.
>      """
>      if user is None:
>          return False
>      roles = IPersonRoles(user)
>      if roles.in_admin:
>          return True
> -    return False
> +    if repository is None:
> +        return False
> +    code_import = repository.code_import
> +    if code_import is None:
> +        return False
> +    return (
> +        roles.in_vcs_imports
> +        or (IPersonRoles(repository.owner).in_vcs_imports
> +            and user.inTerm(code_import.registrant)))

Registrant? Really? Can you delete that disjunct from user_has_special_branch_access too? I don't see how the repository owner's participation is relevant to anything.

Especially since "inTerm" suggests it isn't tested.

> 
> === modified file 'lib/lp/code/model/gitrepository.py'
> --- lib/lp/code/model/gitrepository.py	2016-10-05 09:29:41 +0000
> +++ lib/lp/code/model/gitrepository.py	2016-10-13 12:29:37 +0000
> @@ -1250,6 +1261,17 @@
>          self.affected_object.prerequisite_git_commit_sha1 = None
>  
>  
> +class DeleteCodeImport(DeletionOperation):
> +    """Deletion operation that deletes a repository's import."""
> +
> +    def __init__(self, code_import):
> +        DeletionOperation.__init__(
> +            self, code_import, msg("This is the import data for this branch."))

s/branch/repository/g

> +
> +    def __call__(self):
> +        getUtility(ICodeImportSet).delete(self.affected_object)
> +
> +
>  @implementer(IGitRepositorySet)
>  class GitRepositorySet:
>      """See `IGitRepositorySet`."""


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-code-import-security-deletion/+merge/308319
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.