CRM-8526, CRM-19786, CRM-18141, CRM-19757 : Fix Schedule Reminder tokens #9661

monishdeb · 2017-01-10T05:04:41Z

seamuslee001 · 2017-01-10T05:27:40Z

@monishdeb you may also want to look at #9542 (comment) from @xurizaemon who has been touching similar areas and his also has a test

monishdeb · 2017-01-10T06:13:41Z

@seamuslee001 yes I know. I saw that PR earlier which in my opinion a bit different than my proposed fix, as I am not in a favor of bringing changes in BAO. Also, we need a generic fix to fetch all those missing token values for fields like custom field, greeting etc. I have marked this PR with wip as am writing the test case for it.

seamuslee001 · 2017-01-10T06:20:26Z

Yep just wanted to make sure it was on your radar and agree re generic fix

monishdeb · 2017-01-10T06:23:30Z

I like to cherry-pick the test-case 08f9b27 written by @xurizaemon :) And also extend the existing SR unit test, to assert custom field token for all SR supported entities.

monishdeb · 2017-01-10T14:32:26Z

Jenkin test this please

…eld tokens

totten · 2017-01-13T01:24:43Z

I like latest commits -- they mean that custom_123 tokens are enumerable and that new token-providers can be mixed-in.

One thought: In the call to getCustomTokens(...), it calls CRM_Core_BAO_CustomField::getFields(). In my spot-checking, that function returns both single-value and multi-value CustomGroups. That's perfect for single-value, but multi-value custom fields don't make sense as a basic token. (Multiple values need more constructs like implode or foreach or if empty, and that's a deeper dive.)

But we're probably only trying to solve the single-value scenario? So getCustomTokens() should just omit the multi-value fields.

seamuslee001 · 2017-01-13T02:49:24Z

Agree with Tim i think multi value fields will be difficult to manage.

monishdeb · 2017-01-13T12:46:34Z

@totten @seamuslee001 I have solved the greeting issue and supported multi-valued custom value. This is the mail template

And this is the final mail content

As you can see the multi-value token is replaced by comma-separated labels a, b. Is it appropriate or should I revert the fix and support for single custom field token only?

(This is my latest commit 8640061)

monishdeb · 2017-01-13T14:08:48Z

Also, extended unit-test to assert contact.custom_N token (commit - 2273ddc)

twomice · 2017-01-14T00:14:29Z

Hi @monishdeb. Performing the steps to reproduce error behavior in CRM-18141 (https://issues.civicrm.org/jira/browse/CRM-18141?focusedCommentId=98230&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-98230), I'm finding a fatal error generated by the "Send Scheduled Reminders" scheduled job. Here is the job log:

Parameters parsed (and passed to API method): 
a:1:{s:7:"version";i:3;}

Full message: 
Finished execution of Send Scheduled Reminders with result: Failure, Error message: DB Error: syntax error

And a screenshot of the same:

Oddly, I enabled CiviCRM debug settings including backtrace, but am not getting an on-screen backtrace for this error; not sure why.

My test configuration is, for the moment, still available on the Jenkins test site at http://core-9661-43dy0.test-ubu1204-5.civicrm.org/

Also, I tried merging this patch on my local dev master, but encountered failed hunks. Probably not your problem, but in case it's relevant to you:

as@as76:/var/www/d7cividev/sites/all/modules/civicrm (master)$ git pull upstream master
From git://github.com/civicrm/civicrm-core
 * branch            master     -> FETCH_HEAD
Already up-to-date.
as@as76:/var/www/d7cividev/sites/all/modules/civicrm (master)$ patch --dry-run -p1 < /tmp/9661.patch
checking file Civi/Token/AbstractTokenSubscriber.php
checking file Civi/Token/TokenCompatSubscriber.php
checking file Civi/Token/AbstractTokenSubscriber.php
Hunk #1 FAILED at 163.
1 out of 1 hunk FAILED
checking file Civi/Token/TokenCompatSubscriber.php
Hunk #1 FAILED at 65.
Hunk #2 succeeded at 104 (offset -13 lines).
1 out of 2 hunks FAILED
checking file tests/phpunit/CRM/Core/BAO/ActionScheduleTest.php
checking file CRM/Activity/Tokens.php
checking file CRM/Contribute/Tokens.php
checking file CRM/Event/Tokens.php
checking file CRM/Member/Tokens.php
checking file Civi/Token/AbstractTokenSubscriber.php
Hunk #2 FAILED at 164.
Hunk #3 FAILED at 177.
2 out of 3 hunks FAILED
checking file Civi/Token/TokenCompatSubscriber.php
Hunk #1 FAILED at 62.
1 out of 1 hunk FAILED
checking file Civi/Token/TokenRow.php
checking file CRM/Activity/Tokens.php
Hunk #1 FAILED at 110.
1 out of 1 hunk FAILED
checking file CRM/Contribute/Tokens.php
Hunk #1 FAILED at 136.
1 out of 1 hunk FAILED
checking file CRM/Event/Tokens.php
Hunk #1 FAILED at 142.
1 out of 1 hunk FAILED
checking file CRM/Member/Tokens.php
Hunk #1 FAILED at 98.
1 out of 1 hunk FAILED
checking file Civi/Token/TokenCompatSubscriber.php
Hunk #1 FAILED at 66.
Hunk #2 FAILED at 112.
2 out of 2 hunks FAILED
checking file Civi/Token/TokenRow.php
Hunk #1 FAILED at 130.
1 out of 1 hunk FAILED
checking file tests/phpunit/CRM/Core/BAO/ActionScheduleTest.php
Hunk #1 succeeded at 593 (offset -2 lines).
Hunk #2 succeeded at 637 (offset -1 lines).
Hunk #3 FAILED at 660.
Hunk #4 succeeded at 743 (offset -8 lines).
1 out of 4 hunks FAILED

seamuslee001 · 2017-01-15T22:57:55Z

Tagging in @andrew-cormick-dockery as a watcher to this PR

seamuslee001 · 2017-01-16T04:15:33Z

Just reporting that @andrew-cormick-dockery tested this on our test site and it passed testing

twomice · 2017-01-16T05:21:21Z

@seamuslee001 and @andrew-cormick-dockery Thanks for reporting! Since this PR covers a wide variety of JIRA tickets, can you please share the steps you took to test? Thanks!

seamuslee001 · 2017-01-16T05:26:29Z

We tested this out by Firstly cherry-picking the commits locally (we have core + packages + drupal all in 1 repo).

Once that was done we then set the CIVICRM_MAIL_LOG setting. Then ran the scheduled reminders job checked the output of the emails that were logged. Verified that {{contact.eamil_greeting}} was correctly outputted

twomice · 2017-01-16T16:14:28Z

FYI, after merging this into master on local dev, I'm getting a DB syntax error when sending Scheduled Reminders, but this is happening in without the PR as well. The syntax error isn't caused by this PR, but it's preventing me from testing, so I need to look a little closer.

twomice · 2017-01-16T21:03:57Z

@monishdeb I've tested this successfully for CRM-18141.

Testing environment: civicrm master cloned to local dev environment, with this patch applied.
Testing steps: Using the steps to reproduce described at my comment in CRM-18141
Observation: Custom tokens for contacts and activities are populated correctly in the emailed reminder.

seamuslee001 · 2017-01-16T21:06:44Z

nice work @twomice, difference for me was we were testing on 4.7.15 code base locally not 4.7.16.

twomice · 2017-01-16T21:53:04Z

Thanks @seamuslee001 I was getting thrown off by a regression in 4.7.16 (CRM-19885) but once that was fixed this patch turned out fine (for my purposes, anyway).

seamuslee001 · 2017-01-16T23:14:24Z

@totten @monishdeb @yashodha I think this can be merged now with the successful testing that has happened.

yashodha · 2017-01-17T03:45:43Z

Great, will merge this as per confirmation. Thanks everyone :)

magnolia61 · 2017-02-06T01:20:24Z

I see this PR includes all extendable entities (activities, member, events). Just for clarity, does this also mean custom fields that extend the participant would be available in the scheduled mailings? Would that follow {participant.custom_xxx}? (In terms of the data of the sandbox: I would like to include the Food Preference of the Fall Fundraiser dinner in a scheduled email to the participants)

magnolia61 · 2017-02-06T22:34:54Z

Probably not the best place to comment. I placed my comment also in the JIRA issue: https://issues.civicrm.org/jira/browse/CRM-18141?filter=19310 Thanks all for your help!

JoeMurray · 2017-02-09T15:26:45Z

I have a report that {contribution.custom_xx} is not working. @seamuslee001 was that included in your range of testing that led to @yashodha doing the merges?

seamuslee001 · 2017-02-09T20:03:17Z

@JoeMurray Joe it wasn't part of my teams testing we were mainly looking at the contact.email_greeting token. i'm not sure if @twomice did any testing in that type of token

twomice · 2017-02-09T22:01:55Z

My testing was limited to what I documented above (#9661 (comment)), specifically related to CRM-18141.

It may be that this was PR merged without testing on all of the issues it was expected to cover.

magnolia61 · 2017-02-11T07:57:56Z

There are several JIRA issues open on tokens at the moment and I am afraid that this very welcome fix/improvement will stagnate or even get lost somewhere.
Wouldn't it be wise to extend the unit tests to include the membership, contribution & event/participant tokens and to document the functionality?

civicrm-builder added the master label Jan 10, 2017

monishdeb added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Jan 10, 2017

monishdeb force-pushed the CRM-18141 branch from 8775170 to cd76606 Compare January 10, 2017 05:10

monishdeb changed the title ~~CRM-18141, CRM-19757 : Fix Schedule Reminder tokens~~ CRM-8526, CRM-19786, CRM-18141, CRM-19757 : Fix Schedule Reminder tokens Jan 10, 2017

monishdeb force-pushed the CRM-18141 branch from 09b2b75 to 86c5097 Compare January 10, 2017 11:10

monishdeb mentioned this pull request Jan 10, 2017

Restore greeting token functionality as per 4.6 #9542

Closed

monishdeb and others added 3 commits January 12, 2017 20:02

CRM-18141: Scheduled reminders for activities not rendering custom fi…

a0fcf99

…eld tokens

fix for greeting token

1ae5997

add unit test

bba4f3f

monishdeb force-pushed the CRM-18141 branch from 3c27fa6 to ba25281 Compare January 12, 2017 14:33

alternative way to handle custom field token

4e9b6a6

monishdeb force-pushed the CRM-18141 branch from ba25281 to 4e9b6a6 Compare January 12, 2017 15:51

greeeting token and handle multi-value custom field fixes

8640061

monishdeb force-pushed the CRM-18141 branch from 716813a to 8640061 Compare January 13, 2017 12:38

extended unit test to asset contact custom field token

2273ddc

monishdeb force-pushed the CRM-18141 branch from 8f6de5b to 2273ddc Compare January 13, 2017 14:08

monishdeb removed the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Jan 13, 2017

yashodha merged commit 3197bbc into civicrm:master Jan 17, 2017

twomice added a commit to twomice/civicrm-core that referenced this pull request Jan 23, 2017

Applied PR civicrm#9661

10bbd82

monishdeb deleted the CRM-18141 branch February 3, 2017 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-8526, CRM-19786, CRM-18141, CRM-19757 : Fix Schedule Reminder tokens #9661

CRM-8526, CRM-19786, CRM-18141, CRM-19757 : Fix Schedule Reminder tokens #9661

monishdeb commented Jan 10, 2017 •

edited by civicrm-builder

Loading

seamuslee001 commented Jan 10, 2017

monishdeb commented Jan 10, 2017 •

edited

Loading

seamuslee001 commented Jan 10, 2017

monishdeb commented Jan 10, 2017 •

edited

Loading

monishdeb commented Jan 10, 2017

totten commented Jan 13, 2017

seamuslee001 commented Jan 13, 2017

monishdeb commented Jan 13, 2017 •

edited

Loading

monishdeb commented Jan 13, 2017

twomice commented Jan 14, 2017

seamuslee001 commented Jan 15, 2017

seamuslee001 commented Jan 16, 2017

twomice commented Jan 16, 2017

seamuslee001 commented Jan 16, 2017

twomice commented Jan 16, 2017

twomice commented Jan 16, 2017

seamuslee001 commented Jan 16, 2017

twomice commented Jan 16, 2017

seamuslee001 commented Jan 16, 2017

yashodha commented Jan 17, 2017

magnolia61 commented Feb 6, 2017 •

edited

Loading

magnolia61 commented Feb 6, 2017

JoeMurray commented Feb 9, 2017

seamuslee001 commented Feb 9, 2017

twomice commented Feb 9, 2017

magnolia61 commented Feb 11, 2017

CRM-8526, CRM-19786, CRM-18141, CRM-19757 : Fix Schedule Reminder tokens #9661

CRM-8526, CRM-19786, CRM-18141, CRM-19757 : Fix Schedule Reminder tokens #9661

Conversation

monishdeb commented Jan 10, 2017 • edited by civicrm-builder Loading

seamuslee001 commented Jan 10, 2017

monishdeb commented Jan 10, 2017 • edited Loading

seamuslee001 commented Jan 10, 2017

monishdeb commented Jan 10, 2017 • edited Loading

monishdeb commented Jan 10, 2017

totten commented Jan 13, 2017

seamuslee001 commented Jan 13, 2017

monishdeb commented Jan 13, 2017 • edited Loading

monishdeb commented Jan 13, 2017

twomice commented Jan 14, 2017

seamuslee001 commented Jan 15, 2017

seamuslee001 commented Jan 16, 2017

twomice commented Jan 16, 2017

seamuslee001 commented Jan 16, 2017

twomice commented Jan 16, 2017

twomice commented Jan 16, 2017

seamuslee001 commented Jan 16, 2017

twomice commented Jan 16, 2017

seamuslee001 commented Jan 16, 2017

yashodha commented Jan 17, 2017

magnolia61 commented Feb 6, 2017 • edited Loading

magnolia61 commented Feb 6, 2017

JoeMurray commented Feb 9, 2017

seamuslee001 commented Feb 9, 2017

twomice commented Feb 9, 2017

magnolia61 commented Feb 11, 2017

monishdeb commented Jan 10, 2017 •

edited by civicrm-builder

Loading

monishdeb commented Jan 10, 2017 •

edited

Loading

monishdeb commented Jan 10, 2017 •

edited

Loading

monishdeb commented Jan 13, 2017 •

edited

Loading

magnolia61 commented Feb 6, 2017 •

edited

Loading