CRM-20578: Empty update of Activity assignee/target results into DB e…

[jitendrapurohit]

…rror. https://issues.civicrm.org/jira/browse/CRM-20578

[seamuslee001]

Jenkins re test this please

[eileenmcnaughton]

What happens if we just remove that check, like the code comments suggest to do?

[jitendrapurohit]

This part is to check if correct contact ids are passed in source, assignee and target keys of params. If there are n no. of contacts in civi and n+1 value is passed to any of these three fields, an Invalid contact id exception is thrown.

If I remove this, the error would be replaced by DB error(constraint violation). Similar error is shown on contribution create api if contact_id is passed to a value not present in civicrm_contact.

[eileenmcnaughton]

I think we decided early on with the api that we didn't really want to impose a performance cost on every api call just to give developers clearer information when they get it wrong.

I don't know if it has been changed - since I can't find evidence of calls to https://github.com/civicrm/civicrm-core/blob/master/api/v3/utils.php#L1639-L1639 but there was code that validated constraints AFTER DB constraint error - giving extra information in that case, but not adding unnecessary queries if all the fields were present

[jitendrapurohit]

@eileenmcnaughton Moved foreign key validation for activity contact to wrapper when they are passed as an array. Now the error would be shown as

[eileenmcnaughton]

@jitendrapurohit that seems good as an api behaviour, and the test fails are unrelated.

Is the screen capture you displayed a 'real' one or a simulated one' - if the former should we be catching the exception at the display layer?

[jitendrapurohit]

This is the simulated one which I got when I write the api in Invoke.php(just to test). The real would be the yellow part with the backtrace of where the api is being called(if backtrace is enabled on the site). When tried from the api explorer, it displays like

where the error_message was only DB Error: constraint violation (before the fix in wrapper and removing the api/v3/activity.php sql). Now it contains the msg of invalid contact id passed in params.

[eileenmcnaughton]

Ok - I think that is good. The limited error message is correct & the extra info is good too.
Performance should be slightly improved. Test coverage improved, albeit not overing the failure & code complexity reduced.

Merging

[jitendrapurohit]

Thanks @eileenmcnaughton