Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[16.0][MIG] account_asset_management: Migration to 16.0 #1479

Merged
merged 189 commits into from
Jan 6, 2023

Conversation

RodrigoBM
Copy link
Contributor

@RodrigoBM RodrigoBM commented Oct 17, 2022

@RodrigoBM RodrigoBM force-pushed the 16.0-mig-account_asset_management branch 2 times, most recently from cd24bae to c2ec97d Compare October 17, 2022 17:18
@RodrigoBM
Copy link
Contributor Author

pre-commit fails for double duplicate field, but I need this one in V16 for groups="base.group_multi_company" or I haven't migrated the view properly.

@RodrigoBM
Copy link
Contributor Author

/ocabot migration account_asset_management

@OCA-git-bot
Copy link
Contributor

Sorry @RodrigoBM you are not allowed to mark the addon tobe migrated.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@pedrobaeza
Copy link
Member

/ocabot migration account_asset_management

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Oct 17, 2022
@OCA-git-bot OCA-git-bot mentioned this pull request Oct 17, 2022
33 tasks
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the analytic stuff is not correctly ported.

account_asset_management/models/account_asset.py Outdated Show resolved Hide resolved
asset = super().create(vals)
@api.model_create_multi
def create(self, vals_list):
asset = super().create(vals_list)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returns several records, so you have to take it into account.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think its ok now

"date": depreciation_date,
"asset_id": asset.id,
}
if analytic_id:
move_line_data["analytic_distribution"][analytic_id] = 100
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the same thing as the previous tags.

Copy link
Contributor Author

@RodrigoBM RodrigoBM Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think its ok now

@@ -16,9 +16,6 @@ class AccountAssetProfile(models.Model):
account_analytic_id = fields.Many2one(
comodel_name="account.analytic.account", string="Analytic account"
)
analytic_tag_ids = fields.Many2many(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace it by the corresponding feature

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

for vals in vals_list:
if vals.get("method_time") != "year" and not vals.get("prorata"):
vals["prorata"] = True
profile = super().create(vals_list)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returns several records. Take it into account.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think its ok now

account_asset_management/models/account_move.py Outdated Show resolved Hide resolved
self.assertEqual(ict0.state, "open")
self.assertEqual(ict0.value_depreciated, 500)
self.assertEqual(ict0.value_residual, 1000)
vehicle0.validate()
created_move_ids = vehicle0.depreciation_line_ids[1].create_move()
for move_id in created_move_ids:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep these asserts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -4,7 +4,6 @@
<field name="name">account.move.form.account.asset.management</field>
<field name="model">account.move</field>
<field name="inherit_id" ref="account.view_move_form" />
<field name="groups_id" eval="[(4, ref('account.group_account_invoice'))]" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of the fields should have the group as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think its ok now

@RodrigoBM RodrigoBM force-pushed the 16.0-mig-account_asset_management branch 4 times, most recently from 009253b to b75a3b7 Compare October 20, 2022 11:19
@RodrigoBM RodrigoBM changed the title [16.0] [MIG] account_asset_management [16.0][MIG] account_asset_management: Migration to 16.0 Dec 4, 2022
@RodrigoBM RodrigoBM force-pushed the 16.0-mig-account_asset_management branch 2 times, most recently from cf7a976 to a6b03ad Compare December 9, 2022 16:28
@RodrigoBM RodrigoBM force-pushed the 16.0-mig-account_asset_management branch 2 times, most recently from 62f6a80 to 8637ac4 Compare December 12, 2022 16:09
Copy link
Contributor

@FernandoRomera FernandoRomera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional test with runboat, Ok.

Copy link

@fcvalgar fcvalgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional review is ok.

Review Asset Profile V16 and this model work fine.

@FernandoRomera
Copy link
Contributor

@pedrobaeza
Can you merge this PR, please?

@rafaelbn
Copy link
Member

Hello @pedrobaeza , I think you comments have been attended. Thanks!

lepistone and others added 2 commits December 24, 2022 00:42
add asset management modules

asset mgt update

redo

synch asset mgt with recent V7 changes
[UPD] add places arg in assertAlmostEqual tests

[UPD] flake clean
angel and others added 7 commits December 24, 2022 00:42
Currently translated at 98.8% (260 of 263 strings)

Translation: account-financial-tools-15.0/account-financial-tools-15.0-account_asset_management
Translate-URL: https://translation.odoo-community.org/projects/account-financial-tools-15-0/account-financial-tools-15-0-account_asset_management/es/
Currently translated at 100.0% (263 of 263 strings)

Translation: account-financial-tools-15.0/account-financial-tools-15.0-account_asset_management
Translate-URL: https://translation.odoo-community.org/projects/account-financial-tools-15-0/account-financial-tools-15-0-account_asset_management/es/
…ively

When updating assets massively the performance when searching for deprecation_lines_ids is very low.
It is a difficult problem to detect since massive asset updates are not common.
Indexing in this field dramatically improves the timing performance.
For example, to update 3000 records it took 30 sec. With this index it takes to 2 seconds.
@RodrigoBM RodrigoBM force-pushed the 16.0-mig-account_asset_management branch from 8637ac4 to 2e07ed4 Compare December 23, 2022 23:43
@RodrigoBM RodrigoBM force-pushed the 16.0-mig-account_asset_management branch from 2e07ed4 to 73b3200 Compare December 24, 2022 01:12
@RodrigoBM
Copy link
Contributor Author

Hi @fcvalgar @FernandoRomera

with the comment of @hieulucky111 i think is a good moment to add pr #1513 in this migration, is possible that your functional test again?

@pedrobaeza i add ref commit for change company_currency_id to currency_id and not to add currency_field option in monetary field.

Best regards

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you enter to the asset form without the analytic group, there's this error:

You are not allowed to access 'Analytic Plan's Applicabilities' (account.analytic.applicability) records.

This operation is allowed for the following groups:
	- Technical/Analytic Accounting

Contact your administrator to request access if necessary.

Please check the groups associated to the analytic fields.

amount = fields.Monetary(
string="Amount", required=True, currency_field="company_currency_id"
)
amount = fields.Monetary(required=True, currency_field="company_currency_id")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change also the currency on this model to currency_id and remove all the currency_field references. In fact, the related field should be to the asset currency, not to the company one (although right now is the same).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that in my commit [REF] account_asset_management: the name of the company_currency_id. didn't know if I should apply fixup with commit MIG.

@RodrigoBM RodrigoBM force-pushed the 16.0-mig-account_asset_management branch from 73b3200 to c176383 Compare December 28, 2022 19:35
@RodrigoBM
Copy link
Contributor Author

RodrigoBM commented Dec 28, 2022

If you enter to the asset form without the analytic group, there's this error:

You are not allowed to access 'Analytic Plan's Applicabilities' (account.analytic.applicability) records.

This operation is allowed for the following groups:
	- Technical/Analytic Accounting

Contact your administrator to request access if necessary.

Please check the groups associated to the analytic fields.

I added group_analytic_accounting to analytic_distribution field.

Thanks

Copy link

@hieulucky111 hieulucky111 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@fcvalgar
Copy link

fcvalgar commented Jan 2, 2023

Now I detect an error when I apply an asset profile on the invoice line @RodrigoBM

https://www.loom.com/share/c172198b9acd429da5e35f5400513a9f

@moduon MT-1849

@RodrigoBM RodrigoBM force-pushed the 16.0-mig-account_asset_management branch from c176383 to 12a018d Compare January 4, 2023 15:07
@RodrigoBM
Copy link
Contributor Author

Now I detect an error when I apply an asset profile on the invoice line @RodrigoBM

https://www.loom.com/share/c172198b9acd429da5e35f5400513a9f

@moduon MT-1849

I have already solved the bug. Thank you @fcvalgar

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Rodrigo!

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-1479-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 760f1a5 into OCA:16.0 Jan 6, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 03be733. Thanks a lot for contributing to OCA. ❤️

@RodrigoBM RodrigoBM deleted the 16.0-mig-account_asset_management branch January 6, 2023 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.