django-auditlog
django-auditlog copied to clipboard
jazzband
Denoise changes
Very cool, I am going to make some style updates and push them, also want to write a test for this before merging, if you get around to it before me then that is cool too
Ok so I did some testing and digging into this. If you have a model with the following:
class MyModel(models.Model):
null_blank_charfield = models.CharField(null=True, blank=True, max_length=20)
And you register it with auditlog you will see the following behavior:
obj = MyModel.objects.create()
log = obj.history.first()
print(log.changes_dict)
# nothing printed, no changes, null is null
obj.null_blank_charfield = ''
obj.save()
log = obj.history.first()
print(log.changes_dict)
# prints null => blank log
{'null_blank_charfield': ['None', '']}
obj.null_blank_charfield = ''
obj.save()
log = obj.history.first()
print(log.changes_dict)
# prints null => blank, no new log was created
{'null_blank_charfield': ['None', '']}
obj.null_blank_charfield = None
obj.save()
log = obj.history.first()
print(log.changes_dict)
# prints blank => null, new log created
{'null_blank_charfield': ['', 'None']
So auditlog is rejecting diffs when the field goes from None => None or '' => '', and it records them when it flips between None and '' blank. I think this is sane behavior. If the user doesn't want to see the noise they should remove the blank=True, null=True from the models.CharField definition, it is not recommended to store null strings in the database anyways. You could also exclude the field.
To conclude, it seems with further investigation, the current behavior is desired, if your field is defined to distinguish between null and blank strings, auditlog should capture this and not sweep it under the rug.
Do you know of a case I am forgetting? I thought at first auditlog was always creating diffs on blank string saves, but this is not the case.
If you save an admin form, nulls become blanks. My change was to address admin forms.
Perhaps it could be an option ?
On Wed, Jun 28, 2017, 8:05 PM Ryan Castner [email protected] wrote:
Ok so I did some testing and digging into this. If you have a model with the following:
class MyModel(models.Model): null_blank_charfield = models.CharField(null=True, blank=True, max_length=20)
And you register it with auditlog you will see the following behavior:
obj = MyModel.objects.create() log = obj.history.first()print(log.changes_dict)
nothing printed, no changes, null is null
obj.null_blank_charfield = '' obj.save() log = obj.history.first()print(log.changes_dict)
prints null => blank log
{'null_blank_charfield': ['None', '']} obj.null_blank_charfield = '' obj.save() log = obj.history.first()print(log.changes_dict)
prints null => blank, no new log was created
{'null_blank_charfield': ['None', '']} obj.null_blank_charfield = None obj.save() log = obj.history.first()print(log.changes_dict)
prints blank => null, new log created
{'null_blank_charfield': ['', 'None']
So auditlog is rejecting diffs when the field goes from None => None or '' => '', and it records them when it flips between None and '' blank. I think this is sane behavior. If the user doesn't want to see the noise they should remove the blank=True, null=True from the models.CharField definition, it is not recommended to store null strings in the database anyways. You could also exclude the field.
To conclude, it seems with further investigation, the current behavior is desired, if your field is defined to distinguish between null and blank strings, auditlog should capture this and not sweep it under the rug.
Do you know of a case I am forgetting? I thought at first auditlog was always creating diffs on blank string saves, but this is not the case.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jjkester/django-auditlog/pull/124#issuecomment-311849825, or mute the thread https://github.com/notifications/unsubscribe-auth/ANl_IbQ1Kr3I6YUCXCYMMHWu5lZw49nGks5sIxSTgaJpZM4OBkAz .
I think that is correct (on the road atm)
Documented https://code.djangoproject.com/ticket/9590
On Thu, Jun 29, 2017, 5:43 AM Ryan Castner [email protected] wrote:
So an admin form saving a blank=True, null=True charfield?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jjkester/django-auditlog/pull/124#issuecomment-311954821, or mute the thread https://github.com/notifications/unsubscribe-auth/ANl_IVKnug5EstsQJAlKpffp4g3fi4cdks5sI5vcgaJpZM4OBkAz .
It appears this could be addressed using a new feature in django 1.11: https://docs.djangoproject.com/en/1.11/ref/forms/fields/#django.forms.CharField.empty_value
I think using empty_value, as a form option, would require a lot of changes in admin classes. For that reason I still like this PR.
Would you be interested, if we had a flag like AUDITLOG_IGNORE_NONEBLANK or similar @audiolion ?
sorry @agfunder I haven't had time to look into this yet, once I fully understand the issue I will be able to answer, the part I am struggling with right now is the impact you are seeing in your logs that you want to reduce the noise of. I just need to do the admin form setup that you suggested here to see the diffs.
@hramezani I think we can close this as being solved by https://github.com/django/django/commit/267dc4adddd2882182f71a7f285a06b1d4b15af0
