dev/core/#/229 Fix fatal error on send test mail #12399

eileenmcnaughton · 2018-07-03T02:24:55Z

Overview

Fixes regression in rc whereby test mailings are not sent

https://lab.civicrm.org/dev/core/issues/229

Before

sending test email fails with: Error in call to Mailing_send_test : job_id is not valid : 440

After

send mail works

Technical Details

@jmcclelland can you please test this? I think that passing params through to the BAO on a different entity is a bad practice so I switched to a whitelist approach.

The original MailingJob::Create only handled the 4 params still being handled

 $job = new CRM_Mailing_BAO_MailingJob();
    $job->mailing_id = $params['mailing_id'];
    $job->status = $params['status'];
    $job->scheduled_date = $params['scheduled_date'];
    $job->is_test = $params['is_test'];
    $job->save();
    if ($params['mailing_id']) {
      CRM_Mailing_BAO_Mailing::getRecipients($params['mailing_id']);
      return $job;
    }
    else {
      throw new CRM_Core_Exception("Failed to create job: Unknown mailing ID");
    }

Comments

Thank you for testing the rc @jmcclelland

civibot · 2018-07-03T02:24:57Z

(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 · 2018-07-03T02:26:13Z

api/v3/Mailing.php

@@ -613,19 +614,24 @@ function civicrm_api3_mailing_send_test($params) {
  if (!array_key_exists('test_group', $params) && !array_key_exists('test_email', $params)) {
    throw new API_Exception("Mandatory key(s) missing from params array: test_group and/or test_email field are required");
  }
-  civicrm_api3_verify_mandatory($params,


moved to the spec function

eileenmcnaughton · 2018-07-03T02:26:23Z

api/v3/Mailing.php

@@ -679,8 +685,7 @@ function civicrm_api3_mailing_send_test($params) {
  }

  $isComplete = FALSE;
-  $config = CRM_Core_Config::singleton();


seamuslee001 · 2018-07-03T02:31:52Z

tests/phpunit/api/v3/MailingTest.php

@@ -410,8 +410,10 @@ public function testMailerSendTest_email() {
    $mail = $this->callAPISuccess('mailing', 'create', $this->_params);

    $params = array('mailing_id' => $mail['id'], 'test_email' => 'alice@example.org', 'test_group' => NULL);
+    // Per https://lab.civicrm.org/dev/core/issues/229 ensure this is not passed through!
+    $params['id'] = $mail['id'];


@eileenmcnaughton how has this test been passing is it just that there isn't a difference in ids between the civicrm_mailing table the civicrm_mailing_job table here?

@seamuslee001 I was able to replicate it when I added to the test so it wasn't transposing 'mailing_id' to 'id' in the api wrapper (I haven't fully thought that through) -

in the api 'id' refers to mailing_id - but is passed to MailingJob as if it were MailingJob id - hence the bug

(mailing id is supposed to be required here & it makes sense it should be)

hmm interesting. So the test was succeeding in that it was setting a mailing_id but no id but once the id got set then it started failing is that the case?

seamuslee001 · 2018-07-03T02:38:38Z

api/v3/Mailing.php


-  $testEmailParams = _civicrm_api3_generic_replace_base_params($params);


@eileenmcnaughton is this where the transposition was going on?

@eileenmcnaughton do you think it might be worth adding id to the exclusion on the replace_base_params function instead?

seamuslee001 · 2018-07-03T02:39:12Z

api/v3/Mailing.php

-    FALSE
-  );
+  $testEmailParams = [
+    'mailing_id' => $params['mailing_id'],


@eileenmcnaughton your 100% mailing_id is always getting passed as param right?

@seamuslee001 it's a required field - ie I moved a verify_mandatory call to a spec declaration

ok happy with that then

seamuslee001 · 2018-07-03T04:32:11Z

After chatting with Eileen on matermost i am good with these changes @eileenmcnaughton @jmcclelland

eileenmcnaughton · 2018-07-03T04:37:09Z

OK - let's merge-on-pass & then @jmcclelland can test the new tarball

seamuslee001 · 2018-07-03T04:52:12Z

@eileenmcnaughton should we do the same with my PR as well?

jmcclelland · 2018-07-03T14:28:56Z

Tested and it works - thank you! Although I'm not sure why the tests are now failing.

jmcclelland · 2018-07-03T15:16:54Z

It seems that Mailing.send_test ignores the email_group parameter.

jmcclelland · 2018-07-03T15:39:30Z

I just created an alternative that tries to fix the test errors by adding the ability to send to a group:

#12407

seamuslee001 · 2018-07-03T23:11:59Z

Merging as per the testing of @jmcclelland and as per the tag

civicrm-builder added the master label Jul 3, 2018

eileenmcnaughton changed the base branch from master to 5.3 July 3, 2018 02:25

civicrm-builder added the 5.3 label Jul 3, 2018

eileenmcnaughton force-pushed the jamie branch from e51d3c5 to a61756b Compare July 3, 2018 02:25

eileenmcnaughton commented Jul 3, 2018

View reviewed changes

seamuslee001 reviewed Jul 3, 2018

View reviewed changes

eileenmcnaughton force-pushed the jamie branch from a61756b to e94a3a6 Compare July 3, 2018 04:07

seamuslee001 added the merge on pass label Jul 3, 2018

seamuslee001 mentioned this pull request Jul 3, 2018

dev/core#229 Fix fatal error on sending test email #12404

Closed

jmcclelland mentioned this pull request Jul 3, 2018

sending test message results in error #12407

Closed

Fix fatal error on send test mail

480a934

eileenmcnaughton force-pushed the jamie branch from e94a3a6 to 480a934 Compare July 3, 2018 21:21

seamuslee001 merged commit 6adbfc5 into civicrm:5.3 Jul 3, 2018

eileenmcnaughton deleted the jamie branch July 4, 2018 00:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core/#/229 Fix fatal error on send test mail #12399

dev/core/#/229 Fix fatal error on send test mail #12399

eileenmcnaughton commented Jul 3, 2018 •

edited

civibot bot commented Jul 3, 2018

eileenmcnaughton Jul 3, 2018

eileenmcnaughton Jul 3, 2018

seamuslee001 Jul 3, 2018

eileenmcnaughton Jul 3, 2018

eileenmcnaughton Jul 3, 2018

seamuslee001 Jul 3, 2018

seamuslee001 Jul 3, 2018

seamuslee001 Jul 3, 2018

seamuslee001 Jul 3, 2018

eileenmcnaughton Jul 3, 2018

seamuslee001 Jul 3, 2018

seamuslee001 commented Jul 3, 2018

eileenmcnaughton commented Jul 3, 2018

seamuslee001 commented Jul 3, 2018

jmcclelland commented Jul 3, 2018

jmcclelland commented Jul 3, 2018

jmcclelland commented Jul 3, 2018

seamuslee001 commented Jul 3, 2018


		$testEmailParams = _civicrm_api3_generic_replace_base_params($params);

dev/core/#/229 Fix fatal error on send test mail #12399

dev/core/#/229 Fix fatal error on send test mail #12399

Conversation

eileenmcnaughton commented Jul 3, 2018 • edited

Overview

Before

After

Technical Details

Comments

civibot bot commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Jul 3, 2018

eileenmcnaughton commented Jul 3, 2018

seamuslee001 commented Jul 3, 2018

jmcclelland commented Jul 3, 2018

jmcclelland commented Jul 3, 2018

jmcclelland commented Jul 3, 2018

seamuslee001 commented Jul 3, 2018

eileenmcnaughton commented Jul 3, 2018 •

edited