dev/core#1838 Ensure that no fatal error is triggered if you try to a… #18564
Conversation
|
(Standard links)
|
|
@seamuslee001 should we be doing this in the api or the function that calls the api? |
|
I see it's called here
|
|
@eileenmcnaughton hmm yeh maybe, but also I feel like the error message is not useful so catching the internal exception and throwing a new exception with a better error might be helpful right? |
|
@seamuslee001 yeah - so I don't think we need to log it. We should possibly return some alert to the user at the form level? |
|
@seamuslee001 it;s also OK to use apiv4 within apiv3 internally - we might be able to just construct the query a bit better that way? |
|
@eileenmcnaughton i'm not looking to change anything to apiv4 here |
|
@seamuslee001 you could switch to get rather than getvalue |
seamuslee001
force-pushed
the
dev_core_1838
branch
from
9a2bf23 to
bf2648c
Compare
|
@eileenmcnaughton I have moved the exception catching into the Page and also expanded the UT to cover the page view function |
|
@seamuslee001 I'm OK with that - except just the stylistic thing on exception. |
|
@seamuslee001 jenkins.... |
seamuslee001
force-pushed
the
dev_core_1838
branch
from
bf2648c to
ff35db1
Compare
|
should be sorted now @eileenmcnaughton |
CRM/Contact/Page/View.php
Outdated
| $mergedTo = civicrm_api3('Contact', 'getmergedto', ['contact_id' => $contactId, 'api.Contact.get' => ['return' => 'display_name']]); | ||
| } | ||
| catch (CiviCRM_API3_Exception $e) { | ||
| CRM_Core_Session::singleton()->setStatus(ts('Have not been able to determine which contact this contact merged into'), ts('Unable to determine kept contact')); |
There was a problem hiding this comment.
@seamuslee001 so my last concern is the wording here - someone like @MikeyMJCO might have an idea but maybe something longer like 'This contact was deleted in a merge but the new contact can not be determined / has since been deleted'
My main 'ug' is just the 'Have not been able' doesn't feel right - who has not....
|
@MikeyMJCO any thoughts about the message wording - this is fine technically but I'm not sure the message is that good |
To be clear this error is thrown when one of the contacts involved in the merge has been deleted and the attempt |
|
@MikeyMJCO this error is shown when you view a contact that is in the trash that was trashed because of a merged and the contact it was merged into is no longer on the system |
So something along the lines of: CRM_Core_Session::singleton()->setStatus(ts('This contact was deleted during a merge operation. The contact it was merged into cannot be found and may have been deleted.')); |
…ccess a merged contact and the contact that it was merged into has been permanently deleted Move exception capturing into the page layer and extend unit test to cover page function Update status message as per Mikeý's comment
seamuslee001
force-pushed
the
dev_core_1838
branch
from
ff35db1 to
50bdbb5
Compare
|
Thanks Mikey that works for me have updated now @eileenmcnaughton @MikeyMJCO |
|
thanks @seamuslee001 @MikeyMJCO |
…ccess a merged contact and the contact that it was merged into has been permanently deleted
Overview
Replication details are on the lab https://lab.civicrm.org/dev/core/-/issues/1838 but this seems like an easy fix
Before
Fatal error on viewing the merged contact
After
No fatal error
ping @eileenmcnaughton @andrew-cormick-dockery