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

[17.0][MIG] account_asset_management #1819

Merged
merged 221 commits into from
May 31, 2024

Conversation

sbiosca-s73
Copy link

@sbiosca-s73 sbiosca-s73 commented Feb 13, 2024

Migration module account_asset_management to v17.0

lepistone and others added 24 commits February 13, 2024 13:44
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
…an't be modified if a move is linked with a depriciation line

[IMP][account_asset_management] Define FIELDS_AFFETCS_ASSET_MOVE as a set directly
* Fix compute methods dependencies and small optimizations
* Rename demo file to test and move it into the right folder
This is now supported natively by Odoo 11.
* account_asset: Do not loop on all the lines to search for one linked asset

Before this change, the use of `mapped` on self did loop on all the move
lines that are included in self to get the assets, what could be very
costly for a simple write on a lot of move lines. As the goal is to raise
an error only if at least one move is linked to an asset, we break the
loop if the condition is fulfilled.

* performance improvement

* [RMV] - Remove useless dependency

In 12.0 account_fiscal_year is a standard feature no need to depend on oca
module account_fiscal_year
@sbiosca-s73 sbiosca-s73 changed the title [17.0][MIG] accountasset management [17.0][MIG] account_asset_management Feb 13, 2024
@sbiosca-s73 sbiosca-s73 force-pushed the 17.0-mig-account_asset_management branch 4 times, most recently from 313ef46 to bd94767 Compare February 14, 2024 09:57
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.

We did some functional tests and it seems to work as expected.

@sbiosca-s73
Copy link
Author

@HaraldPanten tests checked

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@HaraldPanten
Copy link
Contributor

Could you separate pre-commit changes from migration commit?

THX.

@sbiosca-s73
Copy link
Author

@HaraldPanten what changes?

@HaraldPanten
Copy link
Contributor

When you open a new PR for a migration, you need to apply pre-commit, right? (black, isort, prettier).

This command makes some changes to your code and they have to be in a separate commit (not in the migration commit).

And then, you start the migration. These changes would be included in a new commit (the migration commit)

Here you can see several examples:

OCA/l10n-spain#3583
OCA/account-invoice-reporting#319

You'll see that the migration PRs that I mentioned have 2 commits. One of them is for the pre-commit stuff.

Maybe @ioans73 can explain you better

@pedrobaeza
Copy link
Member

@ioans73
Copy link
Member

ioans73 commented May 28, 2024

@HaraldPanten @pedrobaeza Thanks for your comments :) I'm going to review it with @sbiosca-s73

@sbiosca-s73 sbiosca-s73 force-pushed the 17.0-mig-account_asset_management branch from 54bfd73 to abfbcc2 Compare May 29, 2024 06:49
@sbiosca-s73 sbiosca-s73 force-pushed the 17.0-mig-account_asset_management branch from abfbcc2 to 66442a1 Compare May 29, 2024 06:53
@sbiosca-s73
Copy link
Author

@HaraldPanten @pedrobaeza changes done, thanks!

Copy link
Contributor

@HaraldPanten HaraldPanten 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. Overall, seems to be OK.

@pedrobaeza Could we merge this one?

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 name search and display_name is wrong.

@@ -495,15 +474,16 @@ def name_search(self, name, args=None, operator="ilike", limit=100):
if operator in expression.NEGATIVE_TERM_OPERATORS:
domain = ["&", "!"] + domain[1:]
assets = self.search(domain + args, limit=limit)
return assets.name_get()
return assets._compute_display_name()
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 correct. You can't call a compute method in a search.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of overriding name_search, use _rec_names_search, as stated in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-16.0 (it wasn't changed in previous migration)

Copy link
Author

Choose a reason for hiding this comment

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

account_asset_management/models/account_asset.py Outdated Show resolved Hide resolved
@sbiosca-s73
Copy link
Author

@pedrobaeza changes done

@sbiosca-s73 sbiosca-s73 force-pushed the 17.0-mig-account_asset_management branch from be9f746 to 24f2315 Compare May 31, 2024 07:23
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

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit b6f57fc into OCA:17.0 May 31, 2024
8 of 9 checks passed
@OCA-git-bot
Copy link
Contributor

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

@ioans73 ioans73 deleted the 17.0-mig-account_asset_management branch July 24, 2024 10:18
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.