Merge pull request #17806 from demeritcowboy/mail-report-csv-bom

#1869 - Include BOM in attachment when sending CSV CiviReport via mail_report job

CRM/Report/Form.php

@@ -3442,6 +3442,7 @@ WHERE cg.extends IN ('" . implode("','", $this->_customGroupExtends) . "') AND
             'fullPath' => $csvFullFilename,
             'mime_type' => 'text/csv',
             'cleanName' => 'CiviReport.csv',
+            'charset' => 'utf-8',
           ];
         }
         if ($this->_outputMode == 'pdf') {

CRM/Report/Utils/Report.php

@@ -209,10 +209,6 @@ WHERE  inst.report_id = %1";
     //Force a download and name the file using the current timestamp.
     $datetime = date('Ymd-Gi', $_SERVER['REQUEST_TIME']);
     CRM_Utils_System::setHttpHeader('Content-Disposition', 'attachment; filename=Report_' . $datetime . '.csv');
-    // Output UTF BOM so that MS Excel copes with diacritics. This is recommended as
-    // the Windows variant but is tested with MS Excel for Mac (Office 365 v 16.31)
-    // and it continues to work on Libre Office, Numbers, Notes etc.
-    echo "\xEF\xBB\xBF";
     echo self::makeCsv($form, $rows);
     CRM_Utils_System::civiExit();
   }
@@ -228,7 +224,11 @@ WHERE  inst.report_id = %1";
    */
   public static function makeCsv(&$form, &$rows) {
     $config = CRM_Core_Config::singleton();
-    $csv = '';
+
+    // Output UTF BOM so that MS Excel copes with diacritics. This is recommended as
+    // the Windows variant but is tested with MS Excel for Mac (Office 365 v 16.31)
+    // and it continues to work on Libre Office, Numbers, Notes etc.
+    $csv = "\xEF\xBB\xBF";
 
     // Add headers if this is the first row.
     $columnHeaders = array_keys($form->_columnHeaders);

CRM/Utils/Mail.php

@@ -261,7 +261,11 @@ class CRM_Utils_Mail {
         $msg->addAttachment(
           $attach['fullPath'],
           $attach['mime_type'],
-          $attach['cleanName']
+          $attach['cleanName'],
+          TRUE,
+          'base64',
+          'attachment',
+          (isset($attach['charset']) ? $attach['charset'] : '')
         );
       }
     }

tests/phpunit/CRM/Report/Form/Contribute/fixtures/DetailPostalCodeTest-ascii.csv

-"Donor Name","First Name","Donor Email","Amount","Postal Code"
+"Donor Name","First Name","Donor Email","Amount","Postal Code"
 " Empowerment Association",,,"$ 50.00","B10 G56"
 "Bachman, Lincoln","Lincoln",,"$ 175.00","B10 G56"
 "Grant, Megan","Megan","grantm@fishmail.net","$ 500.00","B10 G56"

tests/phpunit/CRM/Report/Form/Contribute/fixtures/report-ascii.csv

-"Donor Name","First Name","Donor Email","Amount"
+"Donor Name","First Name","Donor Email","Amount"
 " Empowerment Association",,,"$ 50.00"
 "Bachman, Lincoln","Lincoln",,"$ 175.00"
 "Blackwell, Sanford","Sanford","st.blackwell3@testmail.co.pl","$ 250.00"

tests/phpunit/CRM/Report/Form/Contribute/fixtures/report.csv

-"Donor Name","First Name","Donor Email","Amount"
+"Donor Name","First Name","Donor Email","Amount"
 " Empowerment Association", , ,"$ 50.00"
 "Bachman, Lincoln","Lincoln", ,"$ 175.00"
 "Blackwell, Sanford","Sanford","st.blackwell3@testmail.co.pl","$ 250.00"

tests/phpunit/CRM/Report/Form/TestCaseTest.php

@@ -155,6 +155,12 @@ class CRM_Report_Form_TestCaseTest extends CiviReportTestCase {
     $expectedOutputCsvArray = $this->getArrayFromCsv(dirname(__FILE__) . "/{$expectedOutputCsvFile}");
     try {
       $this->assertCsvArraysEqual($expectedOutputCsvArray, $reportCsvArray);
+      // @todo But...doesn't this mean this test can't ever notify you of a
+      // fail? I think what you could do instead is right here in the try
+      // section throw something that doesn't get caught, since then that means
+      // the previous line passed and so the arrays ARE equal, which means
+      // something is wrong. Or just don't use assertCsvArraysEqual and
+      // explicity compare that they are NOT equal.
     }
     catch (PHPUnit\Framework\AssertionFailedError $e) {
       /* OK */

tests/phpunit/CRM/Report/Utils/ReportTest.php

@@ -47,7 +47,7 @@ class CRM_Report_Utils_ReportTest extends CiviUnitTestCase {
 ENDDETAILS;
 
     $expectedOutput = <<<ENDOUTPUT
-"Activity Type","Subject","Activity Details"\r
+\xEF\xBB\xBF"Activity Type","Subject","Activity Details"\r
 "Meeting","Meeting with the apostrophe's and that person who does ""air quotes"". Some non-ascii characters: дè","Here's some typical data from an activity details field.
   
 дè some non-ascii and html styling and these ̋“weird” quotes’s.

tests/phpunit/api/v3/JobTest.php

@@ -38,6 +38,12 @@ class api_v3_JobTest extends CiviUnitTestCase {
    */
   public $membershipTypeID;
 
+  /**
+   * Report instance used in mail_report tests.
+   * @var array
+   */
+  private $report_instance;
+
   /**
    * Set up for tests.
    */
@@ -55,6 +61,7 @@ class api_v3_JobTest extends CiviUnitTestCase {
       'parameters' => 'Semi-formal explanation of runtime job parameters',
       'is_active' => 1,
     ];
+    $this->report_instance = $this->createReportInstance();
   }
 
   /**
@@ -2253,4 +2260,186 @@ class api_v3_JobTest extends CiviUnitTestCase {
     return [$membershipTypeID, $groupID, $theChosenOneID, $provider];
   }
 
+  /**
+   * Test that the mail_report job sends an email for 'print' format.
+   *
+   * We're not testing that the report itself is correct since in 'print'
+   * format it's a little difficult to parse out, so we're just testing that
+   * the email was sent and it more or less looks like an email we'd expect.
+   */
+  public function testMailReportForPrint() {
+    $mut = new CiviMailUtils($this, TRUE);
+
+    // avoid warnings
+    if (empty($_SERVER['QUERY_STRING'])) {
+      $_SERVER['QUERY_STRING'] = 'reset=1';
+    }
+
+    $this->callAPISuccess('job', 'mail_report', [
+      'instanceId' => $this->report_instance['id'],
+      'format' => 'print',
+    ]);
+
+    $message = $mut->getMostRecentEmail('ezc');
+
+    $this->assertEquals('This is the email subject', $message->subject);
+    $this->assertEquals('reportperson@example.com', $message->to[0]->email);
+
+    $parts = $message->fetchParts(NULL, TRUE);
+    $this->assertCount(1, $parts);
+    $this->assertStringContainsString('test report', $parts[0]->text);
+
+    $mut->clearMessages();
+    $mut->stop();
+  }
+
+  /**
+   * Test that the mail_report job sends an email for 'pdf' format.
+   *
+   * We're not testing that the report itself is correct since in 'pdf'
+   * format it's a little difficult to parse out, so we're just testing that
+   * the email was sent and it more or less looks like an email we'd expect.
+   */
+  public function testMailReportForPdf() {
+    $mut = new CiviMailUtils($this, TRUE);
+
+    // avoid warnings
+    if (empty($_SERVER['QUERY_STRING'])) {
+      $_SERVER['QUERY_STRING'] = 'reset=1';
+    }
+
+    $this->callAPISuccess('job', 'mail_report', [
+      'instanceId' => $this->report_instance['id'],
+      'format' => 'pdf',
+    ]);
+
+    $message = $mut->getMostRecentEmail('ezc');
+
+    $this->assertEquals('This is the email subject', $message->subject);
+    $this->assertEquals('reportperson@example.com', $message->to[0]->email);
+
+    $parts = $message->fetchParts(NULL, TRUE);
+    $this->assertCount(2, $parts);
+    $this->assertStringContainsString('<title>CiviCRM Report</title>', $parts[0]->text);
+    $this->assertEquals(ezcMailFilePart::CONTENT_TYPE_APPLICATION, $parts[1]->contentType);
+    $this->assertEquals('pdf', $parts[1]->mimeType);
+    $this->assertEquals(ezcMailFilePart::DISPLAY_ATTACHMENT, $parts[1]->dispositionType);
+    $this->assertGreaterThan(0, filesize($parts[1]->fileName));
+
+    $mut->clearMessages();
+    $mut->stop();
+  }
+
+  /**
+   * Test that the mail_report job sends an email for 'csv' format.
+   *
+   * As with the print and pdf we're not super-concerned about report
+   * functionality itself - we're more concerned with the mailing part,
+   * but since it's csv we can easily check the output.
+   */
+  public function testMailReportForCsv() {
+    // Create many contacts, in particular so that the report would be more
+    // than a one-pager.
+    for ($i = 0; $i < 110; $i++) {
+      $this->individualCreate([], $i, TRUE);
+    }
+
+    $mut = new CiviMailUtils($this, TRUE);
+
+    // avoid warnings
+    if (empty($_SERVER['QUERY_STRING'])) {
+      $_SERVER['QUERY_STRING'] = 'reset=1';
+    }
+
+    $this->callAPISuccess('job', 'mail_report', [
+      'instanceId' => $this->report_instance['id'],
+      'format' => 'csv',
+    ]);
+
+    $message = $mut->getMostRecentEmail('ezc');
+
+    $this->assertEquals('This is the email subject', $message->subject);
+    $this->assertEquals('reportperson@example.com', $message->to[0]->email);
+
+    $parts = $message->fetchParts(NULL, TRUE);
+    $this->assertCount(2, $parts);
+    $this->assertStringContainsString('<title>CiviCRM Report</title>', $parts[0]->text);
+    $this->assertEquals('csv', $parts[1]->subType);
+
+    // Pull all the contacts to get our expected output.
+    $contacts = $this->callAPISuccess('Contact', 'get', [
+      'return' => 'sort_name',
+      'options' => [
+        'limit' => 0,
+        'sort' => 'sort_name',
+      ],
+    ]);
+    $rows = [];
+    foreach ($contacts['values'] as $contact) {
+      $rows[] = ['civicrm_contact_sort_name' => $contact['sort_name']];
+    }
+    // need this for makeCsv()
+    $fakeForm = new CRM_Report_Form();
+    $fakeForm->_columnHeaders = [
+      'civicrm_contact_sort_name' => [
+        'title' => 'Contact Name',
+        'type' => 2,
+      ],
+    ];
+
+    $this->assertEquals(
+      CRM_Report_Utils_Report::makeCsv($fakeForm, $rows),
+      $parts[1]->text
+    );
+
+    $mut->clearMessages();
+    $mut->stop();
+  }
+
+  /**
+   * Helper to create a report instance of the contact summary report.
+   */
+  private function createReportInstance() {
+    return $this->callAPISuccess('ReportInstance', 'create', [
+      'report_id' => 'contact/summary',
+      'title' => 'test report',
+      'form_values' => [
+        serialize([
+          'fields' => [
+            'sort_name' => '1',
+            'street_address' => '1',
+            'city' => '1',
+            'country_id' => '1',
+          ],
+          'sort_name_op' => 'has',
+          'sort_name_value' => '',
+          'source_op' => 'has',
+          'source_value' => '',
+          'id_min' => '',
+          'id_max' => '',
+          'id_op' => 'lte',
+          'id_value' => '',
+          'country_id_op' => 'in',
+          'country_id_value' => [],
+          'state_province_id_op' => 'in',
+          'state_province_id_value' => [],
+          'gid_op' => 'in',
+          'gid_value' => [],
+          'tagid_op' => 'in',
+          'tagid_value' => [],
+          'description' => 'Provides a list of address and telephone information for constituent records in your system.',
+          'email_subject' => 'This is the email subject',
+          'email_to' => 'reportperson@example.com',
+          'email_cc' => '',
+          'permission' => 'view all contacts',
+          'groups' => '',
+          'domain_id' => 1,
+        ]),
+      ],
+      // Email params need to be repeated outside form_values for some reason
+      'email_subject' => 'This is the email subject',
+      'email_to' => 'reportperson@example.com',
+    ]);
+  }
+
 }