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

[16.0][ADD] stock_release_channel_plan #695

Merged
merged 5 commits into from
Mar 27, 2024
Merged

[16.0][ADD] stock_release_channel_plan #695

merged 5 commits into from
Mar 27, 2024

Conversation

jbaudoux
Copy link
Contributor

@jbaudoux jbaudoux commented Jul 28, 2023 •
edited
Loading

Make a wake-up plan of release channels

image

cc @lmignon @sbejaoui @rousseldenis @sebalix @TDu

@jbaudoux jbaudoux changed the title [ADD] stock_release_channel_plan [16.0][ADD] stock_release_channel_plan Jul 28, 2023
@jbaudoux jbaudoux added this to the 16.0 milestone Jul 28, 2023
@jbaudoux jbaudoux force-pushed the 16-release_channel_plan branch from 5ebb109 to 4a70821 Compare July 28, 2023 16:05
@jbaudoux jbaudoux mentioned this pull request Jul 28, 2023
Merged
@jbaudoux jbaudoux force-pushed the 16-release_channel_plan branch 7 times, most recently from b488d3f to b9b4c75 Compare July 28, 2023 22:19
lmignon
lmignon suggested changes Aug 1, 2023
View reviewed changes
stock_release_channel_plan/wizards/launch_plan.py Outdated Show resolved Hide resolved
stock_release_channel_plan/wizards/launch_plan.py

def action_launch(self):
self.ensure_one()
channels_to_launch = self.preparation_plan_id._get_channels_to_launch()
Copy link
Contributor

Choose a reason for hiding this comment

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

@jbaudoux In our case, the channels to launch could be filtered according to additional criteria on the wizard. The _get_channels_to_launch method should therefore be provided by the wizard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done on purpose as the plan should define the rules and not the wizard. That's the purpose of having a plan. Otherwise you have half a plan and the wizard is providing the missing other half. This is not ideal.
I keep it like that for now.

@jbaudoux jbaudoux force-pushed the 16-release_channel_plan branch 3 times, most recently from f5da70f to 95029a9 Compare August 1, 2023 15:26
TDu
TDu approved these changes Aug 2, 2023
View reviewed changes
Copy link
Member

@TDu TDu left a comment

Choose a reason for hiding this comment

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

LG

@jbaudoux jbaudoux force-pushed the 16-release_channel_plan branch from 95029a9 to 47944ce Compare September 14, 2023 13:37
simahawk
simahawk requested changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

@simahawk simahawk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM but the missing handling of the wizard method

@simahawk
Copy link
Contributor

simahawk commented Oct 6, 2023

@lmignon good for you?

@lmignon
Copy link
Contributor

lmignon commented Oct 6, 2023

The problem is that these changes are not yet tested on our side. JE extracted this code from our project but didn't end the PR to use it back. We've to validate that we can still covers our needs with the change he made into the implementation.

@simahawk
Copy link
Contributor

simahawk commented Oct 7, 2023

Good to know, thanks for your feedback :) Have nice weekend!

cyrilmanuel
cyrilmanuel approved these changes Nov 16, 2023
View reviewed changes
Copy link

@cyrilmanuel cyrilmanuel left a comment

Choose a reason for hiding this comment

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

not an expert but LGTM

@grindtildeath
Copy link
Contributor

jbaudoux#1

@grindtildeath grindtildeath mentioned this pull request Jan 24, 2024
Merged
3 tasks
@jbaudoux jbaudoux force-pushed the 16-release_channel_plan branch from 26f913e to 145d1c5 Compare February 22, 2024 14:51
@jbaudoux
Copy link
Contributor Author

rebased

@jbaudoux
Copy link
Contributor Author

@grindtildeath Can you check why stock_release_channel_propagate_channel_picking is now failing ?

@grindtildeath
Copy link
Contributor

grindtildeath commented Feb 26, 2024 •
edited
Loading

@jbaudoux The test is failing because of this SQL query: https://github.com/OCA/wms/blob/16.0/stock_release_channel_process_end_time/models/stock_picking.py#L40-L56

Basically the query is returning pickings having a scheduled date that is before any channel that has a process_end_date defined.

As we are adding new channels as demo data, these channels will have a process_end_date defined to today when the module is installed, and as tests are frozen in time in 2023, the pickings will always be scheduled before a channel. I'm not sure what is the right way to fix this as it took me already quite a while to figure out what's wrong and why 2023/01/28 was before 2024/02/27 😵

EDIT: please check jbaudoux#3

@jbaudoux jbaudoux force-pushed the 16-release_channel_plan branch from e534373 to c1cdc13 Compare March 21, 2024 15:24
@jbaudoux
Copy link
Contributor Author

rebased

@jbaudoux jbaudoux force-pushed the 16-release_channel_plan branch from c1cdc13 to 42ad1f2 Compare March 22, 2024 12:30
@jbaudoux
Copy link
Contributor Author

I fixed test in stock_release_channel_propagate_channel_picking. Now it's again green :)
@lmignon Can we move on and merge this one ?

@tuantrantg tuantrantg mentioned this pull request Mar 26, 2024
Closed
@cyrilmanuel
Copy link

@lmignon , little ping, is it good for you ?

cyrilmanuel
cyrilmanuel approved these changes Mar 26, 2024
View reviewed changes
Copy link

@cyrilmanuel cyrilmanuel left a comment

Choose a reason for hiding this comment

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

LGTM

@lmignon
Copy link
Contributor

lmignon commented Mar 26, 2024

I fixed test in stock_release_channel_propagate_channel_picking. Now it's again green :) @lmignon Can we move on and merge this one ?

@jbaudoux Yes, you can.

@jbaudoux
Copy link
Contributor Author

/ocabot merge minor

1 similar comment
@jbaudoux
Copy link
Contributor Author

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-695-by-jbaudoux-bump-minor, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Mar 27, 2024
@OCA-git-bot
Merge PR #695 into 16.0
2ae8c9f 
Signed-off-by jbaudoux
@OCA-git-bot
Copy link
Contributor

@jbaudoux your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-695-by-jbaudoux-bump-minor.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@jbaudoux jbaudoux force-pushed the 16-release_channel_plan branch from 3893388 to 3bff483 Compare March 27, 2024 08:10
jbaudoux and others added 3 commits March 27, 2024 10:37
@jbaudoux
fixup! [ADD] stock_release_channel_plan
82ec2d7
@grindtildeath @jbaudoux
[IMP] stock_release_channel_plan: weekdays
9ff70eb 
Add preparation weekdays to stock.release.channel
@jbaudoux
[FIX] stock_release_channel_propagate_channel_picking
107c990 
Fix failing test when more channels are loaded
@jbaudoux jbaudoux force-pushed the 16-release_channel_plan branch from 3bff483 to 107c990 Compare March 27, 2024 09:37
@jbaudoux
[FIX] stock_release_channel_shipment_lead_time
824be0a 
Fix failing tests when demo channels are already asleep
@jbaudoux jbaudoux force-pushed the 16-release_channel_plan branch from 1c4c634 to 824be0a Compare March 27, 2024 09:58
@jbaudoux
Copy link
Contributor Author

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-695-by-jbaudoux-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 27230cc into OCA:16.0 Mar 27, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 47be667. 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

@santostelmo santostelmo santostelmo left review comments

@simahawk simahawk simahawk approved these changes

@lmignon lmignon lmignon requested changes

@cyrilmanuel cyrilmanuel cyrilmanuel approved these changes

@TDu TDu TDu approved these changes

Assignees
No one assigned
Labels
merged 🎉
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
8 participants
@jbaudoux @simahawk @lmignon @grindtildeath @cyrilmanuel @OCA-git-bot @TDu @santostelmo