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

Activity log retention time #2690

Merged
merged 6 commits into from
Jun 24, 2024
Merged

Activity log retention time #2690

merged 6 commits into from
Jun 24, 2024

Conversation

nelsonkopliku
Copy link
Member

@nelsonkopliku nelsonkopliku commented Jun 12, 2024

Description

Adds the foundation for the activity log settings, specifically retention time.

  • migration to add the required fields into the settings STI
  • schema and constraints
  • initial default retention time is provided
  • context exposing basic read/update operations

The context functions can be then used in a controller to expose desired operations.

@nelsonkopliku nelsonkopliku force-pushed the auditing-retention-time branch from b19a934 to c85c47a Compare June 12, 2024 15:41
@nelsonkopliku nelsonkopliku self-assigned this Jun 12, 2024
@nelsonkopliku nelsonkopliku added the enhancement New feature or request label Jun 12, 2024
@nelsonkopliku nelsonkopliku force-pushed the auditing-retention-time branch from c85c47a to e9a101a Compare June 12, 2024 15:49
Copy link
Contributor

@gagandeepb gagandeepb left a comment

Choose a reason for hiding this comment

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

Adding some comments for later consideration.

@nelsonkopliku nelsonkopliku changed the title Auditing retention time Activity log retention time Jun 14, 2024
@nelsonkopliku nelsonkopliku force-pushed the auditing-retention-time branch from e9a101a to 0ee0f05 Compare June 14, 2024 07:33
@nelsonkopliku nelsonkopliku force-pushed the auditing-retention-time branch from 0ee0f05 to 4835dd2 Compare June 14, 2024 07:36
@nelsonkopliku
Copy link
Member Author

hey @gagandeepb, @balanza, I think I have addressed what we discussed:

  • removed the save retention time operation as we do not really have to cover that use case
  • init default retention time to 1 month at application startup, and in seeds for dev environment
  • now the settings table has only one field retention_time, being an embedded value object stored as json *
  • renamed the whole thing to Activity Log, as per naming discussions 🙈

* Note that this moves possible validation errors one level down in Trento.ActivityLog.Settings (we get changeset errors on the embedded RetentionTime), and I am not sure our error handling at controller level is ready for this.
It might end up in not being an issue, but we'll discover more when actually implementing the controller.
If anything needs to be changed, we'll do so.

Thanks for the feedbacks!

@nelsonkopliku nelsonkopliku marked this pull request as ready for review June 14, 2024 08:04
end
end

def down do
Copy link
Member Author

Choose a reason for hiding this comment

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

I realized that while rolling back, only the activity_log_retention_time field was removed, but the record typed activity_log_settings remained, meaning that migrating forward again would result in having the activity log settings record with a null retention time.

In order to avoid unwanted side effects I thought it is reasonable just deleting the whole record.

Copy link
Member

@balanza balanza left a comment

Choose a reason for hiding this comment

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

LGTM, I just have some thoughts on naming but they're non blocking

test/trento/activity_log_test.exs Outdated Show resolved Hide resolved
@nelsonkopliku nelsonkopliku merged commit e3db6a0 into main Jun 24, 2024
26 checks passed
@nelsonkopliku nelsonkopliku deleted the auditing-retention-time branch June 24, 2024 10:14
@nelsonkopliku nelsonkopliku added the elixir Pull requests that update Elixir code label Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
elixir Pull requests that update Elixir code enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants