[ADD] product_abc_classification_finance: Classification by cost and sale price #2002

AJamal13 · 2025-06-21T19:27:36Z

This commit introduces a new module inheriting from product_abc_classification_sale_stock to include additional profile types for classifying products by cost or sale price,.

Adding classification by Cost/Sale Price/Sale Margin

AJamal13 · 2025-06-21T19:51:34Z

Hello @lmignon , @lmignon , @rousseldenis ,
kindly review this code, thanks.

lmignon

@AJamal13 Thank you for your proposal. is all the code in github? There's nothing in the proposed code to fill in the new model that have been added.

AJamal13 · 2025-06-23T06:49:26Z

@AJamal13 Thank you for your proposal. is all the code in github? There's nothing in the proposed code to fill in the new model that have been added.

Thank you for your response, @lmignon
I'm not sure I'm following. would you please explain further ?
I forked the product attribute repo with the purpose of adding a functionality to the following modules:
product_abc_classification
product_abc_classification_sale_stock
to classify by cost/sale price/margin. if I did something in the wrong way please let me know,
Thank you.

lmignon · 2025-06-23T06:57:00Z

@AJamal13 Sorry, the most important file was collapsed by default into the diff. Nevertheless can you add unittest plz to ensure the quality of the new addon.

AJamal13 · 2025-06-23T07:55:41Z

@AJamal13 Sorry, the most important file was collapsed by default into the diff. Nevertheless can you add unittest plz to ensure the quality of the new addon.

Sure, will do

Introduces demo data for finance-based ABC classification, including new XML records and manifest updates. Adds a comprehensive test suite for finance profile logic, validation, and history record creation. Also includes code cleanup, improved formatting, and minor view XML attribute adjustments for consistency.

Added tests to validate that finance profile levels must total 100%, ensure products with negative margin are excluded from ABC ranking for 'sale_margin' profiles, and verify that _get_finance_data_query returns correct SQL for all profile types.

Added tests to validate that finance profile levels must total 100%, ensure products with negative margin are excluded from ABC ranking for 'sale_margin' profiles, and verify that _get_finance_data_query returns correct SQL for all profile types. Add demo data and tests for finance ABC classification Introduces demo data for finance-based ABC classification, including new XML records and manifest updates. Adds a comprehensive test suite for finance profile logic, validation, and history record creation. Also includes code cleanup, improved formatting, and minor view XML attribute adjustments for consistency. fix checks

This reverts commit e1d7e1e.

…into 16.0

AJamal13 · 2025-06-23T22:50:28Z

@lmignon
Hello, kindly review again,

quentinDupont · 2025-06-26T08:30:24Z

Hello !
Thanks for the work !
@AJamal13 Could you ajust your pull request to OCA guidelines please : https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#git
It permits to quickly understand if it's a new module, an improvment, which module is at stake etc.

Thank you !

AJamal13 · 2025-06-26T10:25:50Z

Hello ! Thanks for the work ! @AJamal13 Could you ajust your pull request to OCA guidelines please : https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#git It permits to quickly understand if it's a new module, an improvment, which module is at stake etc.

Thank you !

@quentinDupont Thank you for your response and for the tip,
Done.

lmignon · 2025-06-26T11:03:32Z

product_abc_classification_finance/models/abc_classification_profile.py

+            ),
+            (
+                "sale_margin",
+                "Based on the Sale Margin of delivered sale order line by product",


@AJamal13 AFAIK the sale_margin addon provides you the expected margin if all the products sold into the SO are delivered.... If you need to know the real margin based on the delivered qties for your SOL, you need to use the OCA addon sale_margin_delivered and the margin_delivered field provided by this addon. IMO, you should only keep the 2 profiles 'cost' and 'sale_price' in this addon without extra dependency on sale_margin and create a new addon with the extra dependency required in the case where the classification is based on margins.

@lmignon sorry re-requested by mistake

quentinDupont · 2025-06-26T11:49:33Z

Hello ! Thanks for the work ! @AJamal13 Could you ajust your pull request to OCA guidelines please : https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#git It permits to quickly understand if it's a new module, an improvment, which module is at stake etc.
Thank you !

@quentinDupont Thank you for your response and for the tip, Done.

You're welcome
You could add "product_abc_classification_finance" in the title :)

AJamal13 · 2025-06-26T12:23:24Z

@lmignon Thank you for your response and advise. I excluded the margin from the module, and might add it later.
Would you kindly review the current module ?
Thanks.

lmignon · 2025-06-26T12:39:51Z

product_abc_classification_finance/__manifest__.py

+    ],
+
+    "installable": True,
+    "application": False,


@AJamal13 you should add yourself as the official maintainers 😏

lmignon · 2025-06-26T12:40:58Z

product_abc_classification_finance/models/abc_classification_profile.py

+            finance_data = self._finance_init_collected_data_instance()
+            finance_data.product = ProductProduct.browse(product_id)
+            finance_data.purchase_price = float(
+                getattr(finance_data.product.product_tmpl_id, "standard_price", 0.0)


Why do you use getattr ?

lmignon · 2025-06-26T12:41:16Z

product_abc_classification_finance/models/abc_classification_profile.py

+            # Always set purchase_price (standard cost) from product.template
+            tmpl = finance_data.product.product_tmpl_id
+            finance_data.purchase_price = float(
+                getattr(tmpl, "standard_price", 0.0) or 0.0


Why do you use getattr ?

lmignon · 2025-06-26T12:42:35Z

product_abc_classification_finance/models/abc_classification_profile.py

+        ProductClassification = self.env["abc.classification.product.level"]
+
+        for profile in to_compute:
+            # Get finance data per product (list of FinanceSaleData), plus total for percentage computation
+            finance_data_list, total_value = profile._finance_get_data()
+            existing_level_ids_to_remove = profile._get_existing_level_ids()
+            level_percentage = profile._build_ordered_level_cumulative_percentage()
+            if not level_percentage:
+                continue
+            level, percentage = level_percentage.pop(0)
+            previous_data = None
+            total_products = len(finance_data_list)
+            percentage_products = (100.0 / total_products) if total_products else 0.0
+            # Pick the correct value field for this profile type
+            if profile.profile_type == "cost":
+                value_field = "total_cost"
+            elif profile.profile_type == "sale_price":
+                value_field = "total_sales"
+            else:
+                raise UserError(
+                    _(f"Unknown finance profile_type: {profile.profile_type}")
+                )
+
+            for i, finance_data in enumerate(finance_data_list):
+                finance_data.total_products = total_products
+                finance_data.percentage_products = percentage_products
+                finance_data.cumulated_percentage_products = (
+                    finance_data.percentage_products
+                    if i == 0
+                    else (
+                        finance_data.percentage_products
+                        + previous_data.cumulated_percentage_products
+                    )
+                )
+                # Compute percentages and cumulative percentages for the products
+                value = getattr(finance_data, value_field, 0.0) or 0.0
+                finance_data.percentage = (
+                    (100.0 * value / total_value) if total_value else 0.0
+                )
+                finance_data.cumulated_percentage = (
+                    finance_data.percentage
+                    if i == 0
+                    else (finance_data.percentage + previous_data.cumulated_percentage)
+                )
+                # Allow for floating point imprecision: round to 2 decimals and allow up to 101
+                if float_round(finance_data.cumulated_percentage, 2) > 100.01:
+                    raise UserError(
+                        _(
+                            "Cumulative percentage greater than 100 (actual: %.4f)."
+                            % finance_data.cumulated_percentage
+                        )
+                    )
+                finance_data.sum_cumulated_percentages = (
+                    finance_data.cumulated_percentage
+                    + finance_data.cumulated_percentage_products
+                )
+                # Compute ABC classification for the products based on the
+                # sum of cumulated percentages
+                if (
+                    finance_data.sum_cumulated_percentages > percentage
+                    and len(level_percentage) > 0
+                ):
+                    level, percentage = level_percentage.pop(0)
+                product = finance_data.product
+                levels = product.abc_classification_product_level_ids
+                product_abc_classification = levels.filtered(
+                    lambda p, prof=profile: p.profile_id == prof
+                )
+                finance_data.computed_level = level
+                if product_abc_classification:
+                    existing_level_ids_to_remove.remove(product_abc_classification.id)
+                    if product_abc_classification.level_id != level:
+                        vals = profile._finance_data_to_vals(finance_data, create=False)
+                        product_abc_classification.write(vals)
+                else:
+                    vals = profile._finance_data_to_vals(finance_data, create=True)
+                    product_abc_classification = ProductClassification.create(vals)
+                finance_data.product_level = product_abc_classification
+                previous_data = finance_data
+            if finance_data_list:
+                profile._finance_log_history(finance_data_list)
+            profile._purge_obsolete_level_values(existing_level_ids_to_remove)
+        return res


All this code should be placed into a dedicated method to ease readability and modularity.

thanks for the advice, but in this code i used the method from product_abc_classification_sale_stock as a reference while just adjusting some details, wouldn't it work just fine as it is ?

AJamal13 · 2025-06-26T14:04:46Z

@lmignon thank you for your advices,
updated getattrs, is it possible to keep it working as it is currently with _compute_abc_classification method in it's current form ?

lmignon · 2025-06-27T06:25:59Z

Thank for all your work @AJamal13
I'm still convinced it would be cleaner to put the logic to compute the classification for a specific profile into a dedicated method but I will not block for this. I wonder if the best approach in terms of modularity wouldn't be to make one module per profile.

product_abc_classisifaction_sale_price
product_abc_classification_cost
product_abc_classification_sale_margin

But it's also a lot of work and if you don't see the point, I won't block for this too.

Can you fix pre-commit plz?

AJamal13 · 2025-06-27T09:13:36Z

@lmignon thanks for the advice, Done.
Please check again.

lmignon

Thank you for all your work. Even if I think we could still improve the quality and modularity you already invested a lot of effort to align your initial proposal with the expected OCA quality. The code LGTM, let's move forward 😏

jbaudoux

You need to drop the commits where you merged the mainstream 16.0 branch in your branch. You should rebase on 16.0 instead.
And squash all your commits in a single commit

AJamal13 · 2025-06-27T14:00:31Z

Thank you for all your work. Even if I think we could still improve the quality and modularity you already invested a lot of effort to align your initial proposal with the expected OCA quality. The code LGTM, let's move forward 😏

Thank you!
Next contribution will be smoother 😁

AJamal13 · 2025-06-27T14:01:16Z

You need to drop the commits where you merged the mainstream 16.0 branch in your branch. You should rebase on 16.0 instead. And squash all your commits in a single commit

Ok, will do.

rousseldenis · 2025-06-27T14:46:12Z

@AJamal13 Thanks for this.

As already said, your commits should be cleaned to follow a little bit conventions (they should look like [ADD] product_abc_classification_finance and then, improvement ones like [IMP] product_abc_classification_finance: (....). See this : https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#71commit-message

And you can read the whole page to help you contributing. Thanks

Like this:

rousseldenis · 2025-06-27T15:00:14Z

product_abc_classification_finance/models/abc_classification_profile.py

+                and not rec.warehouse_id
+            ):
+                raise ValidationError(
+                    _("You must specify a warehouse for {profile_name}").format(


You should use translation function arguments instead of format()

rousseldenis · 2025-06-27T15:03:08Z

product_abc_classification_finance/models/abc_classification_profile.py

+                value_field = "total_sales"
+            else:
+                raise UserError(
+                    _(f"Unknown finance profile_type: {profile.profile_type}")


Same as above

rousseldenis · 2025-06-27T15:05:35Z

product_abc_classification_finance/models/abc_classification_profile.py

+            profile._purge_obsolete_level_values(existing_level_ids_to_remove)
+        return res
+
+    def _finance_log_history(self, finance_data_list):


Why this ??? And why passing through csv implementation as you bypass the ORM?

rousseldenis · 2025-07-03T07:41:25Z

@AJamal13

AJamal13 · 2025-07-03T15:28:25Z

@AJamal13

@rousseldenis thank you for your advices, will apply changes and request your review.

AJamal13 added 2 commits June 21, 2025 22:10

Adding classification by Cost/Sale Price/Sale Margin

8082462

Adding classification by Cost/Sale Price/Sale Margin

Update __manifest__.py

46950db

lmignon suggested changes Jun 23, 2025

View reviewed changes

AJamal13 added 7 commits June 24, 2025 00:01

Merge branch 'OCA:16.0' into 16.0

6b01962

fix checks

0d86ee5

Revert "Add tests for finance profile validation and margin exclusion"

f77be24

This reverts commit e1d7e1e.

Merge branch '16.0' of https://github.com/AJamal13/product-attribute …

80c175d

…into 16.0

AJamal13 requested a review from lmignon June 24, 2025 09:56

AJamal13 changed the title ~~Adding classification by cost and sale price and sale margin.~~ [IMP] Adding classification by cost and sale price and sale margin. Jun 26, 2025

lmignon suggested changes Jun 26, 2025

View reviewed changes

AJamal13 requested a review from lmignon June 26, 2025 11:06

AJamal13 changed the title ~~[IMP] Adding classification by cost and sale price and sale margin.~~ [ADD] product_abc_classification_finance: Classification by cost and sale price Jun 26, 2025

lmignon reviewed Jun 26, 2025

View reviewed changes

AJamal13 requested a review from lmignon June 26, 2025 13:56

lmignon approved these changes Jun 27, 2025

View reviewed changes

jbaudoux reviewed Jun 27, 2025

View reviewed changes

product_abc_classification_finance

9581799

AJamal13 force-pushed the 16.0 branch from 15e4104 to 9581799 Compare June 27, 2025 14:17

rousseldenis added the needs fixing label Jun 27, 2025

rousseldenis added this to the 16.0 milestone Jun 27, 2025

rousseldenis requested changes Jun 27, 2025

View reviewed changes

Uh oh!

[ADD] product_abc_classification_finance: Classification by cost and sale price #2002

Are you sure you want to change the base?

[ADD] product_abc_classification_finance: Classification by cost and sale price #2002

Conversation

AJamal13 commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AJamal13 commented Jun 21, 2025

Uh oh!

lmignon left a comment

Choose a reason for hiding this comment

Uh oh!

AJamal13 commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmignon commented Jun 23, 2025

Uh oh!

AJamal13 commented Jun 23, 2025

Uh oh!

AJamal13 commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quentinDupont commented Jun 26, 2025

Uh oh!

AJamal13 commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmignon Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quentinDupont commented Jun 26, 2025

Uh oh!

AJamal13 commented Jun 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AJamal13 commented Jun 26, 2025

Uh oh!

lmignon commented Jun 27, 2025

Uh oh!

AJamal13 commented Jun 27, 2025

Uh oh!

lmignon left a comment

Choose a reason for hiding this comment

Uh oh!

jbaudoux left a comment

Choose a reason for hiding this comment

Uh oh!

AJamal13 commented Jun 27, 2025

Uh oh!

AJamal13 commented Jun 27, 2025

Uh oh!

rousseldenis commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousseldenis commented Jul 3, 2025

Uh oh!

AJamal13 commented Jul 3, 2025

Uh oh!

Reviewers

Assignees

Labels

AJamal13 commented Jun 21, 2025 •

edited

Loading

AJamal13 commented Jun 23, 2025 •

edited

Loading

AJamal13 commented Jun 23, 2025 •

edited

Loading

AJamal13 commented Jun 26, 2025 •

edited

Loading

lmignon Jun 26, 2025 •

edited

Loading

rousseldenis commented Jun 27, 2025 •

edited

Loading