add contact_id to email params in emailLetter function

[masetto]

Overview

I add the contact_id to email params in emailLetter() function.

Before

If you use Mail hooks (alterMailParams and postEmailSend) you do not know which contact you are sending the email to, or at least it is not easy to know.

After

Now, if you use alterMailParams or postEmailSend hooks, you can know contact and get all its details.

Technical Details

I use contact_id key and not contactId because everywhere is called this.

Comments

I removed the unnecessary line:

$params = array_merge($defaults);

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

The problem with putting stuff into core just to support extensions is that it is easy for them to be removed from core later because they are not required by core.

Some ways we protect them if we want them not to break later are

• adding unit tests (eg. in CRM_Contribute_Form_Task_PDFLetterCommonTest)
• ensuring there is an agreed expectation that is documented - ie docs & in this case the doc block on send
• in code comments

Much as I loathe contactId (I think contactID or contact_id are correct) I put a break point into the test here and it is called contactId in the main message render flow

[masetto]

@eileenmcnaughton Please help me to use tests because I never used or created them, sorry.
E.g. I tried to execute phpunit ./tests/phpunit/Civi/WorkflowMessage/ExampleWorkflowMessageTest.php and returns this error:

Uncaught Error: Class 'CiviUnitTestCase' not found in tests/phpunit/Civi/WorkflowMessage/ExampleWorkflowMessageTest.php:21

How hard for just one line of code, but I understand, it is important and I have to learn.

[demeritcowboy]

There's a little bit of setup required - see https://docs.civicrm.org/dev/en/latest/testing/#setup (just the first section there - down to where you fill in .cv.json). The main part is setting up an empty test db and TEST_DB_DSN unless you're ok to use the existing database, but it will wipe the data. If you're using a throwaway buildkit site then this should be already done for you.

Then, the tests currently use phpunit8, so I'd use that version. And then to run it you need to set export CIVICRM_UF=UnitTests. Then you can do phpunit ./tests/phpunit/etc...

Note the test fail here on the PR for CRM_Contribute_Form_Task_PDFLetterCommonTest.testPostProcessGroupByContact is a test you would need to update since it's not expecting contact id. But it means you can probably copy/paste some of that code since it would be a similar test.

[eileenmcnaughton]

@masetto definitely happy to help you get going on tests - once you have them running it really helps. Do you use buildkit? an IDE?

[masetto]

@eileenmcnaughton I'm not using buildkit. I have cv installed but not civibuild and cv vars:show command does not show TEST_DB_DSN property (probably because I have not create site with civibuild). As IDE I'm using vscode.
`

[demeritcowboy]

Run cv vars:fill and then edit ~/.cv.json to fill in the user/pass/db for TEST_DB_DSN. You can create an empty mysql database to use for it.

[eileenmcnaughton]

Thanks @demeritcowboy I am so used to phpstorm + buildkit my brain went blank at anything else

@braders - you use vscode - do you know if it is possible to run unit tests within vscode? The way I run tests in phpstorm is I right click & select 'debug'....

[braders]

@Eileen I must profess to just letting Jenkins run tests for me - it would be good to figure out how to get them going locally though! In terms of VSCode integration, I think people would generally:

1. Use the integrated terminal
2. Create a task https://code.visualstudio.com/docs/editor/tasks (which essentially forms a shortcut to the test)

[demeritcowboy]

I use vscode sometimes - there's a phpunit extension you can install - I don't use it but it might be useful.

[masetto]

@eileenmcnaughton Please look at my last commit.
I did not create a new unit test, but I added a test on the alterMailParams hook. I couldn't to test that the contact_id is exactly as expected. In testPostProcess function I tried to add

\Civi::dispatcher()->addListener('hook_civicrm_alterMailParams', function($e) use ($_individualId) {`

but it never goes inside...

[masetto]

hi, have you looked at the commits?

[demeritcowboy]

Because the default on that form is to JUST create pdfs, the test would need to pass in email_options to the form in order to get the hook to work for testPostProcess(). If I put in some debugging to output stack traces, the only test that is calling the hook is testPostProcessGroupByContact(). So it's working, but if you wanted it to only run for one test, you would put \Civi::dispatcher()->addListener inside testPostProcessGroupByContact(), and then you could test the expected contactId.

[masetto]

If the assertTrue test to verify that contactId is a parameter is sufficient for you, I would leave it at that.

[eileenmcnaughton]

@masetto I'm sorry I haven't had the bandwidth for this issue over the last week. It looks like @demeritcowboy is 'fairly happy' with this - which I think is a high enough bar if the test will protect this change. Merging