-
Notifications
You must be signed in to change notification settings - Fork 5
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
Specify which relationships to generate tokens for #5
Conversation
My last PR passes a |
@@ -101,7 +101,7 @@ function reltoken_civicrm_tokenValues(&$values, $contactIDs, $job = null, $token | |||
$baseToken = preg_replace('/^(.+)___.+$/', '$1', $token); | |||
// dsm($baseToken, '$baseToken'); | |||
// dsm($relatedContactIDs, "\$relatedContactIDs for $token"); | |||
$tokenDetails = CRM_Utils_Token::getTokenDetails($relatedContactIDs, array($baseToken => 1), FALSE, FALSE, NULL, array('contact' => array($baseToken))); | |||
$tokenDetails = CRM_Utils_Token::getTokenDetails($relatedContactIDs, array($baseToken => 1), FALSE, FALSE, NULL, array('contact' => array($baseToken)), 'CRM_Reltoken'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MegaphoneJon Thanks for this. I gather that 'CRM_Reltoken' is an arbitrary string here, since we don't really have a classname. Is that right?
That's correct, it's an arbitrary string. |
CRM/Reltoken/Upgrader.php
Outdated
'name' => 'display_reltokens', | ||
'label' => E::ts('Generate tokens for this relationship'), | ||
'data_type' => 'Boolean', | ||
'default_value' => 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MegaphoneJon I'm thinking '1' here would be better for backwards compatibility. So we'd start with supporting all of them, and admins would need to disable specific ones. Thoughts?
Oh, good thinking. If this is the only change you have, I'll push that on this branch. Otherwise I'll wait until you've reviewed everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other improvement I can see is that I create the custom group/fields but don't remove them on uninstall. The "allow custom fields on RelationshipType" should NOT uninstall itself because this extension doesn't "own" that setting; in fact, there's at least two other extensions (one public) that use that setting.
@MegaphoneJon sure, this is great otherwise. Default to 1 on that custom field, and we can merge. |
@twomice updated! |
Thanks - merged! |
This PR allows a user to configure which relationship types should get tokens generated via the UI. I updated the README to document this and to remove this option from the "Room for Improvement" section.
Note that:
civix generate:upgrader
updatedreltokens.civix.php
and added a bunch of boilerplate.So this looks like a big PR, but the only code changes are:
CRM_Reltoken_Upgrader::addCustomData()
(creates the custom field)CRM_Reltoken_Upgrader::install()
andCRM_Reltoken_Upgrader::enable()
(which calladdCustomData()
;_reltoken_get_hashed_relationship_types()
to filter by the new custom field.