-
-
Notifications
You must be signed in to change notification settings - Fork 763
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
[16.0][MIG] account_asset_management: Migration to 16.0 #1479
Conversation
cd24bae
to
c2ec97d
Compare
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. |
/ocabot migration account_asset_management |
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 |
/ocabot migration account_asset_management |
c2ec97d
to
828063f
Compare
There was a problem hiding this 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.
asset = super().create(vals) | ||
@api.model_create_multi | ||
def create(self, vals_list): | ||
asset = super().create(vals_list) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep these asserts
There was a problem hiding this comment.
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'))]" /> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
009253b
to
b75a3b7
Compare
cf7a976
to
a6b03ad
Compare
62f6a80
to
8637ac4
Compare
There was a problem hiding this 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.
There was a problem hiding this 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.
@pedrobaeza |
Hello @pedrobaeza , I think you comments have been attended. Thanks! |
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
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.
8637ac4
to
2e07ed4
Compare
2e07ed4
to
73b3200
Compare
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 |
There was a problem hiding this 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") |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
73b3200
to
c176383
Compare
I added group_analytic_accounting to analytic_distribution field. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
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 |
…ield is changed to currency_id and currency_field is not required.
c176383
to
12a018d
Compare
I have already solved the bug. Thank you @fcvalgar |
There was a problem hiding this 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
On my way to merge this fine PR! |
Congratulations, your PR was merged at 03be733. Thanks a lot for contributing to OCA. ❤️ |
#1472