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

[IMP] stock_release_channel_process_end_time: freeze_schedule_date option #19

Conversation

anothingguy
Copy link

@anothingguy anothingguy commented Sep 18, 2023

running the test cases for OCA#750 failed because stock_release_channel_process_end_time extended method _after_release_set_expected_date to change scheduled_date which makes test case in stock_available_to_promise_release failed: link.
It's useful to add freeze_scheduled_date in context to freeze it for some special cases

@anothingguy
Copy link
Author

Hi @rousseldenis, could you take a look this issue when you have a chance, thanks

@@ -69,6 +69,9 @@ def _after_release_set_expected_date(self):
rec.release_channel_id
and rec.release_channel_id.process_end_date
and rec.scheduled_date != rec.release_channel_id.process_end_date
# freeze scheduled_date in some cases don't need to change scheduled_date
# like in the test environemnt
and not self.env.context.get('freeze_scheduled_date')
Copy link
Member

Choose a reason for hiding this comment

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

@anothingguy Nope. Introducing something only for tests is bad.

Instead, I would have added a configuration parameter boolean in release channel configuration to enable the date change at release.

So, in tests, this can be tested with and without the feature.

Copy link
Author

Choose a reason for hiding this comment

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

I've just added a configuration parameter boolean in release channel configuration instead of using context

Copy link
Author

Choose a reason for hiding this comment

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

Hi @rousseldenis, could you take a look at it since it's breaking the tests in stock_available_to_promise_release module

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

But where is it broken as 16.0 branch is green ?

Copy link
Author

Choose a reason for hiding this comment

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

yes, it's broken the tests here in my module which needs to depends on stock_release_channel_process_end_time. I'm not sure why your tests was green previous time, but I did run the tests with your module in the local and failed as well. seems odd.

@anothingguy anothingguy force-pushed the 16.0-release_channel_date_freeze_schedule_date branch from db07e38 to c9fcf19 Compare September 18, 2023 13:29
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (16.0-release_channel_date-dro@5809742). Click here to learn what that means.
The diff coverage is n/a.

@@                       Coverage Diff                        @@
##             16.0-release_channel_date-dro      #19   +/-   ##
================================================================
  Coverage                                 ?   95.79%           
================================================================
  Files                                    ?       53           
  Lines                                    ?     2428           
  Branches                                 ?      221           
================================================================
  Hits                                     ?     2326           
  Misses                                   ?       73           
  Partials                                 ?       29           

@rousseldenis rousseldenis merged commit da2138b into acsone:16.0-release_channel_date-dro Sep 19, 2023
3 of 4 checks passed
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.

3 participants