-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
Convert relationship start & end dates to datepicker #15637
Conversation
(Standard links)
|
@@ -511,9 +538,6 @@ public static function relationship(&$form) { | |||
['id' => 'relation_target_group', 'multiple' => 'multiple', 'class' => 'crm-select2'] | |||
); | |||
} | |||
CRM_Core_Form_Date::buildDateRange($form, 'relation_start_date', 1, '_low', '_high', ts('From:'), FALSE, FALSE); | |||
CRM_Core_Form_Date::buildDateRange($form, 'relation_end_date', 1, '_low', '_high', ts('From:'), FALSE, FALSE); | |||
|
|||
CRM_Core_Form_Date::buildDateRange($form, 'relation_active_period_date', 1, '_low', '_high', ts('From:'), FALSE, FALSE); |
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.
@eileenmcnaughton not wanting to convert this one yet?
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.
@seamuslee001 nope - next round
@seamuslee001 also - I feel like
|
38d3909
to
ad9d50c
Compare
@eileenmcnaughton i tested this and it feels like its got some birth_date stuff in there etc but thats ok, i also notice you haven't done the upgrade for the relationship date stuff. However when i manually put in the upgrade stuff its all good and smart groups work right. |
@eileenmcnaughton needs a rebase and needs the relationship date upgrade steps added |
12ccd8b
to
1cbe1bf
Compare
@seamuslee001 hopefully that is right now |
All looks good to me now Merge on pass |
actaully @eileenmcnaughton one thing you have missed you need to update the array in |
@eileenmcnaughton no because it will only process the fields |
@seamuslee001 but didn't we rename the fields first? |
we don't rename the fields within the relative array |
also its like relation_end and relation_start in the relative array key of formValues |
This is a sample form values before upgrade
After the upgrade with your PR applied this is the form values
Note we still have the relative_dates array and also note that rather than having relationship_start_date_relative in the formValues we still have the high & low values |
@seamuslee001 then we should move the coverting to above the renaming for all the fields in the upgrade I think |
@eileenmcnaughton why we have always done re-nameing then converting ever since you brought in this routine |
This seems to work OK - look forwards to seeing you kick the tyres on it & see if you find any issues
1cbe1bf
to
41b8dd1
Compare
@@ -71,6 +71,8 @@ public function datePickerConversion($fields) { | |||
'case_start_date' => 'case_from', | |||
'case_end_date' => 'case_to', | |||
'mailing_job_start_date' => 'mailing_date', | |||
'relationship_start_date' => 'relation_start_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.
I think the old name was just relation_start i think and relation_end for the other i think
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 like relation_start_date here https://github.com/civicrm/civicrm-core/pull/15637/files#diff-54576153cb6a72d6ea7e22a5e8ca63f2L406
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.
odd one because when i created from an advanced search the form values was
s:14:"relative_dates";a:1:{s:14:"relation_start";s:10:"this.month";}
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.
ug
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.
well I guess that is authoritive if you did that test
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.
@seamuslee001 actually the UI ALSO has the field relation_date - which I hadn't made sense of - maybe it's the equivalent of the event_date? Although my initial assumption (ie it's just broken) may also be true & the resulting smart group from the pre-this change search might not actually work .... I guess we need to test that
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.
@eileenmcnaughton i can't even see where that relation_date date search is exposed on the form its self, i think just get rid of it
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.
looking at this @eileenmcnaughton
https://github.com/civicrm/civicrm-core/pull/15637/files#diff-54576153cb6a72d6ea7e22a5e8ca63f2R410 it would suggest it only goes until it hits either _date or _relative which i think matches with it storing relation_start in the db
I'm going to merge this and do a follow up tidy up fix |
Overview
Convert relationship start & end dates to datepicker
Before
Jcalendar
After
Datepicker
Technical Details
Seems to work like this - @seamuslee001 I'll be interested to see if you find anything kicking the tyres on this - needs the others merged to 'clean it up'
Comments