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

[18.0][MIG] purchase_manual_delivery #2432

Open
wants to merge 44 commits into
base: 18.0
Choose a base branch
from

Conversation

StefanRijnhart
Copy link
Member

@StefanRijnhart StefanRijnhart commented Oct 10, 2024

From unmerged #2417

  • Move models and views into their own model file
  • Integrate with purchase_order_line_menu ([18.0][MIG] purchase_order_line_menu: Migration to 18.0 #2430)
  • Refactor out duplicate buttons with additional context key. Introduce a context key to avoid the manual delivery flow instead.
  • Fix faulty behaviour if order quantities are increased on a confirmed order.

@StefanRijnhart StefanRijnhart added this to the 18.0 milestone Oct 10, 2024
@StefanRijnhart StefanRijnhart force-pushed the 18.0-mig-purchase_manual_delivery branch from 2c24f7e to fa9a5cc Compare October 10, 2024 11:18
@StefanRijnhart StefanRijnhart force-pushed the 18.0-mig-purchase_manual_delivery branch from 3ec0395 to a65d3fd Compare October 17, 2024 07:39
Comment on lines 34 to 36
return super(
PurchaseOrder, self.with_context(manual_delivery=True)
).button_confirm()

Choose a reason for hiding this comment

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

Doesn't it seem like an obvious bug?
That it should be return self.with_context(manual_delivery=True) ).button_confirm()? Otherwise, you go back to purchase, and skip all overrides defined there. I'm not even sure that there's a good reason for this design (I frankly doubt it), but having all overrides in one case and not the other seems quite dangerous.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you mean, we should avoid creating a picking even when button_confirm is called directly in some way, I agree. I suggest we turn the mechanism around and make _create_picking do nothing for POs with manual_delivery=True unless a specific context key is present. We can then pass that context key in the wizard to force creating pickings for manualy delivery POs. Do you agree?

Choose a reason for hiding this comment

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

I agree, that's the part where I suggested that I don't think it's a very good design (if the code of button_confirm_manual was directly put into an override of button_confirm that depends on a context key, you can still have 2 different buttons by putting the context in the attributes).

To make my point clearer, suppose you have module purchase_x that depends on purchase, and that it does an override of button_confirm to call fx.
Because purchase_x and purchase_manual_delivery have no relation to each other, we could either have purchase_manual_delivery => purchase_x => purchase, or purchase_x => purchase_manual_delivery => purchase.

If you call button confirm, in all cases you will call fx once. If you call button_confirm_manual, in one case you will call fx but not the other, depending on how the ORM loaded modules. If you decide that you want an easy solution and add an override of button_confirm_manual to also call fx, then you have the possibility to call fx twice, so you'd better be sure to make it idempotent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see what you mean and you are right, that is a straightforward bug and can be fixed with

Suggested change
return super(
PurchaseOrder, self.with_context(manual_delivery=True)
).button_confirm()
return self.with_context(manual_delivery=True).button_confirm()

Do you see additional value in the refactoring that I suggested?

Choose a reason for hiding this comment

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

I sure do! Since a migration is the most appropriate time to do it, I will make a PR with the simple fix for 17.0 and give you a review when you're done.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can do it, if you like.

Copy link
Member Author

Choose a reason for hiding this comment

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

I need to rebase anyway.

Choose a reason for hiding this comment

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

Just ping me when your refactoring is ready for review :-)

17.0 fix: #2573

Copy link
Member Author

Choose a reason for hiding this comment

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

Here you go. I also added an additional fix for the fact that stock moves were still created when increasing quantities on order lines of a confirmed (manual delivery) order. Test failures are unrelated and fixed in #2576

AdriaGForgeFlow and others added 24 commits February 18, 2025 16:14
[ADD] ability to change destination location in wizard
Currently translated at 100.0% (56 of 56 strings)

Translation: purchase-workflow-14.0/purchase-workflow-14.0-purchase_manual_delivery
Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-14-0/purchase-workflow-14-0-purchase_manual_delivery/sl/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: purchase-workflow-16.0/purchase-workflow-16.0-purchase_manual_delivery
Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-16-0/purchase-workflow-16-0-purchase_manual_delivery/
AlexPForgeFlow and others added 15 commits February 18, 2025 16:14
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: purchase-workflow-16.0/purchase-workflow-16.0-purchase_manual_delivery
Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-16-0/purchase-workflow-16-0-purchase_manual_delivery/
Currently translated at 100.0% (65 of 65 strings)

Translation: purchase-workflow-16.0/purchase-workflow-16.0-purchase_manual_delivery
Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-16-0/purchase-workflow-16-0-purchase_manual_delivery/it/
Currently translated at 100.0% (65 of 65 strings)

Translation: purchase-workflow-16.0/purchase-workflow-16.0-purchase_manual_delivery
Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-16-0/purchase-workflow-16-0-purchase_manual_delivery/es/
Currently translated at 100.0% (65 of 65 strings)

Translation: purchase-workflow-16.0/purchase-workflow-16.0-purchase_manual_delivery
Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-16-0/purchase-workflow-16-0-purchase_manual_delivery/it/
Fixed issues when uom_id and uom_po_id of a product are different.
- Comparison for Purchase Order Line pending to receive now uses the product quantity that uses the uom of the Purchase Order Line.
- When creating the Stock Move the quantity is now computed into the uom of the Stock Move
Fixes the following 2 errors:
- When the product_uom_qty changes the recompute is not triggered
- In case that existing_qty > product_uom_qty the line should not be pending_to_receive
Purchase Order Lines that are pending to receive are not considered for stock forecast. Thus, the manual and automatic reordering of products can be done despite having already create a Purchase Order and surpassing the max quantity. Confirmed Purchase Orders with pending to receive Purchase Order Lines are now considered at the forecast and the forecast report.
…' stock moves

Rename existing_qty to qty_in_receipt. It now reflects the quantity for which there
are pending stock moves and no longer includes the quantities of 'done' stock moves.

Co-authored-by: Cas Vissers <[email protected]>
@StefanRijnhart StefanRijnhart force-pushed the 18.0-mig-purchase_manual_delivery branch 2 times, most recently from 68afb36 to b8ffd5f Compare February 18, 2025 15:32
Copy link

@len-foss len-foss left a comment

Choose a reason for hiding this comment

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

LGTM with a minor comment

Comment on lines 29 to 31
order.pending_to_receive = True
if not any(order.mapped("order_line.pending_to_receive")):
order.pending_to_receive = False

Choose a reason for hiding this comment

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

order.pending_to_receive = any(order.mapped("order_line.pending_to_receive") would be simpler? Also I don't think you want orders with no line to be pending_to_receive.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed!

@StefanRijnhart StefanRijnhart force-pushed the 18.0-mig-purchase_manual_delivery branch from b8ffd5f to fca6f05 Compare February 18, 2025 20:01
To prevent interference from other modules. In this case, there is a purchase
order from Odoo's demo data in a confirmed state but without a picking for the
product. The missing picking causes  purchase_manual_delivery to include the
PO line's quantity in the forecasted quantity of the product. This prevents
the procurement to go through in TestPurchaseRequestProcurement.test_orderpoint.

```
 2024-09-26 14:16:17,327 269 ERROR odoo odoo.addons.purchase_request.tests.test_purchase_request_procurement: FAIL: TestPurchaseRequestProcurement.test_orderpoint
Traceback (most recent call last):
  File "/__w/purchase-workflow/purchase-workflow/purchase_request/tests/test_purchase_request_procurement.py", line 81, in test_orderpoint
    self.assertEqual(
AssertionError: 0.0 != 5
```
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

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.