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

dev/core#4123 Support contribution recur tokens when accessing from a… #25563

Merged
merged 1 commit into from
May 9, 2023

Conversation

seamuslee001
Copy link
Contributor

… contribution or membership

Overview

This is an alterative to #22450 and picks up Eileen's comment on that PR to add support for using {contribution.contribution_recur_id.x} or {membership.contribution_recur_id.x} tokens

Before

Cannot access Contribution Recur Related fields from contribution or membership records in token process

After

Can do so

ping @eileenmcnaughton @mattwire @JoeMurray

@civibot
Copy link

civibot bot commented Feb 12, 2023

No issue was found matching the number given in the pull request title. Please check the issue number.

@civibot
Copy link

civibot bot commented Feb 12, 2023

(Standard links)

@civibot civibot bot added the master label Feb 12, 2023
@eileenmcnaughton
Copy link
Contributor

@seamuslee001 tests aside this addresses my concerns on the other PR.

Stylistically though I'd like to converge on something like getRelatedTokens - which is sort of what Contact_Tokens use -

protected function getRelatedEntityTokenMetadata(): array {

The thinking is

  • bespokeTokens are 'weird & wonderful things
  • relatedEntityTokens are still apiv4 style tokens - they just use a join.

@eileenmcnaughton
Copy link
Contributor

Test fail looks to be permissions ? api_v3_PaymentTest::testMultiplePaymentsForContribution
Failure in api call for payment create: Authorization failed

We do have a placeholder property for checkPermissions - although always FALSE atm

@seamuslee001
Copy link
Contributor Author

ok @eileenmcnaughton I have updated that now and I think I have fixed the test failures

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 not quite - also I think you should unadvertise a bunch of tokens - ie the 'Transaction ID' from the recur is ambiguous - best to change the audience on the ones that could be confusing I think.

CRM_Contribute_ActionMapping_ByTypeTest::testTokenRendering

… contribution or membership

Use getRelatedTokens function name instead and also fix tests

Hide some unnecessary tokens and fix tests
@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton I think I have hidden the relevant ones now can you take another look at this?

*/
protected function getRelatedTokens(): array {
$tokens = [];
$hiddenTokens = ['modified_date', 'create_date', 'trxn_id', 'invoice_id', 'is_test', 'payment_token_id', 'payment_processor_id', 'payment_instrument_id', 'cycle_day', 'installments', 'processor_id', 'next_sched_contribution_date', 'failure_count', 'failure_retry_date', 'auto_renew', 'is_email_receipt', 'contribution_status_id'];
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder about next_sched_contribution_date - that feels like something people might use - but non-blocking

'type' => 'mapped',
'options' => $contributionRecurField['options'] ?? NULL,
'data_type' => $contributionRecurField['data_type'],
'audience' => in_array($contributionRecurField['name'], $hiddenTokens) ? 'hidden' : 'user',
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally in_array would have the final TRUE for strict mode. Non blocking

@eileenmcnaughton
Copy link
Contributor

OK - let's merge this - I think it might get a bit confusing when both recurring and membership tokens are available - probably only scheduled reminders. But that is probably not going to be a major concern for anyone & we can think of ways to address if it bubbles up

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

Successfully merging this pull request may close these issues.

2 participants