CRM-19801: Fixing changing a relationship type from A to B or vice versa #9579
Conversation
If you tried to change (A) relationship type of an exiting relationship (Like parent of) to another (B) relationship type (Like Child of) or vice versa then it will show you that the relationship get updated but it will not be it actually get updated. The issue is related to how postProcess handle contact_id_a & contact_id_b paramterters . So in this PR I've refactored the postProcess method for the relationship form and with this refactoring I've have fixed this issue too by properly hanlding conact_id_a and contact_id_b parameters.
|
@omarabuhussein Thanks for the cleanup :-). I've listed few issues found during initial QA. Also, I wasn't able to save Thanks. |
| } | ||
| } | ||
| if (isset($params['note'])) { | ||
| $this->saveRelationshipNotes($relationshipIds, $params['note']); |
There was a problem hiding this comment.
I think saveRelationshipNotes should also be called when $this->_action & CRM_Core_Action::UPDATE is TRUE to satisfy previous behaviour ?
There was a problem hiding this comment.
I took another look into what happens and it appear to me that the previous logic is not that elegant .. for example the $params['note'] was used but there was no kind of checking to confirm if it exist or not . so I refactored this method again with proper checking and corrected how it get called.
| * @return array | ||
| */ | ||
| private function updateAction($params) { | ||
| $params = array_shift($this->preparePostProcessParameters($params)); |
There was a problem hiding this comment.
This throws a notice error for me -
Notice: Only variables should be passed by reference in CRM_Contact_Form_Relationship->updateAction() (line 514 of /home/web/civi_master/civicrm/CRM/Contact/Form/Relationship.php).
May be $this->preparePostProcessParameters($params) should be assigned to a variable instead of passing directly to array_shift() ?
|
thanks @jitendrapurohit for reviewing , will check again and fix the issues |
|
@jitendrapurohit I fixed the issues , can you please take another look |
|
failed test doesn't seem to be related |
|
jenkins, test this please |
|
thanks @jitendrapurohit , tests pass this time . can you please take another look whenever you have the time |
|
@omarabuhussein I see there's a bunch of code which is being removed completely in this PR. This is actually good unless it worries to cause any regressions. I've done a basic manual testing which seems to be working correctly for all insert/update operation. Things that are tested and worked fine -
One bug which I've found and was working before the changes is that clearing of
Thanks. |
|
@omarabuhussein I really like the code cleanup. Can you please look into the current employer issue that Jitendra noticed and we'll get this merged. |
|
thanks so much @jitendrapurohit & @colemanw .. I will look into the reported issue and fix it as soon as possible. |
|
👍 nice work Omar! |
|
thanks @jamienovick .. @jitendrapurohit current employee issue is fixed now ... for your check |
|
confirmed that current employer issue has been fixed. Thanks for the update @omarabuhussein |
|
@colemanw the PR is approved |
If you tried to change A relationship type (Like Parent of ) of an exiting relationship to other B relationship type (Like Child of ) or vice versa then it will show you that the relationship get updated but it will not be it actually updated.
The issue is related to how postProcess handle contact_id_a & contact_id_b parameters .
So in this PR I've refactored the postProcess method for the relationship form and with this refactoring I've fixed this issue too by properly handling contact_id_a and contact_id_b parameters.