Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM-21008 - Checkbox to explicitly change employer when sharing address #13700

Merged
merged 1 commit into from
Mar 11, 2019

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Feb 25, 2019

This is a reviewer's cut of #12741. I've simplified the logic and added a unit test.

@civibot
Copy link

civibot bot commented Feb 25, 2019

(Standard links)

@@ -146,7 +146,25 @@ public function testCreateAddressWithMasterRelationshipOrganization() {
$this->callAPISuccess('relationship', 'getcount', array(
'contact_id_a' => $individualID,
'contact_id_b' => $this->_contactID,
));
), 1);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing unit test of this functionality was actually not testing anything because no expected value was set here.

@@ -166,7 +166,10 @@ public static function buildQuickForm(&$form, $addressBlockCount = NULL, $sharin

// Override the default profile links to add address form
$profileLinks = CRM_Contact_BAO_Contact::getEntityRefCreateLinks('shared_address');
$form->addEntityRef("address[$blockId][master_contact_id]", ts('Share With'), array('create' => $profileLinks));
$form->addEntityRef("address[$blockId][master_contact_id]", ts('Share With'), ['create' => $profileLinks, 'api' => ['extra' => ['contact_type']]]);
Copy link
Member Author

@colemanw colemanw Feb 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samuelsov by sending back the contact type here as extra data, I was able to avoid the added ajax callback from your original PR.

@colemanw colemanw changed the title CRM-21008 - Checkbox to explicitely change employer when sharing address CRM-21008 - Checkbox to explicitly change employer when sharing address Feb 25, 2019
@samuelsov
Copy link
Contributor

Thanks @colemanw, i will test it and get back to you this week.

@eileenmcnaughton
Copy link
Contributor

test this please

2 similar comments
@eileenmcnaughton
Copy link
Contributor

test this please

@colemanw
Copy link
Member Author

test this please

@eileenmcnaughton
Copy link
Contributor

@samuelsov ^^

@samuelsov
Copy link
Contributor

@colemanw @eileenmcnaughton Sorry about the delay.
A week of vacation and a few hours of fighting with my testing VM and i finally tested it.
Everything seems to work as expected, thanks !

@seamuslee001
Copy link
Contributor

Merging as per @samuelsov 's review

@seamuslee001 seamuslee001 merged commit e026b80 into civicrm:master Mar 11, 2019
@colemanw colemanw deleted the CRM-21008 branch March 11, 2019 20:13
@colemanw
Copy link
Member Author

@samuelsov in the future try testing PRs online - each PR gets its own dedicated sandbox which you can reach from civibot's "standard links" above. It expires after a week but you can comment on a pr with the magic "test this" phrase and it will be rebuilt within an hour.

@samuelsov
Copy link
Contributor

@colemanw thanks for the tip !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants