-
-
Notifications
You must be signed in to change notification settings - Fork 999
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
[WIP][DO NOT MERGE][13.0] Add sale_warehouse_calendar #1696
Conversation
Hi @mmequignon! Thank you very much for this contribution. As the addon you are improving does not have a declared maintainer, I take the opportunity to mention that you can consider adopting it. To do so, please read the maintainer role description, and, if interested, create a pull request to add your GitHub login to the |
cd70f9a
to
03c0210
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.
Pre-approving, one small comment
- stage: test | ||
env: | ||
- TESTS="1" ODOO_REPO="OCA/OCB" | ||
- TESTS="1" ODOO_REPO="OCA/OCB" EXCLUDE="sale_warehouse_calendar" |
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.
Can you add a comment about the conflicting module?
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.
done in the doc.
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.
are you sure you are running your tests w/ these settings?
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.
There is likely an error in the computation of the date planned
18f61c7
to
f320d38
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.
Thanks for the comments. Code is very understandable
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.
check the tests pls
- stage: test | ||
env: | ||
- TESTS="1" ODOO_REPO="OCA/OCB" | ||
- TESTS="1" ODOO_REPO="OCA/OCB" EXCLUDE="sale_warehouse_calendar" |
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.
are you sure you are running your tests w/ these settings?
f320d38
to
c571fdb
Compare
c571fdb
to
dc4df08
Compare
DDMRP is broken when a calendar is set on the warehouse. We need to be able to set a calendar whithout breaking DDMRP. Remove this commit when DDMRP is fixed.
Do not merge this (see the last commit description) |
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.
This last commit is not fixing the hours issue in the right way.
If calendar plan_day is hours aware, then it should be fixed just before the call to that method.
@@ -41,6 +41,11 @@ def _sale_warehouse_calendar_expected_date(self, expected_date): | |||
) | |||
# add back the security lead | |||
expected_date += td_security_lead | |||
# This should be the delivery date. | |||
# It is up to the carrier to determine the hour with the customer | |||
expected_date = expected_date.replace( |
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.
This is not TZ compliant and won't work for countries in UTC-X.
I would not change the hours here. If this is necessary in a later calendar computation, it is there that it should be fixed.
workload_days, date_planned, compute_leaves=True | ||
) | ||
res["date_planned"] = date_planned.replace( |
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.
Same remark as above
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
Work in progress
Needs fixing
Do not merge
Computes sale order lead times with respect to to Warehouse calendar