[16.0][IMP] contract: Added domain for pricelist_id

[anusriNPS]

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.

[quirino95]

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]

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?

Nice suggestion. Updated changes to clear pricelist when newly selected journal_id currency does not match with selected pricelist_id currency.

[HekkiMelody]

Code review, LGTM

[HekkiMelody]

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?

[anusriNPS]

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.

[HekkiMelody]

That's a great question. Honestly, I don't know.

I see a couple of ways:

1. 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

2. Override the write method 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)

[anusriNPS]

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.

[HekkiMelody]

Code review, LGTM

[marcos-mendez]

Automated Review -- Tests Passed

All tests for contract passed successfully on Odoo 16.0.
Environment: Minikube + K8s Job + oca-ci/py3.10-odoo16.0

---

Test Coverage Suggestions

Coverage Gaps

The new logic introduces:

1. A constraint check_currency that raises UserError when journal and pricelist currencies don't match
2. A computed field pricelist_id_domain used to filter pricelists by journal currency
3. An onchange method _onchange_journal_id that resets pricelist_id when currencies mismatch

However, there are no tests for the following:

• The pricelist_id_domain computation logic (especially when journal has no currency)
• The _onchange_journal_id behavior when pricelist_id is set to a currency-mismatched pricelist
• Edge case: what happens if journal_id is unset or changed to one with no currency
• The constraint is tested only in one scenario; missing negative cases (e.g., multiple contracts, different journal/pricelist combinations)

---

Suggested Test Cases

def test_pricelist_domain_computation(self):
    """Test that pricelist_id_domain is correctly computed based on journal currency."""
    # Create a journal with a currency
    journal = self.env['account.journal'].create({
        'name': 'Test Journal',
        'code': 'TEST',
        'type': 'sale',
        'currency_id': self.env.ref('base.EUR').id,
    })
    # Create contract and set journal
    contract = self.env['contract.contract'].create({
        'name': 'Test Contract',
        'journal_id': journal.id,
    })
    # Check domain reflects journal currency
    self.assertEqual(
        contract.pricelist_id_domain,
        [('currency_id', '=', journal.currency_id.id)]
    )

def test_onchange_journal_id_resets_pricelist(self):
    """Test that onchange on journal_id clears pricelist_id if currencies don't match."""
    journal = self.env['account.journal'].create({
        'name': 'Test Journal',
        'code': 'TEST',
        'type': 'sale',
        'currency_id': self.env.ref('base.EUR').id,
    })
    pricelist = self.env['product.pricelist'].create({
        'name': 'Test Pricelist',
        'currency_id': self.env.ref('base.USD').id,
    })
    contract = self.env['contract.contract'].create({
        'name': 'Test Contract',
        'journal_id': journal.id,
        'pricelist_id': pricelist.id,
    })
    # Change journal to one with different currency
    new_journal = self.env['account.journal'].create({
        'name': 'New Journal',
        'code': 'NEW',
        'type': 'sale',
        'currency_id': self.env.ref('base.GBP').id,
    })
    contract.journal_id = new_journal.id
    # pricelist_id should be reset
    self.assertFalse(contract.pricelist_id)

def test_constraint_no_journal_currency(self):
    """Test that constraint does not apply if journal has no currency."""
    journal = self.env['account.journal'].create({
        'name': 'Test Journal',
        'code': 'TEST',
        'type': 'sale',
        # No currency_id
    })
    pricelist = self.env['product.pricelist'].create({
        'name': 'Test Pricelist',
        'currency_id': self.env.ref('base.EUR').id,
    })
    contract = self.env['contract.contract'].create({
        'name': 'Test Contract',
        'journal_id': journal.id,
        'pricelist_id': pricelist.id,
    })
    # Should not raise UserError since journal has no currency
    self.assertEqual(contract.pricelist_id, pricelist)

---

Codecov Risk

• check_currency: Already tested, but only one positive case; more negative tests would help.
• _compute_pricelist_id_domain: Computed field logic is tested indirectly in test_currency, but not explicitly.
• _onchange_journal_id: Not directly tested; only through test_currency which indirectly calls it.
• No dedicated test for pricelist_id_domain field behavior in UI or when journal_id is unset.

These areas may reduce coverage unless explicitly tested.

---

Final Note

Coverage looks adequate for the functional changes, but adding explicit tests for the domain logic and onchange behavior would significantly improve robustness and confidence.

---

⏰ PR Aging Alert

This PR by @anusriNPS has been open for 111 days (3 months).
💤 No activity for 107 days. Has this PR been forgotten?

Every ignored PR is a contributor who might not come back. Review time matters. (OCA Aging Report)

---

Reciprocal Review Request

Hi everyone! Thanks for your contribution to OCA. I reviewed and approved this PR. If any of you have a moment, I would really appreciate a review on my open PR(s):

My open PRs across OCA:

• server-tools#3548 [18.0][MIG] base_kanban_stage: Migration to 18.0
• hr-attendance#262 [16.0][ADD] Hr_attendance_idsecure: iDSecure (ControliD) attendance integration
• stock-logistics-workflow#2276 [16.0][ADD] stock_move_line_devaluation
• stock-logistics-workflow#2275 [16.0][ADD] Stock move line analytic account
• stock-logistics-workflow#2268 [16.0][ADD] stock_move_line_picking_partner
• purchase-workflow#2694 [16.0][IMP]Purchase workflow added to review state & exception fix

Reviewing each other's work helps the whole community move forward faster. Thank you!

---

Automated review by OCA Neural Reviewer + qwen3-coder:30b