-
-
Notifications
You must be signed in to change notification settings - Fork 40
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] stock_request_purchase: Migration to 17.0 #35
[17.0][MIG] stock_request_purchase: Migration to 17.0 #35
Conversation
32f3c95
to
747c1f8
Compare
/ocabot migration stock_request_purchase |
747c1f8
to
5fcdbca
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 review ok. The PO is correctly linked to the stock request and stock request order.
@pedrobaeza Hi Pedro, can you review this module in order to merge it? Thanks in advance! |
/ocabot migration stock_request_purchase Sorry, another reviewer should check this PR before merging. |
cls.route_buy = cls.warehouse.buy_pull_id.route_id | ||
cls.supplier = cls.env["res.partner"].create({"name": "Supplier"}) | ||
cls.product.write( | ||
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.
This change is not correct. Please revert it.
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.
Hello @pedrobaeza,
I've reverted the change as requested. The original setup used setUpClass(cls), which caused an AttributeError because cls.warehouse wasn't initialized properly. To address this, I changed it to setUp(self), ensuring the superclass's setUp method was called to initialize the attributes.
As I'm new to Odoo development, could you please advise me on the correct way to initialize these attributes in setUpClass without causing errors?
Thank you for your help.
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.
OK, I see. The problem is that the original test is also with setUp
:
which is not correct at all, as it's not making use of this method that avoids to execute the setup for each of the test methods, running only once instead of N times.
Can you please make a new PR changing the test of stock_request
to setUpClass?
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.
Can you please make a new PR changing the test of
stock_request
to setUpClass?
Agree. I would say in addition of this, moving the setupClass to a common.py is a best practice too.
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.
Hi @pedrobaeza
I noticed that the changes requested for the stock_request module tests in v17 are already done in v16:
https://github.com/OCA/stock-logistics-request/blob/16.0/stock_request/tests/test_stock_request.py
But they are missing in v17:
https://github.com/OCA/stock-logistics-request/blob/17.0/stock_request/tests/test_stock_request.py
Could you please confirm if the changes you want me to make are the same as those made in v16? If so, do you know why these changes are not in v17?
Additionally, should I create a separate PR for the modifications in stock_request, or should I include these changes in another commit within this existing PR I have for migrating stock_request_purchase?
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.
Yes, that's it. You can do it as a separate commit, but inside this PR, as it's an adjustment linked to this migration.
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.
Hi @pedrobaeza , all done.
Could you please review it?
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.
Functional Review LGTM 👍
This PR has the |
5fcdbca
to
6361791
Compare
5408a69
to
03470c6
Compare
OCA Transbot updated translations from Transifex
- Error with the sequence number. - Visible texts that should be in uppercases. - order_id should only be visible if group_stock_request_order option is enabled. - adds more tests - adds consistency between models company-wise
[UPD] Update stock_request_purchase.pot
Currently translated at 100,0% (13 of 13 strings) Translation: stock-logistics-warehouse-11.0/stock-logistics-warehouse-11.0-stock_request_purchase Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-11-0/stock-logistics-warehouse-11-0-stock_request_purchase/es/
[UPD] Update stock_request_purchase.pot [UPD] README.rst
Currently translated at 100.0% (13 of 13 strings) Translation: stock-logistics-warehouse-12.0/stock-logistics-warehouse-12.0-stock_request_purchase Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-12-0/stock-logistics-warehouse-12-0-stock_request_purchase/es/
[UPD] Update stock_request_purchase.pot [UPD] README.rst Update translation files Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: stock-logistics-warehouse-13.0/stock-logistics-warehouse-13.0-stock_request_purchase Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-13-0/stock-logistics-warehouse-13-0-stock_request_purchase/ Update translation files Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: stock-logistics-warehouse-13.0/stock-logistics-warehouse-13.0-stock_request_purchase Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-13-0/stock-logistics-warehouse-13-0-stock_request_purchase/
[UPD] Update stock_request_purchase.pot
[UPD] Update stock_request_purchase.pot [UPD] README.rst stock_request_purchase 15.0.1.0.1
…urchases TT43414 stock_request_purchase 15.0.1.0.2
…w_stock_request action of purchase.order TT43297
… all stock requests from action. Allows inheritance by other modules TT43297 stock_request_purchase 15.0.1.1.0 [UPD] README.rst
TT44811 stock_request_purchase 15.0.1.2.0 [UPD] README.rst Update translation files Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: stock-logistics-warehouse-15.0/stock-logistics-warehouse-15.0-stock_request_purchase Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-15-0/stock-logistics-warehouse-15-0-stock_request_purchase/
Currently translated at 100.0% (13 of 13 strings) Translation: stock-logistics-warehouse-15.0/stock-logistics-warehouse-15.0-stock_request_purchase Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-15-0/stock-logistics-warehouse-15-0-stock_request_purchase/es/
Translated using Weblate (Italian) Currently translated at 100.0% (13 of 13 strings) Translation: stock-logistics-warehouse-15.0/stock-logistics-warehouse-15.0-stock_request_purchase Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-15-0/stock-logistics-warehouse-15-0-stock_request_purchase/it/
Co-authored-by: Bruno-Zanotti
03470c6
to
5244df8
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.
Great, thanks
/ocabot merge nobump
@pedrobaeza could you please merge it? |
/ocabot merge nobump |
On my way to merge this fine PR! |
Congratulations, your PR was merged at 911a66e. Thanks a lot for contributing to OCA. ❤️ |
No description provided.