[14.0][FIX] Do not raise error if protected fields are unchanged.

[gfcapalbo]

The standard write would check if protected fields were in dict, if they are, it would check if account.move is not draft and then raise error.
Unfortunately the odoo web client posts to write dict all non-readonly fields in a view, therefore some fields , that will be factually unchanged, are still posted in the dict , therefore raising an error.
When needed, this MR , cleans the dict of those ( inconsequential) fields.

[gfcapalbo]

@thomaspaulb there will be need to change the tests because our mod does not influence those usecases
coverage may be reduced, waiting for CI results.

[gfcapalbo]

@thomaspaulb There was a test failing! but not the one we thought. All other write tests were actually modifying the value of the field, therefore our modification did not influence those tests. but one specific date change test expected a user error while writing the same date as the one on the account.move.line.
obviously our mod allowed that. re-wrote the test. Now increasing coverage to cover our new code branches.

[gfcapalbo]

coverage was increased to keep percentage equal. 1 test was modified, there was a noop (same value) write of a protected field that should had raised errors , it now should pass because the value is popped out of the write. I also added an extra write to test that the same field when writing a non-noop value would raise error.

[NL66278]

What happens if a non x2m field in the database (existing value) has a falsey value (0.0 or empty string)? From the code I think it will fail the first condition of the if, and therefore proceed to the part where it checks the x2m field. Or am I mistaken in what happens here?

[NL66278]

I think the code can be more clear if you first have a simple condition for the type of fields and then handle regular fields and x2m fields separately.

[gfcapalbo]

What happens if a non x2m field in the database (existing value) has a falsey value (0.0 or empty string)? From the code I think it will fail the first condition of the if, and therefore proceed to the part where it checks the x2m field. Or am I mistaken in what happens here?

It will not continue, because the first return always works.
We first compare all non-m2m and non-o2m, and return that result.

the second return activates only if the field is m2m or o2m.

As for the 0.0 , empty string case , automatic typecasts seemed to work well , except for "None":

>>> 0==False
True
>>> 0.0==False
True
>>> ''==False
False
>>> ''==0-0
False
>>> ''==0.0
False
>>> None==False
False

I will explicitly convert them in "Bool" to make this more solid:

   >>> bool('')==bool(0.0)
True
>>> bool(None)==bool(0.0)
True
>>> bool(None)==bool(False)
True

[NL66278]

I meant it like this (note that by inversing conditions you can get rid of all the not's, and with some helper values stuff will get more readable):

field = self._fields.get(changed_protected_field)
old_value = self[changed_protected_field]
current_value = values[changed_protected_field]
if field.type in ["many2many", "one2many"]:
    return(
          len(current_value) == 1
          and current_value[0][0] == 6
          and current_value[0][2] == old_value.ids
    )
if current_value:
    return old_value == current_value
return bool(old_value) == bool(current_value)

[NL66278]

See other comments

[NL66278]

Also add a test for protected x2many field, one with noop change, one with real change. Maybe for the tax_ids field.

[gfcapalbo]

@NL66278 @astirpe all comments addressed , wrote and currently running extra tests for the M2M case on tax_ids change.

[NL66278]

Use old_value and new_value also in these lines.

[NL66278]

One more optimization I just see is to switch the conditions, as _l10n_nl_vat_statement_should_check_write(values) has an @api.model decorator, it is the same for all records:

if self._l10n_nl_vat_statement_should_check_write(values):
    for this in self:
        this._l10n_nl_vat_statement_check_state()

[NL66278]

An ever more drastic performance enhancement is also possible, but that would involve an even more drastic restructuring of the code:

• Take a subset of the records in self, that do have l10n_nl_vat_statement_id set AND are not in draft state
• Check only for those records whether a protected field has really changed, in that case immediately raise an error (no more needs for an invalid_fields set)
• If an error is not raised, just call super() with the original self

[gfcapalbo]

true , check_write is @model, because it conly depends on what keys are present in the write dict. We can keep it out of loop.

First proposal: we can eliminate that condition all together. We already check that :

 59         if not self._l10n_nl_vat_statement_should_check_write(values):                  
 60             return super().write(values)                          

So i eliminated it altogether:

 78         for this in self:                                                               
 79             this._l10n_nl_vat_statement_check_state()                                   
 80         return super(AccountMoveLine, self).write(values)      

I implemented the first proposal, not the more deep one for this reason-

the second restructure, while faster, because it works only on a subset of non-draft state records, does move the logic of _l10n_nl_vat_statement_check_state outside , and put it in this write.
I wrote this with the preference of keeping the logic in the check_state function...
What we would save is not exponential , but linear. the check_state is really just a comparison operation.

[NL66278]

@gfcapalbo You cannot eliminate it altogether. The first check is based on all values passed originally, the second check - that you now eliminated - is based only on the valiues that are changed in one or more records. By eliminating the second check, you undo all the work of checking whether values really changed.

[gfcapalbo]

@NL66278 Fixed: 2ec172d

[NL66278]

👍 I see still more room for streamlining and one functional enhancements, but I do not want better to kill good enough. Specifically if a protected field is changed in a move line that is in draft, or that is not related to a vat statement, but not changed in the move lines that should not be changed, an error will still be raised unnecessarily. Granted that is a rather theoretical situation. Anyhow merging this as is will help a lot.

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[NL66278]

/ocabot merge minor

[OCA-git-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-405-by-NL66278-bump-minor, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at 7864b45. Thanks a lot for contributing to OCA. ❤️