[16.0][IMP] contract: Added domain for pricelist_id #1350
Conversation
quirino95
left a comment
quirino95
left a comment
There was a problem hiding this comment.
Code and functional review: LGTM
Suggestion (non-blocking): what do you think about clearing pricelist_id when journal changes and the selected pricelist is not in the domain anymore?
anusriNPS
force-pushed
the
16.0-pricelist_domain
branch
from
af46db5 to
3e2672a
Compare
Nice suggestion. Updated changes to clear pricelist when newly selected journal_id currency does not match with selected pricelist_id currency. |
contract/models/abstract_contract.py
Outdated
| @api.depends("journal_id") | ||
| def _compute_pricelist_id(self): | ||
| for rec in self: | ||
| if ( | ||
| rec.journal_id.currency_id | ||
| and rec.journal_id.currency_id != rec.pricelist_id.currency_id | ||
| ): | ||
| rec.pricelist_id = None |
There was a problem hiding this comment.
thought (non-blocking): I wonder if this should be an onchange instead of a depends.
On the other hand, while the domain is helpful for the user when filling in the contract (I'd keep it as is), the currency consistency check should probably be an api.constrains and not simply applied to the Form view via a domain.
What do you think?
There was a problem hiding this comment.
Agree, adding api.constraints should be handled for currency consistency check and also onchange would be better as it is more relevant from interface change for journal_id. Updated test cases accordingly.
Question: What happens when pricelist_id.currency_id or journal_id.currency_id changes?
I believe this scenario needs to be taken care, as onchange and api constrains will not able to catch this change so currency mismatch would be seen.
I tried to make pricelist_id in contract None using compute for pricelist_id whenever journal_id.currency_id or pricelist_id.currency_id changes. However, this behaviour impacts other testcases under contract_sale_invoicing_pricelist module. So, I would like to know your views about the above scenario.
Your inputs would be helpful to conclude the approach.
There was a problem hiding this comment.
That's a great question. Honestly, I don't know.
I see a couple of ways:
-
Leave it as is. We're trying our best to avoid inconsistencies, but we can't block every pathway. It's still an improvement over the previous situation where there was no consistency check at all
-
Override the
writemethod of both pricelists and journals: if the currency is changed, run the consistency check for any related contract again. But maybe only for contracts that are valid now or in the future? Otherwise a single expired contract could block the user from changing the currencies on related pricelists and journals forever. Even if the user was going to update both to be consistent, they couldn't change both pricelist and journal at the same time, so the constraint would still block them. Trying to account for every variation can become surprisingly complex very fast.
I think we can leave it as is for now. It may not be perfect but it's still an improvement over the existing situation, and then it can be improved even further in the future if need be. (Also, pragmatically, I can't imagine that currency changes on journals and pricelists are all that common)
There was a problem hiding this comment.
Thank you for the detailed response. Agree, it's not a common scenario changing currency on journal/Pricelist. However, based on the details I could understand the complexity involved on handling it. For now, updated PR with onchange and api constraint check as a small improvement for currency check on a contract.
anusriNPS
force-pushed
the
16.0-pricelist_domain
branch
2 times, most recently
from
094fb6e to
9deb916
Compare
Using domain for field "pricelist_id" which lists pricelist whose currency is matched with "journal_id" currency. This domain helps to avoid using different currency within same contract. Similary, api constraint check added to check currency consistency between journal_id and pricelist_id fields.
anusriNPS
force-pushed
the
16.0-pricelist_domain
branch
from
9deb916 to
4b05c42
Compare
4 participants
Using domain for field "pricelist_id" which lists pricelist whose currency is matched with journal_id currency. This domain helps to avoid using different currency within same contract.