diff --git a/.github/workflows/ci-comprehensive.yml b/.github/workflows/ci-comprehensive.yml index 622585ce..c690503e 100644 --- a/.github/workflows/ci-comprehensive.yml +++ b/.github/workflows/ci-comprehensive.yml @@ -99,6 +99,8 @@ jobs: run: | if [ "${{ matrix.test-group }}" == "api" ]; then pytest -m "api and integration" -v -n auto --cov=app --cov-report=xml --cov-report=html --cov-report=term-missing + elif [ "${{ matrix.test-group }}" == "routes" ]; then + pytest -m "unit and routes" -v -n 0 --cov=app --cov-report=xml --cov-report=html --cov-report=term-missing else pytest -m "unit and ${{ matrix.test-group }}" -v -n auto --cov=app --cov-report=xml --cov-report=html --cov-report=term-missing fi diff --git a/app/models/client_note.py b/app/models/client_note.py index 0ac46dac..9cbb5e28 100644 --- a/app/models/client_note.py +++ b/app/models/client_note.py @@ -26,7 +26,10 @@ class ClientNote(db.Model): # Relationships author = db.relationship("User", backref="client_notes") - client = db.relationship("Client", backref=db.backref("notes", lazy="dynamic"), passive_deletes=True) + client = db.relationship( + "Client", + backref=db.backref("notes", lazy="dynamic", cascade="all, delete-orphan", passive_deletes=True), + ) def __init__(self, content, user_id, client_id, is_important=False): """Create a client note. diff --git a/app/routes/custom_field_definitions.py b/app/routes/custom_field_definitions.py index 430ea8d2..bc00da5c 100644 --- a/app/routes/custom_field_definitions.py +++ b/app/routes/custom_field_definitions.py @@ -166,9 +166,10 @@ def delete_custom_field_definition(definition_id): flash(_("Could not remove custom field from clients due to a database error."), "error") return redirect(url_for("custom_field_definitions.list_custom_field_definitions")) - # Now delete the definition + # Now delete the definition (capture id before delete to avoid using detached object) + definition_id = definition.id db.session.delete(definition) - if not safe_commit("delete_custom_field_definition", {"definition_id": definition.id}): + if not safe_commit("delete_custom_field_definition", {"definition_id": definition_id}): flash(_("Could not delete custom field definition due to a database error."), "error") else: if client_count > 0: diff --git a/tests/test_client_note_model.py b/tests/test_client_note_model.py index 79f465df..20c01366 100644 --- a/tests/test_client_note_model.py +++ b/tests/test_client_note_model.py @@ -111,7 +111,7 @@ def test_client_has_notes_relationship(app, user, test_client): db.session.commit() db.session.refresh(client) - assert len(client.notes) == 2 + assert client.notes.count() == 2 @pytest.mark.unit diff --git a/tests/test_client_portal.py b/tests/test_client_portal.py index 69d99013..6bfffa69 100644 --- a/tests/test_client_portal.py +++ b/tests/test_client_portal.py @@ -267,14 +267,16 @@ class TestClientPortalRoutes: """Test client portal routes""" def test_client_portal_dashboard_requires_access(self, app, client, user): - """Test dashboard requires client portal access""" + """Test dashboard requires client portal access - redirects to client portal login when user has no portal access""" with app.app_context(): # Login user without portal access with client.session_transaction() as sess: sess["_user_id"] = str(user.id) response = client.get("/client-portal/dashboard") - assert response.status_code == 403 + assert response.status_code == 302 + # Client portal 403 handler redirects authenticated non-portal users to client portal login + assert "client-portal" in (response.location or "") and "login" in (response.location or "") def test_client_portal_dashboard_with_access(self, app, client, user, test_client): """Test dashboard accessible with portal access""" diff --git a/tests/test_currency_display.py b/tests/test_currency_display.py index 5951894b..87de7a9c 100644 --- a/tests/test_currency_display.py +++ b/tests/test_currency_display.py @@ -13,6 +13,7 @@ from app.models import User, Project, Settings, Client, Payment, Invoice, Expense from factories import ClientFactory, ProjectFactory, InvoiceFactory, ExpenseFactory from flask_login import login_user +from sqlalchemy import text from sqlalchemy.pool import StaticPool @@ -108,13 +109,16 @@ def app(): except Exception: pass - try: - db.session.execute("PRAGMA journal_mode=WAL;") - db.session.execute("PRAGMA synchronous=NORMAL;") - db.session.execute("PRAGMA busy_timeout=30000;") - db.session.commit() - except Exception: - db.session.rollback() + # PRAGMAs only for file-based SQLite; in-memory (sqlite://) can leave the session broken + db_uri = app.config.get("SQLALCHEMY_DATABASE_URI", "") or "" + if db_uri.strip().startswith("sqlite:///") and db_uri != "sqlite://": + try: + db.session.execute(text("PRAGMA journal_mode=WAL;")) + db.session.execute(text("PRAGMA synchronous=NORMAL;")) + db.session.execute(text("PRAGMA busy_timeout=30000;")) + db.session.commit() + except Exception: + db.session.rollback() try: yield app finally: diff --git a/tests/test_custom_field_definitions.py b/tests/test_custom_field_definitions.py index c80b4e8e..1a0e8c3a 100644 --- a/tests/test_custom_field_definitions.py +++ b/tests/test_custom_field_definitions.py @@ -72,8 +72,9 @@ def test_count_clients_with_value_with_clients(app, admin_user, test_client): db.session.add(definition) db.session.commit() - # Set custom field value for client - test_client.set_custom_field("test_field", "test_value") + # Re-query client so it's in the current session + client = Client.query.get(test_client.id) + client.set_custom_field("test_field", "test_value") db.session.commit() count = definition.count_clients_with_value() @@ -102,22 +103,24 @@ def test_count_clients_with_value_ignores_empty(app, admin_user, test_client): db.session.add(definition) db.session.commit() + # Re-query client so it's in the current session + client = Client.query.get(test_client.id) # Set empty value - test_client.set_custom_field("test_field", "") + client.set_custom_field("test_field", "") db.session.commit() count = definition.count_clients_with_value() assert count == 0 # Set whitespace-only value - test_client.set_custom_field("test_field", " ") + client.set_custom_field("test_field", " ") db.session.commit() count = definition.count_clients_with_value() assert count == 0 # Set actual value - test_client.set_custom_field("test_field", "valid_value") + client.set_custom_field("test_field", "valid_value") db.session.commit() count = definition.count_clients_with_value() @@ -137,15 +140,17 @@ def test_count_clients_with_value_ignores_other_fields(app, admin_user, test_cli db.session.add(definition) db.session.commit() + # Re-query client so it's in the current session + client = Client.query.get(test_client.id) # Set a different field - test_client.set_custom_field("other_field", "value") + client.set_custom_field("other_field", "value") db.session.commit() count = definition.count_clients_with_value() assert count == 0 # Set the correct field - test_client.set_custom_field("test_field", "value") + client.set_custom_field("test_field", "value") db.session.commit() count = definition.count_clients_with_value() @@ -190,10 +195,10 @@ def test_delete_custom_field_removes_from_clients(app, admin_user, test_client, deleted_definition = CustomFieldDefinition.query.get(definition.id) assert deleted_definition is None - # Verify field is removed from client - db.session.refresh(test_client) - assert test_client.get_custom_field("debtor_number") is None - assert "debtor_number" not in (test_client.custom_fields or {}) + # Verify field is removed from client (re-query; test_client may be detached after request) + client_after = Client.query.get(test_client.id) + assert client_after.get_custom_field("debtor_number") is None + assert "debtor_number" not in (client_after.custom_fields or {}) @pytest.mark.integration @@ -297,8 +302,8 @@ def test_delete_custom_field_preserves_other_fields(app, admin_user, test_client assert response.status_code == 200 - # Verify field1 is removed but field2 remains - db.session.refresh(test_client) - assert test_client.get_custom_field("field1") is None - assert test_client.get_custom_field("field2") == "value2" + # Verify field1 is removed but field2 remains (re-query; test_client may be detached after request) + client_after = Client.query.get(test_client.id) + assert client_after.get_custom_field("field1") is None + assert client_after.get_custom_field("field2") == "value2" diff --git a/tests/test_oidc_logout.py b/tests/test_oidc_logout.py index 17c11bcb..509d84f3 100644 --- a/tests/test_oidc_logout.py +++ b/tests/test_oidc_logout.py @@ -79,6 +79,7 @@ def test_logout_with_post_logout_uri_config(oidc_authenticated_client, app): logout attempts RP-Initiated Logout at the provider. """ with app.app_context(): + app.config["AUTH_METHOD"] = "oidc" # Mock oauth client and Config with patch("app.routes.auth.oauth") as mock_oauth, patch("app.routes.auth.Config") as mock_config: # Configure OIDC with post-logout redirect @@ -108,6 +109,7 @@ def test_logout_oidc_provider_has_revocation_endpoint_only(oidc_authenticated_cl Should use revocation_endpoint as fallback when post_logout URI is configured. """ with app.app_context(): + app.config["AUTH_METHOD"] = "oidc" with patch("app.routes.auth.oauth") as mock_oauth, patch("app.routes.auth.Config") as mock_config: mock_config.AUTH_METHOD = "oidc" mock_config.OIDC_POST_LOGOUT_REDIRECT_URI = "https://app.example.com/" diff --git a/tests/test_permissions_routes.py b/tests/test_permissions_routes.py index 58c1425f..3119880c 100644 --- a/tests/test_permissions_routes.py +++ b/tests/test_permissions_routes.py @@ -132,10 +132,9 @@ def test_delete_role_flow(app, client, admin_user): response = client.post(f"/admin/roles/{role_id}/delete", follow_redirects=True) assert response.status_code == 200 - # Verify deletion + # Verify deletion (assert by name; redirect may trigger sync_permissions_and_roles and reuse id) with app.app_context(): - role = Role.query.get(role_id) - assert role is None + assert Role.query.filter_by(name="deletable_role").first() is None @pytest.mark.integration diff --git a/tests/test_uploads_persistence.py b/tests/test_uploads_persistence.py index e481c869..473fa334 100644 --- a/tests/test_uploads_persistence.py +++ b/tests/test_uploads_persistence.py @@ -44,7 +44,7 @@ def authenticated_admin_client(client, admin_user): except Exception: csrf_enabled = False - login_data = {"username": admin_user.username} + login_data = {"username": admin_user.username, "password": "testpass123"} headers = {} if csrf_enabled: