-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[17.0] [MIG] sale_stock_delivery_address #3107
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
base: 17.0
Are you sure you want to change the base?
[17.0] [MIG] sale_stock_delivery_address #3107
Conversation
|
/ocabot migration sale_stock_delivery_address |
| @@ -1,14 +1,14 @@ | |||
| # Copyright 2020-21 ForgeFlow S.L. | |||
| # License AGPL-3.0 or later (https://www.gnu.org/licenses/lgpl.html). | |||
| # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html). | |||
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.
@LoisRForgeFlow Was this really AGPL or LGPL (not consistent between manifest and files) ?
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.
It was originally improperly licenced as LGPL but as pointed and fixed by Pedro it needs to be AGPL to depend on sale_procurement_group_by_line.
Short answer: AGPL
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: code review
|
@rousseldenis could you approve the CI workflow so we can see if it is all good? |
|
|
||
|
|
||
| class TestStockSourcingAddress(TransactionCase): | ||
| def setUp(self): |
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.
Could you change this to class method ?
24ed4df to
e7d546a
Compare
|
@Mikheil-21 I think you need to start a new PR following the migration guide: https://github.com/OCA/maintainer-tools/wiki#migration |
I think what the reviewer is suggesting is to squash all (your) migration commits. Everytime you add a commit, you're violating OCA migration convention. Instead, after creating a new commit to fix an issue pointed out by a reviewer here, you need to squash that commit into your previous commit and force-push. Same goes for any commits added by bots and other migration contributors. @francesco-ooops pls correct me if I'm wrong. source: wiki/Merge-commits-in-pull-requests @Mikheil-21 As for the 2 failed tests, these are automated tests and for you to fix. They are part of the addon and need to be migrated as well. You can ask Ana, she's worked with automated tests in OCA before. |
|
/ocabot rebase |
|
Congratulations, PR rebased to 17.0. |
6b7bfc5 to
257c5e5
Compare
Tested based on instructions in the module README.