[16.0][FIX] auditlog: Fix Missing Log Lines When Setting Empty Field Values #3386
Conversation
613f0e8 to
0f3383d
Compare
|
@amh-mw @StefanRijnhart |
|
Thanks for the prompt response. Indeed, if we change the initial old value from |
|
Isn't it the case that one never gets the old values with 'fast' logging? So having both an empty value for old and new is what is to be expected. |
43e2eaf to
b8cae5a
Compare
|
That seems to be the case, yeah. So this is an intentional performance optimisation? How about we add an option to the auditlog rules (applicable only for the fast mode) to let the user decide whether old values should be logged? |
|
@BT-pgaiser Isn't that what 'full' logging is for? |
|
If that's the only difference between "fast" and "full" log mode, then yes, simply activating the "full" mode solves the issue. I was thinking about having something in between those two modes, but maybe that does not make a lot of sense. |
|
@BT-pgaiser Yes please! Keep logging the empty values when specified explicitely in the write vals (in the case of write 'fast'), but don't read old values to compare. |
Fix the comparison with old values when setting fields to empty values. The old values for the comparison when using fast log mode were initialized with False. This causes the problem that when a field is set to an emty value by the user, auditlog does not create a log line because internal checks compare False to False and do not consider logging the change. Full log mode is not affected by this as there everything is logged by default. However, even in fast log mode, some companies have regulatory requirements which force them to log user actions that set empty values to fields.
c7e77b4 to
99fa146
Compare
|
Commits squashed and test case refactored. |
|
This PR has the |
|
@StefanRijnhart Can we please merge this? Thanks. |
|
@BT-pgaiser I would like to see another review from a tools maintainer or previous contributor to this module. The other reviews on this PR so far are clearly 'team service' because I don't see any other reviews in the OCA from them except for one or two other 'BT' PRs. |
|
@StefanRijnhart Fair enough. |
auditlog/tests/test_auditlog.py
Outdated
| cls.auditlog_rule.subscribe() | ||
|
|
||
| def test_when_removing_field_value_a_log_line_is_created(self): | ||
| self.test_record.write({"phone": False}) |
There was a problem hiding this comment.
can you do the same test with writing None here? Both are valid input, and I suspect you're just replacing when the misbehavior happens.
chances are you need some sentinel object like
class FalsyValue():
def __bool__(self):
return Falseto initialize the dict above with
There was a problem hiding this comment.
I've added a test for writing None values. However, AFAIK, Odoo always represents missing/empty values as False and this is consistent with how the web client behaves. Anyhow, I've never seen None being used for that in Odoo. Surely, if I write a separate program that directly communicates with the Odoo backend, then I can send it None values, but I'd say that's unspecified behaviour so it does not need to be supported. Anyways, the introduced sentinel object will handle both cases now.
Introduce sentinel object as a placeholder for absent values when comparing diff. Add test for setting None values.
| # of the data in the database | ||
| vals2 = dict(vals) | ||
| old_vals2 = dict.fromkeys(list(vals2.keys()), None) | ||
| old_vals2 = dict.fromkeys(list(vals2.keys()), _Sentinel()) |
There was a problem hiding this comment.
wait, for better performance, please create an instance of this above and recycle this object, no need to keep creating it
Fixes the comparison with old values when setting fields to empty values.
The old values for the comparison when using fast log mode were initialized with
False. This causes the problem that when a field is set to an emty value by the user, auditlog does not create a log line because internal checks compareFalsetoFalseand do not consider logging the change. Full log mode is not affected by this as there everything is logged by default. However, even in fast log mode, some companies have regulatory requirements which force them to log user actions that set empty values to fields, so that should be logged properly.