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

[15.0][MIG] stock_change_qty_reason #1608

Merged
merged 30 commits into from
Jan 19, 2023

Conversation

MiquelRForgeFlow
Copy link
Contributor

Standard migration (without using stock_inventory module).

@MiquelRForgeFlow
Copy link
Contributor Author

/ocabot migration stock_change_qty_reason

@OCA-git-bot
Copy link
Contributor

Sorry @MiquelRForgeFlow you are not allowed to mark the addon tobe migrated.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 15.0-truemig-stock_change_qty_reason branch 3 times, most recently from 41fc231 to 8553eee Compare January 12, 2023 18:12
Copy link

@GuillemCForgeFlow GuillemCForgeFlow left a comment

Choose a reason for hiding this comment

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

You should extend the method _get_inventory_fields_write for the stock.quant model adding the two new fields. Otherwise, when applying the inventory adjustment it will fail.

@rousseldenis
Copy link
Contributor

/ocabot migration stock_change_qty_reason

@OCA-git-bot OCA-git-bot added this to the 15.0 milestone Jan 13, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Jan 13, 2023
80 tasks
@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 15.0-truemig-stock_change_qty_reason branch 4 times, most recently from 377be5f to 3c563c0 Compare January 13, 2023 10:59
@MiquelRForgeFlow
Copy link
Contributor Author

@GuillemCForgeFlow ready

Copy link

@GuillemCForgeFlow GuillemCForgeFlow left a comment

Choose a reason for hiding this comment

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

thank you 😄

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 15.0-truemig-stock_change_qty_reason branch 2 times, most recently from 247a60b to 4779479 Compare January 16, 2023 17:53
@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 15.0-truemig-stock_change_qty_reason branch from 4779479 to 51b9b0e Compare January 17, 2023 10:16
@AaronHForgeFlow
Copy link
Contributor

Why the reason is in both, the quants and the stock move lines?

@MiquelRForgeFlow
Copy link
Contributor Author

@AaronHForgeFlow In the quant, to be able to temporarily introduce the reason in the adjustment view. In the move line, because it's were it ends saved (inventory_history).

@rousseldenis
Copy link
Contributor

@AaronHForgeFlow In the quant, to be able to temporarily introduce the reason in the adjustment view. In the move line, because it's were it ends saved (inventory_history).

@MiquelRForgeFlow Shouldn't it be located in stock move instead? As stock.move.line seems too volatile for me to be as safe as wanted.

@MiquelRForgeFlow
Copy link
Contributor Author

@rousseldenis inventory history is handled by move lines, not moves, see odoo code here.

Have you tested functionally the module?

1st step:
Selection_727
2nd step:
Selection_728
3rd step:
Selection_729

@rousseldenis
Copy link
Contributor

@rousseldenis inventory history is handled by move lines, not moves, see odoo code here.

Have you tested functionally the module?

1st step: Selection_727 2nd step: Selection_728 3rd step: Selection_729

Yes of course. But inventories entry point is stock.move:

https://github.com/odoo/odoo/blob/15.0/addons/stock/models/stock_quant.py#L603

So, I would have put the reason fields on that level and relateds on move lines

@MiquelRForgeFlow
Copy link
Contributor Author

So, I would have put the reason fields on that level and relateds on move lines

@rousseldenis Yes, I could do that. Doable. But, in my opinion, what you propose doesn't improve anything substantially, it's practically the same. I prefer in stock move lines for continuity reasons. In this module, in v14, you could have a different reason for each inventory line, independently of the global reason of the stock inventory. Although in v15 each inventory stock move only has a move line, I wanted to maintain that feature of v14 in some way, and thus, deciding to put the reason in the stock move line. Imagine if a custom module enables doing adjustments to two products at the same time, and thus, generating a move with two stock move lines. Then, what if one may want a different reason for each move line?

@rousseldenis
Copy link
Contributor

rousseldenis commented Jan 17, 2023

s to two products at the same time, and thus, generating a move with two stock move lines.

It's impossible as it will generate one move per product.

But ok, let's go forward with that. I wanted to highlight the fact that quite every stock reporting is done on move level and people should be aware to get the information from the line.

@MiquelRForgeFlow
Copy link
Contributor Author

Ok, thanks, could you approve? :)

Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

Code review

@rousseldenis
Copy link
Contributor

@MiquelRForgeFlow Ready ?

@MiquelRForgeFlow
Copy link
Contributor Author

Yes

@rousseldenis
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 15.0-ocabot-merge-pr-1608-by-rousseldenis-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 9029dc9 into OCA:15.0 Jan 19, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 4968f45. Thanks a lot for contributing to OCA. ❤️

@MiquelRForgeFlow MiquelRForgeFlow deleted the 15.0-truemig-stock_change_qty_reason branch January 19, 2023 09:31
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.