[16.0][IMP] sale_order_line_multi_warehouse: sale order line multi warehouse widget#3778
Conversation
manuelregidor
force-pushed
the
16.0-imp-sale_order_line_multi_warehouse
branch
from
96cc4c0 to
2d61749
Compare
manuelregidor
force-pushed
the
16.0-imp-sale_order_line_multi_warehouse
branch
from
debb523 to
cb24ea4
Compare
|
@etobella Would you make any changes to the widget? |
alexey-pelykh
left a comment
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thank you for this contribution -- the per-warehouse availability widget is a useful addition. I have a few observations that I believe should be addressed before merge.
Summary
| # | Finding | Severity | File |
|---|---|---|---|
| 1 | fields.Binary used for JSON data -- prefer fields.Json (available in Odoo 16) |
medium | models/sale_order_line.py |
| 2 | max(moves.mapped("forecast_expected_date")) can raise ValueError if any element is False |
high | models/sale_order_line.py |
| 3 | Invalid HTML: <p> directly inside <tr> without <td> wrapper |
medium | static/src/widgets/qty_at_date_by_warehouse_widget.xml |
| 4 | Badge URL typo: license changed to licence is a regression (shields.io uses license) |
low | README.rst |
| 5 | Test comments reference wrong product names in several replenish blocks | low | tests/test_so_line_multiwarehouse.py |
| 6 | Mixed use of datetime.now() vs fields.Datetime.now() in tests |
low | tests/test_so_line_multiwarehouse.py |
| ) | ||
| qty_by_warehouse = fields.Binary( | ||
| compute="_compute_qty_by_warehouse", exportable=False | ||
| ) |
There was a problem hiding this comment.
Consider using fields.Json instead of fields.Binary for storing structured data. fields.Json is available in Odoo 16 and is the idiomatic way to store JSON-serializable Python dicts. With fields.Binary, the JS side needs to do JSON.parse(JSON.stringify(...)) workarounds, whereas fields.Json handles serialization natively.
| # forecast_expected_date | ||
| forecast_expected_date = False | ||
| if moves: | ||
| forecast_expected_date = max(moves.mapped("forecast_expected_date")) |
There was a problem hiding this comment.
Potential ValueError here: moves.mapped("forecast_expected_date") can contain False values (the field defaults to False when not set). max() will fail when comparing datetime and bool types.
Suggested fix:
forecast_dates = [d for d in moves.mapped("forecast_expected_date") if d]
forecast_expected_date = max(forecast_dates) if forecast_dates else False| </tr> | ||
| <tr> | ||
| <p>This product is replenished on demand.</p> | ||
| </tr> |
There was a problem hiding this comment.
Invalid HTML structure: <p> is not a valid child of <tr>. This should be wrapped in a <td> element:
<tr>
<td colspan="2"><p>This product is replenished on demand.</p></td>
</tr>| .. image:: https://odoo-community.org/readme-banner-image | ||
| :target: https://odoo-community.org/get-involved?utm_source=readme | ||
| :alt: Odoo Community Association | ||
|
|
There was a problem hiding this comment.
This changes license to licence in the badge URL. The shields.io badge text parameter is license (American English) -- changing it to licence will alter the badge display text. This looks like an unintended change; please revert.
|
|
||
| # 5 extra units of product_1 in alternative_warehouse_2 available in 7 days | ||
| self.replenish_qty( | ||
| self.product_1, |
There was a problem hiding this comment.
Several comments in this section say "5 extra units of product_1" when the code is actually operating on self.product_2. For example:
- "5 extra units of product_1 in warehouse available in 7 days" should be "product_2"
- "5 extra units of product_1 in alternative_warehouse_1 available in 7 days" should be "product_2"
- "5 extra units of product_1 in alternative_warehouse_2 available in 7 days" should be "product_2"
Similar mismatch occurs in the replenish blocks for 20 extra units.
| self.assertFalse(forecasted_issue) | ||
| for warehouse in warehouses: | ||
| values = self.QUANTITIES.get(warehouse["warehouse_name"]) | ||
| self.assertTrue(values) |
There was a problem hiding this comment.
The test mixes datetime.now() (naive, no timezone) with fields.Datetime.now() (used later in the file). For consistency and to avoid subtle timezone issues, consider using fields.Datetime.now() throughout.
|
Hi @alexey-pelykh Finally we're not going to continue with that. Closing PR, but you can continue using our work, if you want |
5 participants
This improvement adds a new widget to sale order lines when the multi warehouse options are active. This widget shows the availability of the product in the main warehouse and all its alternative warehouses. It replicates the way the availability widget in module sale_stock works.
T-8536