-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[13.0][ADD] base_time_window #1798
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] base_time_window #1798
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.
IMO, to be complete, we should add a demo module with the appropriate unittest. I prefer to keep the tests related to the logic implemented into this new addon into the same repository and without dependencies on other addon.
@lmignon I added a test_base_time_window module in the last commits, with your test suite and an extra one related to the usage I introduced with mixin. |
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.
Great. Thank you @grindtildeath for your refactoring and generalization of this transversal concept. (Code review)
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
# here we use a plain SQL query to benefit of the numrange | ||
# function available in PostgresSQL | ||
# (http://www.postgresql.org/docs/current/static/rangetypes.html) | ||
SQL = """ |
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'd split this to _get_time_window_sql
which returns (sql, sql_params)
|
||
_inherit = "res.partner" | ||
|
||
time_window_ids = fields.One2many( |
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 field and the method IMO could stay in another mixin time.window.consumer.mixin
89eb8ba
to
3fb64b3
Compare
3fb64b3
to
8e2c16c
Compare
/ocabot merge |
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at 19f7342. Thanks a lot for contributing to OCA. ❤️ |
This module provides base classes and models to manage time windows through
time.window.mixin
.Example implementation for the mixin can be found in module
test_base_time_window
.As a time window will always be linked to a related model thourgh a M2o relation,
when defining the new model inheriting the mixin, one should pay attention to the
following points in order to have the overlapping check work properly:
Define class property
_overlap_check_field
: This must state the M2o field touse for the to check of overlapping time window records linked to a specific
record of the related model.
Add the M2o field to the related model in the
api.constrains
:For example:
.. code-block:: python
class PartnerTimeWindow(models.Model):
_name = 'partner.time.window'
_inherit = 'time.window.mixin'
partner_id = fields.Many2one(
res.partner', required=True, index=True, ondelete='cascade'
)
_overlap_check_field = 'partner_id'
@api.constrains('partner_id')
def check_window_no_overlaps(self):
return super().check_window_no_overlaps()