Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[16.0][MIG] procurement_mto_analytic: Migration to 16.0 #637

Open
wants to merge 25 commits into
base: 16.0
Choose a base branch
from

Conversation

AungKoKoLin1997
Copy link
Contributor

@AungKoKoLin1997 AungKoKoLin1997 commented Mar 26, 2024

@qrtl QT4346

Comment on lines 12 to 14
# In case of using stock_analytic and mrp_stock_analytic
if not analytic_distribution and hasattr(self, "analytic_distribution"):
analytic_distribution = self.analytic_distribution
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# In case of using stock_analytic and mrp_stock_analytic
if not analytic_distribution and hasattr(self, "analytic_distribution"):
analytic_distribution = self.analytic_distribution

Let's remove this as stock_analytic already takes care of this:

def _prepare_procurement_values(self):
"""
Allows to transmit analytic account from moves to new
moves through procurement.
"""
res = super()._prepare_procurement_values()
if self.analytic_distribution:
res.update(
{
"analytic_distribution": self.analytic_distribution,
}
)
return res

Comment on lines 11 to 38
def _find_matching_analytic_distribution_record(self, analytic_distribution):
# Prepare the JSONB structure as a string for the SQL query
analytic_distribution_str = json.dumps(analytic_distribution)
# Use a parameterized query for safety
query = """
SELECT id FROM purchase_order_line
WHERE analytic_distribution::jsonb = %s::jsonb;
"""
self.env.cr.execute(query, (analytic_distribution_str,))
result = self.env.cr.fetchall()
# Extract IDs from the result
matching_po_line = [res[0] for res in result]
return matching_po_line

def _make_po_get_domain(self, company_id, values, partner):
domain = super()._make_po_get_domain(company_id, values, partner)
if values.get("analytic_distribution"):
# Fetch matching record IDs based on dynamic analytic_distribution
matching_po_line = self._find_matching_analytic_distribution_record(
values["analytic_distribution"]
)
if matching_po_line:
domain += (("order_line", "in", tuple(matching_po_line)),)
else:
# To create new PO
domain += (("order_line", "=", 0000000),)
else:
domain += (("order_line.analytic_distribution", "=", False),)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please check if this works.

Suggested change
def _find_matching_analytic_distribution_record(self, analytic_distribution):
# Prepare the JSONB structure as a string for the SQL query
analytic_distribution_str = json.dumps(analytic_distribution)
# Use a parameterized query for safety
query = """
SELECT id FROM purchase_order_line
WHERE analytic_distribution::jsonb = %s::jsonb;
"""
self.env.cr.execute(query, (analytic_distribution_str,))
result = self.env.cr.fetchall()
# Extract IDs from the result
matching_po_line = [res[0] for res in result]
return matching_po_line
def _make_po_get_domain(self, company_id, values, partner):
domain = super()._make_po_get_domain(company_id, values, partner)
if values.get("analytic_distribution"):
# Fetch matching record IDs based on dynamic analytic_distribution
matching_po_line = self._find_matching_analytic_distribution_record(
values["analytic_distribution"]
)
if matching_po_line:
domain += (("order_line", "in", tuple(matching_po_line)),)
else:
# To create new PO
domain += (("order_line", "=", 0000000),)
else:
domain += (("order_line.analytic_distribution", "=", False),)
def _make_po_get_domain(self, company_id, values, partner):
domain = super()._make_po_get_domain(company_id, values, partner)
matching_lines = self.env["purchase.order"].sudo().search([dom for dom in domain]).order_line.filtered(lambda x: x.analytic_distribution == values.get("analytic_distribution"))
if matching_lines:
domain += (("order_line", "in", matching_lines.ids),)
else:
# To create new PO
domain += (("order_line", "=", 0000000),)

Comment on lines 9 to 27
def _make_po_get_domain(self, company_id, values, partner):
domain = super()._make_po_get_domain(company_id, values, partner)
if not values.get("analytic_distribution"):
return domain
matching_lines = (
self.env["purchase.order"]
.sudo()
.search([dom for dom in domain])
.order_line.filtered(
lambda x: x.analytic_distribution == values.get("analytic_distribution")
)
)
if matching_lines:
domain += (("order_line", "in", tuple(matching_lines.ids)),)
else:
domain += (
("order_line", "=", False),
) # This might need adjustment based on your intent.
return domain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing the design, I now doubt that this should be the way to go, as there can be cases where one purchase order holds mutplie analytic distributions. We should not be extending this method.

We should instead create a PR in odoo/odoo to split https://github.com/OCA/OCB/blob/1e9b8db/addons/purchase_stock/models/stock_rule.py#L125 into a method, and extend that method in this module to add analytic distribution as a key.

"""
original_key = super()._get_po_line_grouping_key(line)
if line.analytic_distribution:
analytic_distribution_key = tuple(line.analytic_distribution.items())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Python, dict entries are (basically) random-ordered in versions 3.6 and below, and insertion-ordered after that. So we have the following:

Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.16.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: a = {"1": 100, "2": 100}
In [2]: b = {"2": 100, "1": 100}
In [3]: tuple(a.items()) == tuple(b.items())

Out[3]: False

In [4]: a.items()
Out[4]: dict_items([('1', 100), ('2', 100)])

In [5]: b.items()
Out[5]: dict_items([('2', 100), ('1', 100)])

To me, this intuitively doesn't make sense: if I have a distribution with the same accounts and the same percentages, those should be the same regardless of the order, right? I suggest adding a sorted call into the key generation:

Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.16.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: a = {"1": 100, "2": 100}
In [2]: b = {"2": 100, "1": 100}
In [3]: tuple(sorted(a.items())) == tuple(sorted(b.items()))

Out[3]: True

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I have a distribution with the same accounts and the same percentages, those should be the same regardless of the order, right? I suggest adding a sorted call into the key generation

@aisopuro items of analytic_distribution are always sorted by default. So, we don't need to worry about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you link to the place in Odoo's source code that ensures they are always sorted, regardless of whether:

  • The distribution was added by a user through the UI
  • The distribution was added by business logic in Python
  • The distribution was imported via excel
  • etc

Or does the JSONB column type in Postgresql guarantee keys are ordered?

Copy link
Contributor Author

@AungKoKoLin1997 AungKoKoLin1997 May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on this source(https://www.postgresql.org/docs/10/datatype-json.html#JSON-TYPE-MAPPING-TABLE:~:text=There%20are%20two,value%20is%20kept.), I conclude analytic_distribution are sorted depends on the efficiency.
Whenever we insert the key in any order, it will be sorted if they have same data.
Eg, when we insert {"1":60, "2":40} and {"2":40, "1":60}, they will be sorted. So, the result will be the same.
Screenshot 2024-05-17 at 10 23 02

Copy link
Contributor

@aisopuro aisopuro May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍, my suggestion is then not needed.

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-mig-procurment_mto_analytic branch from 723dbf7 to 9f07687 Compare May 24, 2024 03:41
Sergej Lozikov and others added 21 commits July 9, 2024 10:00
Currently translated at 100.0% (3 of 3 strings)

Translation: account-analytic-13.0/account-analytic-13.0-procurement_mto_analytic
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-13-0/account-analytic-13-0-procurement_mto_analytic/es/
Currently translated at 100.0% (3 of 3 strings)

Translation: account-analytic-13.0/account-analytic-13.0-procurement_mto_analytic
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-13-0/account-analytic-13-0-procurement_mto_analytic/ca/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: account-analytic-15.0/account-analytic-15.0-procurement_mto_analytic
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-15-0/account-analytic-15-0-procurement_mto_analytic/
@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-mig-procurment_mto_analytic branch from 9f07687 to 48656ad Compare July 9, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants