api: optimize sync process with batch task execution

AUTHORS.rst

@@ -35,3 +35,4 @@ Invenio module that adds GitHub integration to the platform.
 - Jiri Kuncar <[email protected]>
 - Lars Holm Nielsen <[email protected]>
 - Tibor Simko <[email protected]>
+- Till Korten <[email protected]>

invenio_github/api.py

@@ -246,7 +246,11 @@ def _sync_hooks(self, repos, asynchronous=True):
         else:
             # If hooks will run asynchronously, we need to commit any changes done so far
             db.session.commit()
-            sync_hooks_task.delay(self.user_id, repos)
+            # Create a task for several batches of repositories to speed up the process
+            # and ensure that each task finishes within the time limit on Zenodo
+            batch_size = current_app.config["GITHUB_WEBHOOK_SYNC_BATCH_SIZE"]
+            for i in range(0, len(repos), batch_size):
+                sync_hooks_task.delay(self.user_id, repos[i : i + batch_size])
 
     def _valid_webhook(self, url):
         """Check if webhook url is valid.

invenio_github/assets/semantic-ui/js/invenio_github/index.js

@@ -75,7 +75,7 @@ if (sync_button) {
             resultMessage,
             "positive",
             "checkmark",
-            "Repositories synced successfully. Please reload the page."
+            "Repositories synced successfully. Some items may still be updating, please reload the page in about a minute."
           );
           sync_button.classList.remove("disabled");
           setTimeout(function () {
@@ -95,7 +95,7 @@ if (sync_button) {
         }
       } catch (error) {
         loaderIcon.classList.remove("loading");
-        if(error.message === "timeout"){
+        if (error.message === "timeout") {
           addResultMessage(
             resultMessage,
             "warning",
@@ -110,7 +110,7 @@ if (sync_button) {
             "cancel",
             `There has been a problem: ${error}`
           );
-           setTimeout(function () {
+          setTimeout(function () {
             resultMessage.classList.add("hidden");
           }, 7000);
         }

invenio_github/config.py

@@ -41,6 +41,16 @@
     context, doesn't work as expected.
 """
 
+GITHUB_WEBHOOK_SYNC_BATCH_SIZE = 20
+"""Number of repositories to be processed in a single batch when syncing hooks. 
+
+If the user has more than 20 repositories, multiple tasks will be created, 
+syncing them in parallel. Thereby the sync process should finish in a timely 
+manner and we avoid timeouts on platforms like Zenodo.
+
+Decrease this value if you experience task timeouts.
+"""
+
 GITHUB_SHARED_SECRET = "CHANGEME"
 """Shared secret between you and GitHub.
 

invenio_github/views/github.py

@@ -155,7 +155,7 @@ def sync_user_repositories():
         """
         try:
             github = GitHubAPI(user_id=current_user.id)
-            github.sync(async_hooks=False)
+            github.sync()
             db.session.commit()
         except Exception as exc:
             current_app.logger.exception(str(exc))
@@ -171,7 +171,7 @@ def init_user_github():
         try:
             github = GitHubAPI(user_id=current_user.id)
             github.init_account()
-            github.sync(async_hooks=False)
+            github.sync()
             db.session.commit()
         except Exception as exc:
             current_app.logger.exception(str(exc))

tests/conftest.py

@@ -207,16 +207,25 @@ def test_repo_data_three():
     return {"name": "arepo", "id": 3}
 
 
-@pytest.fixture()
-def github_api(
-    running_app,
-    db,
-    test_repo_data_one,
-    test_repo_data_two,
-    test_repo_data_three,
-    test_user,
+def _create_github_api_mock(
+    test_repo_data_one, test_repo_data_two, test_repo_data_three
 ):
-    """Github API mock."""
+    """Create a GitHub API mock with common configuration.
+
+    Parameters
+    ----------
+    test_repo_data_one : dict
+        Test repository data for first repo
+    test_repo_data_two : dict
+        Test repository data for second repo
+    test_repo_data_three : dict
+        Test repository data for third repo
+
+    Returns
+    -------
+    MagicMock
+        Configured GitHub API mock object
+    """
     mock_api = MagicMock()
     mock_api.session = MagicMock()
     mock_api.me.return_value = github3.users.User(
@@ -230,11 +239,15 @@ def github_api(
         ),
         mock_api.session,
     )
-    repo_1.hooks = MagicMock(return_value=[])
-    repo_1.file_contents = MagicMock(return_value=None)
     # Mock hook creation to retun the hook id '12345'
     hook_instance = MagicMock()
     hook_instance.id = 12345
+    hook_instance.config = {
+        "url": "http://localhost:5000/api/receivers/github/events/?access_token=test"
+    }
+
+    repo_1.hooks = MagicMock(return_value=[hook_instance])
+    repo_1.file_contents = MagicMock(return_value=None)
     repo_1.create_hook = MagicMock(return_value=hook_instance)
 
     repo_2 = github3.repos.Repository(
@@ -305,6 +318,49 @@ def mock_head_status_by_repo_url(url, **kwargs):
     mock_api.session.head.side_effect = mock_head_status_by_repo_url
     mock_api.session.get.return_value = MagicMock(raw=ZIPBALL())
 
+    return mock_api
+
+
+@pytest.fixture()
+def github_api(
+    running_app,
+    db,
+    test_repo_data_one,
+    test_repo_data_two,
+    test_repo_data_three,
+    test_user,
+):
+    """Github API mock."""
+    mock_api = _create_github_api_mock(
+        test_repo_data_one, test_repo_data_two, test_repo_data_three
+    )
+
     with patch("invenio_github.api.GitHubAPI.api", new=mock_api):
         with patch("invenio_github.api.GitHubAPI._sync_hooks"):
+            with patch(
+                "invenio_github.api.GitHubAPI._valid_webhook", return_value=True
+            ):
+                yield mock_api
+
+
+@pytest.fixture()
+def github_api_with_hooks(
+    running_app,
+    db,
+    test_repo_data_one,
+    test_repo_data_two,
+    test_repo_data_three,
+    test_user,
+):
+    """Github API mock with _sync_hooks not patched.
+
+    This fixture is similar to github_api but allows _sync_hooks to run normally
+    instead of being mocked out.
+    """
+    mock_api = _create_github_api_mock(
+        test_repo_data_one, test_repo_data_two, test_repo_data_three
+    )
+
+    with patch("invenio_github.api.GitHubAPI.api", new=mock_api):
+        with patch("invenio_github.api.GitHubAPI._valid_webhook", return_value=True):
             yield mock_api

tests/test_api.py

@@ -7,12 +7,14 @@
 """Test invenio-github api."""
 
 import json
+import time
+from unittest.mock import patch
 
 import pytest
 from invenio_webhooks.models import Event
 
 from invenio_github.api import GitHubAPI, GitHubRelease
-from invenio_github.models import Release, ReleaseStatus
+from invenio_github.models import Release, ReleaseStatus, Repository
 
 from .fixtures import PAYLOAD as github_payload_fixture
 
@@ -117,3 +119,131 @@ def test_release_branch_tag_conflict(app, test_user, github_api):
         assert resolved_url == ref_tag_url
         # Check that the original zipball URL from the event payload is not the same
         assert rel_api.release_zipball_url != ref_tag_url
+
+
+def test_sync_basic(app, test_user, github_api):
+    """Test basic sync functionality."""
+    api = GitHubAPI(test_user.id)
+    api.init_account()
+
+    # Test sync without hooks
+    api.sync(hooks=False)
+
+    # Verify account extra data is updated
+    assert api.account.extra_data["repos"] is not None
+    assert api.account.extra_data["last_sync"] is not None
+
+
+def test_sync_updates_repo_names(app, test_user, github_api, db):
+    """Test that sync updates repository names when they change on GitHub."""
+
+    api = GitHubAPI(test_user.id)
+    api.init_account()
+
+    # Create a repository with old name
+    repo = Repository.create(test_user.id, 1, "old-name/repo")
+    db.session.add(repo)
+    db.session.commit()
+
+    old_repo = Repository.query.filter_by(github_id=1).first()
+    assert old_repo.name == "old-name/repo"
+
+    # Sync (GitHub API mock should return updated name)
+    api.sync(hooks=False)
+
+    # Check if repository name was updated
+    updated_repo = Repository.query.filter_by(github_id=1).first()
+    assert updated_repo.name == "auser/repo-1"
+
+
+def test_sync_updates_account_extra_data(app, test_user, github_api):
+    """Test that sync properly updates account extra data."""
+    api = GitHubAPI(test_user.id)
+    api.init_account()
+
+    assert "last_sync" in api.account.extra_data
+    old_last_sync = api.account.extra_data["last_sync"]
+
+    # Sync
+    api.sync(hooks=False)
+
+    # Verify extra data was updated
+    assert api.account.extra_data["repos"] is not None
+    assert api.account.extra_data["last_sync"] != old_last_sync
+
+
+def test_sync_updates_hooks_asynchronously(app, test_user, github_api_with_hooks, db):
+    """Test that sync properly updates hooks when async_hooks=True."""
+    api = GitHubAPI(test_user.id)
+    api.init_account()
+
+    num_repos = len(api.api.repositories())
+
+    # patch the GitHubAPI to simulate some delay in hook synchronization
+    delay = 0.2
+    expected_duration = delay * num_repos
+    with patch.object(
+        api, "sync_repo_hook", side_effect=lambda *args, **kwargs: time.sleep(delay)
+    ):
+        # Measure how long the Sync takes with async_hooks=True
+        start_time = time.time()
+        api.sync(async_hooks=True)
+        duration = time.time() - start_time
+        assert duration < expected_duration
+
+        # Measure how long the Sync takes with async_hooks=False
+        start_time = time.time()
+        api.sync(hooks=True, async_hooks=False)
+        duration = time.time() - start_time
+        # assert that sync_repo_hook was called once for each of the three repos
+        # during sync with async_hooks=False
+        assert api.sync_repo_hook.call_count == num_repos
+        # assert that the duration is at least three times the simulated delay
+        assert duration >= expected_duration
+
+
+def test_sync_repo_hook_creates_repo_when_hook_exists(app, test_user, github_api, db):
+    """Test that sync_repo_hook creates a repository when a valid hook exists on GitHub."""
+    api = GitHubAPI(test_user.id)
+    api.init_account()
+
+    repo_id = 1
+
+    # Sync repo hook - should create repository since hook exists in mocked GitHub
+    api.sync_repo_hook(repo_id)
+
+    # Commit changes to ensure repository is persisted
+    db.session.commit()
+
+    # Verify repository was created
+    repo = Repository.query.filter_by(github_id=repo_id).first()
+    assert repo is not None
+    assert repo.github_id == repo_id
+    assert repo.name == "auser/repo-1"
+    assert repo.user_id == test_user.id
+    assert repo.hook is not None
+
+
+def test_sync_repo_hook_enables_existing_repo_when_hook_exists(
+    app, test_user, github_api, db
+):
+    """Test that sync_repo_hook enables an existing disabled repository when hook exists."""
+    api = GitHubAPI(test_user.id)
+    api.init_account()
+
+    repo_id = 1
+    repo_name = "auser/repo-1"
+
+    # Create a disabled repository
+    repo = Repository.create(test_user.id, repo_id, repo_name)
+    repo.hook = 0
+    db.session.add(repo)
+    db.session.commit()
+
+    # Sync repo hook
+    api.sync_repo_hook(repo_id)
+
+    # Verify repository was enabled
+    updated_repo = Repository.query.filter_by(github_id=repo_id).first()
+    assert updated_repo.user_id == test_user.id
+    assert updated_repo.hook > 0