[UPD] account_banking_pain_base: sepa hybrid mode

[ntsirintanis]

No description provided.

[thomaspaulb]

So to discuss about the methodology (source: this doc):

1. What the current version 16.0 module does, before this PR, is to create an unstructured address block with no TwnNm and just a Ctry and two AdrLine blocks. I understand that this has become forbidden.
2. In the new situation we can choose the structured address block, however, unless OCA's base_address_extended is installed and all fields of the partner duly filled, it's pretty hard to get this right
3. So, it is more realistic to aim to generate the hybrid address block, where PstCd is optional, TwnNm and Ctry are mandatory and the address is complemented by two 70-character-max AdrLine blocks.

What we have implemented now is that when the "enforce hybrid mode" checkbox is checked, it adds TwnNm and Ctry and does not include any AdrLine blocks.

Perhaps what's even better is that we always assume hybrid mode and:

• Always add TwnNm, eg if partner.city is empty, raise an error or fully omit the address block
• Always try adding PstCd from the "zip" field in Odoo
• Still add AdrLine blocks, but don't repeat any information in it eg don't put post code and city there

[Kiplangatdan]

@ntsirintanis I can't find anywhere where the payment_order_id is passed to the context.

In anycase, self should get you the current payment order.
So bank = po.journal_id.bank_account_id.bank_id should work also.

[Kiplangatdan]

@ntsirintanis This is not reached when using the account_banking_sepa_credit_transfer or the account_banking_sepa_direct_debit.

It would be better to inject the bank_id to open2generated() instead.

[ntsirintanis]

Note that this is an api.model so self may or may not be an empty recordset

[mihien]

@ntsirintanis Is this PR doing the same as #1550 but in 16.0, and is it still active? I'm guessing it's not a port since it pre-dates the other PR

[ntsirintanis]

@ntsirintanis Is this PR doing the same as #1550 but in 16.0, and is it still active? I'm guessing it's not a port since it pre-dates the other PR

@mihien Yeah, this was developed in parallel for versions 14, 16, and 18 (#1553)

[marcos-mendez]

Automated Review -- Tests Failed

1. Root Cause

The test failure is due to a missing Python dependency unidecode, required by the account_banking_pain_base module, which prevents the module from being installed and loaded during the test run.

---

2. Suggested Fix

Add unidecode to the external_dependencies list in the __manifest__.py of the account_banking_pain_base module:

# In account_banking_pain_base/__manifest__.py
"external_dependencies": {
    "python": ["unidecode"],
},

This ensures that the dependency is checked and installed before the module is loaded.

---

3. Additional Code Issues

• In generate_address_block: The condition if not partner.country_id: return True is a valid early return, but the logic for handling pain_flavor starts with pain.001.001. and pain.008.001. — ensure this logic is consistent across all SEPA pain flavors. Consider adding a helper method or a more robust check for supported pain flavors.

• In open2generated: The method self_with_context_bank is constructed using with_context, but self is not reassigned, which could be confusing. It's better to assign the result back to self or use a clearer variable name like self_contextualized.

  # Instead of:
  self_with_context_bank = self.with_context(export_bank_id=bank.id) if bank else self
  return super(AccountPaymentOrder, self_with_context_bank).open2generated()
  
  # Prefer:
  self_contextualized = self.with_context(export_bank_id=bank.id) if bank else self
  return super(AccountPaymentOrder, self_contextualized).open2generated()

---

4. Test Improvements

• Add tests for different pain_flavor values (e.g., pain.001.003.03, pain.008.001.02) to ensure address block generation behaves correctly.
• Add a test case to verify that enforce_sepa_hybrid_mode = False does not alter the address block structure (i.e., full address should be preserved).
• Use SavepointCase instead of TransactionCase if tests are expected to be isolated and run in parallel.
• Add a test to assert that export_bank_id is not passed in context when no bank is present in the payment order.

# Example test improvement:
def test_open2generated_no_bank(self):
    """Test that open2generated does not inject export_bank_id if no bank is set."""
    self.po.journal_id.bank_account_id.bank_id = False
    self.po.open2generated()
    # Assert that no bank context was set
    self.assertNotIn("export_bank_id", self.po.env.context)

---

Summary

The main issue is a missing dependency. Fix by adding unidecode to external_dependencies. Also, improve code clarity in open2generated and enhance tests to cover hybrid mode and different pain flavors.

---

⏰ PR Aging Alert

This PR by @ntsirintanis has been open for 122 days (4 months).

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

---

Reciprocal Review Request

Hi everyone! I found some test failures on this PR and left detailed feedback above. I am happy to discuss or help debug. In the meantime, if any of you get a chance, I would appreciate a look at my open PR(s):

My open PRs across OCA:

• 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. Thank you!

---

Environment via OCA Neural Reviewer: Minikube + K8s Job + oca-ci/py3.10-odoo16.0 | Odoo 16.0
Automated review by OCA Neural Reviewer + qwen3-coder:30b