-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[18.0][FIX] auditlog: drop tocompute register entirely from ThrowAwayCache #3469
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.
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
base: 18.0
Are you sure you want to change the base?
[18.0][FIX] auditlog: drop tocompute register entirely from ThrowAwayCache #3469
Conversation
905a491 to
c5d8a97
Compare
SodexisTeam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional review, The mentioned issue fixed for us. Thanks!
|
This PR has the |
| "statement_id": stmt.id, | ||
| }, | ||
| ) | ||
| line.flush_recordset() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we add an assert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. Without the fix, this test (with the flush) reproduces the issue. There is no specific value to assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that it tests the code that no error is raised, however, it would be nice to add an assert to know what we are expecting (a created auditlog for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be misleading, because that is not the goal of the test. Checking that auditlogs are created is covered by lots of other tests already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, the test must include the reason of it's existance. Without checking the PR it is not clear why we need this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, thanks! I now added documentation of the issue and the exception it causes to the test method docstring.
c5d8a97 to
09eab01
Compare
Fixes OCA#3424 Previous approach was to create a copy, so that the recomputes were also applied to the real world cache. However, this causes an issue when a field is required that is only available in the real world cache, such as the `date` field in ``` @api.depends('date', 'sequence') def _compute_internal_index(self): for st_line in self.filtered(lambda line: line._origin.id): st_line.internal_index = f'{st_line.date.strftime("%Y%m%d")}' ... ``` It actually makes sense to avoid recomputes in this isolated environment that is meant to retrieve the original values from before the write. Fixes ``` File "/odoo/odoo/addons/account/models/account_bank_statement_line.py", line 427, in create st_line.move_id.write(to_write) File "/odoo/server-tools/auditlog/models/rule.py", line 464, in write_full old_values = {d["id"]: d for d in records_write.read(fields_list)} ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/odoo/server-tools/auditlog/models/rule.py", line 406, in read result = read.origin(self, fields, load, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/odoo/odoo/odoo/models.py", line 3857, in read self._origin.fetch(fields) File "/odoo/odoo/odoo/models.py", line 4153, in fetch fetched = self._fetch_query(query, fields_to_fetch) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/odoo/odoo/odoo/models.py", line 4264, in _fetch_query field.read(fetched) File "/odoo/odoo/odoo/fields.py", line 4654, in read lines = comodel.search_fetch(domain, field_names) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/odoo/odoo/odoo/models.py", line 1781, in search_fetch return self._fetch_query(query, fields_to_fetch) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/odoo/odoo/odoo/models.py", line 4240, in _fetch_query rows = self.env.execute_query(query.select(*sql_terms)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/odoo/odoo/odoo/api.py", line 992, in execute_query self.flush_query(query) File "/odoo/odoo/odoo/api.py", line 984, in flush_query self[model_name].flush_model(field_names) File "/odoo/odoo/odoo/models.py", line 6768, in flush_model self._recompute_model(fnames) File "/odoo/odoo/odoo/models.py", line 7332, in _recompute_model self._recompute_field(field) File "/odoo/odoo/odoo/models.py", line 7360, in _recompute_field field.recompute(records) File "/odoo/odoo/odoo/fields.py", line 1471, in recompute apply_except_missing(self.compute_value, recs) File "/odoo/odoo/odoo/fields.py", line 1444, in apply_except_missing func(records) File "/odoo/odoo/odoo/fields.py", line 1493, in compute_value records._compute_field_value(self) File "/odoo/odoo/odoo/models.py", line 5297, in _compute_field_value fields.determine(field.compute, self) File "/odoo/odoo/odoo/fields.py", line 110, in determine return needle(*args) ^^^^^^^^^^^^^ File "/odoo/odoo/addons/account/models/account_bank_statement_line.py", line 295, in _compute_internal_index st_line.internal_index = f'{st_line.date.strftime("%Y%m%d")}' \ ^^^^^^^^^^^^^^^^^^^^^ AttributeError: 'bool' object has no attribute 'strftime' ```
09eab01 to
15530bb
Compare
Fixes #3424
Previous approach was to create a copy of the
tocomputeregistry, so that the recomputes were also applied to the real world cache. However, this causes an issue when a field is required that is only available in the real world cache, such as thedatefield inIt actually makes sense to avoid recomputes in this isolated environment that is meant to retrieve the original values from before the write.
Fixes