fix: make organisation audit changes and affected users explicit

documentation/changelog.rst

@@ -48,6 +48,7 @@ Infrastructure / Support
 Bugfixes
 -----------
 
+* Organisation audit logs now show only changed fields with their previous and new values, and user role and active-status changes identify the affected user [see `PR #2522 <https://www.github.com/FlexMeasures/flexmeasures/pull/2522>`_]
 * Where several data sources report the same event, which one a search keeps is now decided the same way every time: a source version only counts against other versions of that source, and a caller that lists its sources gets the order it asked for [see `PR #2494 <https://www.github.com/FlexMeasures/flexmeasures/pull/2494>`_]
 * A KPI on the asset page counted an event once per data source that reported it, so a total could come out higher than any source reported; it now reduces one value per event, from the latest source version and the most recent belief within it [see `PR #2472 <https://www.github.com/FlexMeasures/flexmeasures/pull/2472>`_]
 * ``flexmeasures add schedule --dry-run`` no longer saves a schedule when it is combined with ``--as-job``, where the flag used to be dropped without a word and the queued job stored its schedule anyway; that combination is now rejected, and a dry run says how many beliefs it would have saved and which events they cover [see `PR #2483 <https://www.github.com/FlexMeasures/flexmeasures/pull/2483>`_]

flexmeasures/api/v3_0/accounts.py

@@ -14,7 +14,7 @@
 )
 from flexmeasures.auth.decorators import permission_required_for_context
 from flexmeasures.data.models.annotations import Annotation, get_or_create_annotation
-from flexmeasures.data.models.audit_log import AuditLog
+from flexmeasures.data.models.audit_log import AuditLog, truncate_string
 from flexmeasures.data.models.user import Account, AccountRole, Plan, User
 from flexmeasures.data.models.generic_assets import GenericAsset
 from flexmeasures.data.services.accounts import get_accounts, get_audit_log_records
@@ -386,30 +386,37 @@ def patch(self, account_data: dict, id: int, account: Account):
             "account_roles",
         ]
 
-        modified_fields = {
-            field: getattr(account, field)
-            for field in fields_to_check
-            if account_data.get(field) != getattr(account, field)
-        }
-
-        # Compile modified fields string
-        modified_fields_str = ", ".join(modified_fields.keys())
+        modified_fields = {}
+        for field in fields_to_check:
+            if field not in account_data:
+                continue
+            old_value = getattr(account, field)
+            new_value = account_data[field]
+            if field == "account_roles":
+                old_value = sorted(role.name for role in old_value)
+                new_value = sorted(role.name for role in new_value)
+            if old_value != new_value:
+                modified_fields[field] = (old_value, new_value)
+
+        # Compile modified fields with their old and new values before updating.
+        changes = "; ".join(
+            f"{field}: {old_value!r} -> {new_value!r}"
+            for field, (old_value, new_value) in modified_fields.items()
+        )
 
         for k, v in account_data.items():
             setattr(account, k, v)
 
-        event_message = f"Account Updated, Field: {modified_fields_str}"
-
-        # Add Audit log
-        account_audit_log = AuditLog(
-            event_datetime=server_now(),
-            event=event_message,
-            active_user_id=current_user.id,
-            active_user_name=current_user.username,
-            affected_user_id=current_user.id,
-            affected_account_id=account.id,
-        )
-        db.session.add(account_audit_log)
+        # Add Audit log only when values actually changed.
+        if modified_fields:
+            account_audit_log = AuditLog(
+                event_datetime=server_now(),
+                event=truncate_string(f"Organisation updated: {changes}", 500),
+                active_user_id=current_user.id,
+                active_user_name=current_user.username,
+                affected_account_id=account.id,
+            )
+            db.session.add(account_audit_log)
         db.session.commit()
         return account_schema.dump(account), 200
 

flexmeasures/api/v3_0/tests/test_accounts_audit_fresh_db.py

@@ -0,0 +1,139 @@
+import json
+
+from flask import url_for
+import pytest
+from sqlalchemy import select
+
+from flexmeasures.data.models.audit_log import AuditLog
+from flexmeasures.data.models.user import Account
+from flexmeasures.data.services.users import find_user_by_email
+
+pytestmark = pytest.mark.parametrize(
+    "requesting_user", ["test_admin_user@seita.nl"], indirect=True
+)
+
+
+def account_events(db, account_id):
+    return db.session.scalars(
+        select(AuditLog).filter_by(affected_account_id=account_id)
+    ).all()
+
+
+@pytest.mark.parametrize("resubmit_unchanged_fields", [False, True])
+def test_account_attribute_audit_reports_only_changes(
+    fresh_db,
+    client,
+    setup_roles_users_fresh_db,
+    requesting_user,
+    resubmit_unchanged_fields,
+):
+    """Attribute edits record old and new values without listing unchanged fields."""
+    account = find_user_by_email("test_prosumer_user@seita.nl").account
+    account.attributes = {"max_power_kw": 10}
+    fresh_db.session.commit()
+    previous_ids = {log.id for log in account_events(fresh_db, account.id)}
+    payload = {"attributes": json.dumps({"max_power_kw": 50})}
+    if resubmit_unchanged_fields:
+        payload.update(
+            name=account.name,
+            logo_url=account.logo_url,
+            account_roles=[role.id for role in account.account_roles],
+        )
+
+    response = client.patch(url_for("AccountAPI:patch", id=account.id), json=payload)
+
+    assert response.status_code == 200, response.json
+    logs = [
+        log
+        for log in account_events(fresh_db, account.id)
+        if log.id not in previous_ids
+    ]
+    assert len(logs) == 1
+    event = logs[0].event
+    assert "attributes" in event
+    assert "max_power_kw" in event
+    assert "10" in event and "50" in event
+    for unchanged_field in ("name", "logo_url", "account_roles", "plan_id"):
+        assert unchanged_field not in event
+    assert logs[0].active_user_id == requesting_user.id
+
+
+@pytest.mark.parametrize("payload_type", ["empty", "same", "reordered_roles"])
+def test_account_noop_patch_does_not_add_audit_log(
+    fresh_db, client, setup_roles_users_fresh_db, requesting_user, payload_type
+):
+    """Unchanged values and reordered role lists produce no audit entry."""
+    account = fresh_db.session.execute(
+        select(Account).filter_by(name="Multi Role Account")
+    ).scalar_one()
+    payload = {}
+    if payload_type == "same":
+        payload = {"name": account.name, "attributes": json.dumps(account.attributes)}
+    elif payload_type == "reordered_roles":
+        payload = {
+            "account_roles": [role.id for role in reversed(account.account_roles)]
+        }
+    previous_ids = {log.id for log in account_events(fresh_db, account.id)}
+
+    response = client.patch(url_for("AccountAPI:patch", id=account.id), json=payload)
+
+    assert response.status_code == 200, response.json
+    assert {log.id for log in account_events(fresh_db, account.id)} == previous_ids
+
+
+def test_account_large_attribute_audit_fits_storage(
+    fresh_db, client, setup_roles_users_fresh_db, requesting_user
+):
+    """Large attribute values can be saved with bounded audit events."""
+    account = find_user_by_email("test_prosumer_user@seita.nl").account
+    account.attributes = {"description": "before" * 200}
+    fresh_db.session.commit()
+    previous_ids = {log.id for log in account_events(fresh_db, account.id)}
+    attributes = {"description": "after" * 200}
+
+    response = client.patch(
+        url_for("AccountAPI:patch", id=account.id),
+        json={"attributes": json.dumps(attributes)},
+    )
+
+    assert response.status_code == 200, response.json
+    assert json.loads(response.json["attributes"]) == attributes
+    logs = [
+        log
+        for log in account_events(fresh_db, account.id)
+        if log.id not in previous_ids
+    ]
+    assert len(logs) == 1
+    assert len(logs[0].event) <= 500
+    assert "attributes" in logs[0].event
+    assert "before" in logs[0].event and "after" in logs[0].event
+
+
+def test_account_role_audit_uses_role_names(
+    fresh_db, client, setup_roles_users_fresh_db, requesting_user
+):
+    """Role edits display meaningful old and new role names."""
+    account = fresh_db.session.execute(
+        select(Account).filter_by(name="Multi Role Account")
+    ).scalar_one()
+    remaining_role = next(
+        role for role in account.account_roles if role.name == "Prosumer"
+    )
+    previous_ids = {log.id for log in account_events(fresh_db, account.id)}
+
+    response = client.patch(
+        url_for("AccountAPI:patch", id=account.id),
+        json={"account_roles": [remaining_role.id]},
+    )
+
+    assert response.status_code == 200, response.json
+    logs = [
+        log
+        for log in account_events(fresh_db, account.id)
+        if log.id not in previous_ids
+    ]
+    assert len(logs) == 1
+    assert "account_roles" in logs[0].event
+    for role_name in ("Prosumer", "Supplier", "Dummy"):
+        assert role_name in logs[0].event
+    assert "object at" not in logs[0].event

flexmeasures/api/v3_0/tests/test_api_v3_0_users.py

@@ -222,7 +222,10 @@ def test_edit_user(
         assert db.session.execute(
             select(AuditLog).filter_by(
                 affected_user_id=user.id,
-                event="Active status set to 'False'.",
+                event=(
+                    f"Updated user {user.username!r} (ID: {user.id}): "
+                    "Active status changed from 'True' to 'False'."
+                ),
                 active_user_id=requesting_user.id,
                 affected_account_id=user.account_id,
             )

flexmeasures/api/v3_0/tests/test_api_v3_0_users_fresh_db.py

@@ -5,7 +5,7 @@
 from flexmeasures.api.tests.utils import UserContext
 from flexmeasures.data.services.users import find_user_by_email
 from flexmeasures.data.models.audit_log import AuditLog
-from flexmeasures.data.models.user import Account
+from flexmeasures.data.models.user import Account, User
 
 
 @pytest.mark.parametrize(
@@ -75,20 +75,75 @@ def test_user_reset_password(
     indirect=["requesting_user"],
 )
 def test_user_role_successful_modification_permission(
+    fresh_db,
     client,
     setup_roles_users_fresh_db,
     requesting_user,
     expected_status_code,
     user_to_update,
     expected_role,
 ):
+    user = fresh_db.session.get(User, user_to_update)
+    username, account_id = user.username, user.account_id
+    previous_ids = set(fresh_db.session.scalars(select(AuditLog.id)).all())
     patch_user_response = client.patch(
         url_for("UserAPI:patch", id=user_to_update),
         json={"flexmeasures_roles": expected_role},
     )
 
     print("Server responded with:\n%s" % patch_user_response.data)
     assert patch_user_response.status_code == expected_status_code
+    logs = fresh_db.session.scalars(
+        select(AuditLog).filter_by(affected_user_id=user_to_update)
+    ).all()
+    logs = [log for log in logs if log.id not in previous_ids]
+    assert len(logs) == 1
+    event = logs[0].event
+    assert username in event
+    assert str(user_to_update) in event
+    assert ("Added role(s)" if expected_role else "Removed role(s)") in event
+    assert logs[0].active_user_id == requesting_user.id
+    assert logs[0].affected_account_id == account_id
+
+    account_audit_response = client.get(url_for("AccountAPI:auditlog", id=account_id))
+    assert account_audit_response.status_code == 200
+    assert event in [log["event"] for log in account_audit_response.json]
+
+
+@pytest.mark.parametrize("requesting_user", ["test_admin_user@seita.nl"], indirect=True)
+@pytest.mark.parametrize(
+    "initial_active, active", [(True, True), (True, False), (False, True)]
+)
+def test_user_active_status_audit(
+    fresh_db,
+    client,
+    setup_roles_users_fresh_db,
+    requesting_user,
+    initial_active,
+    active,
+):
+    """Status changes identify the user and values; unchanged status adds no entry."""
+    user = find_user_by_email("test_prosumer_user@seita.nl")
+    user.active = initial_active
+    fresh_db.session.commit()
+    previous_ids = set(fresh_db.session.scalars(select(AuditLog.id)).all())
+
+    response = client.patch(
+        url_for("UserAPI:patch", id=user.id), json={"active": active}
+    )
+
+    assert response.status_code == 200, response.json
+    logs = fresh_db.session.scalars(
+        select(AuditLog).filter_by(affected_user_id=user.id)
+    ).all()
+    logs = [log for log in logs if log.id not in previous_ids]
+    if active == initial_active:
+        assert not logs
+    else:
+        assert len(logs) == 1
+        assert user.username in logs[0].event
+        assert str(user.id) in logs[0].event
+        assert f"from '{initial_active}' to '{active}'" in logs[0].event
 
 
 @pytest.mark.parametrize(

flexmeasures/api/v3_0/users.py

@@ -11,7 +11,7 @@
 from werkzeug.exceptions import Forbidden
 from flexmeasures.auth.policy import can_modify_role, check_access
 
-from flexmeasures.data.models.audit_log import AuditLog
+from flexmeasures.data.models.audit_log import AuditLog, truncate_string
 from flexmeasures.data.models.user import User as UserModel, Account
 from flexmeasures.api.common.schemas.users import AccountIdField, UserIdField
 from flexmeasures.api.common.utils.api_utils import get_accessible_accounts
@@ -476,7 +476,7 @@ def patch(self, id: int, user: UserModel, **user_data):  # noqa C901
                             f"You are not allowed to remove ({role.name}) role from this user."
                         )
                 if roles_being_removed:
-                    audit_event += f"Removed role(s): [{','.join([r.name for r in roles_being_removed])}]."
+                    audit_event += f"Removed role(s): [{','.join(sorted(r.name for r in roles_being_removed))}]. "
 
                 roles_being_added = new_roles - current_roles
                 for role in roles_being_added:
@@ -485,13 +485,14 @@ def patch(self, id: int, user: UserModel, **user_data):  # noqa C901
                             f"You are not allowed to add ({role.name}) role to this user."
                         )
                 if roles_being_added:
-                    audit_event += f"Added role(s): [{','.join([r.name for r in roles_being_added])}]."
+                    audit_event += f"Added role(s): [{','.join(sorted(r.name for r in roles_being_added))}]. "
 
+            old_value = getattr(user, k)
             setattr(user, k, v)
             if k == "active" and v is False:
                 remove_cookie_and_token_access(user)
-            if k == "active":
-                audit_event += f"Active status set to '{v}'."
+            if k == "active" and old_value != v:
+                audit_event += f"Active status changed from '{old_value}' to '{v}'. "
         if audit_event:
             user_audit_log = create_user_audit_log(audit_event, user)
             db.session.add(user_audit_log)
@@ -690,7 +691,10 @@ def create_user_audit_log(audit_event: str, user: UserModel):
         )
     return AuditLog(
         event_datetime=server_now(),
-        event=audit_event,
+        event=truncate_string(
+            f"Updated user {user.username!r} (ID: {user.id}): {audit_event.strip()}",
+            500,
+        ),
         active_user_id=active_user_id,
         active_user_name=active_user_name,
         affected_user_id=user.id,