[18.0][MIG] project_wbs: Migration to 18.0#1690
Conversation
the css was not migrated
enable opening the related view even when empty (needed for quick project creation)
[IMP]test_coverage
Currently translated at 1.8% (1 of 55 strings) Translation: project-12.0/project-12.0-project_wbs Translate-URL: https://translation.odoo-community.org/projects/project-12-0/project-12-0-project_wbs/de/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: project-12.0/project-12.0-project_wbs Translate-URL: https://translation.odoo-community.org/projects/project-12-0/project-12-0-project_wbs/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: project-15.0/project-15.0-project_wbs Translate-URL: https://translation.odoo-community.org/projects/project-15-0/project-15-0-project_wbs/
Currently translated at 100.0% (53 of 53 strings) Translation: project-15.0/project-15.0-project_wbs Translate-URL: https://translation.odoo-community.org/projects/project-15-0/project-15-0-project_wbs/it/
hbrunn
left a comment
There was a problem hiding this comment.
this needs much more work, amongst others compatibility with project_parent, lack of which is part of why tests fail left and right here
5c7d053 to
de7b1ab
Compare
58fdc87 to
e29d1be
Compare
e29d1be to
1f4b048
Compare
1f4b048 to
1293d87
Compare
|
Done @AaronHForgeFlow ! Agreed, that makes sense as the next step. |
|
Hi @hbrunn and @AaronHForgeFlow , can you review please? |
AaronHForgeFlow
left a comment
There was a problem hiding this comment.
LGTM Functional.
Also I am no so sure we should make it depend on project_parent. This module relies on the analytic_account hierarchy not the project hierarchy. Moving all the features from the analytic account to the project is a dramatic change that affects all the features in the module and all the module that relies on this one, and requires complex migration scripts, for so little benefit.
hbrunn
left a comment
There was a problem hiding this comment.
Also I am no so sure we should make it depend on project_parent. This module relies on the analytic_account hierarchy not the project hierarchy. Moving all the features from the analytic account to the project is a dramatic change that affects all the features in the module and all the module that relies on this one, and requires complex migration scripts, for so little benefit.
nobody has asked for this. But not breaking it is mandatory.
Out of scope for the migration, but I'd like to see this module gate its functionality with a type field/feature flag on accounts and projects, so that you can have analytic accounts and projects that don't have anything to do with WBS
| @api.model | ||
| def _default_parent(self): | ||
| return self.env.context.get("parent_id", None) | ||
|
|
||
| @api.model | ||
| def _default_partner(self): | ||
| return self.env.context.get("partner_id", None) |
There was a problem hiding this comment.
please remove those and change context keys to default_* where appropriate
| _inherit = "account.analytic.account" | ||
| _order = "complete_wbs_code" | ||
|
|
||
| def get_child_accounts(self): |
There was a problem hiding this comment.
are you sure this is faster than a modern child_of search?
| account._complete_wbs_code_calc() | ||
| account._complete_wbs_name_calc() | ||
| if "active" in vals: | ||
| for account in self: |
There was a problem hiding this comment.
| for account in self: | |
| for account in self.with_context(active_test=False): |
otherwise you won't find nonactive projects
| parent_id = fields.Many2one( | ||
| default=_default_parent, string="Parent Analytic Account" | ||
| ) | ||
| partner_id = fields.Many2one(default=_default_partner) |
| default = {} | ||
| default["code"] = self.env["ir.sequence"].next_by_code( | ||
| "account.analytic.account.code" | ||
| ) |
There was a problem hiding this comment.
why do we need this? the default should be provided by account_analytic_sequence already.
in any case, you overwrite existing defaults here which is a no-no
| "within the project WBS hierarchy", | ||
| store=True, | ||
| ) | ||
| code = fields.Char(copy=False) |
There was a problem hiding this comment.
remove, account_analytic_sequence already does this
| def copy(self, default=None): | ||
| if self.mapped("project_ids"): | ||
| raise ValidationError( | ||
| _("Duplicate the project instead of the " "Analytic Account") |
There was a problem hiding this comment.
join the string. but I don't think I agree with making this a hard failure anyways
| return res | ||
|
|
||
| @api.depends("name") | ||
| def name_get(self): |
There was a problem hiding this comment.
name_get isn't a thing any more, see https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-17.0
code_get should be integrated here as it's not used anywhere else
| "UNIQUE (complete_wbs_code)", | ||
| "The full wbs code must be unique!", | ||
| ), | ||
| ] |
There was a problem hiding this comment.
I stopped reviewing here, before asking for another review I invite you to adhere to all migration notes for all versions mentioned in https://github.com/OCA/maintainer-tools/wiki#migration, including reordering fields and functions, assigning compute functions by name and not by reference, and having a critical look on all field definitions if you don't redefine things that are already done elsewhere


Module migrated to version 18.0
cc https://github.com/APSL
@miquelalzanillas @javierobcn @mpascuall @BernatObrador @ppyczko @peluko00 please review