dev/core#2308 do not require fields if activity_id is present #19439
Conversation
Note this is only one place they are required & it will still fail later with this merged
|
(Standard links)
|
| throw new CRM_Core_Exception(ts('Missing required fields: Activity type label or Activity type ID')); | ||
| } | ||
| if (!$this->getFieldValue($values, 'activity_date_time')) { | ||
| throw new CRM_Core_Exception(ts('Missing required fields')); |
There was a problem hiding this comment.
@eileenmcnaughton any reason why you split the activity_date_time out from the other 2?
There was a problem hiding this comment.
The other 2 are an either or so a bit more complex
| if ($val) { | ||
| $dateValue = CRM_Utils_Date::formatDate($val, $dateType); | ||
| if ($dateValue) { | ||
| $params[$key] = $dateValue; |
There was a problem hiding this comment.
I think removing this params assignment is ok. It gets passed on to isErrorInCustomData below but this field isn't custom data. In testing it still catches invalid dates.
And just noting this is now a silly loop, looping thru all the params just to find engagement_level. And the comments on line 160 and 172 no longer apply.
|
This looks ok to me. I'm assuming "it will still fail later with this merged" is referring to the overall general concept of updating by providing an activity id, not that requirement checking for new activities is now broken (it doesn't seem to be broken). |
|
@demeritcowboy I do actually have a folow on patch on engagement level - #19421 - I can push it into this if you want |
|
Actually probably a separate PR since you have OK'd this - @seamuslee001 wanna merge it ? Note the chain eventually winds up with https://github.com/civicrm/civicrm-core/pull/19421/files#diff-6347a7ef5c13763509b0cbbf696e881473ea89931a7ae2859870719a69986896R341 |
Overview
dev/core#2308 do not require fields if activity_id is present - towards supporting update by import
Before
requirement for activity type & date fields still enforced when id is present
After
Not enforced, in this place
Technical Details
Note this is only one place they are required & it will still fail later with this merged
Comments