Fix notification & audit trail for dataclip retention policy changes

CHANGELOG.md

@@ -38,6 +38,15 @@ and this project adheres to
   edits an already-saved workflow. Name-uniqueness validation now excludes the
   workflow being edited, so its own name isn't treated as a clash.
   [#5009](https://github.com/OpenFn/lightning/pull/5009)
+- Project owner and admins are notified properly when the I/O data retention
+  policy is changed. [#5088](https://github.com/OpenFn/lightning/issues/5088)
+- I/O data retention policy changes are now properly appended to the audit
+  trail. [#5090](https://github.com/OpenFn/lightning/issues/5090)
+- Switching the retention policy to "erase all" without touching the period
+  fields now clears the stale dataclip retention period, instead of leaving the
+  old window active. The notification email also no longer quotes an I/O data
+  retention window when the policy says I/O data is never saved.
+  [#5088](https://github.com/OpenFn/lightning/issues/5088)
 
 ## [2.18.0] - 2026-08-20
 

lib/lightning/accounts/user_notifier.ex

@@ -147,6 +147,21 @@ defmodule Lightning.Accounts.UserNotifier do
 
     io_data_saved = updated_project.retention_policy != :erase_all
 
+    # No I/O retention bullet under :erase_all — I/O data is never saved,
+    # so quoting a retention window for it would contradict the line above.
+    details =
+      [
+        "- #{history_retention_period} #{pluralize_with_s(history_retention_period, "day")} history retention",
+        "- input/output (I/O) data #{if io_data_saved, do: "is", else: "is not"} saved for reprocessing"
+      ] ++
+        if io_data_saved do
+          [
+            "- #{io_data_retention_period} #{pluralize_with_s(io_data_retention_period, "day")} I/O data retention"
+          ]
+        else
+          []
+        end
+
     deliver(
       user,
       "The data retention policy for #{updated_project.name} has been modified",
@@ -155,9 +170,7 @@ defmodule Lightning.Accounts.UserNotifier do
 
       The data retention policy for your project, #{updated_project.name}, has been updated. Here are the new details:
 
-      - #{history_retention_period} #{pluralize_with_s(history_retention_period, "day")} history retention
-      - input/output (I/O) data #{if io_data_saved, do: "is", else: "is not"} saved for reprocessing
-      - #{io_data_retention_period} #{pluralize_with_s(io_data_retention_period, "day")} I/O data retention
+      #{Enum.join(details, "\n")}
 
       This policy can be changed by owners and administrators. If you haven't approved this change, please reset the policy by visiting the URL below:
 

lib/lightning/projects.ex

@@ -641,7 +641,8 @@ defmodule Lightning.Projects do
   end
 
   defp retention_setting_updated?(changeset) do
-    Map.has_key?(changeset.changes, :history_retention_period) or
+    Map.has_key?(changeset.changes, :retention_policy) or
+      Map.has_key?(changeset.changes, :history_retention_period) or
       Map.has_key?(changeset.changes, :dataclip_retention_period)
   end
 

lib/lightning/projects/audit.ex

@@ -9,7 +9,8 @@ defmodule Lightning.Projects.Audit do
       "allow_support_access_updated",
       "dataclip_retention_period_updated",
       "history_retention_period_updated",
-      "requires_mfa_updated"
+      "requires_mfa_updated",
+      "retention_policy_updated"
     ]
 
   alias Ecto.Multi
@@ -19,7 +20,8 @@ defmodule Lightning.Projects.Audit do
       :allow_support_access,
       :dataclip_retention_period,
       :history_retention_period,
-      :requires_mfa
+      :requires_mfa,
+      :retention_policy
     ]
     |> Enum.reduce(multi, fn field, multi ->
       changeset

lib/lightning/projects/project.ex

@@ -228,7 +228,8 @@ defmodule Lightning.Projects.Project do
 
   defp maybe_validate_dataclip_retention_period(changeset) do
     if get_change(changeset, :history_retention_period) ||
-         get_change(changeset, :dataclip_retention_period) do
+         get_change(changeset, :dataclip_retention_period) ||
+         get_change(changeset, :retention_policy) do
       validate_dataclip_retention_period(changeset)
     else
       changeset

test/lightning/accounts/user_notifier_test.exs

@@ -62,6 +62,51 @@ defmodule Lightning.Accounts.UserNotifierTest do
       )
     end
 
+    test "send_data_retention_change_email/2" do
+      user =
+        Lightning.AccountsFixtures.user_fixture(
+          email: "user@openfn.org",
+          first_name: "User"
+        )
+
+      project = %Project{
+        id: Ecto.UUID.generate(),
+        name: "project-a",
+        history_retention_period: 14,
+        dataclip_retention_period: 7,
+        retention_policy: :retain_all
+      }
+
+      settings_url =
+        LightningWeb.Endpoint.url() <>
+          "/projects/#{project.id}/settings#data-storage"
+
+      UserNotifier.send_data_retention_change_email(user, project)
+
+      assert_email_sent(
+        subject: "The data retention policy for project-a has been modified",
+        to: Swoosh.Email.Recipient.format(user),
+        text_body: """
+        Hi User,\n\nThe data retention policy for your project, project-a, has been updated. Here are the new details:\n\n- 14 days history retention\n- input/output (I/O) data is saved for reprocessing\n- 7 days I/O data retention\n\nThis policy can be changed by owners and administrators. If you haven't approved this change, please reset the policy by visiting the URL below:\n\n#{settings_url}\n\nOpenFn
+        """
+      )
+
+      # Under :erase_all the dataclip period is nil (the changeset clears it)
+      # and the email omits the I/O retention bullet entirely.
+      UserNotifier.send_data_retention_change_email(
+        user,
+        %{project | retention_policy: :erase_all, dataclip_retention_period: nil}
+      )
+
+      assert_email_sent(
+        subject: "The data retention policy for project-a has been modified",
+        to: Swoosh.Email.Recipient.format(user),
+        text_body: """
+        Hi User,\n\nThe data retention policy for your project, project-a, has been updated. Here are the new details:\n\n- 14 days history retention\n- input/output (I/O) data is not saved for reprocessing\n\nThis policy can be changed by owners and administrators. If you haven't approved this change, please reset the policy by visiting the URL below:\n\n#{settings_url}\n\nOpenFn
+        """
+      )
+    end
+
     test "remind_account_confirmation/2" do
       token = "sometoken"
 

test/lightning/projects/audit_test.exs

@@ -27,8 +27,7 @@ defmodule Lightning.Projects.AuditTest do
     } do
       attrs = %{
         # dataclip_retention_period: 7,
-        history_retention_period: 30,
-        retention_policy: :retain_with_errors
+        history_retention_period: 30
       }
 
       [{"audit_history_retention_period", {:insert, changeset, []}}] =
@@ -66,12 +65,47 @@ defmodule Lightning.Projects.AuditTest do
                Audit.derive_events(
                  Multi.new(),
                  project
-                 |> Project.changeset(%{retention_policy: :retain_with_errors}),
+                 |> Project.changeset(%{description: "a new description"}),
                  user
                )
                |> Multi.to_list()
     end
 
+    test "if retention policy is updated, returns multi for update", %{
+      project: %{id: project_id} = project,
+      user: %{id: user_id} = user
+    } do
+      attrs = %{
+        retention_policy: :retain_with_errors
+      }
+
+      [{"audit_retention_policy", {:insert, changeset, []}}] =
+        Audit.derive_events(
+          Multi.new(),
+          Project.changeset(project, attrs),
+          user
+        )
+        |> Multi.to_list()
+
+      assert %{
+               changes: %{
+                 event: "retention_policy_updated",
+                 item_type: "project",
+                 item_id: ^project_id,
+                 actor_id: ^user_id,
+                 changes: %{
+                   changes: audit_changes
+                 }
+               },
+               valid?: true
+             } = changeset
+
+      assert audit_changes == %{
+               before: %{retention_policy: :retain_all},
+               after: %{retention_policy: :retain_with_errors}
+             }
+    end
+
     test "if dataclip retention period is updated, returns multi for update", %{
       project: %{id: project_id} = project,
       user: %{id: user_id} = user
@@ -128,7 +162,8 @@ defmodule Lightning.Projects.AuditTest do
       for {name, change} <- events_multi do
         assert name in [
                  "audit_dataclip_retention_period",
-                 "audit_history_retention_period"
+                 "audit_history_retention_period",
+                 "audit_retention_policy"
                ]
 
         assert {:insert, _, []} = change

test/lightning/projects/project_test.exs

@@ -75,6 +75,22 @@ defmodule Lightning.Projects.ProjectTest do
       assert Ecto.Changeset.get_change(cs2, :dataclip_retention_period) == nil
     end
 
+    test "policy-only change to :erase_all nulls existing dataclip_retention_period" do
+      project =
+        insert(:project,
+          retention_policy: :retain_all,
+          history_retention_period: 30,
+          dataclip_retention_period: 14
+        )
+
+      cs = Project.changeset(project, %{retention_policy: :erase_all})
+
+      assert cs.valid?
+
+      assert {:ok, nil} =
+               Ecto.Changeset.fetch_change(cs, :dataclip_retention_period)
+    end
+
     test "validates dataclip_retention_period on existing project when only history changes" do
       # Simulate an existing project with dataclip > history (the bug scenario)
       project =

test/lightning/projects_test.exs

@@ -2914,6 +2914,44 @@ defmodule Lightning.ProjectsTest do
       end
     end
 
+    test "update_project/3 emails admins and owners when only the retention policy changes" do
+      project =
+        insert(:project,
+          retention_policy: :retain_all,
+          project_users:
+            Enum.map(
+              [:viewer, :editor, :admin, :owner],
+              fn role -> build(:project_user, user: build(:user), role: role) end
+            )
+        )
+
+      assert {:ok, updated_project} =
+               Projects.update_project(project, %{retention_policy: :erase_all})
+
+      assert updated_project.retention_policy == :erase_all
+
+      subject =
+        "The data retention policy for #{updated_project.name} has been modified"
+
+      for %{role: role, user: user} <- project.project_users do
+        email = Swoosh.Email.Recipient.format(user)
+
+        if role in [:admin, :owner] do
+          assert_receive {:email,
+                          %Swoosh.Email{
+                            subject: ^subject,
+                            to: [^email],
+                            text_body: body
+                          }}
+
+          assert body =~
+                   "input/output (I/O) data is not saved for reprocessing"
+        else
+          refute_receive {:email, %Swoosh.Email{to: [^email]}}
+        end
+      end
+    end
+
     test "update_project/3 rejects lowering history below existing dataclip retention" do
       project =
         insert(:project,