Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2589 retention period events #2599

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rorymckinley
Copy link
Collaborator

@rorymckinley rorymckinley commented Oct 22, 2024

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:

image

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

image

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

Copy link

codecov bot commented Oct 22, 2024

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.

defp report_error(error_data) do
Logger.error(%{error: "Saving audit event"} |> Map.merge(error_data))

Sentry.capture_message(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rorymckinley rorymckinley marked this pull request as ready for review October 22, 2024 09:57
Copy link
Member

@jyeshe jyeshe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

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
Copy link
Member

jyeshe commented Oct 23, 2024

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
Copy link
Collaborator Author

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
Copy link
Member

jyeshe commented Oct 23, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@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.

~p"/projects/#{project.id}/settings#data-storage"
)

with_mock(Lightning.Repo, [:passthrough],
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New Issues
Development

Successfully merging this pull request may close these issues.

Track events related to the changing of retention periods
2 participants