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

Add username to activity log metadata on user deletion #3091

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

balanza
Copy link
Member

@balanza balanza commented Oct 22, 2024

Description

Add username to activity log metadata on user deletion.

Being the user have been soft deleted, its username must be polished before presenting.

lib/trento/activity_logging/parser/metadata_enricher.ex Outdated Show resolved Hide resolved
lib/trento/users.ex Outdated Show resolved Hide resolved
lib/trento/users.ex Show resolved Hide resolved
@balanza balanza force-pushed the enrich-username branch 2 times, most recently from ce8005c to 4d0516d Compare October 22, 2024 16:27
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Hey @balanza thanks for this.

The main thing we need to address is making sure the tagging enrichment detection does not conflict with username enrichment detection.

Then there is the point about the deleted user's username.
I personally believe it'd be more friendly tracking without the leading deletion timestamp. I know, it's ugly, but it is what it is.

lib/trento/users.ex Show resolved Hide resolved
test/trento/activity_logging/activity_logger_test.exs Outdated Show resolved Hide resolved
@balanza balanza force-pushed the enrich-username branch 2 times, most recently from 2b3ec87 to 30f29cb Compare October 24, 2024 10:00
@balanza balanza requested a review from nelsonkopliku October 24, 2024 10:23
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

The only reserve I have is that I'd find it more consistent if in the log we track the deleted user's username stripped from the deletion timestamp.

Let's proceed anyways. If needed we can follow up on that.

@nelsonkopliku nelsonkopliku added the enhancement New feature or request label Oct 25, 2024

defp detect_enrichment(_target_entity, {_activity, _metadata}),
do: {:error, :no_enrichment_needed}

defp polish_entity(%{username: username, deleted_at: deleted_at} = entity) do
Copy link
Member

Choose a reason for hiding this comment

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

I thought you would add this to the Users.by_id/1 👀 However, as you wish.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, huge no. I won't add a custom patch to a context. I'm already ashamed to add a patch at this level, but I can swallow it.

Whether the data is "dirty" or not must be decided by the context, in a dedicated discussion hopefully

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I see. Fine with this, thanks 👍

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

We might want to rebase and have a clean history of the PR.
Besides this LGTM :shipit:

Comment on lines +92 to +95
clean_username =
String.trim_trailing(username, "__" <> DateTime.to_string(deleted_at))

Copy link
Contributor

@gagandeepb gagandeepb Nov 4, 2024

Choose a reason for hiding this comment

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

[Minor clarification] I wonder: do we need to guard against the deleted_at value being nil ? Otherwise looks good to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should never happen but I see no harm in doing it.

@balanza balanza enabled auto-merge (squash) November 4, 2024 16:14
@balanza balanza merged commit a4e6d74 into main Nov 4, 2024
26 checks passed
@balanza balanza deleted the enrich-username branch November 4, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants