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.

Sign up for GitHub

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

fix: audit log creation failure when a segment override is deleted #4829

Merged
merged 1 commit into from
Dec 6, 2024
Merged

fix: audit log creation failure when a segment override is deleted #4829

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion api/audit/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,9 @@ def create_audit_log_from_historical_record(
if not (history_user or override_author or history_instance.master_api_key):
return

environment, project = instance.get_environment_and_project()
environment = project = None
if instance.__class__.objects.filter(pk=instance.pk).exists():
environment, project = instance.get_environment_and_project()
Comment on lines +92 to +94
Copy link
Member

@khvn26 khvn26 Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the contribution, @uinstinct.

Two thoughts on the PR:

  1. Let's add a unit test that reproduces the bug/problem we're trying to fix here.
  2. An AuditLog instance lacking both project and environment relations is not valid. Even for a soft-deleted instance it shouldn't be a problem to retrieve them. I believe the problem is the way the related descriptors are used in FeatureSegment._get_environment and FeatureSegment._get_project implementations — they probably should account for soft-deleted instances. This should be clearer when we have a test with the actual reproducing code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on it

Copy link
Contributor

Choose a reason for hiding this comment

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

@uinstinct , just checking in to see when you think you'll have time to make the suggested changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

@uinstinct ?


related_object_id = instance.get_audit_log_related_object_id(history_instance)
related_object_type = instance.get_audit_log_related_object_type(history_instance)
Expand Down
Loading