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][MIG] repair_picking_after_done: Migration to 17.0 #65

Open
wants to merge 11 commits into
base: 17.0
Choose a base branch
from

Conversation

ppyczko
Copy link
Contributor

@ppyczko ppyczko commented Nov 5, 2024

Module migrated to version 17.0

cc https://github.com/APSL 162593

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

Depends on:

An [IMP] has been added to prevent the creation of transfers exceeding remaining quantities in repair orders.

@ppyczko ppyczko force-pushed the 17.0-mig-repair_picking_after_done branch from 7c9e0f5 to 804879f Compare November 5, 2024 07:58
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

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

@ppyczko ppyczko force-pushed the 17.0-mig-repair_picking_after_done branch from 804879f to 3672843 Compare November 5, 2024 08:14
@ppyczko
Copy link
Contributor Author

ppyczko commented Nov 5, 2024

Hi @LoisRForgeFlow, sorry to ping directly. Could you please review this module and the dependency (#63) when you have a moment? Thank you!

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

@LoisRForgeFlow
Copy link

/ocabot migration repair_picking_after_done

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Nov 6, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request Nov 6, 2024
3 tasks
@LoisRForgeFlow
Copy link

@ppyczko Dependency merged, you can remove the requirements commit

@ppyczko ppyczko force-pushed the 17.0-mig-repair_picking_after_done branch from 3672843 to d2d752d Compare November 6, 2024 15:31
@ppyczko
Copy link
Contributor Author

ppyczko commented Nov 6, 2024

@ppyczko Dependency merged, you can remove the requirements commit

Done! Thank you @LoisRForgeFlow

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.

Overall looks good! One detail that I realized while testing (probably hapening in previous versions) is that when creating a backorder of one of these repair after done transfers, the backorder is not linked to the reapair order and you cannot find it navigating with the smart button.

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

@ppyczko
Copy link
Contributor Author

ppyczko commented Nov 12, 2024

Overall looks good! One detail that I realized while testing (probably hapening in previous versions) is that when creating a backorder of one of these repair after done transfers, the backorder is not linked to the reapair order and you cannot find it navigating with the smart button.

Thank you for taking the time to review this module and for your feedback @LoisRForgeFlow!

While reviewing your input, I found out that these backorders are not only not linked to the repair order, but they actually aren’t created at all. This is due to this piece of code in core of Odoo:

This behavior seems intentional and aligns with the idea that, once a repair is marked as done, the items are fully ready and available, not partially available.

In my view, this makes sense -- backorders after a completed repair could cause confusion. In my opinion, when a repair is completed, it implies that all items in that order are ready to be returned to the customer. A backorder might imply partial availability, which doesn’t seem to fit with the concept of a finished repair.

That said, I may be missing some use case or context here, so I’d really appreciate any thoughts or perspectives you might have on this. If there’s a scenario where backorders after a repair could add value, I’d love to understand it better.

After all, if you think we do need backorders, it would be as easy as overriding this _split method, and we could make that adjustment quickly.

@ppyczko ppyczko force-pushed the 17.0-mig-repair_picking_after_done branch from d2d752d to 9a3d37e Compare November 12, 2024 11:00
@LoisRForgeFlow
Copy link

@ppyczko Thanks for investigating and for the detailed feedback.

For me, Odoo has decided that they do not want to handle backorders of repairs, I am not against that as long as the code is consistent with that idea and it seems so, as you pointed out in the code. That part is fine, but here we are talking about an additional operation/transfer that takes places just after the repair is done, this operation is not anything else than an internal operation or a delivery like any other, it is just linked to the repair order for traceability purposes. Therefore from my perspective, this post-repair operation should allow backorders as any other, whatever the reason, I don't see why we should not allow that.

In short, I would keep restriction to split stock moves of the repair order and I would allow split in this related post-repair operations.

Does this make sense to you?

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.