[18.0][MIG] fieldservice_account_analytic #1226

EdgarRetes · 2024-10-15T21:55:03Z

@max3903

max3903 · 2024-10-16T05:07:19Z

/ocabot migration fieldservice_account_analytic

max3903 · 2024-11-15T21:11:27Z

/ocabot migration fieldservice_account_analytic

EdgarRetes · 2024-11-22T22:14:21Z

@brian10048 @max3903

fieldservice_account_analytic/readme/USAGE.md

max3903 · 2024-11-23T20:23:19Z

@EdgarRetes We need to create 3 analytic plans:

FSM Orders
FSM Locations
FSM Routes

and:

Create a new account in the FSM Orders plan for each order
Create a new account in the FSM Locations plan for each location
CReate a new account in the FSM Routes plan for each route

When creating the customer invoice, vendor bill line or timesheet entry, we need to set the corresponding analytic account in each plan.

We also need to activate the following settings/groups:

Technical/Analytic Accounting
Technical/Show Full Accounting Features --> Add a dependency on account_usability

fieldservice_account_analytic/data/analytic_plan.xml

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: field-service-17.0/field-service-17.0-fieldservice_account_analytic Translate-URL: https://translation.odoo-community.org/projects/field-service-17-0/field-service-17-0-fieldservice_account_analytic/

max3903 · 2024-12-06T13:58:18Z

@EdgarRetes

I think all the features around route can be moved to fieldservice and hidden with a group (activated in the settings).

All the analytic features can be moved to fieldservice_account and hidden with the existing group/setting.

EdgarRetes · 2024-12-06T18:42:57Z

@EdgarRetes

I think all the features around route can be moved to fieldservice and hidden with a group (activated in the settings).

All the analytic features can be moved to fieldservice_account and hidden with the existing group/setting.

Do you have any example of a repository that does this?

are-agilebg · 2025-03-10T16:13:04Z

@max3903 since the changes you're proposing are, if I'm not mistaken, refactoring of the actual logic and this PR is stale for a long time, don't you think we can postpone the refactoring to a later PR and prioritize the pure migration of the logic as-is? Also, this module not migrated 'blocks' the migration of fieldservice_isp_account.

Do you think this could be feasible? Thanks

max3903 · 2025-03-10T19:42:37Z

Let me discuss it with @santiagordz and give it higher priority.

are-agilebg · 2025-04-28T09:52:51Z

Let me discuss it with @santiagordz and give it higher priority.

Sorry if I bother you: have you had a chance to discuss it?

max3903 · 2025-04-28T17:31:16Z

Sorry if I bother you: have you had a chance to discuss it?

Nope. @SMaciasOSI Can you get this in the todo list of @dcamacho20?

are-agilebg · 2025-05-22T11:21:52Z

Sorry if I bother you: have you had a chance to discuss it?

Nope. @SMaciasOSI Can you get this in the todo list of @dcamacho20?

Thanks for looking into it! We'll probably start anyway to migrate fieldservice_isp_account internally to 18.0 using this open PR as a dependency.

vvrossem · 2025-08-19T11:34:00Z

Hello 👋
I'm migrating fieldservice_isp_account (#1442) but several unit tests are failing because of required fields introduced in fieldservice_account_analytic.
Example: fieldservice_crm (which doesn't depend on fieldservice_account_analytic) fails to create a fsm.location because the required field customer_id is not present.

@EdgarRetes would you mind rebasing this PR? I suspect the CI will fail too: some modules have been merged meanwhile.

ivantodorovich

Hello 👋🏻 !

I just did a review commit by commit.
Some of my comments may be outdated, as the code is overwritten by following commits. However, the content is still valid in most cases.

Can you clean up a bit the commits, please?
I believe the changes are not atomic and well organized. If not possible, then it's better to squash everything into the migration commit and write a nice message describing the main changes.

Some things are blocking for me, like the addition of the new dependency, and the new init_hook.

ivantodorovich · 2025-08-19T17:03:48Z

fieldservice_account_analytic/models/analytic_account.py

+                    vals["account_id"] = order.location_id.analytic_account_id.id
+                else:
+                    raise ValidationError(
+                        _("No analytic account set " "on the order's Location.")


Please use self.env._

ivantodorovich · 2025-08-19T17:05:19Z

fieldservice_account_analytic/models/analytic_account.py

-        return super().create(vals)
+    @api.model_create_multi
+    def create(self, vals_list):
+        for vals in vals_list:


I know it was like this before, but please lets only loop if vals.get("fsm_order_id"), as it makes no sense to get into this iteration otherwise

ivantodorovich · 2025-08-19T17:06:41Z

fieldservice_account_analytic/models/analytic_account.py

+
+
+class AnalyticAccount(models.Model):
+    _inherit = "account.analytic.account"


Can we split this into two files: account_analytic_line.py and account_analytic_account.py ?

ivantodorovich · 2025-08-19T17:06:54Z

fieldservice_account_analytic/models/analytic_account.py

+    _inherit = "account.analytic.account"
+
+    fsm_order_id = fields.One2many("fsm.order", "analytic_account_id", copy=False)
+    # route_id = fields.One2many("tms.route", "analytic_account_id", copy=False)


What's with this commented out line?

NOTE: This line is overwriten in a following commit.
Still: clearly commits are not atomic and require some cleaning

ivantodorovich · 2025-08-19T17:09:13Z

fieldservice_account_analytic/models/fsm_order.py

+            {
+                "name": vals.get("name"),
+                "plan_id": self.env.ref(
+                    "fieldservice_account_analytic.fsm_order_analytic_plan"
+                ).id,
+                "fsm_order_id": record,
+            }


Please move the vals preparation to a separate method. This makes it possible to easily override in other modules if needed. e.g: _prepare_analytic_account_vals

ivantodorovich · 2025-08-19T17:13:16Z

fieldservice_account_analytic/models/fsm_order.py

+                "plan_id": self.env.ref(
+                    "fieldservice_account_analytic.fsm_order_analytic_plan"
+                ).id,
+                "fsm_order_id": record,


set also the company_id and partner_id

ivantodorovich · 2025-08-19T17:14:23Z

fieldservice_account_analytic/models/fsm_location.py

+    @api.model
+    def create(self, vals):
+        record = super().create(vals)
+        analytic_account = self.env["account.analytic.account"].create(


Same comments than the fsm.order model:

use model_create_multi

separate method for vals preparation

create only if not explicitly set in vals

ivantodorovich · 2025-08-19T17:15:06Z

fieldservice_account_analytic/models/fsm_location.py

 # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).

-from odoo import api, fields, models
+from odoo import _, api, fields, models


Use self.env._ instead, everywhere

ivantodorovich · 2025-08-19T17:16:08Z

fieldservice_account_analytic/__manifest__.py

        "analytic",
        "product",
        "fieldservice_route",
+        "account_usability",


What's with this new dependency?

ivantodorovich · 2025-08-19T17:17:51Z

fieldservice_account_analytic/init_hook.py

+        users = env["res.users"].search([])
+
+        for user in users:
+            user.write({"groups_id": [(4, group_analytic_accounting.id)]})


This is not correct.

You can't just give this group to every user in the db

ivantodorovich · 2025-08-21T17:23:38Z

fieldservice_account_analytic/data/ir_rule.xml

+        <field name="name">account.analytic.line.fsm.manager</field>
+        <field name="model_id" ref="model_account_analytic_line" />
+        <field name="global" eval="False" />
+        <field name="domain_force">[(1, '=', 1)]</field>


This is coming from previous versions, but it's not correct.
It grants too much power.

It should only grant access to lines having a fsm_order_id, IMO:

Suggested change

<field name="domain_force">[(1, '=', 1)]</field>

<field name="domain_force">[('fsm_order_id', '!=', False)]</field>

ivantodorovich · 2025-08-21T17:26:23Z

fieldservice_account_analytic/data/ir_rule.xml

+        <field name="domain_force">[(1, '=', 1)]</field>
+        <field name="groups" eval="[(4, ref('fieldservice.group_fsm_dispatcher'))]" />
+        <field name="perm_read" eval="True" />
+        <field name="perm_write" eval="True" />
+        <field name="perm_create" eval="True" />
+        <field name="perm_unlink" eval="True" />


Same comment as before, but it's also giving too much power.
The dispatcher should probably only see them, but I'm not sure if it should modify them 🤔
TBD

Suggested change

<field name="domain_force">[(1, '=', 1)]</field>

<field name="groups" eval="[(4, ref('fieldservice.group_fsm_dispatcher'))]" />

<field name="perm_read" eval="True" />

<field name="perm_write" eval="True" />

<field name="perm_create" eval="True" />

<field name="perm_unlink" eval="True" />

<field name="domain_force">[('fsm_order_id', '!=', False)]</field>

<field name="groups" eval="[(4, ref('fieldservice.group_fsm_dispatcher'))]" />

<field name="perm_read" eval="True" />

<field name="perm_write" eval="False" />

<field name="perm_create" eval="False" />

<field name="perm_unlink" eval="False" />

ivantodorovich · 2025-08-21T17:30:59Z

fieldservice_account_analytic/data/ir_rule.xml

+        <field name="perm_create" eval="True" />
+        <field name="perm_unlink" eval="True" />
+    </record>
+</odoo>


Moreover, the user should be able to see and record his own timesheets

Suggested change

</odoo>

<record id="analytic_account_fsm_user" model="ir.rule">

<field name="name">account.analytic.line.fsm.user</field>

<field name="model_id" ref="model_account_analytic_line" />

<field name="domain_force">[('fsm_order_id', '!=', False), ('user_id', '=', user.id)]</field>

<field name="groups" eval="[Command.link(ref('fieldservice.group_fsm_user'))]" />

</record>

</odoo>

ivantodorovich · 2025-08-21T17:31:31Z

fieldservice_account_analytic/data/ir_rule.xml

This file should be moved to security/

ivantodorovich · 2025-08-21T17:37:51Z

fieldservice_account_analytic/models/account_move.py

+    @api.model_create_multi
+    def create(self, vals_list):
+        for vals in vals_list:
+            if vals.get("fsm_order_ids") and len(vals.get("fsm_order_ids")[0]) > 2:
+                fsm_orders = vals.get("fsm_order_ids")[0][2]
+                for order in self.env["fsm.order"].browse(fsm_orders).exists():
+                    if order.location_id.analytic_account_id:
+                        vals["analytic_distribution"] = {
+                            order.location_id.analytic_account_id.id: 100
+                        }
+                    else:
+                        raise ValidationError(
+                            _("No analytic account " "set on the order's Location.")
+                        )
+        return super().create(vals_list)


This should be done in an override of _compute_analytic_distribution, instead of a create override.

ivantodorovich · 2025-08-21T17:44:03Z

fieldservice_account_analytic/models/analytic_account.py

+    @api.onchange("product_id")
+    def onchange_product_id(self):
+        self.name = self.product_id.name if self.product_id else False


This is too invasive for other modules adding their own analytic lines.

I'm not sure we want this, anyway.

My recommendation would be to remove it.
And if we really want it, it shuold be limited only on those analytic.lines that have a fsm_order_id

Suggested change

@api.onchange("product_id")

def onchange_product_id(self):

self.name = self.product_id.name if self.product_id else False

ivantodorovich · 2025-08-21T17:45:57Z

fieldservice_account_analytic/models/fsm_location.py

+    @api.model_create_multi
+    def create(self, vals_list):
+        record = super().create(vals_list)
+        if self.env.user.has_group("analytic.group_analytic_accounting"):


It shouldn't matter if the user creating the FSM order has access to accounting or not.
The behavior should be the same.

Moreover, the account should be created only if it's not already provided in the vals. Otherwise we'd be overwriting it.

ivantodorovich · 2025-08-21T17:47:27Z

fieldservice_account_analytic/models/fsm_location.py

+            for vals in vals_list:
+                analytic_account = self.env["account.analytic.account"].create(
+                    {
+                        "name": vals.get("name"),
+                        "plan_id": self.env.ref(
+                            "fieldservice_account_analytic.fsm_location_analytic_plan"
+                        ).id,
+                        "fsm_location_id": record,
+                    }
+                )
+                record.analytic_account_id = analytic_account


This for..loop is totally wrong.

It's looping through all the vals in vals_list, but for each iteration it's assigning the created analytic account to record (which in fact should be spelled records [plural]).

So, all the created records will end up with the same analytic account --> the one created for the last of them

max3903 added this to the 18.0 milestone Oct 16, 2024

max3903 added needs review migration labels Oct 16, 2024

max3903 self-assigned this Oct 16, 2024

OCA-git-bot mentioned this pull request Oct 16, 2024

Migration to version 18.0 #1220

Open

33 tasks

EdgarRetes force-pushed the 18.0-mig-fieldservice_account_analytic branch from d3e562e to d48f879 Compare November 5, 2024 20:16

max3903 changed the title ~~[18.0][MIG]fieldservice_account_analytic~~ [18.0][MIG] fieldservice_account_analytic Nov 15, 2024

EdgarRetes force-pushed the 18.0-mig-fieldservice_account_analytic branch 5 times, most recently from 6a59d33 to 50d20bf Compare November 22, 2024 22:11

EdgarRetes force-pushed the 18.0-mig-fieldservice_account_analytic branch from 50d20bf to 4a17d52 Compare November 22, 2024 22:42

max3903 reviewed Nov 23, 2024

View reviewed changes

fieldservice_account_analytic/readme/USAGE.md Outdated Show resolved Hide resolved

EdgarRetes force-pushed the 18.0-mig-fieldservice_account_analytic branch 2 times, most recently from f0ef587 to 062bb59 Compare November 27, 2024 20:19

max3903 reviewed Nov 27, 2024

View reviewed changes

fieldservice_account_analytic/data/analytic_plan.xml Outdated Show resolved Hide resolved

max3903 reviewed Nov 27, 2024

View reviewed changes

fieldservice_account_analytic/data/analytic_plan.xml Outdated Show resolved Hide resolved

EdgarRetes force-pushed the 18.0-mig-fieldservice_account_analytic branch 8 times, most recently from 52ad51f to 40d4df5 Compare November 29, 2024 19:51

OCA-git-bot and others added 11 commits December 2, 2024 13:07

[BOT] post-merge updates

323dede

[MIG] fieldservice_account_analytic: Migration to 18.0

af87aea

[ADD] fieldservise_account_analytic: analytic plans

a7c9411

[ADD] fieldservice_account_analytic: Location analytic account

a07c116

[ADD] fieldservice_account_analytic: Route analytic plan

0b4d9cb

[ADD] fieldservice_account_analytic: Smart buttons

6289872

[ADD] fieldservice_account_analytic: account_usability dependency

886398a

Update fieldservice_account_analytic/readme/USAGE.md

ec1e779

Update fsm_order.xml

9017205

Update fieldservice_account_analytic/views/fsm_route.xml

ec44a96

EdgarRetes force-pushed the 18.0-mig-fieldservice_account_analytic branch from e7f8bb5 to 849fdc4 Compare December 2, 2024 19:08

Update fieldservice_account_analytic/models/fsm_order.py

5c93b75

EdgarRetes force-pushed the 18.0-mig-fieldservice_account_analytic branch from 849fdc4 to 5c93b75 Compare December 2, 2024 20:19

EdgarRetes requested a review from max3903 December 2, 2024 20:22

EdgarRetes added 2 commits December 4, 2024 12:36

[ADD] fieldservice_account_analytic: Contractor costs

3dcd910

[ADD] fieldservice_account_analytic: Add group when installed

b7ce772

EdgarRetes force-pushed the 18.0-mig-fieldservice_account_analytic branch from 4181c76 to b7ce772 Compare December 5, 2024 17:05

vvrossem mentioned this pull request Aug 19, 2025

[18.0][MIG] fieldservice_isp_account: Migration to 18.0 #1442

Draft

ivantodorovich requested changes Aug 19, 2025

View reviewed changes

ivantodorovich added the needs fixing label Aug 19, 2025

ivantodorovich reviewed Aug 21, 2025

View reviewed changes



		class AnalyticAccount(models.Model):
		_inherit = "account.analytic.account"

	<field name="domain_force">[(1, '=', 1)]</field>
	<field name="domain_force">[('fsm_order_id', '!=', False)]</field>

-</odoo>
+    <record id="analytic_account_fsm_user" model="ir.rule">
+        <field name="name">account.analytic.line.fsm.user</field>
+        <field name="model_id" ref="model_account_analytic_line" />
+        <field name="domain_force">[('fsm_order_id', '!=', False), ('user_id', '=', user.id)]</field>
+        <field name="groups" eval="[Command.link(ref('fieldservice.group_fsm_user'))]" />
+    </record>
+</odoo>

	@api.onchange("product_id")
	def onchange_product_id(self):
	self.name = self.product_id.name if self.product_id else False

[18.0][MIG] fieldservice_account_analytic #1226

Are you sure you want to change the base?

[18.0][MIG] fieldservice_account_analytic #1226

Uh oh!

Conversation

EdgarRetes commented Oct 15, 2024

Uh oh!

max3903 commented Oct 16, 2024

Uh oh!

max3903 commented Nov 15, 2024

Uh oh!

EdgarRetes commented Nov 22, 2024

Uh oh!

Uh oh!

max3903 commented Nov 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

max3903 commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EdgarRetes commented Dec 6, 2024

Uh oh!

are-agilebg commented Mar 10, 2025

Uh oh!

max3903 commented Mar 10, 2025

Uh oh!

are-agilebg commented Apr 28, 2025

Uh oh!

max3903 commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

are-agilebg commented May 22, 2025

Uh oh!

vvrossem commented Aug 19, 2025

Uh oh!

ivantodorovich left a comment

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!

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!

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

max3903 commented Nov 23, 2024 •

edited

Loading

max3903 commented Dec 6, 2024 •

edited

Loading

max3903 commented Apr 28, 2025 •

edited

Loading