[19.0][ADD] project_profitability_threshold_alert: Notify project followers and project manager about costs exceeding based on a set threshold by nihelgabsi-acsone · Pull Request #1660 · OCA/project

nihelgabsi-acsone · 2026-01-22T08:31:02Z

Notify followers and project manager about costs exceeding based on a set threshold

… and project manager about budget exceeding based on a set threshold

project_profitability_threshold_alert/models/project_project.py

rousseldenis · 2026-01-28T15:41:50Z

project_profitability_threshold_alert/models/project_project.py

+    def _cron_find_costs_threshold_exceeded(self):
+        projects = self.env["project.project"].search([("active", "=", True)])
+        for project in projects:
+            project._compute_is_cost_exceeded()


Is this really necessary ? The compute is triggered by its depends.

I need to execute _get_profitability_items() to get updated data related to project costs and revenues

rousseldenis · 2026-01-28T15:43:12Z

project_profitability_threshold_alert/models/project_project.py

+        projects = self.env["project.project"].search([("active", "=", True)])
+        for project in projects:
+            project._compute_is_cost_exceeded()
+            if project.is_cost_exceeded and not project.is_notfication_sent:


Those fields should be included in search criteria (add a dedicated method for the domain like _get_costs_threshold_notification_domain())

rousseldenis · 2026-01-28T15:44:13Z

project_profitability_threshold_alert/models/project_project.py

+            email_layout_xmlid=None,
+        )
+
+    @api.depends("costs_threshold", "company_id.project_costs_threshold")


So, no need of company_id.project_cost_threshold

project_profitability_threshold_alert/models/project_project.py

rousseldenis · 2026-01-28T15:48:02Z

project_profitability_threshold_alert/models/project_project.py

+            internal_user_followers += manager
+        return internal_user_followers
+
+    @api.onchange("costs_threshold")


Set is_notification_sent as computed / store=True depending on costs_threshold to avoid onchanges.

project_profitability_threshold_alert/models/res_config_settings.py

- Rename module to be more generic - Add costs vs revenues threshold

…_threshold_alert

Add translations Fix the margin computation Fix the 'message_subscribe()' call in tests

…ected margin

…parameter

alexey-pelykh

Nice module, the margin threshold alert with cron + activity + email is well thought out. Small typo: "Threashold" should be "Threshold" in the mail template subject and body.

alexey-pelykh · 2026-02-28T16:37:09Z

project_minimum_margin_threshold_alert/data/mail_template.xml

+     License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). -->
+<odoo>
+    <record id="project_minimum_margin_exceeded_template" model="mail.template">
+        <field name="name">Project: Minimum Margin threashold exceeded</field>


Typo: threashold → threshold (in the template name and subject line)

alexey-pelykh

Review: `project_minimum_margin_threshold_alert` (Draft)

This PR is marked as draft/WIP, so this is an early-stage review to help guide development. The module concept -- alerting project stakeholders when margin thresholds are breached -- is valuable and well-structured. Below are findings to address before this moves out of draft.

PR Title vs Module Name Mismatch

The PR title references project_profitability_threshold_alert but the module is named project_minimum_margin_threshold_alert. The title should be updated to match.

Critical / High Priority

1. Typo in field name: notfication instead of notification

The field is_margin_threshold_exceeded_notfication_sent has a typo that propagates through the codebase (model, views, filters, i18n). Since this is a stored+indexed field, the typo will be baked into the database column name. This should be fixed now before any deployment.

2. _cron_margin_threshold_exceeded searches ALL projects then marks ALL as notified

At line 46 of project_project.py, the cron does projects = self.env["project.project"].search([]) then at the end sets projects.is_margin_threshold_exceeded_notfication_sent = True on ALL projects -- even those that were NOT notified (filtered out by the domain). This incorrectly marks projects that don't meet the notification criteria as "sent", preventing future notifications if their status changes.

3. _get_profitability_values() does not exist in standard Odoo project.project

The _update_is_margin_threshold_exceeded method calls project._get_profitability_values() which returns (margin_values, _show) and then reads margin_values.get("expected_percentage", "0"). However, the standard Odoo project module provides _get_profitability_items() (which the tests mock), not _get_profitability_values(). This will raise an AttributeError at runtime. The tests pass because they mock _get_profitability_items but the actual code calls a different method. The test mocking and the runtime code are out of sync.

4. Missing ir.model.access (security rules)

The __manifest__.py does not list any security/ir.model.access.csv file. While the module inherits existing models (project.project, res.users, res.config.settings), OCA guidelines typically require this file to be present. Verify whether any new model access rules or record rules are needed.

Moderate Priority

5. Typo: "Threashold" in mail template

mail_template.xml has "threashold" (should be "threshold") in both the template name and subject line. This was noted in a previous review but appears to still be present.

6. Incomplete help texts in res_config_settings.py

project_margin_threshold_create_activity help: "...when project margin threshold (minimum) is ." -- sentence is incomplete ("is" what?).
project_margin_threshold_send_email help: "...sending of emailsto internal users..." -- missing space between "emails" and "to".

7. _default_margin_threshold uses @property with float() on potentially falsy value

If the config parameter is not set, get_param() returns False. Calling float(False) returns 0.0 which works, but this is fragile. Consider using get_param(..., default=0.0) for clarity.

8. Missing __all__ or explicit exports consideration

The import from odoo.addons.base.models.res_users import ResUsers in project_project.py is used only as a type hint in _get_internal_users_for_margin_threshold. This direct import of a specific model class is unusual in Odoo modules and couples to internal structure. Consider using self.env["res.users"] return type or standard Odoo patterns instead.

9. Cron defined without noupdate="1"

data/cron.xml does not use noupdate="1". This means module updates will reset any user customization of the cron schedule. Consider wrapping it in <odoo noupdate="1">.

10. .pot file has extra entries not in .po

The .pot file contains "margin Notifications Management" (lowercase 'm') and "margin notifications" (lowercase) which differ from the .po file entries ("Margin Notifications Management" and "Margin Notifications"). This suggests the view XML was updated but translations were not regenerated.

Minor / Suggestions

11. Test naming is misleading

test_margin_not_exceeded_15 actually asserts assertTrue(self.project.is_margin_threshold_exceeded), contradicting the method name which says "not exceeded".

12. Consider adding installable: True to manifest

While it defaults to True, OCA convention is to be explicit.

13. fr.po is missing newline at end of file

Review posted via CorporateHub OCA review campaign

alexey-pelykh · 2026-03-02T06:46:15Z

project_minimum_margin_threshold_alert/models/project_project.py

+        tracking=True,
+    )
+    is_margin_threshold_exceeded_notfication_sent = fields.Boolean(
+        compute="_compute_is_margin_threshold_exceeded_notfication_sent",


Typo: notfication should be notification. Since this is a stored + indexed field, this typo will become the database column name and propagate everywhere (views, filters, translations). Better to fix it now before any deployment.

Suggested change

compute="_compute_is_margin_threshold_exceeded_notfication_sent",

is_margin_threshold_exceeded_notification_sent = fields.Boolean(

alexey-pelykh · 2026-03-02T06:46:15Z

project_minimum_margin_threshold_alert/models/project_project.py

+            - Notify users
+        """
+        projects = self.env["project.project"].search([])  # pylint: disable=no-search-all
+        projects._update_is_margin_threshold_exceeded()


This line sets is_margin_threshold_exceeded_notfication_sent = True on ALL searched projects, not just the ones that were actually notified in the loop above. Projects that were filtered out (did not match _get_margin_threshold_to_notify_domain()) will also be incorrectly marked as "sent".

Consider moving this inside the loop to only mark projects that were actually notified:

for project in projects.filtered_domain(...): # ... notification logic ... project.is_margin_threshold_exceeded_notfication_sent = True

alexey-pelykh · 2026-03-02T06:46:15Z

project_minimum_margin_threshold_alert/models/project_project.py

+                lambda u: u.receive_project_margin_threshold_notification
+            ).partner_id.ids,
+            body=f"Cost threshold exceeded for project {self.name}!",
+            subject="Project Cost Threshold Exceeded",


This calls _get_profitability_values() but the tests mock _get_profitability_items(). In standard Odoo 19.0, project.project has _get_profitability_items() which returns a dict (not a tuple). The method _get_profitability_values() does not exist by default.

The test passes because the mock is applied to a different method than the one actually called at runtime. This will likely cause an AttributeError in production.

Please verify which method provides the margin/profitability data you need and align both the code and tests.

alexey-pelykh · 2026-03-02T06:46:15Z

project_minimum_margin_threshold_alert/models/res_config_settings.py

+    )
+    project_margin_threshold_create_activity = fields.Boolean(
+        config_parameter="project_margin_threshold_alert.project_margin_threshold_create_activity",
+        help="Check this to enable by default the creation of activity"


Incomplete help text: "...when project margin threshold (minimum) is ." -- the sentence ends with "is ." which appears truncated. Also missing space: "activityfor" should be "activity for".

alexey-pelykh · 2026-03-02T06:46:15Z

project_minimum_margin_threshold_alert/data/mail_template.xml

+<odoo>
+    <record id="project_minimum_margin_exceeded_template" model="mail.template">
+        <field name="name">Project: Minimum Margin threashold exceeded</field>
+        <field name="model_id" ref="project.model_project_project" />


Typo: "threashold" should be "threshold" in both the template name and the subject line (line 8).

alexey-pelykh · 2026-03-02T06:46:15Z

project_minimum_margin_threshold_alert/data/cron.xml

+<?xml version="1.0" encoding="utf-8" ?>
+<!-- Copyright 2026 ACSONE SA/NV
+     License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). -->
+<odoo>


Consider wrapping this in <odoo noupdate="1"> so that module updates do not overwrite any user customizations to the cron schedule (interval, active status, etc.).

alexey-pelykh · 2026-03-02T06:46:15Z

project_minimum_margin_threshold_alert/tests/test_project_minimum_margin_alert.py

+            margin_mock.return_value = self.project_margin_items_15
+            self.project.margin_threshold = 0.2
+            self.project._update_is_margin_threshold_exceeded()
+            self.assertTrue(self.project.is_margin_threshold_exceeded)


The method is named test_margin_not_exceeded_15 but the assertion is self.assertTrue(self.project.is_margin_threshold_exceeded) -- the name says "not exceeded" but the test verifies it IS exceeded. Consider renaming to test_margin_exceeded_15 for clarity.

alexey-pelykh · 2026-03-02T06:46:15Z

project_minimum_margin_threshold_alert/models/project_project.py

+# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl).
+from odoo import api, fields, models
+
+from odoo.addons.base.models.res_users import ResUsers


This direct import of ResUsers from odoo.addons.base.models.res_users is unusual in Odoo modules and couples to an internal module path. It is only used as a type annotation for _get_internal_users_for_margin_threshold. Consider removing this import and using standard Odoo recordset patterns instead (the method can just return self.env["res.users"] typed recordsets without the explicit import).

[19.0][ADD] project_budget_threshold_alert : Notify project followers…

d705f8d

… and project manager about budget exceeding based on a set threshold

nihelgabsi-acsone marked this pull request as draft January 22, 2026 08:31

nihelgabsi-acsone changed the title ~~[19.0][ADD] project_budget_threshold_alert : Notify project followers…~~ [19.0][ADD] project_budget_threshold_alert : Notify project followers and project manager about budget exceeding based on a set threshold Jan 22, 2026

nihelgabsi-acsone force-pushed the 19.0-project_budget_threshold_alert-nga branch 2 times, most recently from 050db67 to be238cd Compare January 28, 2026 14:34

nihelgabsi-acsone force-pushed the 19.0-project_budget_threshold_alert-nga branch from be238cd to 5a29fe3 Compare January 28, 2026 15:42

rousseldenis requested changes Jan 28, 2026

View reviewed changes

[19.0][IMP] project_profitability_threshold_alert :

1653c40

- Rename module to be more generic - Add costs vs revenues threshold

nihelgabsi-acsone force-pushed the 19.0-project_budget_threshold_alert-nga branch from 5a29fe3 to 1653c40 Compare January 29, 2026 09:19

[REF] project_profitability_threshold_alert -> project_minimum_margin…

600cc54

…_threshold_alert

rousseldenis force-pushed the 19.0-project_budget_threshold_alert-nga branch from 305f5bc to 600cc54 Compare January 30, 2026 17:53

rousseldenis added 3 commits February 2, 2026 15:24

[IMP] project_minimum_margin_threshold_alert: Several enhancements

4ebe0d5

Add translations Fix the margin computation Fix the 'message_subscribe()' call in tests

[IMP] project_minimum_margin_treshold_alert: Base computations on exp…

8d02e22

…ected margin

[FIX] project_minimum_margin_threshold_alert: Use sudo for ir_config_…

b12a5ca

…parameter

alexey-pelykh reviewed Feb 28, 2026

View reviewed changes

alexey-pelykh reviewed Mar 2, 2026

View reviewed changes

	compute="_compute_is_margin_threshold_exceeded_notfication_sent",
	is_margin_threshold_exceeded_notification_sent = fields.Boolean(

Uh oh!

Conversation

nihelgabsi-acsone commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexey-pelykh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-pelykh left a comment

Choose a reason for hiding this comment

Review: project_minimum_margin_threshold_alert (Draft)

PR Title vs Module Name Mismatch

Critical / High Priority

Moderate Priority

Minor / Suggestions

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nihelgabsi-acsone commented Jan 22, 2026 •

edited

Loading

Review: `project_minimum_margin_threshold_alert` (Draft)