[FIX] project : prevent mismatch with analytic account and their plan #190746

damrOdoo · 2024-12-16T16:22:07Z

Currently, when the plan of an analytic account is updated, there is no sanity check, nor any computation done on the project accounts. This leads to account being linked to the wrong plan in the project setting, as well as strange computation result in the project profitability.

step to reproduce :

open project setting
go to the analytic folder
If there are no analytic account on the project plan, add a new one
go to the form view of the analytic account
change the plan from project to department (or any other plan)
go back to the project setting -> analytic

=> the analytic account is still set on the 'project plan' field of the project.

Solution : adds a warning message for the user when he tries to update the plan of analytic account from the form or the list view of analytic account. If the user confirms his update, the account are removed from their associated project. An sql constraint was also added in case some use case update the analytic plan without making use of the form view or the list view.

task - 4334520
affected version : 18.0 - master

robodoo · 2024-12-16T16:22:10Z

deneuvillem

Thanks for your good work @damrOdoo
I left comments and questions for you 😄 Do not hesitate to reach me out if something is unclear!
Also, don't forget to add all the modules impacted in the commit title :)

deneuvillem · 2025-01-07T12:41:59Z

addons/project/models/account_analytic_account.py

+    @api.constrains('plan_id')
+    def _ensure_project_plan_consistency(self):
+        domain = expression.OR([
+            [(fnames, 'in', self.ids)] for fnames in self.env['project.project']._get_plan_fnames()


Suggested change

[(fnames, 'in', self.ids)] for fnames in self.env['project.project']._get_plan_fnames()

[(fname, 'in', self.ids)] for fname in self.env['project.project']._get_plan_fnames()

deneuvillem · 2025-01-07T13:41:34Z

addons/project/models/project_project.py

@@ -475,6 +475,23 @@ def name_create(self, name):
    def create(self, vals_list):
        # Prevent double project creation
        self = self.with_context(mail_create_nosubscribe=True)
+        # check that the analytic account are set on the correct plan.


Instead of making the check in the create() and write(), we could do it once for all in this constraint: https://github.com/odoo/odoo/blob/18.0/addons/project/models/project_project.py#L963

We would iterate over each project and their accounts and check whether the accounts's plans are the same as the plans they should belong to.

Also, I suggest to use the root_plan_id field (instead of plan_id) to make comparisons, as we only allowed selecting AAs belonging to root plans in the different views.

We could also make the check generic and implement it in 'analytic.plan.fields.mixin' directly but not sure tho.

deneuvillem · 2025-01-07T13:46:47Z

addons/project/models/project_project.py

+                    account_record = self.env['account.analytic.account'].browse(account_id)
+                    account_per_account_id[account_id] = account_record
+                if account_record.plan_id._column_name() != plan_column_id:
+                    raise UserError(f"The account '{account_record.name}' is linked to the plan '{account_record.plan_id.name}'. "


We may want to translate the error message.

deneuvillem · 2025-01-07T13:49:09Z

addons/hr_timesheet/tests/test_timesheet.py

-        cls.analytic_plan = cls.env['account.analytic.plan'].create({
-            'name': 'Timesheet Plan Test',
-        })
+        cls.project_plan = cls.env['account.analytic.plan'].browse(int(cls.env['ir.config_parameter'].get_param('analytic.project_plan', 0)))


Suggested change

cls.project_plan = cls.env['account.analytic.plan'].browse(int(cls.env['ir.config_parameter'].get_param('analytic.project_plan', 0)))

cls.project_plan, _other_plans = cls.env['account.analytic.plan']._get_all_plans()

I feel like it's more convenient to do it this way, even if we don't use _other_plans

I agree its a bit more readable, but the browse is literally the first line executed by the get_all_plan function. We might as well do it ourselves and save us the following search of the function.

Well okay but maybe we could keep my suggestion and use cls.other_plans in test_mandatory_plan_timesheet_applicability, so we avoid creating a new plan for nothing :)

deneuvillem · 2025-01-07T14:13:38Z

addons/project/models/account_analytic_account.py

+        ])
+        project = self.env['project.project'].search(domain, limit=1)
+        if project:
+            raise ValidationError(_("At least one of the account is linked to a project. Unlink it from its project(s) before updating its plan."))


It would be nice for the user to see which projects (names) are impacted by the change of plans.

deneuvillem · 2025-01-07T16:36:52Z

addons/project_sale_expense/tests/test_project_sale_expense.py

-        project.account_id = self.analytic_account_1
+        project_plan = self.env['account.analytic.plan'].browse(int(self.env['ir.config_parameter'].get_param('analytic.project_plan', 0)))
+
+        project.account_id= self.env['account.analytic.account'].create({


Suggested change

project.account_id= self.env['account.analytic.account'].create({

project.account_id = self.env['account.analytic.account'].create({

deneuvillem · 2025-01-07T16:37:33Z

addons/sale_project/tests/test_sale_project.py

@@ -16,9 +16,10 @@ def setUpClass(cls):
        cls.analytic_plan = cls.env['account.analytic.plan'].create({
            'name': 'Plan Test',
        })
+        cls.project_plan = cls.env['account.analytic.plan'].browse(int(cls.env['ir.config_parameter'].get_param('analytic.project_plan', 0)))


Suggested change

cls.project_plan = cls.env['account.analytic.plan'].browse(int(cls.env['ir.config_parameter'].get_param('analytic.project_plan', 0)))

cls.project_plan, _other_plans = cls.env['account.analytic.plan']._get_all_plans()

deneuvillem · 2025-01-07T16:39:39Z

addons/project/tests/test_analytic_account_consistency.py

+            'name': 'account_2', 'company_id': self.env.company.id, 'plan_id': analytic_plan.id
+        }])
+
+        with self.assertRaises(UserError):


We can add a message to the assertion to make it clearer why it should fail

deneuvillem · 2025-01-07T16:42:01Z

addons/project/tests/test_analytic_account_consistency.py

+@tagged('-at_install', 'post_install')
+class TestAnalyticAccountConsistency(TestProjectCommon):
+
+    def test_account_consistency_orm_method(self):


Suggested change

def test_account_consistency_orm_method(self):

def test_account_plan_with_project_consistency(self):

deneuvillem · 2025-01-07T16:43:43Z

addons/project/tests/test_analytic_account_consistency.py

+        analytic_plan = self.env['account.analytic.plan'].create({'name': 'secondary plan'})
+        project_plan = self.env['account.analytic.plan'].browse(int(self.env['ir.config_parameter'].get_param('analytic.project_plan', 0)))
+        account_project_plan, account_second_plan = self.env['account.analytic.account'].create([{
+            'name': 'account_1', 'company_id': self.env.company.id, 'plan_id': project_plan.id
+        }, {
+            'name': 'account_2', 'company_id': self.env.company.id, 'plan_id': analytic_plan.id
+        }])


Let's move these in the setUpClass()

deneuvillem

Thanks for the changes @damrOdoo 💯
Second batch of comments :)

deneuvillem · 2025-01-13T09:41:00Z

addons/project/models/account_analytic_account.py

+            raise ValidationError(_(("At least one of the accounts is linked to the following projects : %s. "
+                                    "Unlink it from its project(s) before updating its plan."),  ', '.join(projects.mapped('name'))))


Suggested change

raise ValidationError(_(("At least one of the accounts is linked to the following projects : %s. "

"Unlink it from its project(s) before updating its plan."), ', '.join(projects.mapped('name'))))

raise ValidationError(_((

"At least one of the accounts is linked to the following projects : %(projects_names)s. "

"Unlink it from its project(s) before updating its plan."),

projects_names=format_list(self.env, projects.mapped('name')),

))

I think format_list is the new standard for that kind of stuff

deneuvillem · 2025-01-13T09:44:04Z

addons/project/models/account_analytic_account.py

+        domain = expression.OR([
+            [(fname, 'in', self.ids)] for fname in self.env['project.project']._get_plan_fnames()
+        ])


Suggested change

domain = expression.OR([

[(fname, 'in', self.ids)] for fname in self.env['project.project']._get_plan_fnames()

])

domain = [('auto_account_id', 'in', self.ids)]

One remark, if we change the plan of an account linked to a project, but the new plan belongs to the same root plan (it's a sub plan of that plan), I don't think we should trigger any Error as it's still consistent.

Let's take an example:

We have one root plan 'Project' and two sub plans 'Sub Project 1' and 'Sub Project 2'

We have one project with an account 'Account A' defined on the plan 'Project/Sub Project 1'

=> We want to change the plan of the account 'Account A' to 'Sub Project 2'
=> IMO, that should not trigger any error because they belong to the same root plan

(if we keep that, let's add a test)

I wasn't aware of the sub plans feature. I agree with your suggestion on this point. I'll made the change & tests.

About the auto_account_id field however, i dont think we can use it in this case. We're using it for a search on project, and since we only have roots plan, if the account is linked to a sub-plan, then the search will trigger an error, since we'll be searching on a non-existent field on projects. (if i correctly understood the how auto_account_id field works)

It will work because we accept accounts from sub plans in the domain of analytic plan fields of project.project.
Example: the field account_id of project.project can contain accounts from the 'Project' plan but also accounts from sub plans of 'Project' :)

deneuvillem · 2025-01-13T09:45:56Z

addons/project/models/project_project.py

@@ -475,6 +475,7 @@ def name_create(self, name):
    def create(self, vals_list):
        # Prevent double project creation
        self = self.with_context(mail_create_nosubscribe=True)
+


Useless diff 🤷‍♂️

deneuvillem · 2025-01-13T09:46:25Z

addons/project/models/project_project.py

+        fnames = self._get_plan_fnames()
+        domain = OR([[(fname, 'in', account_ids)] for fname in fnames])
+        projects = self.search(domain)


Let's search on the auto_account_id field instead 😄

deneuvillem · 2025-01-13T09:47:02Z

addons/project/models/project_project.py

+        fnames = self._get_plan_fnames()
+        domain = OR([[(fname, 'in', account_ids)] for fname in fnames])
+        projects = self.search(domain)


deneuvillem · 2025-01-13T09:51:32Z

addons/project/models/project_project.py

+        """
+        :param account_ids: the account selected on the list view/ the active account of the form view
+        :return: projects_name_per_account: a dict with 2 keys (account_id, other_plan) which contains the names of
+        the project on which one of the account are set. This separation is done in order to display an accurate


Suggested change

the project on which one of the account are set. This separation is done in order to display an accurate

the project on which one of the accounts is set. This separation is done in order to display an accurate

deneuvillem · 2025-01-13T09:58:27Z

addons/project/models/project_project.py

+                account = project[plan_column_id]
+                if not account:
+                    continue
+                if account.plan_id._column_name() != plan_column_id:


Suggested change

if account.plan_id._column_name() != plan_column_id:

if account.root_plan_id._column_name() != plan_column_id:

We want to compare the root plans, as _get_plan_fnames() only returns root plans.
(There exists sub plans but we only allowed to link AAs to root plans in the project views)

deneuvillem · 2025-01-13T09:59:56Z

addons/project/models/project_project.py

+                if not account:
+                    continue
+                if account.plan_id._column_name() != plan_column_id:
+                    raise UserError(_('The account %(account_name)s is linked to the plan %(plan_name)s. You are not allowed to set it on other plan on your projects.',


Suggested change

raise UserError(_('The account %(account_name)s is linked to the plan %(plan_name)s. You are not allowed to set it on other plan on your projects.',

raise UserError(_('The account %(account_name)s is linked to the root plan %(plan_name)s. You are not allowed to set it on other root plan on your projects.',

deneuvillem · 2025-01-13T10:12:53Z

addons/project/models/project_project.py

@@ -971,7 +1006,16 @@ def _get_projects_to_make_billable_domain(self):
    @api.constrains(lambda self: self._get_plan_fnames())
    def _check_account_id(self):
        # Overriden from 'analytic.plan.fields.mixin'
-        pass
+        # check that the analytic account are set on the correct plan.
+        plan_column_ids = self._get_plan_fnames()


Suggested change

plan_column_ids = self._get_plan_fnames()

plan_column_names = self._get_plan_fnames()

deneuvillem · 2025-01-13T10:33:41Z

addons/hr_timesheet/tests/test_timesheet.py

-        cls.analytic_plan = cls.env['account.analytic.plan'].create({
-            'name': 'Timesheet Plan Test',
-        })
+        cls.project_plan = cls.env['account.analytic.plan'].browse(int(cls.env['ir.config_parameter'].get_param('analytic.project_plan', 0)))


Well okay but maybe we could keep my suggestion and use cls.other_plans in test_mandatory_plan_timesheet_applicability, so we avoid creating a new plan for nothing :)

Currently, when the plan of an analytic account is updated, there is no sanity check, nor any computation done on the project accounts. This leads to account being linked to the wrong plan in the project setting, as well as strange computation result in the project profitability. step to reproduce : - open project setting - go to the analytic folder - If there are no analytic account on the project plan, add a new one - go to the form view of the analytic account - change the plan from project to department (or any other plan) - go back to the project setting -> analytic => the analytic account is still set on the 'project plan' field of the project. Solution : adds a warning message for the user when he tries to update the plan of analytic account from the form or the list view of analytic account. If the user confirms his update, the account are removed from their associated project. An sql constraint was also added in case some use case update the analytic plan without making use of the form view or the list view. task - 4334520 affected version : 18.0 - master

C3POdoo added the RD research & development, internal work label Dec 16, 2024

damrOdoo force-pushed the 18.0-fix-analytic-plan-consistency-with-project-damr branch 2 times, most recently from bace06c to e8df0e1 Compare December 23, 2024 12:52

C3POdoo requested review from a team and malb-odoo and removed request for a team December 23, 2024 12:55

damrOdoo force-pushed the 18.0-fix-analytic-plan-consistency-with-project-damr branch 2 times, most recently from e8287cf to a9a5569 Compare December 23, 2024 17:10

C3POdoo requested review from a team and svs-odoo and removed request for a team December 23, 2024 17:13

damrOdoo force-pushed the 18.0-fix-analytic-plan-consistency-with-project-damr branch 2 times, most recently from 9c6764e to 7e928ca Compare December 27, 2024 09:35

damrOdoo force-pushed the 18.0-fix-analytic-plan-consistency-with-project-damr branch from 7e928ca to ff2fc97 Compare January 3, 2025 09:30

deneuvillem reviewed Jan 7, 2025

View reviewed changes

damrOdoo force-pushed the 18.0-fix-analytic-plan-consistency-with-project-damr branch 8 times, most recently from 6f40be4 to 9297270 Compare January 10, 2025 14:18

deneuvillem reviewed Jan 13, 2025

View reviewed changes

malb-odoo removed their request for review January 15, 2025 08:26

damrOdoo force-pushed the 18.0-fix-analytic-plan-consistency-with-project-damr branch 2 times, most recently from 99a9055 to dfb244a Compare January 17, 2025 12:16

damrOdoo force-pushed the 18.0-fix-analytic-plan-consistency-with-project-damr branch from dfb244a to 8c67351 Compare January 17, 2025 13:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] project : prevent mismatch with analytic account and their plan #190746

[FIX] project : prevent mismatch with analytic account and their plan #190746

damrOdoo commented Dec 16, 2024

robodoo commented Dec 16, 2024

deneuvillem left a comment •

edited

Loading

deneuvillem Jan 7, 2025

deneuvillem Jan 7, 2025

deneuvillem Jan 7, 2025

deneuvillem Jan 7, 2025

damrOdoo Jan 10, 2025

deneuvillem Jan 13, 2025

deneuvillem Jan 7, 2025

deneuvillem Jan 7, 2025

deneuvillem Jan 7, 2025

deneuvillem Jan 7, 2025

deneuvillem Jan 7, 2025

deneuvillem Jan 7, 2025

deneuvillem left a comment

deneuvillem Jan 13, 2025

deneuvillem Jan 13, 2025 •

edited

Loading

deneuvillem Jan 13, 2025

damrOdoo Jan 16, 2025

deneuvillem Jan 16, 2025

deneuvillem Jan 13, 2025

deneuvillem Jan 13, 2025

deneuvillem Jan 13, 2025

deneuvillem Jan 13, 2025

deneuvillem Jan 13, 2025

deneuvillem Jan 13, 2025

deneuvillem Jan 13, 2025

deneuvillem Jan 13, 2025

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

	[(fnames, 'in', self.ids)] for fnames in self.env['project.project']._get_plan_fnames()
	[(fname, 'in', self.ids)] for fname in self.env['project.project']._get_plan_fnames()

	cls.project_plan = cls.env['account.analytic.plan'].browse(int(cls.env['ir.config_parameter'].get_param('analytic.project_plan', 0)))
	cls.project_plan, _other_plans = cls.env['account.analytic.plan']._get_all_plans()

	project.account_id= self.env['account.analytic.account'].create({
	project.account_id = self.env['account.analytic.account'].create({

	def test_account_consistency_orm_method(self):
	def test_account_plan_with_project_consistency(self):

		raise ValidationError(_(("At least one of the accounts is linked to the following projects : %s. "
		"Unlink it from its project(s) before updating its plan."), ', '.join(projects.mapped('name'))))

	the project on which one of the account are set. This separation is done in order to display an accurate
	the project on which one of the accounts is set. This separation is done in order to display an accurate

	if account.plan_id._column_name() != plan_column_id:
	if account.root_plan_id._column_name() != plan_column_id:

	raise UserError(_('The account %(account_name)s is linked to the plan %(plan_name)s. You are not allowed to set it on other plan on your projects.',
	raise UserError(_('The account %(account_name)s is linked to the root plan %(plan_name)s. You are not allowed to set it on other root plan on your projects.',

	plan_column_ids = self._get_plan_fnames()
	plan_column_names = self._get_plan_fnames()

[FIX] project : prevent mismatch with analytic account and their plan #190746

Are you sure you want to change the base?

[FIX] project : prevent mismatch with analytic account and their plan #190746

Conversation

damrOdoo commented Dec 16, 2024

robodoo commented Dec 16, 2024

deneuvillem left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deneuvillem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deneuvillem Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deneuvillem left a comment •

edited

Loading

deneuvillem Jan 13, 2025 •

edited

Loading