IMP l10n_it_account removing check_balance_sign_coherence. #4431
Conversation
SirAionTech
added
the
needs fixing
eLBati
force-pushed
the
16.0-check_balance_sign_coherence
branch
4 times, most recently
from
0e0c3c4
to
a559471
Compare
eLBati
force-pushed
the
16.0-check_balance_sign_coherence
branch
from
a559471
to
2a146b2
Compare
|
pre-commit verde |
SirAionTech
removed
the
needs fixing
There was a problem hiding this comment.
It's worth noting that with this PR, it is possible to have accounts with opposite sign in the same group:
without this PR, the same setup gives:
I think this is at least unexpected, but I'll rely on functional reviews for its correctness.
Could you please write in the commit the reason of this change instead of just describing it? Right now it is
[REF] l10n_it_account making making account_balance_sign computed and removing check_balance_sign_coherence
This is also suggested in https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#71commit-message:
commit messages should be self explanatory (long enough) including the name of the module that has been changed and the reason behind that change.
| return True | ||
| benchmark = to_check[0].account_balance_sign | ||
| return all(a.account_balance_sign == benchmark for a in to_check) | ||
| def _compute_account_balance_sign(self): |
There was a problem hiding this comment.
I would add a @api.depends("account_type"): I think it might help caching the correct value for an account instead of recomputing it every time.
| "account_type": "asset_current", | ||
| } | ||
| ) | ||
| with self.assertRaises(ValidationError): |
There was a problem hiding this comment.
Please change the test or add a new one to verify the new behavior instead of removing it
eLBati
force-pushed
the
16.0-check_balance_sign_coherence
branch
from
2a146b2
to
ad2f698
Compare
…ng check_balance_sign_coherence. account_balance_sign is now computed, based on account type. Therefore many checks are now redundant. There is not need to save account's sign directly on account , as sign substantially depends on account type. Saving it on account and check coherence afterwards often produces errors hard to understand by users, as they are not needed.
eLBati
force-pushed
the
16.0-check_balance_sign_coherence
branch
from
ad2f698
to
92e11cc
Compare
Mi pare il comportamento atteso invece, come mai non dovrebbe?
Esteso |
Nel gruppo di conti c'è un campo segno, nei singoli conti all'interno del gruppo c'è un campo segno; visto che sono oggetti imparentati mi aspetto che ci sia una specie di ereditarietà: se un gruppo ha un segno allora anche tutti i conti nel gruppo hanno lo stesso segno. Se è il comportamento atteso da questa PR lo puoi chiarire? Sfido qualsiasi funzionale a capire che
o
Vuol dire che i conti all'interno di un gruppo possono avere segno diverso dal gruppo P.S. ma quindi inglese o italiano? |
SirAionTech
added
the
needs fixing
This check can block chart of accounts configuration and is not mandatory. account_balance_sign is only used by l10n_it_financial_statements_report and, in case of incoherent signs, user can visualize the amounts in financial_statements_report and fix the sign, in case