-
-
Notifications
You must be signed in to change notification settings - Fork 196
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
[16.0][FIX] sale_stock_release_channel_partner_by_date*: fix computation of release channel #955
base: 16.0
Are you sure you want to change the base?
Conversation
@@ -42,7 +42,8 @@ def _compute_release_channel_id(self): | |||
if not rec._check_release_channel_partner_date_requirements(): | |||
continue | |||
channel_date = rec.release_channel_partner_date_id | |||
rec.release_channel_id = channel_date.release_channel_id | |||
if channel_date: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can check here that the warehouse is compatible with the currently selected channel? Like you did for the carrier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes indeed. But I have still one open questions about this (you can check the test where there are FIXME in this PR):
If we have configured a channel X (with a carrier) and Y (no carrier), and have a specific channel date set for X.
Currently if we do not define a carrier on the SO, the computed release channel wil be taken from the configured specific channel (as it's the only one that exists). So the SO will show the release channel X.
Then we validate the SO, and we trigger the channel assignment on the delivery: it is now assigned to the release channel Y (of course, there was no carrier defined, I guess it fallbacks on Y because of that).
IMO this is not what the user will expect.
How should we handle SO without carrier (same question for SO without warehouse) regarding release channels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you should not include False for warehouse/carrier in _get_release_channel_partner_date_domain
. The warehouse should be set by a default value. For the carrier, it is set by delivery_auto_refresh. So if one of them is empty, then we should not show a channel that has a restriction that is not respected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree (I don't remember why we searched on carrier_id=False in the initial implementation, we did that together and on purpose 🤔 ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the warehouse, while we could state it is always set on the SO (I could not find some without WH in customers DBs), it is however not mandatory on the release channel. We should consider these release channels as elligible, and thus keep our check on warehouse_id=False in the domain.
sale_stock_release_channel_partner_by_date_delivery/tests/test_sale_release_channel.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR has the |
be46bf4
to
ee3d2b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebalix Can you make one commit per module ?
@sebalix Can you make one commit per module ? (reminder) |
…of release channel
ee3d2b8
to
502c45b
Compare
@jbaudoux done, and I pushed two others changes (last two commits), can you do a pass on them? |
Aims to supersede #954