[16.0][FIX] auditlog: Fix Missing Log Lines When Setting Empty Field Values - #3386
Conversation
613f0e8 to
0f3383d
Compare
BT-mlopez
left a comment
There was a problem hiding this comment.
LGTM, unit test look quite complete.
BT-ojossen
left a comment
There was a problem hiding this comment.
LGTM, but you should add yourself to the contributors
|
@amh-mw @StefanRijnhart |
StefanRijnhart
left a comment
There was a problem hiding this comment.
Thanks! I see the issue. But doing a read is undermining the 'fast' aspect of the 'fast' type rule. Would it work to force any falsy value in the written vals to show up in the log lines (conditionally overriding the result of DictDiffer)?
|
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. |
StefanRijnhart
left a comment
There was a problem hiding this comment.
Can you squash commits afterwards?
c7e77b4 to
99fa146
Compare
|
Commits squashed and test case refactored. |
StefanRijnhart
left a comment
There was a problem hiding this comment.
Thanks, looking good now!
|
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. |
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.
Introduce sentinel object as a placeholder for absent values when comparing diff. Add test for setting None values.
235ca6b to
09f43fe
Compare
|
@hbrunn Sorry for the delay, I was OoO last week. |
|
This PR has the |
hbrunn
left a comment
There was a problem hiding this comment.
thank you!
/ocabot merge patch
|
On my way to merge this fine PR! |
|
Congratulations, your PR was merged at 2c3896e. Thanks a lot for contributing to OCA. ❤️ |
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.