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] product_pricelist_supplierinfo: Migration to 17.0 #1632

Merged
merged 53 commits into from
Jun 26, 2024

Conversation

mdurepos
Copy link

No description provided.

@suniagajose
Copy link

hi @luisg123v

can you review and merge this please?

thanks,

@@ -17,23 +17,24 @@ Supplier info prices in sales pricelists
:target: http://www.gnu.org/licenses/agpl-3.0-standalone.html
:alt: License: AGPL-3
.. |badge3| image:: https://img.shields.io/badge/github-OCA%2Fproduct--attribute-lightgray.png?logo=github
:target: https://github.com/OCA/product-attribute/tree/16.0/product_pricelist_supplierinfo
:target: https://github.com/OCA/product-attribute/tree/17.0/product_pricelist_supplierinfo

Choose a reason for hiding this comment

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

This is in the migration commit, but it should be on a separate commit that includes precommit changes. Presentation should be:

  • Module history from the previous version (several commits
  • A commit running precommit, named [IMP] module_name: pre-commit auto fixes
  • The actual migration commit. Here you should remove [17.0], this should only be included in PR's title

Copy link
Author

Choose a reason for hiding this comment

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

@luisg123v thanks for the review. I'll correct and re-push when I get some time.

{
"name": "Supplier info prices in sales pricelists",
"summary": "Allows to create priceslists based on supplier info",
"version": "17.0.1.0.1",

Choose a reason for hiding this comment

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

Suggested change
"version": "17.0.1.0.1",
"version": "17.0.1.0.0",

@luisg123v
Copy link

/ocabot migration product_pricelist_supplierinfo

@OCA-git-bot
Copy link
Contributor

Sorry @luisg123v 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.

@suniagajose
Copy link

hi @luisg123v

can you check with someone at OCA why the bot did not allow you to merge it?

thanks,

@luisg123v
Copy link

@suniagajose I didn't try to merge, I just tried to mark it as a migration PR

@suniagajose
Copy link

ok @luisg123v

whom can i ask to mark as migration instead?

@moylop260
Copy link
Contributor

/ocabot migration product_pricelist_supplierinfo

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Jun 17, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request Jun 17, 2024
59 tasks
@luisg123v
Copy link

Hi @sbidoul, @pedrobaeza,

I tried to mark this module as a migration one on #1632 (comment), but it failed with a permission error, despite being a maintainer. Besides, I noticed I wasn't notified for review.

Is there any issue with the bot you are aware of?

Regards,

CC @desdelinux

@pedrobaeza
Copy link
Member

Please split the pre-commit initial changes from the migration commit as stated in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-17.0

@sbidoul
Copy link
Member

sbidoul commented Jun 24, 2024

@luisg123v

I tried to mark this module as a migration one on #1632 (comment), but it failed with a permission error, despite being a maintainer.

I don't think the ocabot migration command checks for maintainers. Wondering if the bot could detect migration PRs automatically.

Besides, I noticed I wasn't notified for review.

OCA/oca-github-bot#193 Seems there is an open PR, but it's a big diff to review.

@pedrobaeza
Copy link
Member

I don't think the ocabot migration command checks for maintainers. Wondering if the bot could detect migration PRs automatically.

That's a very difficult task, as there's nothing characteristic that can be used for that. The only thing may be the PR title, and people is not very standard with it... so it will be the same time to ask a contributor to change the PR title (and to detect such changes from the bot), than launching ocabot migrat... command.

@suniagajose
Copy link

Please split the pre-commit initial changes from the migration commit as stated in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-17.0

Hi @mdurepos

Can you do it, please?

Regards,

cubells and others added 12 commits June 26, 2024 06:30
* Don't depend on sales
* Extended README
* Tests focused on module specific features
* Code optimization
* Don't mix pricelist info with supplier info on criteria
Date order is passed by context, so we have to take this into account for
computing properly available supplierinfo records.
If not, you can twist the order in the form and rules won't be applied according criteria
If you set the value "Based on" for using supplier info, but then you change the
computation type to another one (like fixed price or discount), this code is still
acting, so we should check both fields.
Currently translated at 93.3% (14 of 15 strings)

Translation: product-attribute-12.0/product-attribute-12.0-product_pricelist_supplierinfo
Translate-URL: https://translation.odoo-community.org/projects/product-attribute-12-0/product-attribute-12-0-product_pricelist_supplierinfo/es/
When having price per qty at the supplier level and if you want to
base your pricelist on the supplier price, now you can
define a margin on the product.supplierinfo
francesco-ooops and others added 16 commits June 26, 2024 06:30
Currently translated at 59.0% (13 of 22 strings)

Translation: product-attribute-15.0/product-attribute-15.0-product_pricelist_supplierinfo
Translate-URL: https://translation.odoo-community.org/projects/product-attribute-15-0/product-attribute-15-0-product_pricelist_supplierinfo/it/
Assertion in tests involving the pricelist field in products were
removed, as that field doesn't exist anymore [1], and such assertions
were intended only to ensure computed price were propagated correctly to
that field.

[1] odoo/odoo@9e99a9df
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: product-attribute-16.0/product-attribute-16.0-product_pricelist_supplierinfo
Translate-URL: https://translation.odoo-community.org/projects/product-attribute-16-0/product-attribute-16-0-product_pricelist_supplierinfo/
…od in product_pricelist model

In version 15.0 the _compute_price_rule method was used to obtain the
product price because the price was a computed field, however, in
version 16.0 we need to inherit _compute_price from product_pricelist_item
because this method is used directly when creating a new sale order line.
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: product-attribute-16.0/product-attribute-16.0-product_pricelist_supplierinfo
Translate-URL: https://translation.odoo-community.org/projects/product-attribute-16-0/product-attribute-16-0-product_pricelist_supplierinfo/
Currently translated at 100.0% (14 of 14 strings)

Translation: product-attribute-16.0/product-attribute-16.0-product_pricelist_supplierinfo
Translate-URL: https://translation.odoo-community.org/projects/product-attribute-16-0/product-attribute-16-0-product_pricelist_supplierinfo/es/
Currently translated at 100.0% (14 of 14 strings)

Translation: product-attribute-16.0/product-attribute-16.0-product_pricelist_supplierinfo
Translate-URL: https://translation.odoo-community.org/projects/product-attribute-16-0/product-attribute-16-0-product_pricelist_supplierinfo/pt_BR/
Currently translated at 100.0% (14 of 14 strings)

Translation: product-attribute-16.0/product-attribute-16.0-product_pricelist_supplierinfo
Translate-URL: https://translation.odoo-community.org/projects/product-attribute-16-0/product-attribute-16-0-product_pricelist_supplierinfo/it/
@mdurepos mdurepos force-pushed the 17.0-mig-product_pricelist_supplierinfo branch 3 times, most recently from ef8cdac to 5cd2fab Compare June 26, 2024 10:52
@mdurepos mdurepos force-pushed the 17.0-mig-product_pricelist_supplierinfo branch from 5cd2fab to 126f0bc Compare June 26, 2024 10:55
@mdurepos
Copy link
Author

@suniagajose I just force pushed with the auto-fix commit and real manual changes separated. I believe I caught everything from the previous comments.

@suniagajose
Copy link

thanks @mdurepos

@moylop260 @pedrobaeza i think now is ready....

@moylop260
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 17.0-ocabot-merge-pr-1632-by-moylop260-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 57076fa into OCA:17.0 Jun 26, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at cfeb3e1. 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.