From ad324b90a1264cc328494af77fceb189f5ff879a Mon Sep 17 00:00:00 2001 From: Taylor Downs Date: Sat, 22 Aug 2026 12:28:23 +0200 Subject: [PATCH 1/4] fix notification when dataclip retention policy changes, closes #5088 --- lib/lightning/projects.ex | 3 +- .../lightning/accounts/user_notifier_test.exs | 43 +++++++++++++++++++ test/lightning/projects_test.exs | 38 ++++++++++++++++ 3 files changed, 83 insertions(+), 1 deletion(-) diff --git a/lib/lightning/projects.ex b/lib/lightning/projects.ex index 564f45c9747..de364755137 100644 --- a/lib/lightning/projects.ex +++ b/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 diff --git a/test/lightning/accounts/user_notifier_test.exs b/test/lightning/accounts/user_notifier_test.exs index 6390f29684a..92b05fe222d 100644 --- a/test/lightning/accounts/user_notifier_test.exs +++ b/test/lightning/accounts/user_notifier_test.exs @@ -62,6 +62,49 @@ 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 + """ + ) + + UserNotifier.send_data_retention_change_email( + user, + %{project | retention_policy: :erase_all} + ) + + 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- 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 + """ + ) + end + test "remind_account_confirmation/2" do token = "sometoken" diff --git a/test/lightning/projects_test.exs b/test/lightning/projects_test.exs index 5f9f45d0dee..cad7794cc25 100644 --- a/test/lightning/projects_test.exs +++ b/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, From 020858a92eeccc4367ef8a9395ae62a6bcebc35b Mon Sep 17 00:00:00 2001 From: Taylor Downs Date: Sat, 22 Aug 2026 12:32:21 +0200 Subject: [PATCH 2/4] cl --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 672f70f5a30..dc1635491df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,8 @@ 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) ## [2.18.0] - 2026-08-20 From 3f3f5b11361914911afed0ec68579abf643aa2e5 Mon Sep 17 00:00:00 2001 From: Taylor Downs Date: Sat, 22 Aug 2026 12:58:53 +0200 Subject: [PATCH 3/4] audit trail also --- CHANGELOG.md | 6 ++++ lib/lightning/projects/audit.ex | 6 ++-- lib/lightning/projects/project.ex | 3 +- test/lightning/projects/audit_test.exs | 43 +++++++++++++++++++++--- test/lightning/projects/project_test.exs | 16 +++++++++ 5 files changed, 67 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc1635491df..7a83b86e889 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,12 @@ and this project adheres to [#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 and quoted in the notification email. + [#5088](https://github.com/OpenFn/lightning/issues/5088) ## [2.18.0] - 2026-08-20 diff --git a/lib/lightning/projects/audit.ex b/lib/lightning/projects/audit.ex index 5f61d46562f..ac8a9b52437 100644 --- a/lib/lightning/projects/audit.ex +++ b/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 diff --git a/lib/lightning/projects/project.ex b/lib/lightning/projects/project.ex index 12aeddf755f..01d9a6d67d5 100644 --- a/lib/lightning/projects/project.ex +++ b/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 diff --git a/test/lightning/projects/audit_test.exs b/test/lightning/projects/audit_test.exs index 59c05d5f7fb..9eb68692d8b 100644 --- a/test/lightning/projects/audit_test.exs +++ b/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 diff --git a/test/lightning/projects/project_test.exs b/test/lightning/projects/project_test.exs index 7a819e26314..ffbe1a7a89a 100644 --- a/test/lightning/projects/project_test.exs +++ b/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 = From a3ae7435357dbf846378092b82222e85659f8415 Mon Sep 17 00:00:00 2001 From: Taylor Downs Date: Sat, 22 Aug 2026 13:16:20 +0200 Subject: [PATCH 4/4] conditional bullets for email --- CHANGELOG.md | 3 ++- lib/lightning/accounts/user_notifier.ex | 19 ++++++++++++++++--- .../lightning/accounts/user_notifier_test.exs | 6 ++++-- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a83b86e889..886eb786196 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,7 +44,8 @@ and this project adheres to 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 and quoted in the notification email. + 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 diff --git a/lib/lightning/accounts/user_notifier.ex b/lib/lightning/accounts/user_notifier.ex index 5e46ba92b41..95187469699 100644 --- a/lib/lightning/accounts/user_notifier.ex +++ b/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: diff --git a/test/lightning/accounts/user_notifier_test.exs b/test/lightning/accounts/user_notifier_test.exs index 92b05fe222d..0c6918bf8fd 100644 --- a/test/lightning/accounts/user_notifier_test.exs +++ b/test/lightning/accounts/user_notifier_test.exs @@ -91,16 +91,18 @@ defmodule Lightning.Accounts.UserNotifierTest do """ ) + # 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} + %{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- 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 + 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