Issue 355381002: party_relationship: Add start and end dates to relation (Closed)
Descriptionissue4440
COLLABORATOR=cedric.krier@b2ck.com
Patch Set 1 #Patch Set 2 : Update start date from create_date #
Total comments: 15
Patch Set 3 : Rename start/end to start/end_date #
Total comments: 2
Patch Set 4 : Quote labels #Patch Set 5 : Quote labels #
Total comments: 2
Patch Set 6 : Label #
Total comments: 4
Patch Set 7 : Remove required #
Total comments: 3
Patch Set 8 : Domain eval + changelog #
Total comments: 5
Patch Set 9 : Add active field #
MessagesTotal messages: 44
flake8 OK URL: https://codereview.tryton.org/355381002
Sign in to reply to this message.
We need a CHANGELOG entry https://codereview.tryton.org/355381002/diff/353721002/party.py File party.py (right): https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 party.py:52: start = fields.Date('Start', required=True) should be called "start_date" https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 party.py:52: start = fields.Date('Start', required=True) I prefer if the value is not required https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 party.py:52: start = fields.Date('Start', required=True) Should have a domain that it's before the end_date https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 party.py:52: start = fields.Date('Start', required=True) We use double quotes for human readable strings. https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode53 party.py:53: end = fields.Date('End', domain=[ Idem about field name and double quotes. https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode70 party.py:70: # Migration from 6.0: set start date from create_date Tehre is no need for migration if the field is not requried. https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode77 party.py:77: def default_start(): Not needed if the field is not required.
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/355381002
Sign in to reply to this message.
https://codereview.tryton.org/355381002/diff/353721002/party.py File party.py (right): https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 party.py:52: start = fields.Date('Start', required=True) On 2021/07/09 08:43:34, pokoli wrote: > We use double quotes for human readable strings. Out of curiosity: Since when is this a guideline and why? There are many other occurences of single quotes for field names. I don't find it in https://www.tryton.org/develop/guidelines/code neither is it part of PEP8 https://www.python.org/dev/peps/pep-0008/#string-quotes ...
Sign in to reply to this message.
https://codereview.tryton.org/355381002/diff/353721002/party.py File party.py (right): https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 party.py:52: start = fields.Date('Start', required=True) On 2021/07/09 11:28:53, yangoon wrote: > On 2021/07/09 08:43:34, pokoli wrote: > > We use double quotes for human readable strings. > > Out of curiosity: Since when is this a guideline and why? There are many other > occurences of single quotes for field names. IIRC since two years we are using this convention but only for new code. Updating all codebase is a time consuming task with not so much benefit.
Sign in to reply to this message.
https://codereview.tryton.org/355381002/diff/353721002/party.py File party.py (right): https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 party.py:52: start = fields.Date('Start', required=True) On 2021/07/09 11:35:30, pokoli wrote: > On 2021/07/09 11:28:53, yangoon wrote: > > On 2021/07/09 08:43:34, pokoli wrote: > > > We use double quotes for human readable strings. > > > > Out of curiosity: Since when is this a guideline and why? There are many other > > occurences of single quotes for field names. > > IIRC since two years we are using this convention but only for new code. > Updating all codebase is a time consuming task with not so much benefit. I am wondering what is the benefit at all?
Sign in to reply to this message.
https://codereview.tryton.org/355381002/diff/353721002/party.py File party.py (right): https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 party.py:52: start = fields.Date('Start', required=True) On 2021/07/09 08:43:34, pokoli wrote: > I prefer if the value is not required Still missing https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 party.py:52: start = fields.Date('Start', required=True) On 2021/07/09 08:43:34, pokoli wrote: > We use double quotes for human readable strings. Still missing https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 party.py:52: start = fields.Date('Start', required=True) On 2021/07/09 08:43:34, pokoli wrote: > Should have a domain that it's before the end_date Still missing https://codereview.tryton.org/355381002/diff/342701002/CHANGELOG File CHANGELOG (right): https://codereview.tryton.org/355381002/diff/342701002/CHANGELOG#newcode1 CHANGELOG:1: * Add start/end date relationship start/end -> start and end https://codereview.tryton.org/355381002/diff/342701002/CHANGELOG#newcode1 CHANGELOG:1: * Add start/end date relationship date relationship -> dates to relationship
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/355381002
Sign in to reply to this message.
https://codereview.tryton.org/355381002/diff/353721002/party.py File party.py (right): https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 party.py:52: start = fields.Date('Start', required=True) On 2021/07/09 08:43:34, pokoli wrote: > Should have a domain that it's before the end_date Not required. In case add "start_date", the "end_date" is > "start_date"
Sign in to reply to this message.
https://codereview.tryton.org/355381002/diff/353721002/party.py File party.py (right): https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 party.py:52: start = fields.Date('Start', required=True) On 2021/07/12 06:56:24, raimon wrote: > On 2021/07/09 08:43:34, pokoli wrote: > > Should have a domain that it's before the end_date > > Not required. In case add "start_date", the "end_date" is > "start_date" I do not agree. With your proposal only the end date will be shown as wrong but the problem is generated by both dates. So better to have the domain on both ones. Furthermore, your proposal is incoherent with the behaviour on other modules (for example fiscalyear)
Sign in to reply to this message.
https://codereview.tryton.org/355381002/diff/338461002/party.py File party.py (right): https://codereview.tryton.org/355381002/diff/338461002/party.py#newcode52 party.py:52: start_date = fields.Date("Start", required=True) I think it's better labeled: "Start Date". Is more concrete. https://codereview.tryton.org/355381002/diff/338461002/party.py#newcode52 party.py:52: start_date = fields.Date("Start", required=True) I still think we should not make it required (and not sure why you are ignoring my comment). There are cases where we may not know when the relation started, so we should not enter an arbitary value but leave the field empty.
Sign in to reply to this message.
On 2021/07/12 07:02:56, pokoli wrote: > https://codereview.tryton.org/355381002/diff/338461002/party.py > File party.py (right): > > https://codereview.tryton.org/355381002/diff/338461002/party.py#newcode52 > party.py:52: start_date = fields.Date("Start", required=True) > I think it's better labeled: "Start Date". > Is more concrete. > > https://codereview.tryton.org/355381002/diff/338461002/party.py#newcode52 > party.py:52: start_date = fields.Date("Start", required=True) > I still think we should not make it required (and not sure why you are ignoring > my comment). > > There are cases where we may not know when the relation started, so we should > not enter an arbitary value but leave the field empty. Start date is the date that start you relationship.
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/355381002
Sign in to reply to this message.
On 2021/07/09 11:53:49, yangoon wrote: > https://codereview.tryton.org/355381002/diff/353721002/party.py > File party.py (right): > > https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 > party.py:52: start = fields.Date('Start', required=True) > On 2021/07/09 11:35:30, pokoli wrote: > > On 2021/07/09 11:28:53, yangoon wrote: > > > On 2021/07/09 08:43:34, pokoli wrote: > > > > We use double quotes for human readable strings. > > > > > > Out of curiosity: Since when is this a guideline and why? There are many > other > > > occurences of single quotes for field names. > > > > IIRC since two years we are using this convention but only for new code. > > Updating all codebase is a time consuming task with not so much benefit. > > I am wondering what is the benefit at all? Ping. If you see yourself not so much benefit why should we require contributors to follow such undocumented guidelines? What is the benefit? Just have labels uniformly double-quoted?
Sign in to reply to this message.
On 2021/07/12 07:12:06, raimon wrote: > On 2021/07/12 07:02:56, pokoli wrote: > > https://codereview.tryton.org/355381002/diff/338461002/party.py > > File party.py (right): > > > > https://codereview.tryton.org/355381002/diff/338461002/party.py#newcode52 > > party.py:52: start_date = fields.Date("Start", required=True) > > I think it's better labeled: "Start Date". > > Is more concrete. > > > > https://codereview.tryton.org/355381002/diff/338461002/party.py#newcode52 > > party.py:52: start_date = fields.Date("Start", required=True) > > I still think we should not make it required (and not sure why you are > ignoring > > my comment). > > > > There are cases where we may not know when the relation started, so we should > > not enter an arbitary value but leave the field empty. > > Start date is the date that start you relationship. As this is relationship between parties you may not know. For example, I know that you have a relationship with your wife but I do not know when it started.
Sign in to reply to this message.
On 2021/07/12 07:46:15, yangoon wrote: > On 2021/07/09 11:53:49, yangoon wrote: > > https://codereview.tryton.org/355381002/diff/353721002/party.py > > File party.py (right): > > > > https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 > > party.py:52: start = fields.Date('Start', required=True) > > On 2021/07/09 11:35:30, pokoli wrote: > > > On 2021/07/09 11:28:53, yangoon wrote: > > > > On 2021/07/09 08:43:34, pokoli wrote: > > > > > We use double quotes for human readable strings. > > > > > > > > Out of curiosity: Since when is this a guideline and why? There are many > > other > > > > occurences of single quotes for field names. > > > > > > IIRC since two years we are using this convention but only for new code. > > > Updating all codebase is a time consuming task with not so much benefit. > > > > I am wondering what is the benefit at all? > > Ping. If you see yourself not so much benefit why should we require contributors > to follow such undocumented guidelines? What is the benefit? Just have labels > uniformly double-quoted? For no benefit I mean there is not so much benefit by converting all the existing codebase. BTW: I do not think this is the right place to discuss such guidelines
Sign in to reply to this message.
For me it is missing a mechanism that prevent to use the relation outside the date range. https://codereview.tryton.org/355381002/diff/346631002/party.py File party.py (right): https://codereview.tryton.org/355381002/diff/346631002/party.py#newcode52 party.py:52: start_date = fields.Date("Start Date", required=True) For me start_date must be optional. For example a relationship could exist since the existence of both parties or the user may not know. https://codereview.tryton.org/355381002/diff/346631002/party.py#newcode56 party.py:56: ('end_date', '>', Eval('start_date')), I think we should allow the same date. https://codereview.tryton.org/355381002/diff/346631002/party.py#newcode78 party.py:78: return datetime.datetime.now().date() For me a relationship does not start when the user encode it. https://codereview.tryton.org/355381002/diff/346631002/view/relation_form.xml File view/relation_form.xml (right): https://codereview.tryton.org/355381002/diff/346631002/view/relation_form.xml... view/relation_form.xml:14: <field name="end_date"/> I think a better layout should be find as this is on 6 columns and dates are only using 4. Maybe we should left empty the 2 middle columns.
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/355381002
Sign in to reply to this message.
I think the distance functions fields should take in account the dates of the relation. https://codereview.tryton.org/355381002/diff/344521002/party.py File party.py (right): https://codereview.tryton.org/355381002/diff/344521002/party.py#newcode53 party.py:53: ('start_date', '<=', Eval('end_date')), We are missing a clause to allow None values https://codereview.tryton.org/355381002/diff/344521002/party.py#newcode59 party.py:59: ('end_date', '>', Eval('start_date')), It does not make sense to compare to start_date when the latter is not set.
Sign in to reply to this message.
On 2021/07/12 08:17:39, pokoli wrote: > On 2021/07/12 07:46:15, yangoon wrote: > > On 2021/07/09 11:53:49, yangoon wrote: > > > https://codereview.tryton.org/355381002/diff/353721002/party.py > > > File party.py (right): > > > > > > https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 > > > party.py:52: start = fields.Date('Start', required=True) > > > On 2021/07/09 11:35:30, pokoli wrote: > > > > On 2021/07/09 11:28:53, yangoon wrote: > > > > > On 2021/07/09 08:43:34, pokoli wrote: > > > > > > We use double quotes for human readable strings. > > > > > > > > > > Out of curiosity: Since when is this a guideline and why? There are many > > > other > > > > > occurences of single quotes for field names. > > > > > > > > IIRC since two years we are using this convention but only for new code. > > > > Updating all codebase is a time consuming task with not so much benefit. > > > > > > I am wondering what is the benefit at all? > > > > Ping. If you see yourself not so much benefit why should we require > contributors > > to follow such undocumented guidelines? What is the benefit? Just have labels > > uniformly double-quoted? > > For no benefit I mean there is not so much benefit by converting all the > existing codebase. Really, please help me to understand the benefit. What is the benefit to have double-quoted field labels instead of single-quoted?
Sign in to reply to this message.
https://codereview.tryton.org/355381002/diff/344521002/CHANGELOG File CHANGELOG (right): https://codereview.tryton.org/355381002/diff/344521002/CHANGELOG#newcode1 CHANGELOG:1: * Add start/end date relationship date relationship -> date to relationship
Sign in to reply to this message.
On 2021/07/12 18:12:57, yangoon wrote: > On 2021/07/12 08:17:39, pokoli wrote: > > On 2021/07/12 07:46:15, yangoon wrote: > > > On 2021/07/09 11:53:49, yangoon wrote: > > > > https://codereview.tryton.org/355381002/diff/353721002/party.py > > > > File party.py (right): > > > > > > > > https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 > > > > party.py:52: start = fields.Date('Start', required=True) > > > > On 2021/07/09 11:35:30, pokoli wrote: > > > > > On 2021/07/09 11:28:53, yangoon wrote: > > > > > > On 2021/07/09 08:43:34, pokoli wrote: > > > > > > > We use double quotes for human readable strings. > > > > > > > > > > > > Out of curiosity: Since when is this a guideline and why? There are > many > > > > other > > > > > > occurences of single quotes for field names. > > > > > > > > > > IIRC since two years we are using this convention but only for new code. > > > > > > Updating all codebase is a time consuming task with not so much benefit. > > > > > > > > I am wondering what is the benefit at all? > > > > > > Ping. If you see yourself not so much benefit why should we require > > contributors > > > to follow such undocumented guidelines? What is the benefit? Just have > labels > > > uniformly double-quoted? > > > > For no benefit I mean there is not so much benefit by converting all the > > existing codebase. > > Really, please help me to understand the benefit. What is the benefit to have > double-quoted field labels instead of single-quoted? I don't know the reason, but an example: fields.Char("It's a demo")
Sign in to reply to this message.
On 2021/07/13 05:42:58, raimon wrote: > On 2021/07/12 18:12:57, yangoon wrote: > > On 2021/07/12 08:17:39, pokoli wrote: > > > On 2021/07/12 07:46:15, yangoon wrote: > > > > On 2021/07/09 11:53:49, yangoon wrote: > > > > > https://codereview.tryton.org/355381002/diff/353721002/party.py > > > > > File party.py (right): > > > > > > > > > > > https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 > > > > > party.py:52: start = fields.Date('Start', required=True) > > > > > On 2021/07/09 11:35:30, pokoli wrote: > > > > > > On 2021/07/09 11:28:53, yangoon wrote: > > > > > > > On 2021/07/09 08:43:34, pokoli wrote: > > > > > > > > We use double quotes for human readable strings. > > > > > > > > > > > > > > Out of curiosity: Since when is this a guideline and why? There are > > many > > > > > other > > > > > > > occurences of single quotes for field names. > > > > > > > > > > > > IIRC since two years we are using this convention but only for new > code. > > > > > > > > Updating all codebase is a time consuming task with not so much > benefit. > > > > > > > > > > I am wondering what is the benefit at all? > > > > > > > > Ping. If you see yourself not so much benefit why should we require > > > contributors > > > > to follow such undocumented guidelines? What is the benefit? Just have > > labels > > > > uniformly double-quoted? > > > > > > For no benefit I mean there is not so much benefit by converting all the > > > existing codebase. > > > > Really, please help me to understand the benefit. What is the benefit to have > > double-quoted field labels instead of single-quoted? > > I don't know the reason, but an example: > > fields.Char("It's a demo") And what about fields.Char('Show quotes (")') ? There seems to be no reason, but just a convention. If such a convention is required from contributors it would be better to put them beforehand in the guidelines.
Sign in to reply to this message.
https://codereview.tryton.org/355381002/diff/373081002/party.py#newcode53 party.py:53: continuation line over-indented for visual indent https://codereview.tryton.org/355381002/diff/373081002/party.py#newcode58 party.py:58: continuation line over-indented for visual indent URL: https://codereview.tryton.org/355381002
Sign in to reply to this message.
On 2021/07/13 07:16:52, yangoon wrote: > On 2021/07/13 05:42:58, raimon wrote: > > On 2021/07/12 18:12:57, yangoon wrote: > > > On 2021/07/12 08:17:39, pokoli wrote: > > > > On 2021/07/12 07:46:15, yangoon wrote: > > > > > On 2021/07/09 11:53:49, yangoon wrote: > > > > > > https://codereview.tryton.org/355381002/diff/353721002/party.py > > > > > > File party.py (right): > > > > > > > > > > > > > > https://codereview.tryton.org/355381002/diff/353721002/party.py#newcode52 > > > > > > party.py:52: start = fields.Date('Start', required=True) > > > > > > On 2021/07/09 11:35:30, pokoli wrote: > > > > > > > On 2021/07/09 11:28:53, yangoon wrote: > > > > > > > > On 2021/07/09 08:43:34, pokoli wrote: > > > > > > > > > We use double quotes for human readable strings. > > > > > > > > > > > > > > > > Out of curiosity: Since when is this a guideline and why? There > are > > > many > > > > > > other > > > > > > > > occurences of single quotes for field names. > > > > > > > > > > > > > > IIRC since two years we are using this convention but only for new > > code. > > > > > > > > > > Updating all codebase is a time consuming task with not so much > > benefit. > > > > > > > > > > > > I am wondering what is the benefit at all? > > > > > > > > > > Ping. If you see yourself not so much benefit why should we require > > > > contributors > > > > > to follow such undocumented guidelines? What is the benefit? Just have > > > labels > > > > > uniformly double-quoted? > > > > > > > > For no benefit I mean there is not so much benefit by converting all the > > > > existing codebase. > > > > > > Really, please help me to understand the benefit. What is the benefit to > have > > > double-quoted field labels instead of single-quoted? > > > > I don't know the reason, but an example: > > > > fields.Char("It's a demo") > > And what about fields.Char('Show quotes (")') ? > > There seems to be no reason, but just a convention. If such a convention is > required from contributors it would be better to put them beforehand in the > guidelines. See: https://codereview.tryton.org/342711002/
Sign in to reply to this message.
https://codereview.tryton.org/355381002/diff/373081002/party.py File party.py (right): https://codereview.tryton.org/355381002/diff/373081002/party.py#newcode52 party.py:52: If(Eval('start_date') & Eval('end_date'), Being pyson typed, shouldn't it be: If(Bool(Eval('start_date')) & Bool(Eval('end_date')) ....) ??
Sign in to reply to this message.
https://codereview.tryton.org/355381002/diff/373081002/party.py File party.py (right): https://codereview.tryton.org/355381002/diff/373081002/party.py#newcode52 party.py:52: If(Eval('start_date') & Eval('end_date'), On 2021/07/13 09:36:32, albert wrote: > Being pyson typed, shouldn't it be: > > If(Bool(Eval('start_date')) & Bool(Eval('end_date')) ....) > > ?? no.
Sign in to reply to this message.
Still missing a mechanism to prevent to use the relation when it is not more effective. I think about having an computed active a little bit like in account with ActivePeriodMixin. https://codereview.tryton.org/355381002/diff/373081002/view/relation_form.xml File view/relation_form.xml (right): https://codereview.tryton.org/355381002/diff/373081002/view/relation_form.xml... view/relation_form.xml:11: <newline/> no need for new line but you can let an empty line to ease reading. https://codereview.tryton.org/355381002/diff/373081002/view/relation_form.xml... view/relation_form.xml:15: <field name="end_date"/> I think this make an unbalanced view as the end date will below the type. I'm wondering if it will not be better to set empty label between dates. https://codereview.tryton.org/355381002/diff/373081002/view/relation_tree.xml File view/relation_tree.xml (right): https://codereview.tryton.org/355381002/diff/373081002/view/relation_tree.xml... view/relation_tree.xml:9: <field name="end_date"/> I'm not sure it needs to be displayed.
Sign in to reply to this message.
On 2021/08/13 09:17:40, ced wrote: > Still missing a mechanism to prevent to use the relation when it is not more > effective. I don't understang about "missing mechanism..." > I think about having an computed active a little bit like in account with > ActivePeriodMixin. Are you like to add the "active" function field ? (search...)
Sign in to reply to this message.
On 2021/08/16 14:40:31, raimon wrote: > On 2021/08/13 09:17:40, ced wrote: > > Still missing a mechanism to prevent to use the relation when it is not more > > effective. > > I don't understang about "missing mechanism..." It is missing a way to prevent to use the relation when it is not more effective. > > I think about having an computed active a little bit like in account with > > ActivePeriodMixin. > > Are you like to add the "active" function field ? (search...) I think it will be a workable mechanism.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/355381002
Sign in to reply to this message.
New changeset e3ef2a617aa3 by Cédric Krier in branch 'default': Add start and end dates to relation https://hg.tryton.org/modules/party_relationship/rev/e3ef2a617aa3
Sign in to reply to this message.
New changeset 3832e8b0dcd5 by Cédric Krier in branch 'default': Add start and end dates to relation https://hg.tryton.org/tryton-env/rev/3832e8b0dcd5
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
