From 9ee6d648f327a191cfaa8a58e24eb2b0062ccc44 Mon Sep 17 00:00:00 2001 From: Martin Varga Date: Thu, 3 Jul 2025 11:58:37 +0200 Subject: [PATCH 01/29] Add example test for cli --- server/mergin/tests/fixtures.py | 5 +++ server/mergin/tests/test_cli.py | 71 +++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) create mode 100644 server/mergin/tests/test_cli.py diff --git a/server/mergin/tests/fixtures.py b/server/mergin/tests/fixtures.py index 7cff688e..389cd6ac 100644 --- a/server/mergin/tests/fixtures.py +++ b/server/mergin/tests/fixtures.py @@ -238,3 +238,8 @@ def diff_project(app): finally: os.remove(test_gpkg_file) return project + + +@pytest.fixture() +def runner(app): + return app.test_cli_runner() diff --git a/server/mergin/tests/test_cli.py b/server/mergin/tests/test_cli.py new file mode 100644 index 00000000..54cbbdc2 --- /dev/null +++ b/server/mergin/tests/test_cli.py @@ -0,0 +1,71 @@ +# Copyright (C) Lutra Consulting Limited +# +# SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial + +import pytest +from mergin.sync.models import Project +from mergin.tests import ( + test_project, + test_workspace_id, + test_workspace_name, + DEFAULT_USER, +) + + +test_create_project_data = [ + # missing arguments + (("my_project",), 2, "Missing argument"), + # existing project + ( + ( + test_project, + test_workspace_name, + DEFAULT_USER[0], + ), + 0, + "ERROR: Project name already exists", + ), + # not existing creator user + ( + ( + "my_project", + test_workspace_name, + "not_existing", + ), + 0, + "ERROR: User not found", + ), + # not existing workspace + ( + ( + "my_project", + "not_existing", + DEFAULT_USER[0], + ), + 0, + "ERROR: Workspace not found", + ), + # success + ( + ( + "my_project", + test_workspace_name, + DEFAULT_USER[0], + ), + 0, + "Project created", + ), +] + + +@pytest.mark.parametrize("args,code,output", test_create_project_data) +def test_create_project(runner, args, code, output): + """Test create project command""" + result = runner.invoke(args=["project", "create", *args]) + assert result.exit_code == code + assert output in result.output + + if output == "Project created": + assert Project.query.filter_by( + name="my_project", workspace_id=test_workspace_id + ).first() From cecb04d43c0fee90a0fbb0ab9172555fd2e50fc0 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Mon, 21 Jul 2025 14:08:20 +0200 Subject: [PATCH 02/29] typo --- .../lib/src/common/components/data-view/DataViewWrapper.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web-app/packages/lib/src/common/components/data-view/DataViewWrapper.vue b/web-app/packages/lib/src/common/components/data-view/DataViewWrapper.vue index 8b936d85..eb7460ab 100644 --- a/web-app/packages/lib/src/common/components/data-view/DataViewWrapper.vue +++ b/web-app/packages/lib/src/common/components/data-view/DataViewWrapper.vue @@ -45,7 +45,7 @@ @click.prevent="!item.disabled && $emit('rowClick', item)" >
- +
Date: Mon, 21 Jul 2025 14:09:15 +0200 Subject: [PATCH 03/29] simplify test with sso user --- server/mergin/tests/test_auth.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/server/mergin/tests/test_auth.py b/server/mergin/tests/test_auth.py index 3a4c2de4..c6c039c4 100644 --- a/server/mergin/tests/test_auth.py +++ b/server/mergin/tests/test_auth.py @@ -503,11 +503,8 @@ def test_update_user_profile(client): assert user.email == "changed_email@mergin.co.uk" # do not allow to update sso user - sso_user = add_user("sso_user", "sso") + sso_user = add_user("sso_user", "") login(client, sso_user.username, "sso") - sso_user.passwd = None - db.session.add(sso_user) - db.session.commit() resp = client.post( url_for("/.mergin_auth_controller_update_user_profile"), data=json.dumps({"email": "changed_email@sso.co.uk"}), From af7d422d6944ee915f66d1f456cd05d0a9ec5cdb Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Thu, 24 Jul 2025 10:28:21 +0200 Subject: [PATCH 04/29] Add normalize input --- server/mergin/auth/commands.py | 17 ++++++++++------- server/mergin/utils.py | 13 +++++++++++++ 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/server/mergin/auth/commands.py b/server/mergin/auth/commands.py index 80cc7fb8..39738398 100644 --- a/server/mergin/auth/commands.py +++ b/server/mergin/auth/commands.py @@ -2,12 +2,14 @@ # # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial +import sys import click from flask import Flask from sqlalchemy import or_, func from ..app import db from .models import User, UserProfile +from ..utils import normalize_input def add_commands(app: Flask): @@ -17,24 +19,25 @@ def user(): pass @user.command() - @click.argument("username") + @click.argument("username", callback=normalize_input()) @click.argument("password") @click.option("--is-admin", is_flag=True) - @click.option("--email", required=True) + @click.option("--email", required=True, callback=normalize_input()) def create(username, password, is_admin, email): # pylint: disable=W0612 """Create user account""" user = User.query.filter( or_( - func.lower(User.username) == func.lower(username), - func.lower(User.email) == func.lower(email), + func.lower(User.username) == username, + func.lower(User.email) == email, ) - ).first() + ).count() if user: - print("ERROR: User already exists!\n") - return + click.secho("ERROR: User already exists", fg="red", err=True) + sys.exit(1) user = User(username=username, passwd=password, is_admin=is_admin, email=email) user.profile = UserProfile() user.active = True db.session.add(user) db.session.commit() + click.secho("User created", fg="green") diff --git a/server/mergin/utils.py b/server/mergin/utils.py index 9acc6124..4396de05 100644 --- a/server/mergin/utils.py +++ b/server/mergin/utils.py @@ -135,3 +135,16 @@ def save_diagnostic_log_file(app: str, username: str, body: bytes) -> str: f.write(content) return file_name + + +def normalize_input(lowercase=True, strip=True): + def _normalize(ctx, param, value): + if value is None: + return value + if strip: + value = value.strip() + if lowercase: + value = value.lower() + return value + + return _normalize From 19bb6ca5462e8e67cd23ba815ac79aef487f312a Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Thu, 24 Jul 2025 10:29:43 +0200 Subject: [PATCH 05/29] test_create_user --- server/mergin/tests/fixtures.py | 1 - server/mergin/tests/test_cli.py | 47 +++++++++++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/server/mergin/tests/fixtures.py b/server/mergin/tests/fixtures.py index 389cd6ac..368eba6b 100644 --- a/server/mergin/tests/fixtures.py +++ b/server/mergin/tests/fixtures.py @@ -6,7 +6,6 @@ import shutil import sys import uuid -from copy import deepcopy from shutil import copy, move from flask import current_app from sqlalchemy import desc diff --git a/server/mergin/tests/test_cli.py b/server/mergin/tests/test_cli.py index 54cbbdc2..c5c754a9 100644 --- a/server/mergin/tests/test_cli.py +++ b/server/mergin/tests/test_cli.py @@ -3,6 +3,8 @@ # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial import pytest + +from mergin.auth.models import User from mergin.sync.models import Project from mergin.tests import ( test_project, @@ -11,7 +13,6 @@ DEFAULT_USER, ) - test_create_project_data = [ # missing arguments (("my_project",), 2, "Missing argument"), @@ -68,4 +69,46 @@ def test_create_project(runner, args, code, output): if output == "Project created": assert Project.query.filter_by( name="my_project", workspace_id=test_workspace_id - ).first() + ).count() + + +test_create_user_data = [ + ( + ( + f" {DEFAULT_USER[0].upper()} ", + "Il0veMergin", + "--email", + f"{DEFAULT_USER[0]}@example.com", + ), + "ERROR: User already exists", + 1, + ), # existing username after lowercasing and stripping whitespaces + ( + ("cli_user", "Il0veMergin"), + "Error: Missing option '--email'", + 2, + ), # missing email argument + ( + ( + " cli_user ", + "Il0veMergin", + "--is-admin", + "--email", + " CLI_USER@example.com ", + ), + "User created", + 0, + ), # success +] + + +@pytest.mark.parametrize("args,output,code", test_create_user_data) +def test_create_user(runner, args, output, code): + """Test create user command""" + result = runner.invoke(args=["user", "create", *args]) + assert result.exit_code == code + assert output in result.output + if code == 0: + user = User.query.filter_by(username="cli_user").first() + assert user.is_admin + assert user.email == "cli_user@example.com" From 82bbe17cc6e71e634f50d2afc20c367e4505cd65 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Thu, 24 Jul 2025 11:06:13 +0200 Subject: [PATCH 06/29] unify create project --- server/mergin/sync/commands.py | 25 ++++++++++++++----------- server/mergin/sync/workspace.py | 2 +- server/mergin/tests/test_cli.py | 8 ++++---- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/server/mergin/sync/commands.py b/server/mergin/sync/commands.py index 97e85981..5083efc4 100644 --- a/server/mergin/sync/commands.py +++ b/server/mergin/sync/commands.py @@ -3,17 +3,20 @@ # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial import shutil +import sys import click import os import secrets from datetime import datetime from flask import Flask, current_app +from sqlalchemy import func from .files import UploadChanges from ..app import db from .models import Project, ProjectVersion from .utils import split_project_path from ..auth.models import User +from ..utils import normalize_input def add_commands(app: Flask): @@ -23,23 +26,23 @@ def project(): pass @project.command() - @click.argument("name") - @click.argument("namespace") - @click.argument("username") + @click.argument("name", callback=normalize_input(lowercase=False)) + @click.argument("namespace", callback=normalize_input()) + @click.argument("username", callback=normalize_input()) def create(name, namespace, username): # pylint: disable=W0612 """Create blank project""" workspace = current_app.ws_handler.get_by_name(namespace) if not workspace: - print("ERROR: Workspace not found") - return - user = User.query.filter_by(username=username).first() + click.secho("ERROR: Workspace not found", fg="red", err=True) + sys.exit(1) + user = User.query.filter(func.lower(User.username) == username).first() if not user: - print("ERROR: User not found") - return + click.secho("ERROR: User not found", fg="red", err=True) + sys.exit(1) p = Project.query.filter_by(name=name, workspace_id=workspace.id).first() if p: - print("ERROR: Project name already exists") - return + click.secho("ERROR: Project name already exists", fg="red", err=True) + sys.exit(1) project_params = dict( creator=user, name=name, @@ -57,7 +60,7 @@ def create(name, namespace, username): # pylint: disable=W0612 pv.project = p db.session.commit() os.makedirs(p.storage.project_dir, exist_ok=True) - print("Project created") + click.secho("Project created", fg="green") @project.command() @click.argument("project-name") diff --git a/server/mergin/sync/workspace.py b/server/mergin/sync/workspace.py index 3d89a7fb..76245ef3 100644 --- a/server/mergin/sync/workspace.py +++ b/server/mergin/sync/workspace.py @@ -138,7 +138,7 @@ def get(self, id_): return self.factory_method() def get_by_name(self, name): - if name != Configuration.GLOBAL_WORKSPACE: + if name.lower() != Configuration.GLOBAL_WORKSPACE.lower(): return return self.factory_method() diff --git a/server/mergin/tests/test_cli.py b/server/mergin/tests/test_cli.py index c5c754a9..d56ba353 100644 --- a/server/mergin/tests/test_cli.py +++ b/server/mergin/tests/test_cli.py @@ -23,7 +23,7 @@ test_workspace_name, DEFAULT_USER[0], ), - 0, + 1, "ERROR: Project name already exists", ), # not existing creator user @@ -33,7 +33,7 @@ test_workspace_name, "not_existing", ), - 0, + 1, "ERROR: User not found", ), # not existing workspace @@ -43,7 +43,7 @@ "not_existing", DEFAULT_USER[0], ), - 0, + 1, "ERROR: Workspace not found", ), # success @@ -66,7 +66,7 @@ def test_create_project(runner, args, code, output): assert result.exit_code == code assert output in result.output - if output == "Project created": + if code == 0: assert Project.query.filter_by( name="my_project", workspace_id=test_workspace_id ).count() From 1fdac0166fd3537f2b83a03534ad3f04c25c11b8 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Thu, 24 Jul 2025 15:07:15 +0200 Subject: [PATCH 07/29] test_download_project --- server/mergin/sync/commands.py | 31 ++++--- server/mergin/tests/test_cli.py | 144 +++++++++++++++++++++++--------- 2 files changed, 126 insertions(+), 49 deletions(-) diff --git a/server/mergin/sync/commands.py b/server/mergin/sync/commands.py index 5083efc4..301f1c17 100644 --- a/server/mergin/sync/commands.py +++ b/server/mergin/sync/commands.py @@ -57,15 +57,20 @@ def create(name, namespace, username): # pylint: disable=W0612 db.session.add(p) changes = UploadChanges(added=[], updated=[], removed=[]) pv = ProjectVersion(p, 0, user.id, changes, "127.0.0.1") - pv.project = p + db.session.add(pv) db.session.commit() os.makedirs(p.storage.project_dir, exist_ok=True) click.secho("Project created", fg="green") @project.command() - @click.argument("project-name") + @click.argument("project-name", callback=normalize_input(lowercase=False)) @click.option("--version", type=int, required=True) - @click.option("--directory", type=click.Path(), required=True) + @click.option( + "--directory", + type=click.Path(), + required=True, + callback=normalize_input(lowercase=False), + ) def download(project_name, version, directory): # pylint: disable=W0612 """Download files for project at particular version""" ws, name = split_project_path(project_name) @@ -76,15 +81,19 @@ def download(project_name, version, directory): # pylint: disable=W0612 .first() ) if not project: - print("ERROR: Project does not exist") - return + click.secho("ERROR: Project does not exist", fg="red", err=True) + sys.exit(1) pv = ProjectVersion.query.filter_by(project_id=project.id, name=version).first() if not pv: - print("ERROR:Project version does not exist") - return + click.secho("ERROR: Project version does not exist", fg="red", err=True) + sys.exit(1) if os.path.exists(directory): - print(f"ERROR: Destination directory {directory} already exist") - return + click.secho( + f"ERROR: Destination directory '{directory}' already exist", + fg="red", + err=True, + ) + sys.exit(1) os.mkdir(directory) files = pv.files @@ -97,7 +106,7 @@ def download(project_name, version, directory): # pylint: disable=W0612 os.path.join(project.storage.project_dir, f.location), os.path.join(directory, f.path), ) - print("Project downloaded successfully") + click.secho("Project downloaded", fg="green") @project.command() @click.argument("project-name") @@ -117,6 +126,6 @@ def remove(project_name): print("ERROR: Project does not exist") return project.removed_at = datetime.utcnow() - project.removed_by = None + project.removed_by = "cli_command" db.session.commit() print("Project removed successfully") diff --git a/server/mergin/tests/test_cli.py b/server/mergin/tests/test_cli.py index d56ba353..7c852840 100644 --- a/server/mergin/tests/test_cli.py +++ b/server/mergin/tests/test_cli.py @@ -1,51 +1,55 @@ # Copyright (C) Lutra Consulting Limited # # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial +import shutil import pytest +import os +from pathlib import Path from mergin.auth.models import User -from mergin.sync.models import Project +from mergin.sync.models import Project, ProjectVersion from mergin.tests import ( test_project, test_workspace_id, test_workspace_name, DEFAULT_USER, ) +from mergin.sync.config import Configuration as sync_config test_create_project_data = [ # missing arguments - (("my_project",), 2, "Missing argument"), - # existing project - ( - ( - test_project, - test_workspace_name, - DEFAULT_USER[0], - ), - 1, - "ERROR: Project name already exists", - ), - # not existing creator user - ( - ( - "my_project", - test_workspace_name, - "not_existing", - ), - 1, - "ERROR: User not found", - ), - # not existing workspace - ( - ( - "my_project", - "not_existing", - DEFAULT_USER[0], - ), - 1, - "ERROR: Workspace not found", - ), + # (("my_project",), 2, "Missing argument"), + # # existing project + # ( + # ( + # test_project, + # test_workspace_name, + # DEFAULT_USER[0], + # ), + # 1, + # "ERROR: Project name already exists", + # ), + # # not existing creator user + # ( + # ( + # "my_project", + # test_workspace_name, + # "not_existing", + # ), + # 1, + # "ERROR: User not found", + # ), + # # not existing workspace + # ( + # ( + # "my_project", + # "not_existing", + # DEFAULT_USER[0], + # ), + # 1, + # "ERROR: Workspace not found", + # ), # success ( ( @@ -60,16 +64,18 @@ @pytest.mark.parametrize("args,code,output", test_create_project_data) -def test_create_project(runner, args, code, output): +def test_manipulate_project(runner, args, code, output): """Test create project command""" - result = runner.invoke(args=["project", "create", *args]) - assert result.exit_code == code - assert output in result.output + # create project + create = runner.invoke(args=["project", "create", *args]) + assert create.exit_code == code + assert output in create.output if code == 0: - assert Project.query.filter_by( + project = Project.query.filter_by( name="my_project", workspace_id=test_workspace_id - ).count() + ).first() + assert ProjectVersion.query.filter_by(project_id=project.id).count() test_create_user_data = [ @@ -112,3 +118,65 @@ def test_create_user(runner, args, output, code): user = User.query.filter_by(username="cli_user").first() assert user.is_admin assert user.email == "cli_user@example.com" + + +download_project_data = [ + ( + ( + f" {test_workspace_name}/{test_project} ", + "--version", + 1, + "--directory", + sync_config.TEMP_DIR, + ), + 0, + "Project downloaded", + ), + ( + ( + f"{test_workspace_name}/non-existing", + "--version", + 1, + "--directory", + sync_config.TEMP_DIR, + ), + 1, + "ERROR: Project does not exist", + ), + ( + ( + f"{test_workspace_name}/{test_project}", + "--version", + 2, + "--directory", + sync_config.TEMP_DIR, + ), + 1, + "ERROR: Project version does not exist", + ), + ( + ( + f"{test_workspace_name}/{test_project}", + "--version", + 1, + "--directory", + "/tmp", + ), + 1, + "ERROR: Destination directory '/tmp' already exist", + ), +] + + +@pytest.mark.parametrize("args,code,output", download_project_data) +def test_download_project(runner, args, code, output): + """Test download project command""" + if os.path.exists(sync_config.TEMP_DIR): + shutil.rmtree(sync_config.TEMP_DIR) + result = runner.invoke(args=["project", "download", *args]) + assert result.exit_code == code + assert output in result.output + if code == 0: + assert os.path.exists(sync_config.TEMP_DIR) and os.path.isdir( + sync_config.TEMP_DIR + ) From 692eeec85c768937e51f3075c10e50bf62df45ee Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Thu, 24 Jul 2025 15:35:38 +0200 Subject: [PATCH 08/29] test_remove_project --- server/mergin/sync/commands.py | 14 +++++++------- server/mergin/tests/test_cli.py | 30 +++++++++++++++++++++++++++++- 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/server/mergin/sync/commands.py b/server/mergin/sync/commands.py index 301f1c17..615e0c73 100644 --- a/server/mergin/sync/commands.py +++ b/server/mergin/sync/commands.py @@ -109,23 +109,23 @@ def download(project_name, version, directory): # pylint: disable=W0612 click.secho("Project downloaded", fg="green") @project.command() - @click.argument("project-name") + @click.argument("project-name", callback=normalize_input(lowercase=False)) def remove(project_name): """Delete a project""" ws, name = split_project_path(project_name) workspace = current_app.ws_handler.get_by_name(ws) if not workspace: - print("ERROR: Workspace does not exist") - return + click.secho("ERROR: Workspace does not exist", fg="red", err=True) + sys.exit(1) project = ( Project.query.filter_by(workspace_id=workspace.id, name=name) .filter(Project.storage_params.isnot(None)) .first() ) if not project: - print("ERROR: Project does not exist") - return + click.secho("ERROR: Project does not exist", fg="red", err=True) + sys.exit(1) project.removed_at = datetime.utcnow() - project.removed_by = "cli_command" + project.removed_by = None db.session.commit() - print("Project removed successfully") + click.secho("Project removed", fg="green") diff --git a/server/mergin/tests/test_cli.py b/server/mergin/tests/test_cli.py index 7c852840..e5f14301 100644 --- a/server/mergin/tests/test_cli.py +++ b/server/mergin/tests/test_cli.py @@ -64,7 +64,7 @@ @pytest.mark.parametrize("args,code,output", test_create_project_data) -def test_manipulate_project(runner, args, code, output): +def test_create_project(runner, args, code, output): """Test create project command""" # create project create = runner.invoke(args=["project", "create", *args]) @@ -180,3 +180,31 @@ def test_download_project(runner, args, code, output): assert os.path.exists(sync_config.TEMP_DIR) and os.path.isdir( sync_config.TEMP_DIR ) + + +remove_project_data = [ + ( + f" {test_workspace_name}/{test_project} ", + 0, + "Project removed", + ), + ( + f" {test_workspace_name}/non-existing ", + 1, + "ERROR: Project does not exist", + ), +] + + +@pytest.mark.parametrize("project_name,code,output", remove_project_data) +def test_remove_project(runner, project_name, code, output): + """Test remove project command""" + remove = runner.invoke(args=["project", "remove", project_name]) + assert remove.exit_code == code + assert output in remove.output + if code == 0: + project = Project.query.filter_by( + name=test_project, workspace_id=test_workspace_id + ).first() + assert project.removed_at + assert not project.removed_by From ff5caddc125688c8decdbf146ca6235b4469e252 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Mon, 28 Jul 2025 14:47:10 +0200 Subject: [PATCH 09/29] Address review --- server/mergin/auth/commands.py | 2 +- server/mergin/commands.py | 13 ++++++ server/mergin/sync/commands.py | 3 +- server/mergin/tests/test_cli.py | 72 +++++++++++++++++---------------- server/mergin/utils.py | 13 ------ 5 files changed, 53 insertions(+), 50 deletions(-) diff --git a/server/mergin/auth/commands.py b/server/mergin/auth/commands.py index 39738398..f9ac636e 100644 --- a/server/mergin/auth/commands.py +++ b/server/mergin/auth/commands.py @@ -9,7 +9,7 @@ from ..app import db from .models import User, UserProfile -from ..utils import normalize_input +from ..commands import normalize_input def add_commands(app: Flask): diff --git a/server/mergin/commands.py b/server/mergin/commands.py index f02a536c..2d18da28 100644 --- a/server/mergin/commands.py +++ b/server/mergin/commands.py @@ -237,3 +237,16 @@ def check(): def permissions(path: str): """Check for specific path write permission""" _check_permissions(path) + + +def normalize_input(lowercase=True, strip=True): + def _normalize(ctx, param, value): + if value is None: + return value + if strip: + value = value.strip() + if lowercase: + value = value.lower() + return value + + return _normalize diff --git a/server/mergin/sync/commands.py b/server/mergin/sync/commands.py index 615e0c73..dd85829d 100644 --- a/server/mergin/sync/commands.py +++ b/server/mergin/sync/commands.py @@ -16,7 +16,7 @@ from .models import Project, ProjectVersion from .utils import split_project_path from ..auth.models import User -from ..utils import normalize_input +from ..commands import normalize_input def add_commands(app: Flask): @@ -69,7 +69,6 @@ def create(name, namespace, username): # pylint: disable=W0612 "--directory", type=click.Path(), required=True, - callback=normalize_input(lowercase=False), ) def download(project_name, version, directory): # pylint: disable=W0612 """Download files for project at particular version""" diff --git a/server/mergin/tests/test_cli.py b/server/mergin/tests/test_cli.py index e5f14301..2fb6b5b6 100644 --- a/server/mergin/tests/test_cli.py +++ b/server/mergin/tests/test_cli.py @@ -5,7 +5,6 @@ import pytest import os -from pathlib import Path from mergin.auth.models import User from mergin.sync.models import Project, ProjectVersion @@ -14,42 +13,43 @@ test_workspace_id, test_workspace_name, DEFAULT_USER, + test_project_dir, ) from mergin.sync.config import Configuration as sync_config test_create_project_data = [ # missing arguments - # (("my_project",), 2, "Missing argument"), - # # existing project - # ( - # ( - # test_project, - # test_workspace_name, - # DEFAULT_USER[0], - # ), - # 1, - # "ERROR: Project name already exists", - # ), - # # not existing creator user - # ( - # ( - # "my_project", - # test_workspace_name, - # "not_existing", - # ), - # 1, - # "ERROR: User not found", - # ), - # # not existing workspace - # ( - # ( - # "my_project", - # "not_existing", - # DEFAULT_USER[0], - # ), - # 1, - # "ERROR: Workspace not found", - # ), + (("my_project",), 2, "Missing argument"), + # existing project + ( + ( + test_project, + test_workspace_name, + DEFAULT_USER[0], + ), + 1, + "ERROR: Project name already exists", + ), + # not existing creator user + ( + ( + "my_project", + test_workspace_name, + "not_existing", + ), + 1, + "ERROR: User not found", + ), + # not existing workspace + ( + ( + "my_project", + "not_existing", + DEFAULT_USER[0], + ), + 1, + "ERROR: Workspace not found", + ), # success ( ( @@ -121,6 +121,7 @@ def test_create_user(runner, args, output, code): download_project_data = [ + # success ( ( f" {test_workspace_name}/{test_project} ", @@ -132,6 +133,7 @@ def test_create_user(runner, args, output, code): 0, "Project downloaded", ), + # project does not exist in the workspace ( ( f"{test_workspace_name}/non-existing", @@ -143,6 +145,7 @@ def test_create_user(runner, args, output, code): 1, "ERROR: Project does not exist", ), + # request project version does not exist ( ( f"{test_workspace_name}/{test_project}", @@ -154,16 +157,17 @@ def test_create_user(runner, args, output, code): 1, "ERROR: Project version does not exist", ), + # destination dir already exists ( ( f"{test_workspace_name}/{test_project}", "--version", 1, "--directory", - "/tmp", + test_project_dir, ), 1, - "ERROR: Destination directory '/tmp' already exist", + f"ERROR: Destination directory '{test_project_dir}' already exists", ), ] diff --git a/server/mergin/utils.py b/server/mergin/utils.py index 4396de05..9acc6124 100644 --- a/server/mergin/utils.py +++ b/server/mergin/utils.py @@ -135,16 +135,3 @@ def save_diagnostic_log_file(app: str, username: str, body: bytes) -> str: f.write(content) return file_name - - -def normalize_input(lowercase=True, strip=True): - def _normalize(ctx, param, value): - if value is None: - return value - if strip: - value = value.strip() - if lowercase: - value = value.lower() - return value - - return _normalize From b6ea1da30ae713a54f117a3dd677c5d6d7a344f0 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Tue, 29 Jul 2025 00:15:58 +0200 Subject: [PATCH 10/29] Make internal commands function accesible --- server/mergin/commands.py | 314 ++++++++++++++++++++------------------ 1 file changed, 165 insertions(+), 149 deletions(-) diff --git a/server/mergin/commands.py b/server/mergin/commands.py index 2d18da28..b0b8ca7a 100644 --- a/server/mergin/commands.py +++ b/server/mergin/commands.py @@ -1,9 +1,12 @@ +# Copyright (C) Lutra Consulting Limited +# +# SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial + import click from flask import Flask import random import string import os -from datetime import datetime, timezone def _echo_title(title): @@ -13,164 +16,172 @@ def _echo_title(title): def _echo_error(msg): - click.secho("Error: ", fg="red", nl=False, bold=True) + click.secho("Error: ", fg="red", nl=False, bold=True, err=True) click.secho(msg, fg="bright_red") -def add_commands(app: Flask): - from .app import db - from mergin.auth.models import UserProfile - - def _check_celery(): - from celery import current_app - - ping_celery = current_app.control.inspect().ping() - if not ping_celery: - _echo_error( - "Celery process not running properly. Configure celery worker and celery beat. This breaks also email sending from the system.", - ) +def _check_celery() -> bool: + from celery import current_app + + ping_celery = current_app.control.inspect().ping() + if not ping_celery: + _echo_error( + "Celery process not running properly. Configure celery worker and celery beat. This breaks also email sending from the system.", + ) + return False + click.secho("Celery is running properly", fg="green") + return True + + +def _send_statistics(app: Flask): + from .stats.tasks import send_statistics, save_statistics + + _echo_title("Sending statistics.") + # save rows to MerginStatistics table + save_statistics.delay() + + if not app.config.get("COLLECT_STATISTICS"): + click.secho( + "Statistics sending is disabled.", + ) + return + + if not _check_celery(): + return + send_statistics.delay() + click.secho("Statistics sent.", fg="green") + + +def _send_email(app: Flask, email: str): + """Send check email to specified email address.""" + from .celery import send_email_async + + _echo_title(f"Sending check email to specified email address {email}.") + if app.config["MAIL_SUPPRESS_SEND"]: + _echo_error( + "Sending emails is disabled. Please set MAIL_SUPPRESS_SEND=False to enable sending emails." + ) + return + default_sender = app.config.get("MAIL_DEFAULT_SENDER") + if not default_sender: + _echo_error( + "No default sender set. Please set MAIL_DEFAULT_SENDER environment variable", + ) + return + email_data = { + "subject": "Mergin Maps server check", + "html": "Awesome, your email configuration of Mergin Maps server is working.", + "recipients": [email], + "sender": default_sender, + } + try: + is_celery_running = _check_celery() + if not is_celery_running: return - click.secho("Celery is running properly", fg="green") - return True - - def _send_statistics(): - from .stats.tasks import send_statistics, save_statistics + send_email_async.delay(**email_data) + click.secho("Email sent.", fg="green") + except Exception as e: + _echo_error( + f"Error sending email: {e}", + ) - _echo_title("Sending statistics.") - # save rows to MerginStatistics table - save_statistics.delay() - - if not app.config.get("COLLECT_STATISTICS"): - click.secho( - "Statistics sending is disabled.", - ) - return - if not _check_celery(): - return - send_statistics.delay() - click.secho("Statistics sent.", fg="green") +def _check_permissions(path): + """Check for write permission on working folders""" - def _send_email(email: str): - """Send check email to specified email address.""" - from .celery import send_email_async + if not os.access(path, os.W_OK): + _echo_error( + f"Permissions for {path} folder not set correctly. Please review these settings.", + ) + else: + click.secho(f"Permissions granted for {path} folder", fg="green") - _echo_title(f"Sending check email to specified email address {email}.") - if app.config["MAIL_SUPPRESS_SEND"]: - _echo_error( - "Sending emails is disabled. Please set MAIL_SUPPRESS_SEND=False to enable sending emails." - ) - return - default_sender = app.config.get("MAIL_DEFAULT_SENDER") - if not default_sender: - _echo_error( - "No default sender set. Please set MAIL_DEFAULT_SENDER environment variable", - ) - return - email_data = { - "subject": "Mergin Maps server check", - "html": "Awesome, your email configuration of Mergin Maps server is working.", - "recipients": [email], - "sender": default_sender, - } - try: - is_celery_running = _check_celery() - if not is_celery_running: - return - send_email_async.delay(**email_data) - except Exception as e: - _echo_error( - f"Error sending email: {e}", - ) - def _check_permissions(path): - """Check for write permission on working folders""" - - if not os.access(path, os.W_OK): - _echo_error( - f"Permissions for {path} folder not set correctly. Please review these settings.", - ) - else: - click.secho(f"Permissions granted for {path} folder", fg="green") - - def _check_server(): # pylint: disable=W0612 - """Check server configuration.""" - from .stats.models import MerginInfo - - _echo_title("Server health check") - edition_map = { - "ce": "Community Edition", - "ee": "Enterprise Edition", - } - edition = edition_map.get(app.config["SERVER_TYPE"]) - if edition: - click.echo(f"Mergin Maps edition: {edition}") - click.echo(f"Mergin Maps version: {app.config['VERSION']}") - - base_url = app.config["MERGIN_BASE_URL"] - if not base_url: - _echo_error( - "No base URL set. Please set MERGIN_BASE_URL environment variable", - ) - else: - click.secho(f"Base URL of server is {base_url}", fg="green") +def _check_server(app: Flask): # pylint: disable=W0612 + """Check server configuration.""" + from .stats.models import MerginInfo + from .app import db - contact_email = app.config["CONTACT_EMAIL"] - if not contact_email: - _echo_error( - "No contact email set. Please set CONTACT_EMAIL environment variable", - ) - else: - click.secho(f"Your contact email is {contact_email}.", fg="green") + _echo_title("Server health check") + edition_map = { + "ce": "Community Edition", + "ee": "Enterprise Edition", + } + edition = edition_map.get(app.config["SERVER_TYPE"]) + if edition: + click.echo(f"Mergin Maps edition: {edition}") + click.echo(f"Mergin Maps version: {app.config['VERSION']}") + + base_url = app.config["MERGIN_BASE_URL"] + if not base_url: + _echo_error( + "No base URL set. Please set MERGIN_BASE_URL environment variable", + ) + else: + click.secho(f"Base URL of server is {base_url}", fg="green") + + contact_email = app.config["CONTACT_EMAIL"] + if not contact_email: + _echo_error( + "No contact email set. Please set CONTACT_EMAIL environment variable", + ) + else: + click.secho(f"Your contact email is {contact_email}.", fg="green") + + info = MerginInfo.query.first() + service_id = app.config.get("SERVICE_ID") or (info.service_id if info else None) + + if service_id: + click.secho(f"Service ID is {service_id}.", fg="green") + else: + _echo_error("No service ID set.") + + tables = db.engine.table_names() + if not tables: + _echo_error("Database not initialized. Run flask init-db command") + else: + click.secho("Database initialized properly", fg="green") + + _check_permissions(app.config.get("LOCAL_PROJECTS")) + + _check_celery() + + +def _init_db(app: Flask): + """Create database tables.""" + from .stats.models import MerginInfo + from .app import db + _echo_title("Database initialization") + with click.progressbar( + label="Creating database", length=4, show_eta=False + ) as progress_bar: + progress_bar.update(0) + db.drop_all(bind=None) + progress_bar.update(1) + db.create_all(bind=None) + progress_bar.update(2) + db.session.commit() + progress_bar.update(3) info = MerginInfo.query.first() - service_id = app.config.get("SERVICE_ID") or (info.service_id if info else None) + if not info and app.config.get("COLLECT_STATISTICS"): + # create new info with random service id + service_id = app.config.get("SERVICE_ID", None) + info = MerginInfo(service_id) + db.session.add(info) + db.session.commit() + progress_bar.update(4) - if service_id: - click.secho(f"Service ID is {service_id}.", fg="green") - else: - _echo_error("No service ID set.") + click.secho("Tables created.", fg="green") - tables = db.engine.table_names() - if not tables: - _echo_error("Database not initialized. Run flask init-db command") - else: - click.secho("Database initialized properly", fg="green") - - _check_permissions(app.config.get("LOCAL_PROJECTS")) - - _check_celery() - - def _init_db(): - """Create database tables.""" - from .stats.models import MerginInfo - - _echo_title("Database initialization") - with click.progressbar( - label="Creating database", length=4, show_eta=False - ) as progress_bar: - progress_bar.update(0) - db.drop_all(bind=None) - progress_bar.update(1) - db.create_all(bind=None) - progress_bar.update(2) - db.session.commit() - progress_bar.update(3) - info = MerginInfo.query.first() - if not info and app.config.get("COLLECT_STATISTICS"): - # create new info with random service id - service_id = app.config.get("SERVICE_ID", None) - info = MerginInfo(service_id) - db.session.add(info) - db.session.commit() - progress_bar.update(4) - - click.secho("Tables created.", fg="green") + +def add_commands(app: Flask): + from .app import db @app.cli.command() def init_db(): """Re-create database tables.""" - _init_db() + _init_db(app) @app.cli.command() @click.option("--email", "-e", required=True, envvar="CONTACT_EMAIL") @@ -183,7 +194,7 @@ def init_db(): ) def init(email: str, recreate: bool): """Initialize database if does not exist or -r is provided. Perform check of server configuration. Send statistics, respecting your setup.""" - + from mergin.auth.models import UserProfile from .auth.models import User tables = db.engine.table_names() @@ -195,7 +206,7 @@ def init(email: str, recreate: bool): ) if not tables or recreate: - _init_db() + _init_db(app) _echo_title("Creating admin user. Copy generated password.") username = User.generate_username(email) @@ -212,9 +223,9 @@ def init(email: str, recreate: bool): click.secho(f"Email: {email}") click.secho(f"Username: {username}") click.secho(f"Password: {password}") - _check_server() - _send_email(email) - _send_statistics() + _check_server(app) + _send_email(app, email) + _send_statistics(app) @app.cli.group() def server(): @@ -222,15 +233,15 @@ def server(): pass @server.command() - @click.option("--email", required=True) + @click.option("--email", required=True, callback=normalize_input()) def send_check_email(email: str): # pylint: disable=W0612 """Send check email to specified email address.""" - _send_email(email) + _send_email(app, email) @server.command() def check(): """Check server configuration.""" - _check_server() + _check_server(app) @server.command() @click.option("--path", required=False, default=app.config.get("LOCAL_PROJECTS")) @@ -238,6 +249,11 @@ def permissions(path: str): """Check for specific path write permission""" _check_permissions(path) + @server.command() + def send_statistics(): + """Send usage statistics""" + _send_statistics(app) + def normalize_input(lowercase=True, strip=True): def _normalize(ctx, param, value): From 626e6cb3c23116ca06a89afb8be4174bf5e95916 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Tue, 29 Jul 2025 00:36:24 +0200 Subject: [PATCH 11/29] test statistics, test check email --- server/mergin/tests/test_cli.py | 82 +++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/server/mergin/tests/test_cli.py b/server/mergin/tests/test_cli.py index 2fb6b5b6..d7efb983 100644 --- a/server/mergin/tests/test_cli.py +++ b/server/mergin/tests/test_cli.py @@ -1,10 +1,14 @@ # Copyright (C) Lutra Consulting Limited # # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial + import shutil import pytest import os +from unittest.mock import patch + +# from flask import current_app from mergin.auth.models import User from mergin.sync.models import Project, ProjectVersion @@ -17,6 +21,7 @@ ) from mergin.sync.config import Configuration as sync_config + test_create_project_data = [ # missing arguments (("my_project",), 2, "Missing argument"), @@ -212,3 +217,80 @@ def test_remove_project(runner, project_name, code, output): ).first() assert project.removed_at assert not project.removed_by + + +@pytest.mark.parametrize("collect_stats", [True, False]) +@patch("mergin.stats.tasks.send_statistics.delay") +@patch("mergin.stats.tasks.save_statistics.delay") +@patch("mergin.commands._check_celery") +@patch("mergin.commands._echo_title") +def test_send_statistics( + mock_echo_title, + mock_check_celery, + mock_save_statistics, + mock_send_statistics, + collect_stats, + app, +): + """Test send statistics helper""" + from mergin.commands import _send_statistics + + mock_echo_title.return_value = "" + mock_check_celery.return_value = True + mock_save_statistics.return_value = None + mock_send_statistics.return_value = None + app.config["COLLECT_STATISTICS"] = 1 if collect_stats else 0 + + _send_statistics(app) + assert mock_save_statistics.call_count == 1 + assert mock_send_statistics.call_count == (1 if collect_stats else 0) + + +test_check_email_data = [ + (0, 1, 1, 1, "Email sent."), # success + (0, 1, 1, 0, ""), # celery is not running + ( + 1, + 1, + 1, + 1, + "Sending emails is disabled. Please set MAIL_SUPPRESS_SEND=False to enable sending emails.", + ), # emails are suppressed + (0, 0, 1, 1, "Error: Missing option '--email'"), # email parameter is missing + (0, 1, 0, 1, "No default sender set."), # default sender is missing +] + + +@patch("mergin.celery.send_email_async.delay") +@patch("mergin.commands._check_celery") +@pytest.mark.parametrize( + "suppressed,recipient,sender,celery_check,output", test_check_email_data +) +def test_check_email( + mock_check_celery, + mock_send_email, + suppressed, + recipient, + sender, + celery_check, + output, + runner, + app, +): + """Test check_email command""" + mock_send_email.return_value = None + mock_check_celery.return_value = celery_check + app.config["MAIL_SUPPRESS_SEND"] = suppressed + app.config["MAIL_DEFAULT_SENDER"] = "sender@mergin.com" if sender else None + + result = runner.invoke( + args=[ + "server", + "send-check-email", + "--email", + f" test@mergin.com " if recipient else None, + ] + ) + assert output in result.output + sent = 1 if output == "Email sent." else 0 + assert mock_send_email.call_count == sent From 64cc5027dd3cd1be17c1c231bb2d5738250af0f0 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Tue, 29 Jul 2025 00:37:15 +0200 Subject: [PATCH 12/29] Fix assert test_middleware --- server/mergin/tests/test_middleware.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server/mergin/tests/test_middleware.py b/server/mergin/tests/test_middleware.py index 508334d0..82b9cf26 100644 --- a/server/mergin/tests/test_middleware.py +++ b/server/mergin/tests/test_middleware.py @@ -28,10 +28,8 @@ def ping(): assert isinstance(application.wsgi_app, GeventTimeoutMiddleware) == use_middleware # in case of gevent, dummy endpoint it set to time out - assert ( - application.test_client().get("/test").status_code == 504 - if use_middleware - else 200 + assert application.test_client().get("/test").status_code == ( + 504 if use_middleware else 200 ) From 106950b2477ee67e956166ddbb45125cb7d77785 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Wed, 30 Jul 2025 07:21:51 +0200 Subject: [PATCH 13/29] test_check_server and test_check_permission --- server/mergin/commands.py | 4 +- server/mergin/sync/commands.py | 2 +- server/mergin/tests/test_cli.py | 206 ++++++++++++++++++++++++++++++-- 3 files changed, 201 insertions(+), 11 deletions(-) diff --git a/server/mergin/commands.py b/server/mergin/commands.py index b0b8ca7a..29cf7a4e 100644 --- a/server/mergin/commands.py +++ b/server/mergin/commands.py @@ -16,7 +16,7 @@ def _echo_title(title): def _echo_error(msg): - click.secho("Error: ", fg="red", nl=False, bold=True, err=True) + click.secho("Error: ", fg="red", nl=False, bold=True) click.secho(msg, fg="bright_red") @@ -94,7 +94,7 @@ def _check_permissions(path): f"Permissions for {path} folder not set correctly. Please review these settings.", ) else: - click.secho(f"Permissions granted for {path} folder", fg="green") + click.secho(f"Permissions granted for {path} folder.", fg="green") def _check_server(app: Flask): # pylint: disable=W0612 diff --git a/server/mergin/sync/commands.py b/server/mergin/sync/commands.py index dd85829d..6e70d913 100644 --- a/server/mergin/sync/commands.py +++ b/server/mergin/sync/commands.py @@ -88,7 +88,7 @@ def download(project_name, version, directory): # pylint: disable=W0612 sys.exit(1) if os.path.exists(directory): click.secho( - f"ERROR: Destination directory '{directory}' already exist", + f"ERROR: Destination directory '{directory}' already exists", fg="red", err=True, ) diff --git a/server/mergin/tests/test_cli.py b/server/mergin/tests/test_cli.py index d7efb983..1fc4f1fe 100644 --- a/server/mergin/tests/test_cli.py +++ b/server/mergin/tests/test_cli.py @@ -3,14 +3,17 @@ # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial import shutil +from pathlib import Path +import click import pytest import os from unittest.mock import patch -# from flask import current_app - +from mergin.app import db from mergin.auth.models import User +from mergin.commands import _check_permissions +from mergin.stats.models import MerginInfo from mergin.sync.models import Project, ProjectVersion from mergin.tests import ( test_project, @@ -70,7 +73,7 @@ @pytest.mark.parametrize("args,code,output", test_create_project_data) def test_create_project(runner, args, code, output): - """Test create project command""" + """Test 'project create' command""" # create project create = runner.invoke(args=["project", "create", *args]) assert create.exit_code == code @@ -115,7 +118,7 @@ def test_create_project(runner, args, code, output): @pytest.mark.parametrize("args,output,code", test_create_user_data) def test_create_user(runner, args, output, code): - """Test create user command""" + """Test 'user create' command""" result = runner.invoke(args=["user", "create", *args]) assert result.exit_code == code assert output in result.output @@ -179,7 +182,7 @@ def test_create_user(runner, args, output, code): @pytest.mark.parametrize("args,code,output", download_project_data) def test_download_project(runner, args, code, output): - """Test download project command""" + """Test 'project download' command""" if os.path.exists(sync_config.TEMP_DIR): shutil.rmtree(sync_config.TEMP_DIR) result = runner.invoke(args=["project", "download", *args]) @@ -207,7 +210,7 @@ def test_download_project(runner, args, code, output): @pytest.mark.parametrize("project_name,code,output", remove_project_data) def test_remove_project(runner, project_name, code, output): - """Test remove project command""" + """Test 'project remove' command""" remove = runner.invoke(args=["project", "remove", project_name]) assert remove.exit_code == code assert output in remove.output @@ -232,7 +235,7 @@ def test_send_statistics( collect_stats, app, ): - """Test send statistics helper""" + """Test '_send_statistics' helper""" from mergin.commands import _send_statistics mock_echo_title.return_value = "" @@ -277,7 +280,7 @@ def test_check_email( runner, app, ): - """Test check_email command""" + """Test 'send-check-email' command""" mock_send_email.return_value = None mock_check_celery.return_value = celery_check app.config["MAIL_SUPPRESS_SEND"] = suppressed @@ -294,3 +297,190 @@ def test_check_email( assert output in result.output sent = 1 if output == "Email sent." else 0 assert mock_send_email.call_count == sent + + +test_check_permission_data = [(1, 0, "permissions granted"), (0, 1, "access denied")] + + +@pytest.mark.parametrize("writable,error,expected", test_check_permission_data) +def test_check_permission(writable, error, expected, capsys): + """Test check 'permission' command""" + projects_dir = Path(sync_config.TEMP_DIR) + if not os.path.exists(projects_dir): + projects_dir.mkdir() + if not writable: + projects_dir.chmod(0o555) + else: + projects_dir.chmod(0o200) + if expected == "permissions granted": + expected = f"Permissions granted for {projects_dir} folder." + elif expected == "access denied": + expected = f"Permissions for {projects_dir} folder not set correctly. Please review these settings." + + _check_permissions(projects_dir) + out, err = capsys.readouterr() # capture what was printed to stdout + assert ("Error: " in out) == error + assert expected in out + + +test_check_server_data = [ + ( + "ce", + "https://app.dev.merginmaps.com/", + "contact@mergin.com", + "service_id_config", + 1, + 1, + 1, + 0, + "", + ), # success + ( + "ee", + "", + "contact@mergin.com", + "service_id_config", + 1, + 1, + 1, + 1, + "No base URL set.", + ), # no base URL set. + ( + "", + "https://app.dev.merginmaps.com/", + "", + "service_id_config", + 1, + 1, + 1, + 1, + "No contact email set.", + ), # no contact email set. + ( + "ce", + "https://app.dev.merginmaps.com/", + "contact@mergin.com", + "service_id_db", + 1, + 1, + 1, + 0, + "", + ), # success + ( + "ce", + "https://app.dev.merginmaps.com/", + "contact@mergin.com", + None, + 1, + 1, + 1, + 1, + "No service ID set.", + ), # no service ID set. + ( + "ee", + "https://app.dev.merginmaps.com/", + "contact@mergin.com", + "service_id_config", + 0, + 1, + 1, + 1, + "Database not initialized.", + ), # database not initialized. + ( + "", + "https://app.dev.merginmaps.com/", + "contact@mergin.com", + "service_id_config", + 1, + 0, + 1, + 1, + "bad_permissions", + ), # bad permissions + ( + "", + "https://app.dev.merginmaps.com/", + "contact@mergin.com", + "service_id_config", + 1, + 1, + 0, + 1, + "Celery process not running properly.", + ), # celery not running +] + + +@patch("mergin.commands._check_celery") +@patch("mergin.commands._check_permissions") +@patch("mergin.app.db.engine.table_names") +@pytest.mark.parametrize( + "edition,base_url,contact_email,service_id,tables,permission_check,celery_check,error,output", + test_check_server_data, +) +def test_check_server( + table_names_mock, + permission_mock, + celery_mock, + edition, + base_url, + contact_email, + service_id, + tables, + permission_check, + celery_check, + error, + output, + app, + runner, +): + """Test 'check' server command""" + app.config["SERVER_TYPE"] = edition + app.config["MERGIN_BASE_URL"] = base_url + app.config["CONTACT_EMAIL"] = contact_email + + if service_id is None: + app.config["SERVICE_ID"] = "" + MerginInfo.query.delete() + db.session.commit() + + if service_id == "service_id_config": + app.config["SERVICE_ID"] = "service-id-config" + + if not tables: + table_names_mock.return_value = "" + projects_dir = app.config["LOCAL_PROJECTS"] + # workaround to get the correct output as `app` fixture is not initialized yet when parametrize is evaluated + if output == "bad_permissions": + output = f"Permissions for {projects_dir} folder not set correctly." + permission_mock.return_value = None + celery_mock.return_value = None + # mock message echoed to cli, use lambda to deffer execution so that the message gets to `result.output` + permission_mock.side_effect = lambda *args, **kwargs: click.echo( + f"Permissions granted for {projects_dir} folder" + if permission_check + else f"Error: Permissions for {projects_dir} folder not set correctly." + ) + celery_mock.side_effect = lambda *args, **kwargs: click.echo( + "Celery is running properly" + if celery_check + else "Error: Celery process not running properly." + ) + + result = runner.invoke(args=["server", "check"]) + assert permission_mock.called + assert celery_mock.called + assert ("Error:" in result.output) == error + assert base_url in result.output + assert contact_email in result.output + assert output in result.output + if edition == "ce": + assert "Mergin Maps edition: Community Edition" in result.output + elif edition == "ee": + assert "Mergin Maps edition: Enterprise Edition" in result.output + else: + assert "Mergin Maps edition" not in result.output From b55bc6828d257bda7d6b5328a168e1ef4316eed4 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Wed, 30 Jul 2025 07:26:32 +0200 Subject: [PATCH 14/29] imports --- server/mergin/commands.py | 3 +-- server/mergin/tests/test_cli.py | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/server/mergin/commands.py b/server/mergin/commands.py index 29cf7a4e..ff71f586 100644 --- a/server/mergin/commands.py +++ b/server/mergin/commands.py @@ -194,8 +194,7 @@ def init_db(): ) def init(email: str, recreate: bool): """Initialize database if does not exist or -r is provided. Perform check of server configuration. Send statistics, respecting your setup.""" - from mergin.auth.models import UserProfile - from .auth.models import User + from .auth.models import User, UserProfile tables = db.engine.table_names() if recreate and tables: diff --git a/server/mergin/tests/test_cli.py b/server/mergin/tests/test_cli.py index 1fc4f1fe..2815dacd 100644 --- a/server/mergin/tests/test_cli.py +++ b/server/mergin/tests/test_cli.py @@ -3,12 +3,11 @@ # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial import shutil -from pathlib import Path - import click import pytest import os from unittest.mock import patch +from pathlib import Path from mergin.app import db from mergin.auth.models import User From 17786e4a99546f52b93a60df325e7bbaa8957650 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Wed, 30 Jul 2025 08:54:56 +0200 Subject: [PATCH 15/29] Review & test init-db --- server/mergin/commands.py | 5 ----- server/mergin/tests/test_cli.py | 23 ++++++++++++++++------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/server/mergin/commands.py b/server/mergin/commands.py index ff71f586..b1e9a646 100644 --- a/server/mergin/commands.py +++ b/server/mergin/commands.py @@ -248,11 +248,6 @@ def permissions(path: str): """Check for specific path write permission""" _check_permissions(path) - @server.command() - def send_statistics(): - """Send usage statistics""" - _send_statistics(app) - def normalize_input(lowercase=True, strip=True): def _normalize(ctx, param, value): diff --git a/server/mergin/tests/test_cli.py b/server/mergin/tests/test_cli.py index 2815dacd..4d4baaf8 100644 --- a/server/mergin/tests/test_cli.py +++ b/server/mergin/tests/test_cli.py @@ -325,7 +325,7 @@ def test_check_permission(writable, error, expected, capsys): test_check_server_data = [ ( "ce", - "https://app.dev.merginmaps.com/", + "server-base-url", "contact@mergin.com", "service_id_config", 1, @@ -347,7 +347,7 @@ def test_check_permission(writable, error, expected, capsys): ), # no base URL set. ( "", - "https://app.dev.merginmaps.com/", + "server-base-url", "", "service_id_config", 1, @@ -358,7 +358,7 @@ def test_check_permission(writable, error, expected, capsys): ), # no contact email set. ( "ce", - "https://app.dev.merginmaps.com/", + "server-base-url", "contact@mergin.com", "service_id_db", 1, @@ -369,7 +369,7 @@ def test_check_permission(writable, error, expected, capsys): ), # success ( "ce", - "https://app.dev.merginmaps.com/", + "server-base-url", "contact@mergin.com", None, 1, @@ -380,7 +380,7 @@ def test_check_permission(writable, error, expected, capsys): ), # no service ID set. ( "ee", - "https://app.dev.merginmaps.com/", + "server-base-url", "contact@mergin.com", "service_id_config", 0, @@ -391,7 +391,7 @@ def test_check_permission(writable, error, expected, capsys): ), # database not initialized. ( "", - "https://app.dev.merginmaps.com/", + "server-base-url", "contact@mergin.com", "service_id_config", 1, @@ -402,7 +402,7 @@ def test_check_permission(writable, error, expected, capsys): ), # bad permissions ( "", - "https://app.dev.merginmaps.com/", + "server-base-url", "contact@mergin.com", "service_id_config", 1, @@ -483,3 +483,12 @@ def test_check_server( assert "Mergin Maps edition: Enterprise Edition" in result.output else: assert "Mergin Maps edition" not in result.output + + +def test_init_db(runner): + """Test initializing database""" + db.drop_all(bind=None) + assert not db.engine.table_names() + result = runner.invoke(args=["init-db"]) + assert db.engine.table_names() + assert "Tables created." in result.output From 18f70a524bc9b074693b7d373ffab650f43e462c Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Wed, 30 Jul 2025 09:43:15 +0200 Subject: [PATCH 16/29] Fix permissions & test init command --- server/mergin/tests/test_cli.py | 37 +++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/server/mergin/tests/test_cli.py b/server/mergin/tests/test_cli.py index 4d4baaf8..40d8b2aa 100644 --- a/server/mergin/tests/test_cli.py +++ b/server/mergin/tests/test_cli.py @@ -304,13 +304,15 @@ def test_check_email( @pytest.mark.parametrize("writable,error,expected", test_check_permission_data) def test_check_permission(writable, error, expected, capsys): """Test check 'permission' command""" - projects_dir = Path(sync_config.TEMP_DIR) - if not os.path.exists(projects_dir): - projects_dir.mkdir() + projects_dir = Path(sync_config.TEMP_DIR) / "projects" + if projects_dir.exists(): + projects_dir.chmod(0o700) + shutil.rmtree(projects_dir) + projects_dir.mkdir(parents=True) if not writable: projects_dir.chmod(0o555) else: - projects_dir.chmod(0o200) + projects_dir.chmod(0o700) if expected == "permissions granted": expected = f"Permissions granted for {projects_dir} folder." elif expected == "access denied": @@ -492,3 +494,30 @@ def test_init_db(runner): result = runner.invoke(args=["init-db"]) assert db.engine.table_names() assert "Tables created." in result.output + + +@patch("mergin.commands._check_server") +@patch("mergin.commands._send_email") +@patch("mergin.commands._send_statistics") +@pytest.mark.parametrize("tables", [True, False]) +def test_init_server( + send_statistics_mock, send_email_mock, check_server_mock, tables, runner +): + """Test initializing DB with admin and checks""" + if not tables: + db.drop_all(bind=None) + send_statistics_mock.return_value = None + send_email_mock.return_value = None + check_server_mock.return_value = None + email = "init@command.cli" + result = runner.invoke(args=["init", "--email", email]) + assert send_statistics_mock.called + assert send_email_mock.called + assert check_server_mock.called + if not tables: + assert "Admin user created. Please save generated password." in result.output + assert email in result.output + assert not User.query.filter_by(username=DEFAULT_USER[0]).count() + else: + assert email not in result.output + assert User.query.filter_by(username=DEFAULT_USER[0]).count() From 764fc75efb86b63c442e0b1335fe104f44e56fec Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Wed, 30 Jul 2025 09:44:47 +0200 Subject: [PATCH 17/29] Double check permissions cleanup --- server/mergin/tests/test_cli.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/mergin/tests/test_cli.py b/server/mergin/tests/test_cli.py index 40d8b2aa..9bad4a26 100644 --- a/server/mergin/tests/test_cli.py +++ b/server/mergin/tests/test_cli.py @@ -322,6 +322,9 @@ def test_check_permission(writable, error, expected, capsys): out, err = capsys.readouterr() # capture what was printed to stdout assert ("Error: " in out) == error assert expected in out + if projects_dir.exists(): + projects_dir.chmod(0o700) + shutil.rmtree(projects_dir) test_check_server_data = [ From 5cae9670f087feaed924724805befe606994f319 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Wed, 30 Jul 2025 13:39:36 +0200 Subject: [PATCH 18/29] mock sso user to allow login --- server/mergin/tests/test_auth.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/mergin/tests/test_auth.py b/server/mergin/tests/test_auth.py index c6c039c4..3a4c2de4 100644 --- a/server/mergin/tests/test_auth.py +++ b/server/mergin/tests/test_auth.py @@ -503,8 +503,11 @@ def test_update_user_profile(client): assert user.email == "changed_email@mergin.co.uk" # do not allow to update sso user - sso_user = add_user("sso_user", "") + sso_user = add_user("sso_user", "sso") login(client, sso_user.username, "sso") + sso_user.passwd = None + db.session.add(sso_user) + db.session.commit() resp = client.post( url_for("/.mergin_auth_controller_update_user_profile"), data=json.dumps({"email": "changed_email@sso.co.uk"}), From 2058154184fda597907fc3c561e3d9686e815e50 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Wed, 30 Jul 2025 14:23:15 +0200 Subject: [PATCH 19/29] test_check_celery --- server/mergin/tests/test_cli.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/server/mergin/tests/test_cli.py b/server/mergin/tests/test_cli.py index 9bad4a26..d0b91717 100644 --- a/server/mergin/tests/test_cli.py +++ b/server/mergin/tests/test_cli.py @@ -11,7 +11,7 @@ from mergin.app import db from mergin.auth.models import User -from mergin.commands import _check_permissions +from mergin.commands import _check_permissions, _check_celery from mergin.stats.models import MerginInfo from mergin.sync.models import Project, ProjectVersion from mergin.tests import ( @@ -524,3 +524,24 @@ def test_init_server( else: assert email not in result.output assert User.query.filter_by(username=DEFAULT_USER[0]).count() + + +test_check_celery_data = [ + (1, 1, "Celery is running properly"), + ( + 0, + 0, + "Celery process not running properly. Configure celery worker and celery beat.", + ), +] + + +@patch("celery.app.control.Inspect.ping") +@pytest.mark.parametrize("ping,result,output", test_check_celery_data) +def test_check_celery(mock_ping, ping, result, output, capsys): + """Test `_check_celery` function`""" + mock_ping.return_value = ping + assert _check_celery() == result + out, err = capsys.readouterr() # capture what was echoed to stdout + assert ("Error: " not in out) == result + assert output in out From 3736951d342bcb91b30d924d07d538c5b4d03328 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Thu, 31 Jul 2025 16:00:37 +0200 Subject: [PATCH 20/29] normalize email in init cmd --- server/mergin/commands.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/server/mergin/commands.py b/server/mergin/commands.py index b1e9a646..57a4847f 100644 --- a/server/mergin/commands.py +++ b/server/mergin/commands.py @@ -184,7 +184,13 @@ def init_db(): _init_db(app) @app.cli.command() - @click.option("--email", "-e", required=True, envvar="CONTACT_EMAIL") + @click.option( + "--email", + "-e", + required=True, + envvar="CONTACT_EMAIL", + callback=normalize_input(), + ) @click.option( "--recreate", "-r", From efe7a80bf26001b5e4154d7330d6f9f576ed32d7 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Mon, 4 Aug 2025 16:26:58 +0200 Subject: [PATCH 21/29] Adjust avatar font size globally --- .../src/assets/sass/theme-base/components/misc/_avatar.scss | 3 ++- .../assets/sass/themes/mm-theme-light/variables/_misc.scss | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/web-app/packages/lib/src/assets/sass/theme-base/components/misc/_avatar.scss b/web-app/packages/lib/src/assets/sass/theme-base/components/misc/_avatar.scss index d169b82b..76397b2d 100644 --- a/web-app/packages/lib/src/assets/sass/theme-base/components/misc/_avatar.scss +++ b/web-app/packages/lib/src/assets/sass/theme-base/components/misc/_avatar.scss @@ -1,6 +1,7 @@ .p-avatar { background-color: $avatarBg; border-radius: $borderRadius; + font-size: $avatarFontSize !important; &.p-avatar-lg { width: 3rem; @@ -27,4 +28,4 @@ .p-avatar { border: 2px solid $panelContentBg; } -} \ No newline at end of file +} diff --git a/web-app/packages/lib/src/assets/sass/themes/mm-theme-light/variables/_misc.scss b/web-app/packages/lib/src/assets/sass/themes/mm-theme-light/variables/_misc.scss index b393c03d..0d237314 100644 --- a/web-app/packages/lib/src/assets/sass/themes/mm-theme-light/variables/_misc.scss +++ b/web-app/packages/lib/src/assets/sass/themes/mm-theme-light/variables/_misc.scss @@ -82,6 +82,11 @@ $progressSpinnerColorFour:$warningMessageTextColor !default; /// @group misc $avatarBg: map-get($map: $colors, $key: medium-green); +/// Avatar font size +/// @group misc +$avatarFontSize: 0.857rem; + + /// Text color of an avatar /// @group misc $avatarTextColor:$textColor; From ff740b8bf3299b284936507afa5b94aade805d81 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Tue, 12 Aug 2025 15:01:52 +0200 Subject: [PATCH 22/29] Fix avatar font-size - normal, large, xlarge --- .../modules/admin/views/AccountDetailView.vue | 7 ++----- .../theme-base/components/misc/_avatar.scss | 17 ++++++++--------- .../sass/themes/mm-theme-light/_extensions.scss | 5 +++++ .../themes/mm-theme-light/variables/_misc.scss | 5 ----- .../modules/user/views/ProfileViewTemplate.vue | 7 ++----- 5 files changed, 17 insertions(+), 24 deletions(-) diff --git a/web-app/packages/admin-lib/src/modules/admin/views/AccountDetailView.vue b/web-app/packages/admin-lib/src/modules/admin/views/AccountDetailView.vue index fa2bd6e0..cb283c78 100644 --- a/web-app/packages/admin-lib/src/modules/admin/views/AccountDetailView.vue +++ b/web-app/packages/admin-lib/src/modules/admin/views/AccountDetailView.vue @@ -14,14 +14,11 @@ diff --git a/web-app/packages/lib/src/assets/sass/theme-base/components/misc/_avatar.scss b/web-app/packages/lib/src/assets/sass/theme-base/components/misc/_avatar.scss index 76397b2d..66f23259 100644 --- a/web-app/packages/lib/src/assets/sass/theme-base/components/misc/_avatar.scss +++ b/web-app/packages/lib/src/assets/sass/theme-base/components/misc/_avatar.scss @@ -1,25 +1,24 @@ .p-avatar { background-color: $avatarBg; border-radius: $borderRadius; - font-size: $avatarFontSize !important; &.p-avatar-lg { - width: 3rem; - height: 3rem; - font-size: 1.5rem; + width: 2.625rem; + height: 2.625rem; + font-size: 1.125rem; .p-avatar-icon { - font-size: 1.5rem; + font-size: 1.125rem; } } &.p-avatar-xl { - width: 4rem; - height: 4rem; - font-size: 2rem; + width: 7.5rem; + height: 7.5rem; + font-size: 3.214rem; .p-avatar-icon { - font-size: 2rem; + font-size: 3.214rem; } } } diff --git a/web-app/packages/lib/src/assets/sass/themes/mm-theme-light/_extensions.scss b/web-app/packages/lib/src/assets/sass/themes/mm-theme-light/_extensions.scss index 07760497..fa51ab3c 100644 --- a/web-app/packages/lib/src/assets/sass/themes/mm-theme-light/_extensions.scss +++ b/web-app/packages/lib/src/assets/sass/themes/mm-theme-light/_extensions.scss @@ -119,3 +119,8 @@ img { color: map-get($map: $colors, $key: grape); font-weight: 600; } + +// Font size in avatar +.p-avatar:not(.p-avatar-lg):not(.p-avatar-xl) { + font-size: 0.857rem; +} diff --git a/web-app/packages/lib/src/assets/sass/themes/mm-theme-light/variables/_misc.scss b/web-app/packages/lib/src/assets/sass/themes/mm-theme-light/variables/_misc.scss index 0d237314..b393c03d 100644 --- a/web-app/packages/lib/src/assets/sass/themes/mm-theme-light/variables/_misc.scss +++ b/web-app/packages/lib/src/assets/sass/themes/mm-theme-light/variables/_misc.scss @@ -82,11 +82,6 @@ $progressSpinnerColorFour:$warningMessageTextColor !default; /// @group misc $avatarBg: map-get($map: $colors, $key: medium-green); -/// Avatar font size -/// @group misc -$avatarFontSize: 0.857rem; - - /// Text color of an avatar /// @group misc $avatarTextColor:$textColor; diff --git a/web-app/packages/lib/src/modules/user/views/ProfileViewTemplate.vue b/web-app/packages/lib/src/modules/user/views/ProfileViewTemplate.vue index 0a670892..86191186 100644 --- a/web-app/packages/lib/src/modules/user/views/ProfileViewTemplate.vue +++ b/web-app/packages/lib/src/modules/user/views/ProfileViewTemplate.vue @@ -61,13 +61,10 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial From a8f55353b85f51eba36357c5d0709d0c79ba9d79 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Wed, 13 Aug 2025 10:05:10 +0200 Subject: [PATCH 23/29] add comment about custom values --- .../lib/src/assets/sass/theme-base/components/misc/_avatar.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/web-app/packages/lib/src/assets/sass/theme-base/components/misc/_avatar.scss b/web-app/packages/lib/src/assets/sass/theme-base/components/misc/_avatar.scss index 66f23259..e466e9e9 100644 --- a/web-app/packages/lib/src/assets/sass/theme-base/components/misc/_avatar.scss +++ b/web-app/packages/lib/src/assets/sass/theme-base/components/misc/_avatar.scss @@ -1,3 +1,4 @@ +// widths, heights and font-sizes are updated with custom values from our Figma design .p-avatar { background-color: $avatarBg; border-radius: $borderRadius; From 5384ad65dc0eebd16db24479704734439bace0bc Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Wed, 13 Aug 2025 10:26:14 +0200 Subject: [PATCH 24/29] bump version to 2025.6.2 --- server/mergin/version.py | 2 +- server/setup.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/server/mergin/version.py b/server/mergin/version.py index 4de9690b..214212f7 100644 --- a/server/mergin/version.py +++ b/server/mergin/version.py @@ -4,4 +4,4 @@ def get_version(): - return "2025.6.1" + return "2025.6.2" diff --git a/server/setup.py b/server/setup.py index ac06d2ba..2abd701d 100644 --- a/server/setup.py +++ b/server/setup.py @@ -6,7 +6,7 @@ setup( name="mergin", - version="2025.6.1", + version="2025.6.2", url="https://github.com/MerginMaps/mergin", license="AGPL-3.0-only", author="Lutra Consulting Limited", From 8694f1b6eda8292d603f9e2c7dd322a6d60ef365 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Fri, 15 Aug 2025 14:57:38 +0200 Subject: [PATCH 25/29] Improve download endpoint - head request cannot create new celery job - flask does not remove partial zip - celery care of cleaning up abandoned zip partials --- server/mergin/sync/private_api_controller.py | 25 ++-- server/mergin/sync/tasks.py | 12 +- server/mergin/tests/test_celery.py | 42 ++++++- .../mergin/tests/test_private_project_api.py | 116 ++++++++++++++---- 4 files changed, 149 insertions(+), 46 deletions(-) diff --git a/server/mergin/sync/private_api_controller.py b/server/mergin/sync/private_api_controller.py index aaa77216..13f059b9 100644 --- a/server/mergin/sync/private_api_controller.py +++ b/server/mergin/sync/private_api_controller.py @@ -4,7 +4,6 @@ import os from datetime import datetime, timedelta, timezone from urllib.parse import quote -from blinker import signal from connexion import NoContent from flask import ( render_template, @@ -353,17 +352,17 @@ def download_project(id: str, version=None): # noqa: E501 # pylint: disable=W06 f"attachment; filename*=UTF-8''{file_name}" ) return resp - - temp_zip_path = project_version.zip_path + ".partial" - # to be safe we are not in vicious circle remove inactive partial zip - if os.path.exists(temp_zip_path) and datetime.fromtimestamp( - os.path.getmtime(temp_zip_path), tz=timezone.utc - ) < datetime.now(timezone.utc) - timedelta( - seconds=current_app.config["PARTIAL_ZIP_EXPIRATION"] - ): - move_to_tmp(temp_zip_path) - - if not os.path.exists(temp_zip_path): - create_project_version_zip.delay(project_version.id) + # GET request triggers background job if no partial zip or expired one + if request.method == "GET": + temp_zip_path = project_version.zip_path + ".partial" + # create zip if it does not exist yet or has expired + partial_exists = os.path.exists(temp_zip_path) + is_expired = partial_exists and datetime.fromtimestamp( + os.path.getmtime(temp_zip_path), tz=timezone.utc + ) < datetime.now(timezone.utc) - timedelta( + seconds=current_app.config["PARTIAL_ZIP_EXPIRATION"] + ) + if not partial_exists or is_expired: + create_project_version_zip.delay(project_version.id) return "Project zip being prepared, please try again later", 202 diff --git a/server/mergin/sync/tasks.py b/server/mergin/sync/tasks.py index f56fb273..ce92171e 100644 --- a/server/mergin/sync/tasks.py +++ b/server/mergin/sync/tasks.py @@ -114,9 +114,17 @@ def create_project_version_zip(version_id: int): return zip_path = project_version.zip_path + ".partial" - # already running job if os.path.exists(zip_path): - return + mtime = datetime.fromtimestamp(os.path.getmtime(zip_path), tz=timezone.utc) + expiration_time = datetime.now(timezone.utc) - timedelta( + seconds=current_app.config["PARTIAL_ZIP_EXPIRATION"] + ) + if mtime > expiration_time: + # partial zip is recent -> another job is likely running + return + else: + # partial zip is too old -> remove and creating new one + os.remove(zip_path) os.makedirs(os.path.dirname(zip_path), exist_ok=True) try: diff --git a/server/mergin/tests/test_celery.py b/server/mergin/tests/test_celery.py index a5d07f47..348208d9 100644 --- a/server/mergin/tests/test_celery.py +++ b/server/mergin/tests/test_celery.py @@ -4,6 +4,8 @@ import os from datetime import datetime, timedelta +from pathlib import Path + from flask import current_app from flask_mail import Mail from unittest.mock import patch @@ -144,16 +146,44 @@ def test_remove_deleted_project_backups(client): def test_create_project_version_zip(diff_project): + """Test celery tasks for creating project zip and cleaning them up""" latest_version = diff_project.get_latest_version() - assert not os.path.exists(latest_version.zip_path) + zip_path = Path(latest_version.zip_path) + partial_zip_path = Path(latest_version.zip_path + ".partial") + assert not zip_path.exists() + create_project_version_zip(diff_project.latest_version) + assert zip_path.exists() + assert not partial_zip_path.exists() + before_mtime = zip_path.stat().st_mtime + # mock expired partial zip -> celery removes it and creates new zip + partial_zip_path.parent.mkdir(parents=True, exist_ok=True) + partial_zip_path.touch() + new_time = datetime.now() - timedelta( + seconds=current_app.config["PARTIAL_ZIP_EXPIRATION"] + 1 + ) + modify_file_times(partial_zip_path, new_time) + create_project_version_zip(diff_project.latest_version) + assert ( + not partial_zip_path.exists() + ) # after creating zip archive, celery remove partial zip + after_mtime = zip_path.stat().st_mtime + assert before_mtime < after_mtime + before_mtime = after_mtime + # mock valid partial zip -> celery skips creating new zip and returns + partial_zip_path.parent.mkdir(parents=True, exist_ok=True) + partial_zip_path.touch() create_project_version_zip(diff_project.latest_version) - assert os.path.exists(latest_version.zip_path) - assert not os.path.exists(latest_version.zip_path + ".partial") - remove_projects_archives() - assert os.path.exists(latest_version.zip_path) + assert ( + partial_zip_path.exists() + ) # celery does not create zip archive and does not clean partial zip + after_mtime = zip_path.stat().st_mtime + assert before_mtime == after_mtime + os.remove(partial_zip_path) + remove_projects_archives() # zip is valid -> keep + assert zip_path.exists() new_time = datetime.now() - timedelta( days=current_app.config["PROJECTS_ARCHIVES_EXPIRATION"] + 1 ) modify_file_times(latest_version.zip_path, new_time) - remove_projects_archives() + remove_projects_archives() # zip has expired -> remove assert not os.path.exists(latest_version.zip_path) diff --git a/server/mergin/tests/test_private_project_api.py b/server/mergin/tests/test_private_project_api.py index 27021ecb..ef3fd140 100644 --- a/server/mergin/tests/test_private_project_api.py +++ b/server/mergin/tests/test_private_project_api.py @@ -2,15 +2,16 @@ # # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial -import datetime import json import os -from unittest.mock import patch - +import shutil import pytest +from unittest.mock import patch +from pathlib import Path +from datetime import datetime, timedelta from flask import url_for -from ..app import db +from ..app import db, current_app from ..sync.models import AccessRequest, Project, ProjectRole, RequestStatus from ..auth.models import User from ..config import Configuration @@ -20,6 +21,7 @@ login, create_project, create_workspace, + modify_file_times, ) @@ -423,37 +425,76 @@ def test_admin_project_list(client): test_download_proj_data = [ - (None, 202), - ("v1", 202), - ("v100", 404), - (None, 200), + # zips do not exist, version not specified -> call celery task to create zip with latest version + (0, 0, 0, None, 202, 1), + # expired partial zip exists -> call celery task + (0, 1, 1, None, 202, 1), + # valid partial zip exists -> return, do not call celery + (0, 1, 0, None, 202, 0), + # zips do not exist, version specified -> call celery task with specified version + (0, 0, 0, "v1", 202, 1), + # specified version does not exist -> 404 + (0, 0, 0, "v100", 404, 0), + # zip is ready to download + (1, 0, 0, None, 200, 0), ] -@pytest.mark.parametrize("version,expected", test_download_proj_data) +@pytest.mark.parametrize( + "zipfile,partial,expired,version,exp_resp,exp_call", test_download_proj_data +) @patch("mergin.sync.tasks.create_project_version_zip.delay") -def test_download_project(mock_create_zip, client, version, expected, diff_project): - if expected == 200: - project_version = diff_project.get_latest_version() - os.mknod(project_version.zip_path) - resp = client.get( - url_for( - "/app.mergin_sync_private_api_controller_download_project", - id=diff_project.id, - version=version if version else "", +def test_download_project( + mock_create_zip, + client, + zipfile, + partial, + expired, + version, + exp_resp, + exp_call, + diff_project, +): + """Test download endpoint responses and celery task calling""" + # prepare initial state according to testcase + project_version = diff_project.get_latest_version() + if zipfile: + zip_path = Path(project_version.zip_path) + zip_path.parent.mkdir(parents=True, exist_ok=True) + zip_path.touch() + if partial: + temp_zip_path = Path(project_version.zip_path + ".partial") + temp_zip_path.parent.mkdir(parents=True, exist_ok=True) + temp_zip_path.touch() + if expired: + new_time = datetime.now() - timedelta( + seconds=current_app.config["PARTIAL_ZIP_EXPIRATION"] + 1 + ) + modify_file_times(temp_zip_path, new_time) + try: + resp = client.get( + url_for( + "/app.mergin_sync_private_api_controller_download_project", + id=diff_project.id, + version=version if version else "", + ) ) - ) - if expected == 200: - os.remove(project_version.zip_path) - assert resp.status_code == expected - assert mock_create_zip.called if expected == 202 else not mock_create_zip.called - if not version and expected != 200: + finally: + # cleanup + if zipfile: + shutil.rmtree(zip_path.parent, ignore_errors=True) + if partial: + shutil.rmtree(temp_zip_path.parent, ignore_errors=True) + assert resp.status_code == exp_resp + assert mock_create_zip.called == exp_call + if not version and exp_call: call_args, _ = mock_create_zip.call_args args = call_args[0] assert args == diff_project.latest_version def test_large_project_download_fail(client, diff_project): + """Test downloading too large project is refused""" resp = client.get( url_for( "/app.mergin_sync_private_api_controller_download_project", @@ -474,6 +515,11 @@ def test_large_project_download_fail(client, diff_project): assert resp.status_code == 400 +# exists, valid -> return +# exists, expired -> remove, create +# not exists -> create + + @patch("mergin.sync.tasks.create_project_version_zip.delay") def test_remove_abandoned_zip(mock_prepare_zip, client, diff_project): """Test project download removes partial zip which is inactive for some time""" @@ -494,4 +540,24 @@ def test_remove_abandoned_zip(mock_prepare_zip, client, diff_project): ) assert mock_prepare_zip.called assert resp.status_code == 202 - assert not os.path.exists(partial_zip_path) + + +@patch("mergin.sync.tasks.create_project_version_zip.delay") +def test_download_project_request_method(mock_prepare_zip, client, diff_project): + """Test head request does not create a celery job""" + resp = client.head( + url_for( + "/app.mergin_sync_private_api_controller_download_project", + id=diff_project.id, + ) + ) + assert not mock_prepare_zip.called + assert resp.status_code == 202 + resp = client.get( + url_for( + "/app.mergin_sync_private_api_controller_download_project", + id=diff_project.id, + ) + ) + assert mock_prepare_zip.called + assert resp.status_code == 202 From fe217cb95f2f59407abd9cb14f0726e263397a47 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Fri, 15 Aug 2025 15:00:34 +0200 Subject: [PATCH 26/29] cleanup comments --- server/mergin/tests/test_private_project_api.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/server/mergin/tests/test_private_project_api.py b/server/mergin/tests/test_private_project_api.py index ef3fd140..549f3990 100644 --- a/server/mergin/tests/test_private_project_api.py +++ b/server/mergin/tests/test_private_project_api.py @@ -515,11 +515,6 @@ def test_large_project_download_fail(client, diff_project): assert resp.status_code == 400 -# exists, valid -> return -# exists, expired -> remove, create -# not exists -> create - - @patch("mergin.sync.tasks.create_project_version_zip.delay") def test_remove_abandoned_zip(mock_prepare_zip, client, diff_project): """Test project download removes partial zip which is inactive for some time""" From 4c2dd7708391d32a074134cd8c0467d3490f85dc Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Fri, 15 Aug 2025 15:12:27 +0200 Subject: [PATCH 27/29] fix datetime usage after import change --- server/mergin/tests/test_private_project_api.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/mergin/tests/test_private_project_api.py b/server/mergin/tests/test_private_project_api.py index 549f3990..2cd815c9 100644 --- a/server/mergin/tests/test_private_project_api.py +++ b/server/mergin/tests/test_private_project_api.py @@ -176,7 +176,7 @@ def test_project_access_request(client): # try with inactive project rp = create_project("removed_project", test_workspace, user) - rp.removed_at = datetime.datetime.utcnow() + rp.removed_at = datetime.utcnow() db.session.commit() resp = client.post( @@ -195,7 +195,7 @@ def test_project_access_request(client): assert resp.status_code == 200 resp = client.get("/app/project/access-requests?page=1&per_page=10") assert resp.json.get("count") == 1 - rp.removed_at = datetime.datetime.utcnow() + rp.removed_at = datetime.utcnow() db.session.commit() access_request = AccessRequest.query.filter( AccessRequest.project_id == rp.id @@ -377,7 +377,7 @@ def test_admin_project_list(client): assert resp.json.get("count") == 1 # mark as inactive p = Project.query.get(resp.json["items"][0]["id"]) - p.removed_at = datetime.datetime.utcnow() + p.removed_at = datetime.utcnow() p.removed_by = user.id db.session.commit() From 03f62a3b88f1f3dae95d7c4547dda9b12fd28925 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Fri, 15 Aug 2025 15:23:23 +0200 Subject: [PATCH 28/29] improve tests readability --- server/mergin/tests/test_celery.py | 2 +- .../mergin/tests/test_private_project_api.py | 22 ++++++++----------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/server/mergin/tests/test_celery.py b/server/mergin/tests/test_celery.py index 348208d9..50420cf0 100644 --- a/server/mergin/tests/test_celery.py +++ b/server/mergin/tests/test_celery.py @@ -168,8 +168,8 @@ def test_create_project_version_zip(diff_project): ) # after creating zip archive, celery remove partial zip after_mtime = zip_path.stat().st_mtime assert before_mtime < after_mtime - before_mtime = after_mtime # mock valid partial zip -> celery skips creating new zip and returns + before_mtime = zip_path.stat().st_mtime partial_zip_path.parent.mkdir(parents=True, exist_ok=True) partial_zip_path.touch() create_project_version_zip(diff_project.latest_version) diff --git a/server/mergin/tests/test_private_project_api.py b/server/mergin/tests/test_private_project_api.py index 2cd815c9..54575114 100644 --- a/server/mergin/tests/test_private_project_api.py +++ b/server/mergin/tests/test_private_project_api.py @@ -460,10 +460,13 @@ def test_download_project( project_version = diff_project.get_latest_version() if zipfile: zip_path = Path(project_version.zip_path) + if zip_path.parent.exists(): + shutil.rmtree(zip_path.parent, ignore_errors=True) zip_path.parent.mkdir(parents=True, exist_ok=True) zip_path.touch() if partial: temp_zip_path = Path(project_version.zip_path + ".partial") + shutil.rmtree(temp_zip_path.parent, ignore_errors=True) temp_zip_path.parent.mkdir(parents=True, exist_ok=True) temp_zip_path.touch() if expired: @@ -471,20 +474,13 @@ def test_download_project( seconds=current_app.config["PARTIAL_ZIP_EXPIRATION"] + 1 ) modify_file_times(temp_zip_path, new_time) - try: - resp = client.get( - url_for( - "/app.mergin_sync_private_api_controller_download_project", - id=diff_project.id, - version=version if version else "", - ) + resp = client.get( + url_for( + "/app.mergin_sync_private_api_controller_download_project", + id=diff_project.id, + version=version if version else "", ) - finally: - # cleanup - if zipfile: - shutil.rmtree(zip_path.parent, ignore_errors=True) - if partial: - shutil.rmtree(temp_zip_path.parent, ignore_errors=True) + ) assert resp.status_code == exp_resp assert mock_create_zip.called == exp_call if not version and exp_call: From 076202dd8d123105d23622b1f8e11a39f9952d49 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Wed, 27 Aug 2025 08:45:22 +0200 Subject: [PATCH 29/29] Add post request to explicitely prepare download archive (#499) --- server/mergin/sync/private_api.yaml | 40 +++-- server/mergin/sync/private_api_controller.py | 50 ++++-- server/mergin/sync/tasks.py | 2 +- .../mergin/tests/test_private_project_api.py | 145 ++++++++---------- .../lib/src/modules/project/projectApi.ts | 4 + .../packages/lib/src/modules/project/store.ts | 70 +++++---- 6 files changed, 175 insertions(+), 136 deletions(-) diff --git a/server/mergin/sync/private_api.yaml b/server/mergin/sync/private_api.yaml index b5fa2c87..48a88933 100644 --- a/server/mergin/sync/private_api.yaml +++ b/server/mergin/sync/private_api.yaml @@ -384,20 +384,20 @@ paths: $ref: "#/components/responses/NotFoundResp" x-openapi-router-controller: mergin.sync.private_api_controller /projects/{id}/download: + parameters: + - $ref: "#/components/parameters/ProjectId" + - name: version + in: query + description: Particular version to download + required: false + schema: + $ref: "#/components/schemas/VersionName" get: tags: - project summary: Download full project description: Download whole project folder as zip file operationId: download_project - parameters: - - $ref: "#/components/parameters/ProjectId" - - name: version - in: query - description: Particular version to download - required: false - schema: - $ref: "#/components/schemas/VersionName" responses: "200": description: Zip file @@ -417,6 +417,26 @@ paths: "404": $ref: "#/components/responses/NotFoundResp" x-openapi-router-controller: mergin.sync.private_api_controller + post: + tags: + - project + summary: Prepare project archive + description: Prepare project zip archive to download + operationId: prepare_archive + responses: + "201": + description: Project archive creation started + "204": + $ref: "#/components/responses/NoContent" + "400": + $ref: "#/components/responses/BadStatusResp" + "422": + $ref: "#/components/responses/UnprocessableEntity" + "403": + $ref: "#/components/responses/Forbidden" + "404": + $ref: "#/components/responses/NotFoundResp" + x-openapi-router-controller: mergin.sync.private_api_controller components: responses: UnauthorizedError: @@ -436,7 +456,9 @@ components: UnsupportedMediaType: description: Payload format is in an unsupported format. ConflictResp: - description: Request could not be processed becuase of conflict in resources + description: Request could not be processed because of conflict in resources + NoContent: + description: Success. No content returned. parameters: Page: name: page diff --git a/server/mergin/sync/private_api_controller.py b/server/mergin/sync/private_api_controller.py index 13f059b9..dd870aae 100644 --- a/server/mergin/sync/private_api_controller.py +++ b/server/mergin/sync/private_api_controller.py @@ -322,7 +322,7 @@ def get_project_access(id: str): def download_project(id: str, version=None): # noqa: E501 # pylint: disable=W0622 """Download whole project folder as zip file in any version - Return zip file if it exists, otherwise trigger background job to create it""" + Return zip file if it exists, otherwise return 202""" project = require_project_by_uuid(id, ProjectPermissions.Read) lookup_version = ( ProjectVersion.from_v_name(version) if version else project.latest_version @@ -331,9 +331,6 @@ def download_project(id: str, version=None): # noqa: E501 # pylint: disable=W06 project_id=project.id, name=lookup_version ).first_or_404("Project version does not exist") - if project_version.project_size > current_app.config["MAX_DOWNLOAD_ARCHIVE_SIZE"]: - abort(400) - # check zip is already created if os.path.exists(project_version.zip_path): if current_app.config["USE_X_ACCEL"]: @@ -352,17 +349,36 @@ def download_project(id: str, version=None): # noqa: E501 # pylint: disable=W06 f"attachment; filename*=UTF-8''{file_name}" ) return resp - # GET request triggers background job if no partial zip or expired one - if request.method == "GET": - temp_zip_path = project_version.zip_path + ".partial" - # create zip if it does not exist yet or has expired - partial_exists = os.path.exists(temp_zip_path) - is_expired = partial_exists and datetime.fromtimestamp( - os.path.getmtime(temp_zip_path), tz=timezone.utc - ) < datetime.now(timezone.utc) - timedelta( - seconds=current_app.config["PARTIAL_ZIP_EXPIRATION"] - ) - if not partial_exists or is_expired: - create_project_version_zip.delay(project_version.id) - return "Project zip being prepared, please try again later", 202 + return "Project zip being prepared", 202 + + +def prepare_archive(id: str, version=None): + """Triggers background job to create project archive""" + project = require_project_by_uuid(id, ProjectPermissions.Read) + lookup_version = ( + ProjectVersion.from_v_name(version) if version else project.latest_version + ) + pv = ProjectVersion.query.filter_by( + project_id=project.id, name=lookup_version + ).first_or_404() + + if pv.project_size > current_app.config["MAX_DOWNLOAD_ARCHIVE_SIZE"]: + abort(400) + + if os.path.exists(pv.zip_path): + return NoContent, 204 + + # trigger job if no recent partial + temp_zip_path = pv.zip_path + ".partial" + partial_exists = os.path.exists(temp_zip_path) + is_expired = partial_exists and datetime.fromtimestamp( + os.path.getmtime(temp_zip_path), tz=timezone.utc + ) < datetime.now(timezone.utc) - timedelta( + seconds=current_app.config["PARTIAL_ZIP_EXPIRATION"] + ) + if partial_exists and not is_expired: + return NoContent, 204 + + create_project_version_zip.delay(pv.id) + return "Project zip creation started", 201 diff --git a/server/mergin/sync/tasks.py b/server/mergin/sync/tasks.py index ce92171e..770320ea 100644 --- a/server/mergin/sync/tasks.py +++ b/server/mergin/sync/tasks.py @@ -123,7 +123,7 @@ def create_project_version_zip(version_id: int): # partial zip is recent -> another job is likely running return else: - # partial zip is too old -> remove and creating new one + # partial zip is too old -> remove and create new one os.remove(zip_path) os.makedirs(os.path.dirname(zip_path), exist_ok=True) diff --git a/server/mergin/tests/test_private_project_api.py b/server/mergin/tests/test_private_project_api.py index 54575114..5f062ac2 100644 --- a/server/mergin/tests/test_private_project_api.py +++ b/server/mergin/tests/test_private_project_api.py @@ -424,27 +424,81 @@ def test_admin_project_list(client): assert len(resp.json["items"]) == 14 -test_download_proj_data = [ - # zips do not exist, version not specified -> call celery task to create zip with latest version - (0, 0, 0, None, 202, 1), +def test_download_project( + client, + diff_project, +): + """Test download endpoint responses""" + resp = client.head( + url_for( + "/app.mergin_sync_private_api_controller_download_project", + id=diff_project.id, + version="", + ) + ) + # zip archive does not exist yet + assert resp.status_code == 202 + project_version = diff_project.get_latest_version() + # pretend archive has been created + zip_path = Path(project_version.zip_path) + if zip_path.parent.exists(): + shutil.rmtree(zip_path.parent, ignore_errors=True) + zip_path.parent.mkdir(parents=True, exist_ok=True) + zip_path.touch() + resp = client.head( + url_for( + "/app.mergin_sync_private_api_controller_download_project", + id=diff_project.id, + version="", + ) + ) + # zip archive is ready -> download can start + assert resp.status_code == 200 + + +def test_prepare_large_project_fail(client, diff_project): + """Test asking for too large project is refused""" + resp = client.post( + url_for( + "/app.mergin_sync_private_api_controller_prepare_archive", + id=diff_project.id, + version="v1", + ) + ) + assert resp.status_code == 201 + # pretend testing project to be too large by lowering limit + client.application.config["MAX_DOWNLOAD_ARCHIVE_SIZE"] = 10 + resp = client.post( + url_for( + "/app.mergin_sync_private_api_controller_prepare_archive", + id=diff_project.id, + version="v1", + ) + ) + assert resp.status_code == 400 + + +test_prepare_proj_data = [ + # zips do not exist, version not specified -> trigger celery to create zip with latest version + (0, 0, 0, None, 201, 1), # expired partial zip exists -> call celery task - (0, 1, 1, None, 202, 1), - # valid partial zip exists -> return, do not call celery - (0, 1, 0, None, 202, 0), + (0, 1, 1, None, 201, 1), + # valid partial zip exists -> do not call celery + (0, 1, 0, None, 204, 0), # zips do not exist, version specified -> call celery task with specified version - (0, 0, 0, "v1", 202, 1), + (0, 0, 0, "v1", 201, 1), # specified version does not exist -> 404 (0, 0, 0, "v100", 404, 0), - # zip is ready to download - (1, 0, 0, None, 200, 0), + # zip is already prepared to download -> do not call celery + (1, 0, 0, None, 204, 0), ] @pytest.mark.parametrize( - "zipfile,partial,expired,version,exp_resp,exp_call", test_download_proj_data + "zipfile,partial,expired,version,exp_resp,exp_call", test_prepare_proj_data ) @patch("mergin.sync.tasks.create_project_version_zip.delay") -def test_download_project( +def test_prepare_archive( mock_create_zip, client, zipfile, @@ -455,7 +509,7 @@ def test_download_project( exp_call, diff_project, ): - """Test download endpoint responses and celery task calling""" + """Test prepare archive endpoint responses and celery task calling""" # prepare initial state according to testcase project_version = diff_project.get_latest_version() if zipfile: @@ -474,7 +528,7 @@ def test_download_project( seconds=current_app.config["PARTIAL_ZIP_EXPIRATION"] + 1 ) modify_file_times(temp_zip_path, new_time) - resp = client.get( + resp = client.post( url_for( "/app.mergin_sync_private_api_controller_download_project", id=diff_project.id, @@ -487,68 +541,3 @@ def test_download_project( call_args, _ = mock_create_zip.call_args args = call_args[0] assert args == diff_project.latest_version - - -def test_large_project_download_fail(client, diff_project): - """Test downloading too large project is refused""" - resp = client.get( - url_for( - "/app.mergin_sync_private_api_controller_download_project", - id=diff_project.id, - version="v1", - ) - ) - assert resp.status_code == 202 - # pretend testing project to be too large by lowering limit - client.application.config["MAX_DOWNLOAD_ARCHIVE_SIZE"] = 10 - resp = client.get( - url_for( - "/app.mergin_sync_private_api_controller_download_project", - id=diff_project.id, - version="v1", - ) - ) - assert resp.status_code == 400 - - -@patch("mergin.sync.tasks.create_project_version_zip.delay") -def test_remove_abandoned_zip(mock_prepare_zip, client, diff_project): - """Test project download removes partial zip which is inactive for some time""" - latest_version = diff_project.get_latest_version() - # pretend an incomplete zip remained - partial_zip_path = latest_version.zip_path + ".partial" - os.makedirs(os.path.dirname(partial_zip_path), exist_ok=True) - os.mknod(partial_zip_path) - assert os.path.exists(partial_zip_path) - # pretend abandoned partial zip by lowering the expiration limit - client.application.config["PARTIAL_ZIP_EXPIRATION"] = 0 - # download should remove it (move to temp folder) and call a celery task which will try to create a correct zip - resp = client.get( - url_for( - "/app.mergin_sync_private_api_controller_download_project", - id=diff_project.id, - ) - ) - assert mock_prepare_zip.called - assert resp.status_code == 202 - - -@patch("mergin.sync.tasks.create_project_version_zip.delay") -def test_download_project_request_method(mock_prepare_zip, client, diff_project): - """Test head request does not create a celery job""" - resp = client.head( - url_for( - "/app.mergin_sync_private_api_controller_download_project", - id=diff_project.id, - ) - ) - assert not mock_prepare_zip.called - assert resp.status_code == 202 - resp = client.get( - url_for( - "/app.mergin_sync_private_api_controller_download_project", - id=diff_project.id, - ) - ) - assert mock_prepare_zip.called - assert resp.status_code == 202 diff --git a/web-app/packages/lib/src/modules/project/projectApi.ts b/web-app/packages/lib/src/modules/project/projectApi.ts index 91b6edbb..657675c8 100644 --- a/web-app/packages/lib/src/modules/project/projectApi.ts +++ b/web-app/packages/lib/src/modules/project/projectApi.ts @@ -311,6 +311,10 @@ export const ProjectApi = { return ProjectModule.httpService.get(url, { responseType: 'blob' }) }, + async prepareArchive(url: string): Promise> { + return ProjectModule.httpService.post(url) + }, + /** Request head of file download */ async getHeadDownloadFile(url: string): Promise> { return ProjectModule.httpService.head(url) diff --git a/web-app/packages/lib/src/modules/project/store.ts b/web-app/packages/lib/src/modules/project/store.ts index 182bdea2..53150334 100644 --- a/web-app/packages/lib/src/modules/project/store.ts +++ b/web-app/packages/lib/src/modules/project/store.ts @@ -719,8 +719,12 @@ export const useProjectStore = defineStore('projectModule', { const delays = [...Array(3).fill(1000), ...Array(3).fill(3000), 5000] let retryCount = 0 - const pollDownloadArchive = async () => { - try { + try { + // STEP 1: request archive creation + await ProjectApi.prepareArchive(payload.url) + + // STEP 2: start polling HEAD for readiness + const pollDownloadArchive = async () => { if (retryCount > 125) { notificationStore.warn({ text: exceedMessage, @@ -729,38 +733,42 @@ export const useProjectStore = defineStore('projectModule', { await this.cancelDownloadArchive() return } - - const head = await ProjectApi.getHeadDownloadFile(payload.url) - const polling = head.status == 202 - if (polling) { - const delay = delays[Math.min(retryCount, delays.length - 1)] // Select delay based on retry count - retryCount++ // Increment retry count - downloadArchiveTimeout = setTimeout(async () => { - await pollDownloadArchive() - }, delay) - return - } - - // Use browser download instead of playing around with the blob - FileSaver.saveAs(payload.url) - notificationStore.closeNotification() - this.cancelDownloadArchive() - } catch (e) { - if (axios.isAxiosError(e) && e.response?.status === 400) { - notificationStore.error({ - group: 'download-large-error', - text: '', - life: 6000 - }) - } else { - notificationStore.error({ - text: errorMessage - }) + try { + const head = await ProjectApi.getHeadDownloadFile(payload.url) + const polling = head.status === 202 + if (polling) { + const delay = delays[Math.min(retryCount, delays.length - 1)] // Select delay based on retry count + retryCount++ // Increment retry count + downloadArchiveTimeout = setTimeout(async () => { + await pollDownloadArchive() + }, delay) + return + } + + // Use browser download instead of playing around with the blob + FileSaver.saveAs(payload.url) + notificationStore.closeNotification() + this.cancelDownloadArchive() + } catch (e) { + if (axios.isAxiosError(e) && e.response?.status === 400) { + notificationStore.error({ + group: 'download-large-error', + text: '', + life: 6000 + }) + } else { + notificationStore.error({ + text: errorMessage + }) + } + this.cancelDownloadArchive() } - this.cancelDownloadArchive() } + pollDownloadArchive() + } catch (e) { + notificationStore.error({ text: errorMessage }) + this.cancelDownloadArchive() } - pollDownloadArchive() }, async cancelDownloadArchive() {