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

[15.0][MIG] stock_analytic: migration #448

Merged
merged 63 commits into from
Jan 3, 2023
Merged

[15.0][MIG] stock_analytic: migration #448

merged 63 commits into from
Jan 3, 2023

Conversation

Mantux11
Copy link

Migration to 15.0. product file was deleted, because in original odoo 14 code _anglo_saxon_sale_move_lines function was deleted. I don't know why this was left in 14.0 migration, but I say that, if no one had problems, I don't see any issues that is associated with this, so I assume that this is not necessary to leave a file that don't do anything. Improved tests.

@Mantux11 Mantux11 mentioned this pull request Apr 12, 2022
21 tasks
@rousseldenis
Copy link
Contributor

@Mantux11 Can you review this in order to integrate it here too ?

#423

Thanks

Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

Functional LG! Pending to attend comments

@Mantux11
Copy link
Author

@rousseldenis I'm integrated. Migration script's I guess isn't needed because you do that in 14.0 version, so I don't add that. If it's needed, say, I would insert here also.

@rousseldenis
Copy link
Contributor

@rousseldenis I'm integrated. Migration script's I guess isn't needed because you do that in 14.0 version, so I don't add that. If it's needed, say, I would insert here also.

Oops.

The good way is to cherry-pick it in order to keep authorship. Can you do it ?

And indeed, in your migration commit you can remove migration scripts.

@Mantux11
Copy link
Author

@rousseldenis I'll wait for your PR merge, because it would be easier for me to cherry pick.

@Olageibol
Copy link

In order to maintain the data security, I believe that the analytic_account_id field in stock.move.line views needs the analytic group added to it.
https://github.com/vialaurea/OCA-account-analytic/blob/15.0-mig-stock_analytic/stock_analytic/views/stock_move_line.xml

@AaronHForgeFlow
Copy link
Contributor

can you include this? #423

@Mantux11
Copy link
Author

@AaronHForgeFlow yes, I would do that ASAP.

@jlzhou
Copy link

jlzhou commented Oct 6, 2022

@Mantux11 thanks for your effort!

Could you please explain why remove _prepare_procurement_values override from stock.move model?

def _prepare_procurement_values(self): """ Allows to transmit analytic account from moves to new moves through procurement. """ res = super()._prepare_procurement_values() if self.analytic_account_id: res.update( { "analytic_account_id": self.analytic_account_id.id, } ) return res

@oca-clabot
Copy link

Hey @Mantux11, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: https://odoo-community.org/page/cla
Here is a list of the users:

Appreciation of efforts,
OCA CLAbot

@Mantux11 Mantux11 marked this pull request as draft October 7, 2022 09:19
@Mantux11 Mantux11 closed this Oct 7, 2022
Hanane ELKHAL and others added 17 commits October 7, 2022 09:25
… Stock analytic XML part is now migrated

Add logo for generic modules
- migration point, put all the modules to not installable
This module allows the user to generate analytic information from stock
moves.

- Fixed flake8 and pylint errors.
Remove sale and purchase dependency. Add test

Only assign analytic account if account != valuation acc

Changing field name account_analytic_id

Adjust to OCA latest guidelines. Add some usahe info on README
Remove remaining encoding hints.
Correct lint in test
Correct flake8 in test
Fix documentation and test_flake8
@Mantux11 Mantux11 reopened this Oct 7, 2022
@Mantux11 Mantux11 marked this pull request as ready for review October 7, 2022 09:33
@Mantux11
Copy link
Author

Mantux11 commented Oct 7, 2022

@AaronHForgeFlow I added all comitt's from 14. @jlzhou Returned this function,

Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

LG!

@Mantux11
Copy link
Author

@AaronHForgeFlow @jlzhou Could you Merge this PR? It's a valid migration, and it hasn't been reviewed for more than a month.

@AaronHForgeFlow
Copy link
Contributor

@Mantux11 This needs an additional approval for the merge. Perhaps you can ping the people that have commented here but did not do a formal review.

@rafaelbn rafaelbn added this to the 15.0 milestone Nov 28, 2022
@rafaelbn
Copy link
Member

/ocabot migration stock_analytic

@rafaelbn
Copy link
Member

I don't use this module. Ping my in 2 week if it not reviewed by other contributors. Thanks

Copy link
Member

@Jerther Jerther left a comment

Choose a reason for hiding this comment

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

Missing coverage for one method. Otherwise, we've been using a previous version of this PR, identical but without this method, and we've been doing so for 2 or 3 months now and it's working fine. :)

stock_analytic/models/stock.py Show resolved Hide resolved
Copy link

@florenciafrigieri2 florenciafrigieri2 left a comment

Choose a reason for hiding this comment

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

Hi! I was trying to do a functional review in runboat and even though the analytic modules were installed, I was not able to allow analytic funcionalities even in the accounting app. Can you help me with that? Thanks in advance!

@Mantux11
Copy link
Author

@florenciafrigieri2 It's been a long time ago, when I tested this module, so personally, I forgot. If I will find, how and where, I will update this comment.

@Mantux11 Mantux11 requested a review from Jerther November 30, 2022 09:58
Copy link
Member

@Jerther Jerther left a comment

Choose a reason for hiding this comment

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

100% code coverage, and working well. It's good to go!

@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). 🤖

Copy link
Contributor

@nicomacr nicomacr left a comment

Choose a reason for hiding this comment

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

LGTM technical review

@Mantux11
Copy link
Author

@rafaelbn passed 2 weeks and PR has been approved. Could you review it and merge?

Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

Code review

@OCA/accounting-maintainers

@grindtildeath
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 15.0-ocabot-merge-pr-448-by-grindtildeath-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 27cd3a2 into OCA:15.0 Jan 3, 2023
@OCA-git-bot
Copy link
Contributor

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

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.