Fix notification & audit trail for dataclip retention policy changes - #5089
taylordowns2000 wants to merge 4 commits into
Conversation
Security Review ✅
|
|
|
||
| defp retention_setting_updated?(changeset) do | ||
| Map.has_key?(changeset.changes, :history_retention_period) or | ||
| Map.has_key?(changeset.changes, :retention_policy) or |
There was a problem hiding this comment.
we didn't count changing the policy as part of the retention_setting_updated? function
taylordowns2000
changed the title
| 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 |
There was a problem hiding this comment.
Quick question about this line. I was poking at what happens when someone changes only the policy and leaves the periods alone, and noticed the dataclip retention period gets cleared. That got me wondering what the nightly wipe does afterwards, since that period is the only thing it keys off.
So I set up a project with a month of history and a week of dataclip retention, dropped in a record from ten days ago, and flipped just the policy to erase_all. On main the record gets blanked, as you'd expect. On this branch it's still sitting there with its body intact.
Then I went looking to see whether this was new, and mostly it isn't. If you change the policy at the same time as a period, or if a sandbox inherits erase_all from its parent, the old data already stays put on main. So I think this line is really just making things consistent, which I'd guess was the point. It happens to be the one route where the wipe still ran.
Which makes me wonder whether we could leave it out of this PR and deal with the underlying thing on its own in a follow up issue, maybe having erase_all actually clear what's already stored rather than leaning on that period at all. The email side doesn't seem to need it, since the bullets already key off the policy.
Could easily be missing something though, let me know what you think.
| @@ -147,6 +147,21 @@ defmodule Lightning.Accounts.UserNotifier do | |||
|
|
|||
| io_data_saved = updated_project.retention_policy != :erase_all | |||
There was a problem hiding this comment.
This treats retain_with_errors the same as retain_all, so now that a policy-only change sends the email, someone switching to retain_with_errors would get a mail reading exactly like the retain_all one. Nothing to act on today, since that option is still commented out in the settings radio group.
The reason I mention it at all is that the same gap shows up in the save_dataclips? check in projects.ex, which has no clause for retain_with_errors and would raise on it. The TODO next to the commented-out option points at #1694, which is closed, so I'm not sure if that option is still on the cards. Either way it seemed worth knowing before anyone switches it on.
|
Absolutely open to it. Please go ahead and do what you need to fix the
security notification/audit trail bugs, pushing other fixes into subsequent
PRs as you see fit.
Taylor Downs
Founder & CEO
OpenFn <https://www.openfn.org> // the DPG for AI orchestration & public
service automation
…On Sun, Aug 23, 2026 at 23:53 Elias Waly Ba ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/lightning/projects/project.ex
<#5089 (comment)>:
> @@ -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
I might be missing context here, so treating this as a question.
Putting the policy in this check means a policy-only save now clears the
dataclip retention period. As far as I can tell that period is the only
thing the nightly wipe keys off, so clearing it seems to stop the wipe for
data that is already stored.
I ran it both ways to check I wasn't imagining it. Project on 30-day
history and 7-day dataclip, one record 10 days old. Change only the policy
to erase_all, then run the retention job:
- on main: the record gets blanked
- on this branch: it survives, body intact
To be clear, the wider problem doesn't look like it comes from this PR. I
checked the other ways you reach erase_all, changing the policy alongside
a period, and sandboxes inheriting it from a parent, and both already leave
old data in place on main. So this line makes the behaviour consistent,
which I'd guess is the intent. It just happens that the route it changes
was the last one where the wipe still ran.
Would you be open to leaving this line out for now, and fixing the
underlying thing separately? Something like actually clearing stored data
when someone switches to erase_all, rather than relying on that period.
From what I can see the email change doesn't depend on this line, since the
bullets already key off the policy.
Happy to be told I've got this wrong.
------------------------------
In lib/lightning/accounts/user_notifier.ex
<#5089 (comment)>:
> @@ -147,6 +147,21 @@ defmodule Lightning.Accounts.UserNotifier do
io_data_saved = updated_project.retention_policy != :erase_all
Much smaller, and separate from the above.
This treats retain_with_errors like retain_all, so now that a policy-only
change triggers the email, switching to retain_with_errors would send
owners and admins a body identical to the retain_all one.
Not reachable while that option stays commented out in the settings radio
group. Only raising it because the same gap seems to sit in
save_dataclips? in projects.ex, which has no clause for retain_with_errors
and would raise on it. Might be worth tidying both before #1694
<#1694> turns the option on.
—
Reply to this email directly, view it on GitHub
<#5089?email_source=notifications&email_token=ACCUBLJRHQLBYQGGOB62L335LNRU5A5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMBQGM2DMMRSGQY2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-5003462241>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCUBLIA2GU7HLVW6MBG72T5LNRU5AVCNFSNUABFKJSXA33TNF2G64TZHM2DKNBUGE4TEOJQHNEXG43VMU5TKMRSGE2DONZRGIY2C5QC>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
2 participants
Description
This PR fixes #5088 by checking for a change in the
retention_policywhen deciding whether to send the policy change alert. The new functionality in this PR::erase_allThe PR also fixes #5090 (the audit trial) in exactly the same fashion.
Validation steps
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)