From baffa7dac75dc36fa7a012bfd77f080aeb25d0fa Mon Sep 17 00:00:00 2001 From: Jaime Lennox Date: Sat, 9 Mar 2019 10:42:46 +0000 Subject: [PATCH 1/2] Use IntEnum for AccessLevel The values should always be ints, and this makes the code simpler. --- marge/bot.py | 2 +- marge/project.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/marge/bot.py b/marge/bot.py index ae417e8a..11e2ae58 100644 --- a/marge/bot.py +++ b/marge/bot.py @@ -91,7 +91,7 @@ def _process_projects( for project in projects: project_name = project.path_with_namespace - if project.access_level.value < AccessLevel.reporter.value: + if project.access_level < AccessLevel.reporter: log.warning("Don't have enough permissions to browse merge requests in %s!", project_name) continue merge_requests = self._get_merge_requests(project, project_name) diff --git a/marge/project.py b/marge/project.py index 0a21aae3..789588c1 100644 --- a/marge/project.py +++ b/marge/project.py @@ -1,5 +1,5 @@ import logging as log -from enum import Enum, unique +from enum import IntEnum, unique from functools import partial from . import gitlab @@ -78,7 +78,7 @@ def access_level(self): @unique -class AccessLevel(Enum): +class AccessLevel(IntEnum): # See https://docs.gitlab.com/ce/api/access_requests.html guest = 10 reporter = 20 From 5f9a4a968348360283228f0b43ca64ac41efc83a Mon Sep 17 00:00:00 2001 From: Jaime Lennox Date: Sat, 9 Mar 2019 11:21:52 +0000 Subject: [PATCH 2/2] Only fetch projects with min access level If we're on an appropriate GitLab version, that is. GitLab doesn't return the right permissions for projects belonging to the user if the project is in a subgroup, and Marge is set as a member of the top-level group[1]. This is rather annoying as Marge will then complain that she doesn't have permissions for a project when she does. A new field available in GitLab version >= 11.2 allows us to only fetch projects for which we have a certain access level. This returns the right projects even with nested groups, so we can prefer this method if it is available. --- marge/project.py | 16 ++++++++++++++-- tests/test_project.py | 8 +++----- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/marge/project.py b/marge/project.py index 789588c1..422e29de 100644 --- a/marge/project.py +++ b/marge/project.py @@ -27,9 +27,17 @@ def filter_by_path_with_namespace(projects): @classmethod def fetch_all_mine(cls, api): + projects_kwargs = {'membership': True, 'with_merge_requests_enabled': True} + + # GitLab has an issue where projects may not show appropriate permissions in nested groups. Using + # `min_access_level` is known to provide the correct projects, so we'll prefer this method + # if it's available. See #156 for more details. + if api.version().release >= (11, 2): + projects_kwargs["min_access_level"] = int(AccessLevel.developer) + projects_info = api.collect_all_pages(GET( '/projects', - {'membership': True, 'with_merge_requests_enabled': True}, + projects_kwargs, )) def project_seems_ok(project_info): @@ -43,7 +51,11 @@ def project_seems_ok(project_info): return permissions_ok - return [cls(api, project_info) for project_info in projects_info if project_seems_ok(project_info)] + return [ + cls(api, project_info) + for project_info in projects_info + if "min_access_level" in projects_kwargs or project_seems_ok(project_info) + ] @property def path_with_namespace(self): diff --git a/tests/test_project.py b/tests/test_project.py index 2a662b63..f37dbec6 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -1,7 +1,7 @@ from unittest.mock import Mock import pytest -from marge.gitlab import Api, GET +from marge.gitlab import Api, GET, Version from marge.project import AccessLevel, Project @@ -67,12 +67,10 @@ def test_fetch_all_mine(self): api = self.api api.collect_all_pages = Mock(return_value=[prj1, prj2]) + api.version = Mock(return_value=Version.parse("11.2.0-ee")) result = Project.fetch_all_mine(api) - api.collect_all_pages.assert_called_once_with(GET( - '/projects', - {'membership': True, 'with_merge_requests_enabled': True}, - )) + api.collect_all_pages.assert_called_once() assert [prj.info for prj in result] == [prj1, prj2] def test_properties(self):