From afd793f1cb1207f9b96bd6c9e116f3042066e180 Mon Sep 17 00:00:00 2001 From: Dries Peeters Date: Sun, 25 Jan 2026 10:03:25 +0100 Subject: [PATCH 1/2] fix(models): persist Client custom fields and ClientNote cascade delete in tests - Client: call flag_modified() after mutating custom_fields in set_custom_field() and remove_custom_field() so SQLAlchemy persists JSON changes (in-place dict updates are not tracked by default). Fixes test_count_clients_with_value_ignores_empty and test_count_clients_with_value_ignores_other_fields. - ClientNote: add ondelete=CASCADE to client_id FK so schema from db.create_all() matches migration 024 and notes are deleted when client is deleted. Fixes test_client_note_cascade_delete. --- app/models/client.py | 3 +++ app/models/client_note.py | 7 ++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/models/client.py b/app/models/client.py index 9c39fdae..5597a3f2 100644 --- a/app/models/client.py +++ b/app/models/client.py @@ -1,6 +1,7 @@ from datetime import datetime, timedelta from decimal import Decimal from werkzeug.security import generate_password_hash, check_password_hash +from sqlalchemy.orm.attributes import flag_modified from app import db from .client_prepaid_consumption import ClientPrepaidConsumption import secrets @@ -215,12 +216,14 @@ def set_custom_field(self, key, value): self.custom_fields = {} self.custom_fields[key] = value self.updated_at = datetime.utcnow() + flag_modified(self, "custom_fields") def remove_custom_field(self, key): """Remove a custom field""" if self.custom_fields and key in self.custom_fields: del self.custom_fields[key] self.updated_at = datetime.utcnow() + flag_modified(self, "custom_fields") def get_rendered_links(self): """Get all rendered links from active link templates that match this client's custom fields""" diff --git a/app/models/client_note.py b/app/models/client_note.py index 9cbb5e28..2157824c 100644 --- a/app/models/client_note.py +++ b/app/models/client_note.py @@ -12,7 +12,12 @@ class ClientNote(db.Model): content = db.Column(db.Text, nullable=False) # Reference to client - client_id = db.Column(db.Integer, db.ForeignKey("clients.id"), nullable=False, index=True) + client_id = db.Column( + db.Integer, + db.ForeignKey("clients.id", ondelete="CASCADE"), + nullable=False, + index=True, + ) # Author of the note user_id = db.Column(db.Integer, db.ForeignKey("users.id"), nullable=False, index=True) From 12074fc29b0822d34e127ff05ed031d8e9a4934a Mon Sep 17 00:00:00 2001 From: Dries Peeters Date: Sun, 25 Jan 2026 10:09:29 +0100 Subject: [PATCH 2/2] fix: resolve integration test failures (install config dir, settings flush) - Make InstallationConfig config dir overridable via INSTALLATION_CONFIG_DIR so tests and CI use a writable path instead of /data (fixes PermissionError on redirect to /admin/settings after logo upload). - Set INSTALLATION_CONFIG_DIR in conftest before app import and in ci-comprehensive.yml for integration-tests and full-test-suite jobs. - In Settings.get_settings(), add _session_in_flush() and a re-entrancy guard to skip add+commit when called during another commit's flush, fixing ResourceClosedError in currency_display test setup. - Update test_installation_config fixture to set INSTALLATION_CONFIG_DIR so it continues to use its temp dir with the new env-based behavior. --- .github/workflows/ci-comprehensive.yml | 2 ++ app/models/settings.py | 39 ++++++++++++++++++++++---- app/utils/installation.py | 14 +++++---- tests/conftest.py | 5 ++++ tests/test_installation_config.py | 1 + 5 files changed, 49 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci-comprehensive.yml b/.github/workflows/ci-comprehensive.yml index c690503e..616ec15d 100644 --- a/.github/workflows/ci-comprehensive.yml +++ b/.github/workflows/ci-comprehensive.yml @@ -167,6 +167,7 @@ jobs: FLASK_APP: app.py FLASK_ENV: testing PYTHONPATH: ${{ github.workspace }} + INSTALLATION_CONFIG_DIR: ${{ github.workspace }}/.test_installation_config run: | pytest -m integration -v -n auto --cov=app --cov-report=xml --cov-report=html --cov-report=term-missing @@ -453,6 +454,7 @@ jobs: FLASK_APP: app.py FLASK_ENV: testing PYTHONPATH: ${{ github.workspace }} + INSTALLATION_CONFIG_DIR: ${{ github.workspace }}/.test_installation_config run: | pytest -v -n auto --cov=app --cov-report=xml --cov-report=html --cov-report=term-missing \ --junitxml=junit.xml --maxfail=5 diff --git a/app/models/settings.py b/app/models/settings.py index 23655ec2..7338630b 100644 --- a/app/models/settings.py +++ b/app/models/settings.py @@ -1,7 +1,28 @@ from datetime import datetime +import os +import threading + from app import db from app.config import Config -import os + +# Re-entrancy guard: avoid add+commit when get_settings is called from inside a flush/commit +_creating_settings = threading.local() + + +def _session_in_flush(session): + """Return True if the session is currently in a flush (to avoid nested add+commit).""" + try: + # SQLAlchemy sets _flushing on the session during flush + if getattr(session, "_flushing", False): + return True + # Fallback: in a transaction and inside a flush context (if exposed) + if getattr(session, "in_transaction", lambda: False)() and getattr( + session, "_current_flush_context", None + ) is not None: + return True + return False + except Exception: + return False class Settings(db.Model): @@ -482,13 +503,17 @@ def get_settings(cls): return cls() # Avoid performing session writes during flush/commit phases. - # When called from default column factories (e.g., created_at=local_now), + # When called from default column factories or listeners during flush, # SQLAlchemy may be in the middle of a flush. Writing here would raise - # SAWarnings/ResourceClosedError. In that case, return a transient instance - # with sensible defaults; the persistent row can be created later by - # initialization code or explicit admin flows. + # SAWarnings/ResourceClosedError. Skip add+commit and return a transient + # instance; the persistent row can be created later by init or admin flows. try: - if not getattr(db.session, "_flushing", False): + if getattr(_creating_settings, "active", False): + return cls() + if _session_in_flush(db.session): + return cls() + try: + _creating_settings.active = True # Create new settings instance initialized from environment variables settings = cls() # Initialize from environment variables (.env file) @@ -496,6 +521,8 @@ def get_settings(cls): db.session.add(settings) db.session.commit() return settings + finally: + _creating_settings.active = False except Exception: # If anything goes wrong creating the persistent row, rollback and # fall back to an in-memory Settings instance. diff --git a/app/utils/installation.py b/app/utils/installation.py index 66c61ed2..2cd7eb17 100644 --- a/app/utils/installation.py +++ b/app/utils/installation.py @@ -16,17 +16,19 @@ class InstallationConfig: """Manages installation-specific configuration""" - CONFIG_DIR = "/data" + CONFIG_DIR = "/data" # default; overridden by INSTALLATION_CONFIG_DIR when set CONFIG_FILE = "installation.json" def __init__(self): - self.config_path = os.path.join(self.CONFIG_DIR, self.CONFIG_FILE) - self._ensure_config_dir() + effective_dir = os.environ.get("INSTALLATION_CONFIG_DIR", self.CONFIG_DIR) + self.config_path = os.path.join(effective_dir, self.CONFIG_FILE) + self._ensure_config_dir(effective_dir) self._config = self._load_config() - def _ensure_config_dir(self): - """Ensure the configuration directory exists""" - os.makedirs(self.CONFIG_DIR, exist_ok=True) + def _ensure_config_dir(self, config_dir=None): + """Ensure the configuration directory exists.""" + dir_path = config_dir if config_dir is not None else os.environ.get("INSTALLATION_CONFIG_DIR", self.CONFIG_DIR) + os.makedirs(dir_path, exist_ok=True) def _load_config(self) -> Dict: """Load configuration from file""" diff --git a/tests/conftest.py b/tests/conftest.py index e3a0fe4b..76a9023b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,6 +7,11 @@ import os import tempfile import uuid + +# Set before app is imported so InstallationConfig uses a writable dir in tests (avoids /data on CI) +if "INSTALLATION_CONFIG_DIR" not in os.environ: + os.environ["INSTALLATION_CONFIG_DIR"] = tempfile.mkdtemp(prefix="timetracker_install_") + from datetime import datetime, timedelta from decimal import Decimal from sqlalchemy.pool import NullPool diff --git a/tests/test_installation_config.py b/tests/test_installation_config.py index aac8f9f8..db5fc5df 100644 --- a/tests/test_installation_config.py +++ b/tests/test_installation_config.py @@ -22,6 +22,7 @@ def temp_config_dir(tmp_path): def installation_config(temp_config_dir, monkeypatch): """Create an InstallationConfig instance with temporary directory""" monkeypatch.setattr("app.utils.installation.InstallationConfig.CONFIG_DIR", temp_config_dir) + monkeypatch.setenv("INSTALLATION_CONFIG_DIR", temp_config_dir) config = InstallationConfig() return config