CRM-19739 : New Account Relationship Option Screen does not show excep… #10651
Conversation
342e680 to
74c3274
Compare
|
Hi Jitendra, just tested the patch and shows the message. Thanks for fixing it. (Klaas Eikelboom) |
|
Thanks @kainuk. @eileenmcnaughton @monishdeb Can we merge this as it is being tested as per above comment. |
| self::validateRelationship($financialTypeAccount); | ||
| $valid = self::validateRelationship($financialTypeAccount); | ||
| if (!$valid) { | ||
| return FALSE; |
There was a problem hiding this comment.
@jitendrapurohit the function we are calling here appears to throw an exception - which seems really appropriate for the BAO layer. I feel like the form layer should catch the exception & call CRM_Core_Session::statusBounce().
| @@ -300,7 +300,9 @@ public function postProcess() { | |||
| $params['entity_id'] = $this->_aid; | |||
| } | |||
| $financialTypeAccount = CRM_Financial_BAO_FinancialTypeAccount::add($params, $ids); | |||
| CRM_Core_Session::setStatus(ts('The financial type Account has been saved.')); | |||
There was a problem hiding this comment.
So, what I was alluding to above is I think this could be
try {
$financialTypeAccount = CRM_Financial_BAO_FinancialTypeAccount::add($params, $ids);
}
catch (CRM_Core_Exception $e) {
CRM_Core_Session::statusBounce($e->getMessage());
}
There was a problem hiding this comment.
Tested and updated the code with statusBounce(). Thanks for the suggestion 👍
|
@jitendrapurohit style issue :-( I think the testing @kainuk did earlier still stands, although this is slightly different now |
|
I did a new a functional test with the latest code. |
|
You rock! |
| } | ||
| catch (CRM_Core_Exception $e) { | ||
| CRM_Core_Error::statusBounce($e->getMessage()); | ||
| } | ||
| CRM_Core_Session::setStatus(ts('The financial type Account has been saved.')); |
There was a problem hiding this comment.
L308 should be moved after line 303?
There was a problem hiding this comment.
I agree with @pradpnayak - the statusBounce will mean the code does not reach that line, but that will be much clear if you change it as suggested @jitendrapurohit
There was a problem hiding this comment.
Moved the success message to the specified line for clarity.
|
Merging - thanks @jitendrapurohit & @pradpnayak for review & suggestion |
|
also thanks @kainuk |
…tion
Exception fails to show message on ajax pop-up, hence replaced it with
setStatus()and returned false if relationship isn't calculated as valid.