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

[17.0][ADD] repair_stock_dest: New module repair_stock_dest #56

Closed
wants to merge 1 commit into from

Conversation

ppyczko
Copy link
Contributor

@ppyczko ppyczko commented Sep 19, 2024

This module enhances the repair process by adding the ability to transfer a repaired product to a specified destination location upon completion of the repair. It introduces a Product Destination Location field in the repair order, which defines where the repaired product will be transferred. The default value for this field is set from the Default Product Destination Location field of the associated Operation Type.

Upon the completion of a repair order, an additional stock move is automatically created to transfer the repaired product to the specified destination location.

cc https://github.com/APSL 160548

@miquelalzanillas @lbarry-apsl @mpascuall @peluko00 @javierobcn @BernatObrador please review

@ppyczko ppyczko force-pushed the 17.0-add-repair_stock_dest branch from 1dc8179 to 03746a8 Compare September 19, 2024 07:50
@ppyczko ppyczko marked this pull request as draft September 19, 2024 11:13
@ppyczko ppyczko force-pushed the 17.0-add-repair_stock_dest branch 3 times, most recently from cb50e7c to 5e739a7 Compare September 23, 2024 12:30
@ppyczko ppyczko marked this pull request as ready for review September 23, 2024 12:32
Copy link

@mpascuall mpascuall left a comment

Choose a reason for hiding this comment

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

LGTM, tested in runboat

@ppyczko ppyczko force-pushed the 17.0-add-repair_stock_dest branch from 5e739a7 to d15b5f6 Compare September 23, 2024 12:59
Copy link
Contributor

@peluko00 peluko00 left a comment

Choose a reason for hiding this comment

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

LGTM, code review and tested in runboat.

Copy link

@BernatObrador BernatObrador left a comment

Choose a reason for hiding this comment

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

LGTM, tested in runboat

@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). 🤖

Comment on lines 30 to 99
def action_repair_done(self):
res = super().action_repair_done()

for record in self:
record.move_id.location_dest_id = record.product_location_dest_id.id
return res

Choose a reason for hiding this comment

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

@ppyczko This location update seems less than ideal. You are changing the destination location of the repaired product after its completion. If you look at the parent method, the default inheritability of the method that creates the stock move is less than ideal. (And what about the quantities of the product that should be set in the stock.quant table? My guess would be that this is not updated properly.
I think you have 2 options here:

  1. Pass a context value in this method which should be detected the create method of the stock move, then prior to creation of the product stock move update the destination location.
  2. Change the implementation entirely by creating an additional stock move which will transfer the product to a different location after completing the repair. (I believe this will also make the traceability of the product clearer as the repair and the move are split and not done in the same operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the detailed feedback @PieterPaulussen. I went with option 2, of creating an additional stock move to transfer the repaired product, as it makes sense to have this move split from the repair move itself.

Could you review again please? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about it @LoisRForgeFlow?

@ppyczko ppyczko force-pushed the 17.0-add-repair_stock_dest branch from d15b5f6 to 662e0da Compare October 2, 2024 09:47
Copy link

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

@ppyczko
Copy link
Contributor Author

ppyczko commented Oct 25, 2024

Hi @LoisRForgeFlow, thank you for your input! I reviewed the module you suggested, and while both solutions aim to transfer the repaired product to another location, they differ in the user process.

In repair_picking_after_done, to complete the transfer, the user has to:

  1. Create the transfer.
  2. Manually set the destination location and quantity.
  3. Once the transfer is created, you need to set the quantities and validate it.

By contrast, in repair_stock_dest, our objective was to simplify this workflow by automating the transfer to a pre-defined location once the repair is completed. Additionally, if the user needs to change the destination, this can be done directly within the repair order, and the transfer will be handled automatically, eliminating the need for manual creation and validation.

Anyhow, I would appreciate your thoughts on this distinction. If you believe the modules overlap or think any modifications are needed, please let me know, any suggestions are welcome!

@LoisRForgeFlow
Copy link

@ppyczko Thanks for the comparison. Do you think it would make sense to make this module an extension of repair_picking_after_done? Another option is to extend with a repair_picking_after_done to decide if the transfer is automatic or not? This way we would have a more unified solution for transfers after repairs.

@ppyczko
Copy link
Contributor Author

ppyczko commented Oct 28, 2024

Thank you for your suggestion @LoisRForgeFlow! I think it sounds good to adapt this module to extend repair_picking_after_done and incorporate the ability to decide whether the transfer will be automatic or not. I'll start working on the migration to v17 of the repair_picking_after_done module and these changes.

@ppyczko
Copy link
Contributor Author

ppyczko commented Dec 17, 2024

Closing due to #72

@ppyczko ppyczko closed this Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants