Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Tiding up conflicting logic on email alerts (Issue #943) #1249

Merged
merged 3 commits into from

3 participants

@ditorelo

HTML2Text was being called on message, but we're sending messages as HTML. This was producing nasty emails and sad alertees.

Removed the call to HTML2Text and wrapped function on text::auto_p() to produce better looking messages.

Diogo Freire Tyding up conflicting logic on email alerts
HTML2Text was being called on message, but we're sending messages as HTML.
This was producing nasty emails and sad alertees.

Removed the call to HTML2Text and wrapped functino on text::auto_p() to
produce better looking messages
7bd7963
@rjmackay
Owner

The original behaviour here was to send plain text emails.. I was working on the same issue for a client build recently but haven't had a chance to clean up my fix yet.

Any thought on whether plain text or html message will work better?

@ditorelo

I reckon we have more general control of how it renders with HTML markup as I've seen line breaks go completely bust in plain text message in a few cases before. (This is a very weak argument though...)

I'm not sure if SwiftMailer creates a text based version of the message as it sends it. I'll have a look and get back to you.

@ditorelo

I mean, I've seen a few email clients completely bust line breaks in plain text messages before. :)

@ditorelo

SwiftMailer doesn't automagically generate a text version of the message. Having said that, the text version for this particular case is not that unreadable.

mail - zombie reports asdfas

Another possible solution would be to update email_Core::send:

  • if the function is called with $html = TRUE
  • run message content through html::strip_tags
  • call $message->addPart($content, 'text/plain'); to provide a plain text option.

This won't change current behaviour for anyone I think. :)

@rjmackay
Owner

Ooh. Sending an HTML and plain text version would definitely be good. It'd allow us to improve some of the existing message formatting too.
Would it make sense to run it through HTML2Text for the plain text version?

One thing to watch for is that there are a few of the message templates that assume they'll be sent in plain text, so you might need to use nl2br on those..

@ditorelo

I'll have a go at it.

HTML2Text seems to be doing a very bad job on what it promisses - stripping line breaks and such.
I'll have a play with our options and get back to you.

@ditorelo

Update behaviour so:

  • Every message that is created with $html = TRUE will attach a plain text version of itself to message body.
  • Messages with $html = FALSE remain unaltered.

HTML2Text is nice, the way the alerts function was mixing it up with the html header is not.

I reckon this might call for a review of all calls to email::send() around the code. It might be the case that a plain text message is being passed to the function with $html = TRUE, which will make things confusing (this was the cause of the original problem with the alerts).

Wadjuracoon?

@rjmackay rjmackay commented on the diff
system/helpers/email.php
@@ -113,16 +113,22 @@ public static function connect($config = NULL)
* @param boolean send email as HTML
* @return integer number of emails sent
*/
- public static function send($to, $from, $subject, $message, $html = FALSE)
@rjmackay Owner
rjmackay added a note

I think you should be able to override/extend this function by adding a file application/helpers/MY_email.php. If we do it that way its easier to see whats been customised or not.

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

Definitely like this idea. You're right that we'll need to review all the other calls to email::send() since some of them do something similar to alerts.

@rjmackay rjmackay commented on the diff
application/controllers/scheduler/s_alerts.php
((6 lines not shown))
// EMAIL MESSAGE
- $email_message = $incident_description."\n\n".$incident_url;
+ $email_message = $incident_description . "\n\n" . $incident_url;
// SMS MESSAGE
$sms_message = $incident_description;
@rjmackay Owner

Need to update this to make sure we still remove HTML from the SMS message :)

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

reviewed the calls to email::send() and they are on $html = FALSE.

@kamaulynder kamaulynder merged commit e6e619d into ushahidi:develop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 24, 2013
  1. Tyding up conflicting logic on email alerts

    Diogo Freire authored
    HTML2Text was being called on message, but we're sending messages as HTML.
    This was producing nasty emails and sad alertees.
    
    Removed the call to HTML2Text and wrapped functino on text::auto_p() to
    produce better looking messages
Commits on Sep 25, 2013
  1. Updating send class to attach a plain text version to message when $h…

    Diogo Freire authored
    …tml = true
  2. Cleaning up alerts scheduler

    Diogo Freire authored
This page is out of date. Refresh to see the latest.
View
12 application/controllers/scheduler/s_alerts.php
@@ -86,7 +86,7 @@ public function index()
l.latitude, l.longitude FROM ".$this->table_prefix."incident AS i INNER JOIN ".$this->table_prefix."location AS l ON i.location_id = l.id
WHERE i.incident_active=1 AND i.incident_alert_status = 1 ");
// End of New Code
-
+
foreach ($incidents as $incident)
{
// ** Pre-Formatting Message ** //
@@ -94,11 +94,9 @@ public function index()
$incident_description = $incident->incident_description;
$incident_url = url::site().'reports/view/'.$incident->id;
$incident_description = html::clean($incident_description);
- $html2text = new Html2Text($incident_description);
- $incident_description = $html2text->get_text();
// EMAIL MESSAGE
- $email_message = $incident_description."\n\n".$incident_url;
+ $email_message = $incident_description . "\n\n" . $incident_url;
// SMS MESSAGE
$sms_message = $incident_description;
@rjmackay Owner

Need to update this to make sure we still remove HTML from the SMS message :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@@ -179,9 +177,9 @@ public function index()
$from[] = $alerts_email;
$from[] = $site_name;
$subject = "[$site_name] ".$incident->incident_title;
- $message = $email_message
- ."\n\n".$unsubscribe_message
- .$alertee->alert_code."\n";
+ $message = text::auto_p($email_message
+ . "\n\n".$unsubscribe_message
+ . $alertee->alert_code . "\n");
//if (email::send($to, $from, $subject, $message, FALSE) == 1)
if (email::send($to, $from, $subject, $message, TRUE) == 1) // HT: New Code
View
12 system/helpers/email.php
@@ -113,16 +113,22 @@ public static function connect($config = NULL)
* @param boolean send email as HTML
* @return integer number of emails sent
*/
- public static function send($to, $from, $subject, $message, $html = FALSE)
@rjmackay Owner
rjmackay added a note

I think you should be able to override/extend this function by adding a file application/helpers/MY_email.php. If we do it that way its easier to see whats been customised or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ public static function send($to, $from, $subject, $content, $html = FALSE)
{
// Connect to SwiftMailer
(email::$mail === NULL) and email::connect();
// Determine the message type
- $html = ($html === TRUE) ? 'text/html' : 'text/plain';
+ $header_type = ($html === TRUE) ? 'text/html' : 'text/plain';
// Create the message
- $message = new Swift_Message($subject, $message, $html, '8bit', 'utf-8');
+ $message = new Swift_Message($subject, $content, $header_type, '8bit', 'utf-8');
+
+ if ($html === TRUE)
+ {
+ $html2text = new Html2Text($content);
+ $message->attach(new Swift_Message_Part($html2text->get_text(), 'text/plain'));
+ }
if (is_string($to))
{
Something went wrong with that request. Please try again.