2589 retention period events

[rorymckinley]

Description

Functionality to track when a project's history and dataclip retention settings have been changed. Per Stu's suggestion, this serves as a warm up for the main event - tracking the creation of snapshots.

Closes #2589

Validation steps

In Lightning, navigate to the data storage settings for a project of your choice:

Once you have saved the change, navigate to the audit page and you should see the changes reflecting.

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

• Code generation (copilot but not intellisense)
• Learning or fact checking
• Strategy / design
• Optimisation / refactoring
• Translation / spellchecking / doc gen
• Other
• I have not used AI

You can read more details in our Responsible AI Policy

Pre-submission checklist

• I have performed a self-review of my code.
• I have implemented and tested all related authorization policies. (e.g., :owner, :admin, :editor, :viewer)
• I have updated the changelog.
• I have ticked a box in "AI usage" in this PR

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.85%. Comparing base (a706d75) to head (864250a).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2599      +/-   ##
==========================================
+ Coverage   90.81%   90.85%   +0.03%     
==========================================
  Files         320      321       +1     
  Lines       11218    11231      +13     
==========================================
+ Hits        10188    10204      +16     
+ Misses       1030     1027       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rorymckinley]

This will be moved into a wrapper module in a future PR - allowing the use of Mox rather than Mock as well as allowing for the easier replacement of Sentry should it be necessary in the future.

[jyeshe]

Hey @rorymckinley I see you're following the current API structure. It's probably noted but if we had a time window to insert audits with a Ecto.Multi it would be great.

[rorymckinley]

Hey @rorymckinley I see you're following the current API structure. It's probably noted but if we had a time window to insert audits with a Ecto.Multi it would be great.

@jyeshe My understanding of the behaviour of Multi is that a failure to create the audit event will result in the rollback of the entire transaction. I asked Ayodele if that was the behaviour that he wanted and he said that the failure to create an audit event should not result in the operation that is being audited failing.

[jyeshe]

Hey @rorymckinley I see you're following the current API structure. It's probably noted but if we had a time window to insert audits with a Ecto.Multi it would be great.

@jyeshe My understanding of the behaviour of Multi is that a failure to create the audit event will result in the rollback of the entire transaction. I asked Ayodele if that was the behaviour that he wanted and he said that the failure to create an audit event should not result in the operation that is being audited failing.

Your understanding is correct @rorymckinley . However the Multi would fail only due to logic error or database/connection error. The logic should be be safe because it's a shared code among other audit and we are testing the specific use case where all the inputs also come from constrained domains (select boxes and radio button).

[rorymckinley]

Hey @rorymckinley I see you're following the current API structure. It's probably noted but if we had a time window to insert audits with a Ecto.Multi it would be great.

@jyeshe My understanding of the behaviour of Multi is that a failure to create the audit event will result in the rollback of the entire transaction. I asked Ayodele if that was the behaviour that he wanted and he said that the failure to create an audit event should not result in the operation that is being audited failing.

Your understanding is correct @rorymckinley . However the Multi would fail only due to logic error or database/connection error. The logic should be be safe because it's a shared code among other audit and we are testing the specific use case where all the inputs also come from constrained domains (select boxes and radio button).

Agreed, @jyeshe but I notice that you used the phrase 'should be safe' versus 'is safe' :) - and that is absolutely correct. There will always be an element of uncertainty and this element of uncertainty may very well increase over time as the codebase is modified.

I think that the Multi adds very little benefit to the operation and it makes the difference between 'there is a (very) small chance that the act of creating an audit event prevents the primary operation from succeeding' versus 'there is no chance that the act of creating an audit event prevents the primary operation from succeeding'. That being said, if you feel that I am completely missing the point of the Multi here, I am more than happy to be talked off the ledge.

[jyeshe]

Hey @rorymckinley I see you're following the current API structure. It's probably noted but if we had a time window to insert audits with a Ecto.Multi it would be great.

@jyeshe My understanding of the behaviour of Multi is that a failure to create the audit event will result in the rollback of the entire transaction. I asked Ayodele if that was the behaviour that he wanted and he said that the failure to create an audit event should not result in the operation that is being audited failing.

Your understanding is correct @rorymckinley . However the Multi would fail only due to logic error or database/connection error. The logic should be be safe because it's a shared code among other audit and we are testing the specific use case where all the inputs also come from constrained domains (select boxes and radio button).

Agreed, @jyeshe but I notice that you used the phrase 'should be safe' versus 'is safe' :) - and that is absolutely correct. There will always be an element of uncertainty and this element of uncertainty may very well increase over time as the codebase is modified.

I think that the Multi adds very little benefit to the operation and it makes the difference between 'there is a (very) small chance that the act of creating an audit event prevents the primary operation from succeeding' versus 'there is no chance that the act of creating an audit event prevents the primary operation from succeeding'. That being said, if you feel that I am completely missing the point of the Multi here, I am more than happy to be talked off the ledge.

It might be out of the scope but considering two options audit are implemented for retention, there should be one for zero-persistence. It's a critical configuration and I think we should not allow this option to be saved without logging it. The same for the retention period as it can cause data loss. So unfortunately I can't approve this design. @stuartc I need to delegate this risk and decision to you.

[rorymckinley]

Hey @rorymckinley I see you're following the current API structure. It's probably noted but if we had a time window to insert audits with a Ecto.Multi it would be great.

@jyeshe My understanding of the behaviour of Multi is that a failure to create the audit event will result in the rollback of the entire transaction. I asked Ayodele if that was the behaviour that he wanted and he said that the failure to create an audit event should not result in the operation that is being audited failing.

Your understanding is correct @rorymckinley . However the Multi would fail only due to logic error or database/connection error. The logic should be be safe because it's a shared code among other audit and we are testing the specific use case where all the inputs also come from constrained domains (select boxes and radio button).

Agreed, @jyeshe but I notice that you used the phrase 'should be safe' versus 'is safe' :) - and that is absolutely correct. There will always be an element of uncertainty and this element of uncertainty may very well increase over time as the codebase is modified.
I think that the Multi adds very little benefit to the operation and it makes the difference between 'there is a (very) small chance that the act of creating an audit event prevents the primary operation from succeeding' versus 'there is no chance that the act of creating an audit event prevents the primary operation from succeeding'. That being said, if you feel that I am completely missing the point of the Multi here, I am more than happy to be talked off the ledge.

It might be out of the scope but considering two options are implemented for retention, there should be one for zero-persistence. It's a critical configuration and I think we should not allow this option to be saved without logging it. The same for the retention period as it can cause data loss. So unfortunately I can't approve this design. @stuartc I need to delegate this risk and decision to you.

@jyeshe Understood. I have passed your feedback on to Ayodele to see if he wants to amend the requirement and allow for this to be rolled back if there is a failure to save the audit event.

[rorymckinley]

@jyeshe Understood. I have passed your feedback on to Ayodele to see if he wants to amend the requirement and allow for this to be rolled back if there is a failure to save the audit event.

@jyeshe I have reworked this to ensure that changes will be not be made if the Audit record can not be created.

[rorymckinley]

This test knows a bit too much about the inner workings, but I thought I would apply the mock here as this file is already async: false.