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.

Sign up for GitHub

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

Jump to bottom

[15.0][IMP] mrp_workorder_sequence: Multiple improvements #921

Merged
merged 2 commits into from
Dec 28, 2022
Merged

[15.0][IMP] mrp_workorder_sequence: Multiple improvements #921

merged 2 commits into from
Dec 28, 2022

Conversation

grindtildeath
Copy link
Contributor

No description provided.

@OCA-git-bot
Copy link
Contributor

Hi @LoisRForgeFlow,
some modules you are maintaining are being modified, check this out!

@grindtildeath grindtildeath mentioned this pull request Dec 20, 2022
Merged
1 task
ivantodorovich
ivantodorovich approved these changes Dec 21, 2022
View reviewed changes
mrp_workorder_sequence/hooks.py Outdated Show resolved Hide resolved
mrp_workorder_sequence/models/mrp_workorder.py Outdated Show resolved Hide resolved
@LoisRForgeFlow
Copy link
Contributor

@grindtildeath Thanks for the proposal, it looks in general correct. From your comment above I understand it is still a work in progress, I will label it as such.

Please, ping me when ready to review.

@grindtildeath
Copy link
Contributor Author

@LoisRForgeFlow Thanks for checking in. The override of create function is improved in latest commit, I'll rebase and squash when ready to merge after reviews.

@LoisRForgeFlow
Copy link
Contributor

@grindtildeath functional review LGTM 👍

Could you squash the commits? we should be good to merge.

@grindtildeath
Copy link
Contributor Author

grindtildeath commented Dec 23, 2022 •
edited

@LoisRForgeFlow Thank you.

Just wondering, before we merge, shouldn't we do this change in this module?

https://github.com/OCA/manufacture/pull/922/files#diff-0b88d6e9569b2683f2777f452068029449a562f57c984924af27f423eb80af75R10

I didn't do it here since it wasn't strictly needed and could be a breaking change, but IMO it would make more sense to have it in this module. (but I also understand one could prefer keeping the original _order)

@LoisRForgeFlow
Copy link
Contributor

@LoisRForgeFlow Thank you.

Just wondering, before we merge, shouldn't we do this change in this module?

https://github.com/OCA/manufacture/pull/922/files#diff-0b88d6e9569b2683f2777f452068029449a562f57c984924af27f423eb80af75R10

I didn't do it here since it wasn't strictly needed and could be a breaking change, but IMO it would make more sense to have it in this module. (but I also understand one could prefer keeping the original _order)

I actually thought it when testing, so yes, if you don't mind I think it makes sense to include it here.

@grindtildeath
Copy link
Contributor Author

@LoisRForgeFlow done 👍

@LoisRForgeFlow
Copy link
Contributor

@grindtildeath thanks! time to squash then.

grindtildeath and others added 2 commits December 23, 2022 11:05
@grindtildeath
Ensure workorder sequence is applied on new workorders
0310520 
Test sequence is applied for multiple operations
Rewrite _reset_work_order_sequence in a more pythonic way
Change _order of mrp.workorder to use sequence
Set sequence on existing workorders after module install
@grindtildeath @ivantodorovich
Use SQL in post_init_hook to avoid potential Memory error
97e8a98 
Co-authored-by: Iván Todorovich <ivan.todorovich@gmail.com>
@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). 🤖

LoisRForgeFlow
LoisRForgeFlow approved these changes Dec 28, 2022
View reviewed changes
Copy link
Contributor

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

LGTM 👍

@LoisRForgeFlow
Copy link
Contributor

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 15.0-ocabot-merge-pr-921-by-LoisRForgeFlow-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit f84957e into OCA:15.0 Dec 28, 2022
@OCA-git-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivantodorovich ivantodorovich ivantodorovich approved these changes

@sebalix sebalix sebalix approved these changes

@LoisRForgeFlow LoisRForgeFlow LoisRForgeFlow approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@grindtildeath @OCA-git-bot @LoisRForgeFlow @ivantodorovich @sebalix