-
-
Notifications
You must be signed in to change notification settings - Fork 167
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
[14.0][MIG] maintenance_stock: Migration to 14.0 #347
base: 14.0
Are you sure you want to change the base?
Conversation
Hello @Reyes4711-S73 your PR is based on #337 , isn't it? Please check, because that PR has new adjustments since you've cloned it |
edb1746
to
78728b3
Compare
@dalonsod I added your last changes in my migration PR |
78728b3
to
f20543b
Compare
Please check errors in tests |
f20543b
to
bc0a774
Compare
</function> | ||
<function model="stock.inventory" name="action_validate"> | ||
<function | ||
eval="[[('state','=','draft'),('id', '=', ref('maintenance_stock.stock_inventory_toner'))]]" |
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.
The inventory for demo product is In Progress when installing the module:
Perhaps it is this line that does not allow validating the inventory. Maybe the state at this moment is confirm (not tested).
eval="[[('state','=','draft'),('id', '=', ref('maintenance_stock.stock_inventory_toner'))]]" | |
eval="[[('state','=','confirm'),('id', '=', ref('maintenance_stock.stock_inventory_toner'))]]" |
Can you review please ?
@Reyes4711-S73 I've still found some issues in commit history:
|
Currently translated at 96.7% (29 of 30 strings) Translation: maintenance-12.0/maintenance-12.0-maintenance_stock Translate-URL: https://translation.odoo-community.org/projects/maintenance-12-0/maintenance-12-0-maintenance_stock/es/
Currently translated at 100.0% (30 of 30 strings) Translation: maintenance-12.0/maintenance-12.0-maintenance_stock Translate-URL: https://translation.odoo-community.org/projects/maintenance-12-0/maintenance-12-0-maintenance_stock/pt_BR/
Currently translated at 100.0% (30 of 30 strings) Translation: maintenance-12.0/maintenance-12.0-maintenance_stock Translate-URL: https://translation.odoo-community.org/projects/maintenance-12-0/maintenance-12-0-maintenance_stock/it/
Currently translated at 100.0% (30 of 30 strings) Translation: maintenance-12.0/maintenance-12.0-maintenance_stock Translate-URL: https://translation.odoo-community.org/projects/maintenance-12-0/maintenance-12-0-maintenance_stock/it/
c9e4180
to
3ee7028
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.
Hello @Reyes4711-S73 Thanks for the applied fixes.
Only a comment about the value used for field delivery_steps
in stock.warehouse
can you review please.
3ee7028
to
117ae55
Compare
74ce106
to
3c453fe
Compare
@mamcode done |
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.
Thanks @Reyes4711-S73
@dalonsod Can you check this PR again please? |
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.
Tested in runboat 👍 but see at code review required changes
9efae1f
to
b573f37
Compare
@dalonsod @Reyes4711-S73 Hello, I see errors in the Odoo test job that seem to have nothing to do with the code of this PR, how can we run the jobs again? |
b573f37
to
3ab39ac
Compare
Hello @mamcode , it's a known issue, please check odoo/odoo#122569 (comment) |
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
7754dd7
to
07b3541
Compare
@etobella is this PR ready to be merged? 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.
I have a few questions before merging
def _get_locations_values(self, vals): | ||
sub_locations = super()._get_locations_values(vals) | ||
def _get_locations_values(self, vals, code=False): | ||
sub_locations = super()._get_locations_values(vals, code) | ||
code = vals.get("code") or self.code |
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.
Is it this the same code provided by the inherited function?
Also, you are overriding a parameter of the function... you should review 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.
@etobella you're right, that code was discussed in v13 here: #337 (review)
I've noticed two things:
- v13 recommendation wasn't finally attended (
code
method parameter was finally unused), not fault of this PR. - commit history is definetely wrong, because this change shouldn't appear here.
@Reyes4711-S73 please rebuild commit history. See e.g.a7fa359#diff-6f3891aa1e983a123b23df8d101c909e1d2e6812915e7dbda2a820400a25c8b0 , that commit does not belong to v13 implementation and have merge conflicts syntax indeed. You should start again the migration process and strictly add needed changes in last Migration to 14.0
commit.
@etobella sorry I didn't notice that problem...
@@ -173,11 +137,12 @@ def test_picking(self): | |||
("product_id", "=", self.product1.id), | |||
("location_id", "=", self.maintenance_warehouse.wh_cons_loc_id.id), | |||
] | |||
self.assertEqual(stock_quant_obj.search(domain_from).quantity, 0) | |||
self.assertEqual(stock_quant_obj.search(domain_from).quantity, 5) |
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.
Why now it is 5 and before it was 0?
This PR has the |
1 similar comment
This PR has the |
{ | ||
"name": "Test equipment", | ||
"allow_consumptions": True, | ||
"equipment_assign_to": "employee", |
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.
"equipment_assign_to": "employee", |
@Reyes4711-S73 @dalonsod @etobella
I think this field equipment_assign_to
should be removed to avoid the dependence of hr_maintenance
.
Everything else looks very good.
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
07b3541
to
22bdf26
Compare
Standard migration to 14.0