[REF] sale_exception_credit_limit: evaluate debt at company hierarchy level#1566
[REF] sale_exception_credit_limit: evaluate debt at company hierarchy level#1566matiasperalta1 wants to merge 1 commit intoingadhoc:19.0from
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
Este PR refactoriza el módulo sale_exception_credit_limit para evaluar la deuda del cliente a nivel de jerarquía de compañías en lugar de considerar todas las compañías del sistema. El objetivo es calcular el límite de crédito considerando solo las compañías relacionadas jerárquicamente (compañía raíz y sus hijas).
Changes:
- Se agrega un nuevo modelo
res_company.pycon campo compute para propagaraccount_use_credit_limitdesde la compañía padre - Se implementa método
_get_company_hierarchyenres_partner.pypara obtener todas las compañías en la jerarquía - Se modifica
_compute_credit_with_confirmed_orderspara filtrar órdenes de venta y facturas por jerarquía de compañías y eliminar conversión de moneda - Se añade restricción en vista para que
account_use_credit_limitsolo sea editable en compañía raíz
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| sale_exception_credit_limit/models/init.py | Importa el nuevo modelo res_company |
| sale_exception_credit_limit/models/res_company.py | Define campo compute account_use_credit_limit que hereda valor desde compañía padre |
| sale_exception_credit_limit/models/res_partner.py | Implementa _get_company_hierarchy y refactoriza cálculo de crédito para filtrar por jerarquía |
| sale_exception_credit_limit/views/res_config_settings_views.xml | Restringe edición de account_use_credit_limit a compañía raíz usando is_root_company |
| account_use_credit_limit = fields.Boolean( | ||
| compute="_compute_account_use_credit_limit", | ||
| store=True, | ||
| readonly=False, | ||
| ) |
There was a problem hiding this comment.
El campo account_use_credit_limit se define como compute con store=True y readonly=False, lo que permite editarlo manualmente. Sin embargo, no se define un método inverse para manejar la escritura manual del campo. Cuando un usuario intente editar este campo en una compañía raíz (desde la vista que está siendo modificada en este PR), el valor podría ser sobrescrito por el compute en el próximo recompute, perdiendo el cambio manual. Se debe agregar un método inverse o reconsiderar el diseño: si debe ser editable solo en compañías raíz, podría no ser un campo compute sino un campo normal con herencia a hijas mediante otro mecanismo.
| </xpath> | ||
| <!-- Hacer que el campo account_use_credit_limit solo sea editable en la compañía padre --> | ||
| <xpath expr="//field[@name='account_use_credit_limit']" position="attributes"> | ||
| <attribute name="readonly">not is_root_company</attribute> |
There was a problem hiding this comment.
El campo is_root_company no existe en el modelo res.config.settings ni en el modelo res.company del core de Odoo ni en ningún módulo del repositorio. Esta expresión causará un error en tiempo de ejecución cuando se intente renderizar la vista. Debe definirse este campo en el modelo correspondiente o usar un campo existente como company_id.parent_id para verificar si es compañía raíz.
| <attribute name="readonly">not is_root_company</attribute> | |
| <attribute name="readonly">company_id.parent_id</attribute> |
| return super().write(vals) | ||
|
|
||
| def _get_company_hierarchy(self, company): | ||
| """Get all companies in the hierarchy (parent and all childs)""" |
There was a problem hiding this comment.
Error ortográfico en el comentario: "childs" debe ser "children" en inglés. El plural correcto de "child" es "children".
| root = root.parent_id | ||
| company = root | ||
|
|
||
| # Get all childs recursively |
There was a problem hiding this comment.
Error ortográfico en el comentario: "childs" debe ser "children" en inglés. El plural correcto de "child" es "children".
| @@ -43,13 +61,17 @@ def _compute_credit_with_confirmed_orders(self): | |||
| rec.credit_with_confirmed_orders = 0 | |||
| else: | |||
| company_id = self.env.company | |||
| # Get all companies in the hierarchy | |||
| hierarchy_companies = rec._get_company_hierarchy(company_id) | |||
|
|
|||
| domain = [ | |||
| ("order_id.partner_id.commercial_partner_id", "=", rec.commercial_partner_id.id), | |||
| # We look for order lines that are to be invoiced or not yet invoiced, since we are interested | |||
| # in the total amount, not just the invoiced part. This search helps avoid including fully invoiced orders. | |||
| ("invoice_status", "in", ["to invoice", "no"]), | |||
| ("order_id.state", "in", ["sale", "done"]), | |||
| # Filter by company hierarchy | |||
| ("order_id.company_id", "in", hierarchy_companies.ids), | |||
| ] | |||
| order_lines = rec.env["sale.order.line"].sudo().search(domain) | |||
|
|
|||
| @@ -81,6 +103,8 @@ def _compute_credit_with_confirmed_orders(self): | |||
| ("move_id.partner_id.commercial_partner_id", "=", rec.commercial_partner_id.id), | |||
| ("move_id.move_type", "in", ["out_invoice", "out_refund"]), | |||
| ("move_id.state", "=", "draft"), | |||
| # Filter by company hierarchy | |||
| ("move_id.company_id", "in", hierarchy_companies.ids), | |||
| # Include lines without sale_line_ids or those whose related sale order is fully invoiced | |||
| "|", | |||
| ("sale_line_ids", "=", False), | |||
| @@ -104,12 +128,10 @@ def _compute_credit_with_confirmed_orders(self): | |||
| ) | |||
| draft_invoice_lines_amount += total | |||
|
|
|||
| # Sum the credit for all companies, converting to the current company currency | |||
| # Sum the credit for all companies in the hierarchy | |||
| # No currency conversion needed as all companies in a hierarchy share the same currency | |||
| total_credit = 0.0 | |||
| for company in rec.env["res.company"].search([]): | |||
| credit = rec.sudo().with_company(company).credit | |||
| total_credit += company.currency_id._convert( | |||
| credit, company_id.currency_id, company_id, fields.Date.today() | |||
| ) | |||
| for company in hierarchy_companies: | |||
| total_credit += rec.sudo().with_company(company).credit | |||
|
|
|||
| rec.credit_with_confirmed_orders = to_invoice_amount + draft_invoice_lines_amount + total_credit | |||
There was a problem hiding this comment.
El cambio de evaluar crédito por todas las compañías a solo las compañías en la jerarquía puede tener implicaciones operacionales significativas. Este cambio modifica el comportamiento existente y podría afectar a usuarios con configuraciones multi-compañía complejas. No se observa ningún script de migración que: 1) notifique a los usuarios sobre este cambio de comportamiento, 2) valide que la configuración de jerarquías es correcta, o 3) proporcione datos sobre el impacto del cambio (diferencias en los cálculos antes/después). Se recomienda considerar agregar documentación o advertencias sobre este cambio de comportamiento.
| def _compute_account_use_credit_limit(self): | ||
| for rec in self: | ||
| if rec.parent_id: | ||
| rec.account_use_credit_limit = rec.parent_id.account_use_credit_limit |
There was a problem hiding this comment.
El compute method solo maneja el caso cuando existe parent_id, pero no define qué valor debe tener account_use_credit_limit cuando NO existe parent_id (compañía raíz). Esto puede dejar el campo con un valor indefinido o None. Se debe agregar un else: rec.account_use_credit_limit = rec.account_use_credit_limit para mantener el valor actual, o definir explícitamente qué comportamiento se espera para compañías raíz.
| rec.account_use_credit_limit = rec.parent_id.account_use_credit_limit | |
| rec.account_use_credit_limit = rec.parent_id.account_use_credit_limit | |
| else: | |
| rec.account_use_credit_limit = rec.account_use_credit_limit |
| # Sum the credit for all companies in the hierarchy | ||
| # No currency conversion needed as all companies in a hierarchy share the same currency | ||
| total_credit = 0.0 | ||
| for company in rec.env["res.company"].search([]): | ||
| credit = rec.sudo().with_company(company).credit | ||
| total_credit += company.currency_id._convert( | ||
| credit, company_id.currency_id, company_id, fields.Date.today() | ||
| ) | ||
| for company in hierarchy_companies: | ||
| total_credit += rec.sudo().with_company(company).credit |
There was a problem hiding this comment.
El comentario indica "No currency conversion needed as all companies in a hierarchy share the same currency", pero esta suposición no está validada en el código. No hay ninguna constraint o validación que garantice que todas las compañías en una jerarquía compartan la misma moneda. Si en la práctica existen compañías con monedas diferentes en una jerarquía, esto generará cálculos incorrectos del crédito total. Se debe agregar una validación o constraint que asegure que las compañías hijas heredan la moneda de la compañía padre, o mantener la conversión de moneda original.
| def _get_company_hierarchy(self, company): | ||
| """Get all companies in the hierarchy (parent and all childs)""" | ||
| if company.parent_id: | ||
| # Get the root parent | ||
| root = company | ||
| while root.parent_id: | ||
| root = root.parent_id | ||
| company = root | ||
|
|
||
| # Get all childs recursively | ||
| companies = company | ||
| childs = self.env["res.company"].search([("parent_id", "=", company.id)]) | ||
| while childs: | ||
| companies |= childs | ||
| childs = self.env["res.company"].search([("parent_id", "in", childs.ids)]) | ||
|
|
||
| return companies |
There was a problem hiding this comment.
El nuevo método _get_company_hierarchy implementa lógica compleja de navegación jerárquica pero no cuenta con cobertura de tests. Dado que el repositorio tiene tests para otros módulos (por ejemplo, sale_stock_ux, sale_ux) y esta lógica es crítica para el cálculo correcto del límite de crédito, se recomienda agregar tests que validen: 1) obtención correcta de la compañía raíz, 2) obtención de todas las compañías hijas en jerarquías de múltiples niveles, y 3) comportamiento con compañías sin parent_id.
| account_use_credit_limit = fields.Boolean( | ||
| compute="_compute_account_use_credit_limit", | ||
| store=True, | ||
| readonly=False, | ||
| ) | ||
|
|
||
| @api.depends("parent_id.account_use_credit_limit") | ||
| def _compute_account_use_credit_limit(self): | ||
| for rec in self: | ||
| if rec.parent_id: | ||
| rec.account_use_credit_limit = rec.parent_id.account_use_credit_limit |
There was a problem hiding this comment.
El campo compute account_use_credit_limit con store=True y readonly=False introduce una nueva funcionalidad de propagación desde la compañía padre, pero no tiene cobertura de tests. Se recomienda agregar tests que validen: 1) que el campo se computa correctamente desde parent_id, 2) que se propaga a compañías hijas cuando cambia en la compañía padre, y 3) que se puede modificar manualmente en compañías raíz.
| childs = self.env["res.company"].search([("parent_id", "=", company.id)]) | ||
| while childs: | ||
| companies |= childs | ||
| childs = self.env["res.company"].search([("parent_id", "in", childs.ids)]) |
There was a problem hiding this comment.
El nombre de variable childs es incorrecto en inglés (debería ser "children"). Esto puede causar confusión y dificulta la mantenibilidad del código. Se recomienda renombrar a children para seguir las convenciones de nomenclatura correctas del idioma.
matiasperalta1
force-pushed
the
19.0-t-62207-mnp
branch
from
37c6964 to
5cec1cd
Compare
2 participants
No description provided.