Add information about deleted objects in auditlog table#3705
Add information about deleted objects in auditlog table#3705
Conversation
b72d5f6 to
fc5b01d
Compare
hmpf
left a comment
There was a problem hiding this comment.
You need to do this for target and actor as well.
We could also do the HTML escape here, by escaping actor, object, target and also summary in a SerializerMethodField.
| def get_object(self, instance): | ||
| if instance.object: | ||
| return str(instance.object) | ||
| if instance.object_model and instance.object_pk: | ||
| return f"{instance.object_model} #{instance.object_pk} has been deleted" | ||
| return None | ||
|
|
There was a problem hiding this comment.
When deleting an account, for some reason in my db the object_pk of the log entry delete-account was set to zero so this is not sufficient, it would print as "account #None has been deleted".
There was a problem hiding this comment.
Also, actor_pk, object_pk, target_pk need not be a number, it can be a string. They are stored as strings.
There was a problem hiding this comment.
Sounds like a bug in the Account module. The object_pk should always point to a previously referenced object, no?
There was a problem hiding this comment.
In auditlogs for other systems (Microsoft Azure is one) I have seen they only show "deleted", just the pk, or a symbol.
In the same systems, still existing objects are frequently linked up to a details-page (very much out of scope!)
There was a problem hiding this comment.
Whether any _pk should always point to something after having been set once is undefined behavior I think.
There was a problem hiding this comment.
I have made #3709 for tracking the missing _pk matter.
Test results 27 files 27 suites 45m 35s ⏱️ Results for commit 92eb9c4. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3705 +/- ##
==========================================
+ Coverage 63.07% 63.16% +0.09%
==========================================
Files 612 612
Lines 45247 45320 +73
Branches 43 43
==========================================
+ Hits 28539 28627 +88
+ Misses 16698 16683 -15
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hmpf
left a comment
There was a problem hiding this comment.
We should get a second opinion about when _pk has been set to None on deletion.
We need a second opinion about when _pk is missing.
7f343b7 to
193cb40
Compare
python/nav/auditlog/api.py
Outdated
| # Debug return all log entries | ||
| def list(self, request, *args, **kwargs): | ||
| listed = super().list(request, *args, **kwargs) | ||
| return listed |
There was a problem hiding this comment.
I added this method to debug what the view returns, and forgot to remove it. It's fixed now, and I squashed the fixups :)
Not sure I understand what you're referring to here. |
193cb40 to
d98f0cf
Compare
|
See #3709 |
Display broken deleted actors/objects/targets robustly
|
|
When something has been deleted it is now shown as "TABLENAME deleted", no matter if the deletion was done correctly or not. It still makes for a wider column than columns pointing to still existing objects, though. |
lunkwill42
left a comment
There was a problem hiding this comment.
I prefer a version where the PK of the missing object is included in the description (when it is present in the auditlog), so I can at least see which auditlog entries that definitely refer to the same thing. It seems the original version before PR feedback did in fact include that...
But the format was only correct for integers and was incredibly verbose. |
|
I understand from @Simrayz (via Slack) that this PR may in reality have been superseded by another Auditlog PR (but not sure which one)... |
|
Made irrelevant by #3776 |
Scope and purpose
Resolves #1727
This PR adds information to theObjectcolumn in the Auditlog table. If the LogEntry refers to an object that has been deleted, it displays a message in the format "{object name} #{object ID} has been deleted". This should make it easier to distinguish a deleted entry from an entry that has no associated object.This PR adds information to the
Actor,ObjectandTargetcolumn in the Auditlog table. If the LogEntry refers to a model that has been deleted, it displays a message in the format "{model name} #{model ID} has been deleted". This should make it easier to distinguish entries that have no associated model from those where the reference has been deleted.Screenshots
Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to NAV can be found in the
Hacker's guide to NAV.
<major>.<minor>.x). For a new feature or other additions, it should be based onmaster.