[16.0][IMP] partner_manual_rank : Using customer_rank/supplier_rank values in partner creation for is_customer/is_supplier values#2257
Conversation
|
Hi @luisg123v, @frahikLV, |
…ues through create method This refactoring will allow us to determine default values for these fields using other values in the create method values list.
…lt is_customer/is_supplier computation Currently, when creating a partner with this module, the customer_rank/supplier_rank values get trumped by the inverse methods, if the is_supplier/is_customer values are not set in the initial creation values. This can cause some incompatibility between modules creating partners using these values through code, and running into unintended behavior. For instance, the OCA/edi module account_invoice_import, which creates a partner on an account.move record using the supplier_rank value and does not use the res_partner_search_mode context value. As such, this modification allows for users to create a partner with a default is_customer/is_supplier value using customer_rank/supplier_rank.
8191587 to
2b529d4
Compare
marcos-mendez
left a comment
There was a problem hiding this comment.
Automated Review -- Tests Failed
1. Root Cause
The test failure occurs because the create method in res_partner.py does not properly handle the case where vals_list is empty, leading to an error during the super().create(vals_list) call. This causes a database connection error in the test environment, likely due to improper handling of empty value lists.
2. Suggested Fix
Add a check for an empty vals_list before processing it in the create method:
@api.model_create_multi
def create(self, vals_list):
if not vals_list:
return super().create(vals_list)
search_partner_mode = self.env.context.get("res_partner_search_mode")
ctx_customer = search_partner_mode == "customer"
ctx_supplier = search_partner_mode == "supplier"
for vals in vals_list:
if bool(vals.get("customer_rank", 0)) or ctx_customer:
vals["is_customer"] = True
if bool(vals.get("supplier_rank", 0)) or ctx_supplier:
vals["is_supplier"] = True
return super().create(vals_list)3. Additional Code Issues
- The
_default_is_customer()and_default_is_supplier()methods were removed but their logic is now duplicated in thecreatemethod. Consider keeping these methods for consistency or ensuring the logic remains centralized. - The
createmethod modifiesvalsin-place, which may be unexpected behavior; it's better to create a copy ofvalsbefore modifying it.
4. Test Improvements
Add tests for:
- Empty
vals_listincreatemethod (to ensure robustness) - Edge cases with
customer_rankandsupplier_rankvalues (e.g., negative or non-integer values) - Context handling when
res_partner_search_modeis not set - Use
SavepointCasefor tests involvingcreateto ensure data isolation
Example test case:
def test_create_empty_vals_list(self):
# Test that creating with empty vals_list doesn't crash
partners = self.env["res.partner"].create([])
self.assertEqual(len(partners), 0)This follows OCA patterns by ensuring robustness and data integrity in edge cases.
⏰ This PR has been open for 59 days.
💤 Last activity was 33 days ago.
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:
- server-tools#3554 [MIG] datetime_formatter: Migration to 18.0
- 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. 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
Currently, when creating a partner using the
customer_rank/supplier_rankvalues, these get trumped by the_inverse_is_customer/_inverse_is_suppliermethods, since the default values are only based in theres_partner_search_modecontext values.This can cause conflicts between modules, which originally didn't depend on this module to create partners as suppliers/customers, and can disrupt some usages.
For instance, the
OCA/edimoduleaccount_invoice_importuses thesupplier_rankvalue to create a Supplier partner for an imported partner - installing this module for front-end convenience would then disrupt that process, and create a non-supplier partner.So, this PR aims to allow users to create new supplier/customer partners, using the
customer_rank/supplier_rankvalues