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.

Sign up for GitHub

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

Jump to bottom

dev/core#2866 Generate text version of message at send time if not present #22632

Merged
merged 1 commit into from
Feb 2, 2022
Merged

dev/core#2866 Generate text version of message at send time if not present #22632

merged 1 commit into from
Feb 2, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

dev/core#2866 Generate text version of message at send time if not present

Before

send function only sends text version of the email if it is present

After

text version always included, converted from html

Technical Details

https://lab.civicrm.org/dev/core/-/issues/2866

Comments

@civibot
Copy link

civibot bot commented Jan 26, 2022

(Standard links)

@civibot civibot bot added the master label Jan 26, 2022
@eileenmcnaughton eileenmcnaughton force-pushed the msg_send branch 2 times, most recently from d1d2266 to d3892ca Compare January 27, 2022 00:32
@yashodha
Copy link
Contributor

test this please

demeritcowboy
demeritcowboy reviewed Jan 31, 2022
View reviewed changes
CRM/Utils/Mail.php Outdated
// CRM-6224
if (trim(CRM_Utils_String::htmlToText($htmlMessage)) == '') {
$htmlMessage = $params['html'] ?? FALSE;
if (trim(CRM_Utils_String::htmlToText((string) $htmlMessage))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be == '' like before, otherwise it always sets htmlMessage to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opps - yes!

@demeritcowboy
Copy link
Contributor 

not ok 42 - Failure: CRM_Activity_BAO_ActivityTest::testSendEmailBasicWithoutAnyTokens
  ---
  message: 'testSendEmailBasicWithoutAnyTokens html .  not found in  MIME-Version: 1.0'
  severity: fail
  ...

There's a couple more like that.

@eileenmcnaughton
Copy link
Contributor Author

I think the remaining test fails probably should be fixed in the test

api_v3_JobTest::testMailReportForPrint
Failed asserting that actual size 2 matches expected size 1.

/home/jenkins/bknix-dfl/build/core-22632-6dhic/web/sites/all/modules/civicrm/tests/phpunit/api/v3/JobTest.php:2359
/home/jenkins/bknix-dfl/build/core-22632-6dhic/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:262
/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:671

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy I've pushed up a fix to the tests - basically when we send out reports there is now a text version & an html version & the text version is shifted onto the start of the array. I think having the text version as well is kinda neutral (in that no-one has ever complained we didn't have it before :-))

@demeritcowboy
Copy link
Contributor

I think that's fair. It's unlikely somebody is autoparsing these reports as system-to-system communications, as might have been a not-so-crazy thing to do in the previous century.

demeritcowboy
demeritcowboy reviewed Feb 1, 2022
View reviewed changes
CRM/Utils/Mail.php
$htmlMessage = FALSE;
}
$attachments = $params['attachments'] ?? NULL;
if (!empty($params['text'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This means you wouldn't be able to explicitly set the text to the empty string, but I can't think many people are currently doing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@demeritcowboy yeah my thinking is there isn't much use-case for it

@demeritcowboy
Copy link
Contributor

Looks like line 2468 in the test also needs shifting by one array element.

@demeritcowboy demeritcowboy merged commit c4f91c0 into civicrm:master Feb 2, 2022
@eileenmcnaughton eileenmcnaughton deleted the msg_send branch February 2, 2022 20:02
@eileenmcnaughton
Copy link
Contributor Author

thanks @demeritcowboy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@demeritcowboy demeritcowboy demeritcowboy left review comments

Assignees
No one assigned
Labels
master merge on pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eileenmcnaughton @yashodha @demeritcowboy