-
-
Notifications
You must be signed in to change notification settings - Fork 871
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
[13.0] Add module partner_tz #876
Conversation
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 as far as I can tell
6d085cd
to
14765e0
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.
Looks good, could you check travis error?
@grindtildeath Hi, this module is a dependency for several modules you are proposing, if we get this merged it will ease testing and attract reviewers. Can you take a few minutes to fix travis and push this forward? 🚀 |
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.
Please limit the module to adding the timezone to the partner form.
But you might think about the suggested functionality of setting the default partner timezone based on the partners country...
@@ -0,0 +1,58 @@ | |||
# Copyright 2020 Camptocamp SA |
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.
The functions presented here are very thin wrappers around the original functions from pytz. I think this is of questionable value. Better use the original pytz functions that are generally available and not depending on having this particular module installed.
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.
Thx for your review @NL66278
The value of a function is quite subjective in any case, but to me they make tz conversions more explicit and avoid repeats. Moreover they are only tools that won't change anything when someone installs this module.
So I'm sorry but I don't see why having this file here, and using it in other modules having this module as a dependency would be a problem...
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.
Functional review
@grindtildeath I'd prefer explicit words in module name, partner_timezone is better |
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.
Code review. Apart module name
And some tests for conversion functions should be great |
/ocabot merge nobump |
Merging as Alpha version is set. Thank everyone for the review. |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at 89850e4. Thanks a lot for contributing to OCA. ❤️ |
This module removes timezone default value on res.partner and display the field
on form view.