[IMP] Always get maintainers from all branches

src/oca_github_bot/manifest.py

@@ -97,18 +97,6 @@ def set_manifest_version(addon_dir, version):
         f.write(manifest)
 
 
-def is_maintainer(username, addon_dirs):
-    for addon_dir in addon_dirs:
-        try:
-            manifest = get_manifest(addon_dir)
-        except NoManifestFound:
-            return False
-        maintainers = manifest.get("maintainers", [])
-        if username not in maintainers:
-            return False
-    return True
-
-
 def bump_version(version, mode):
     mo = VERSION_RE.match(version)
     if not mo:
@@ -249,20 +237,14 @@ def user_can_push(gh, org, repo, username, addons_dir, target_branch):
     current_branch = git_get_current_branch(cwd=addons_dir)
     try:
         check_call(["git", "checkout", target_branch], cwd=addons_dir)
-        result = is_maintainer(username, modified_addon_dirs)
+        result = is_maintainer(gh_repo, username, modified_addon_dirs)
     finally:
         check_call(["git", "checkout", current_branch], cwd=addons_dir)
 
     if result:
         return True
 
-    other_branches = list(config.MAINTAINER_CHECK_ODOO_RELEASES)
-    if target_branch in other_branches:
-        other_branches.remove(target_branch)
-
-    return is_maintainer_other_branches(
-        gh_repo, username, modified_addons, other_branches
-    )
+    return is_maintainer(gh_repo, username, modified_addons)
 
 
 def _get_manifest_from_api(gh_repo, addon, branch, manifest_file):
@@ -288,24 +270,49 @@ def _get_manifest_from_api(gh_repo, addon, branch, manifest_file):
         return None
 
 
-def is_maintainer_other_branches(gh_repo, username, modified_addons, other_branches):
-    """Check if username is maintainer of modified_addons in any configured branch.
+def get_maintainers(gh_repo, modified_addons, branches=None):
+    """Get maintainers of modified_addons in `branches`.
 
     The authenticated GitHub contents API is used to read manifests. This
     avoids rate-limiting and transient network errors that could spuriously deny
     maintainer privileges during migrations.
     """
+    if not branches:
+        branches = config.MAINTAINER_CHECK_ODOO_RELEASES
+
+    maintainers_dict = dict()
     for addon in modified_addons:
-        is_maintainer = False
-        for branch in other_branches:
-            manifest_file = (
-                "__openerp__.py" if float(branch) < 10.0 else "__manifest__.py"
-            )
+        maintainers_dict[addon] = set()
+        for branch in branches:
+            try:
+                branch_version = float(branch)
+            except ValueError:
+                # master, main, ...
+                manifest_file = "__manifest__.py"
+            else:
+                manifest_file = (
+                    "__openerp__.py" if branch_version < 10.0 else "__manifest__.py"
+                )
             manifest = _get_manifest_from_api(gh_repo, addon, branch, manifest_file)
-            if manifest and username in manifest.get("maintainers", []):
-                is_maintainer = True
-                break
+            if manifest:
+                branch_maintainers = manifest.get("maintainers", set())
+                maintainers_dict[addon] = maintainers_dict[addon].union(
+                    branch_maintainers
+                )
+    return maintainers_dict
+
+
+def is_maintainer(gh_repo, username, modified_addons, branches=None):
+    """Check if username is maintainer of modified_addons in any configured branch.
 
-        if not is_maintainer:
+    The authenticated GitHub contents API is used to read manifests. This
+    avoids rate-limiting and transient network errors that could spuriously deny
+    maintainer privileges during migrations.
+    """
+    if not branches:
+        branches = config.MAINTAINER_CHECK_ODOO_RELEASES
+    maintainers_dict = get_maintainers(gh_repo, modified_addons, branches=branches)
+    for _addon, maintainers in maintainers_dict.items():
+        if username not in maintainers:
             return False
     return True

src/oca_github_bot/tasks/mention_maintainer.py

@@ -4,8 +4,7 @@
 from .. import config, github
 from ..config import switchable
 from ..manifest import (
-    addon_dirs_in,
-    get_manifest,
+    get_maintainers,
     git_modified_addon_dirs,
     is_addon_dir,
 )
@@ -22,10 +21,6 @@ def mention_maintainer(org, repo, pr, dry_run=False):
         gh_pr = gh.pull_request(org, repo, pr)
         target_branch = gh_pr.base.ref
         with github.temporary_clone(org, repo, target_branch) as clonedir:
-            # Get maintainers existing before the PR changes
-            addon_dirs = addon_dirs_in(clonedir, installable_only=True)
-            maintainers_dict = get_maintainers(addon_dirs)
-
             # Get list of addons modified in the PR.
             pr_branch = f"tmp-pr-{pr}"
             check_call(
@@ -41,6 +36,10 @@ def mention_maintainer(org, repo, pr, dry_run=False):
                 d for d in modified_addon_dirs if is_addon_dir(d, installable_only=True)
             ]
 
+        # Get maintainers existing before the PR changes
+        gh_repo = gh.repository(org, repo)
+        maintainers_dict = get_maintainers(gh_repo, modified_addon_dirs)
+
         modified_addons_maintainers = set()
         for modified_addon in modified_addon_dirs:
             addon_maintainers = maintainers_dict.get(modified_addon, list())
@@ -81,15 +80,3 @@ def get_adopt_mention(pr_opener):
     if config.ADOPT_AN_ADDON_MENTION:
         return config.ADOPT_AN_ADDON_MENTION.format(pr_opener=pr_opener)
     return None
-
-
-def get_maintainers(addon_dirs):
-    """Get maintainer for each addon in `addon_dirs`.
-
-    :return: Dictionary {'addon_dir': <list of addon's maintainers>}
-    """
-    addon_maintainers_dict = dict()
-    for addon_dir in addon_dirs:
-        maintainers = get_manifest(addon_dir).get("maintainers", [])
-        addon_maintainers_dict.setdefault(addon_dir, maintainers)
-    return addon_maintainers_dict

tests/test_manifest.py

@@ -21,10 +21,11 @@
     is_addon_dir,
     is_addons_dir,
     is_maintainer,
-    is_maintainer_other_branches,
     set_manifest_version,
 )
 
+from .common import set_config
+
 
 def test_is_addons_dir_empty(tmpdir):
     tmpdir.mkdir("addon")
@@ -223,7 +224,7 @@ def test_get_odoo_series_from_version():
         get_odoo_series_from_version("12.0.1")
 
 
-def test_is_maintainer(tmp_path):
+def test_is_maintainer(mocker, tmp_path):
     addon1 = tmp_path / "addon1"
     addon1.mkdir()
     (addon1 / "__manifest__.py").write_text(
@@ -235,12 +236,22 @@ def test_is_maintainer(tmp_path):
     addon3 = tmp_path / "addon3"
     addon3.mkdir()
     (addon3 / "__manifest__.py").write_text("{'name': 'addon3'}")
-    assert is_maintainer("u1", [addon1])
-    assert not is_maintainer("u1", [addon2])
-    assert not is_maintainer("u1", [addon1, addon2])
-    assert is_maintainer("u2", [addon1, addon2])
-    assert not is_maintainer("u2", [addon1, addon2, addon3])
-    assert not is_maintainer("u1", [tmp_path / "not_an_addon"])
+    gh_repo_mock = mocker.MagicMock()
+
+    def _file_contents(path, ref=None):
+        file_contents = mocker.MagicMock()
+        file_contents.content = (tmp_path / path).read_bytes()
+        return file_contents
+
+    gh_repo_mock.file_contents.side_effect = _file_contents
+
+    with set_config(MAINTAINER_CHECK_ODOO_RELEASES="10.0"):
+        assert is_maintainer(gh_repo_mock, "u1", [addon1])
+        assert not is_maintainer(gh_repo_mock, "u1", [addon2])
+        assert not is_maintainer(gh_repo_mock, "u1", [addon1, addon2])
+        assert is_maintainer(gh_repo_mock, "u2", [addon1, addon2])
+        assert not is_maintainer(gh_repo_mock, "u2", [addon1, addon2, addon3])
+        assert not is_maintainer(gh_repo_mock, "u1", [tmp_path / "not_an_addon"])
 
 
 def test_is_maintainer_other_branches(mocker):
@@ -258,12 +269,8 @@ def _file_contents(path, ref=None):
 
     gh_repo_mock.file_contents.side_effect = _file_contents
 
-    assert is_maintainer_other_branches(
-        gh_repo_mock, "sbidoul", {"mis_builder"}, ["12.0"]
-    )
-    assert not is_maintainer_other_branches(
-        gh_repo_mock, "fpdoo", {"mis_builder"}, ["12.0"]
-    )
+    assert is_maintainer(gh_repo_mock, "sbidoul", {"mis_builder"}, ["12.0"])
+    assert not is_maintainer(gh_repo_mock, "fpdoo", {"mis_builder"}, ["12.0"])
 
 
 def test_is_maintainer_other_branches_with_gh(mocker):
@@ -273,7 +280,7 @@ def test_is_maintainer_other_branches_with_gh(mocker):
     file_contents_mock.content = b"{'name': 'addon1', 'maintainers': ['u1']}"
     gh_repo_mock.file_contents.return_value = file_contents_mock
 
-    assert is_maintainer_other_branches(gh_repo_mock, "u1", {"addon1"}, ["15.0"])
+    assert is_maintainer(gh_repo_mock, "u1", {"addon1"}, ["15.0"])
     gh_repo_mock.file_contents.assert_called_once_with(
         "addon1/__manifest__.py", ref="15.0"
     )
@@ -287,7 +294,5 @@ def test_is_maintainer_other_branches_api_errors(mocker, caplog):
     gh_repo_mock.file_contents.side_effect = [None, file_contents_mock]
     caplog.set_level(logging.WARNING, logger="oca_github_bot.manifest")
 
-    assert not is_maintainer_other_branches(
-        gh_repo_mock, "u1", {"addon1"}, ["15.0", "14.0"]
-    )
+    assert not is_maintainer(gh_repo_mock, "u1", {"addon1"}, ["15.0", "14.0"])
     assert "Failed to parse manifest addon1/__manifest__.py@14.0" in caplog.text