Skip to content

Commit

Permalink
Merge pull request #177 from catalyst/fix-email-inconsistency
Browse files Browse the repository at this point in the history 
[#139] Ensure all emails send same message
  • Loading branch information
@matthewhilton
matthewhilton authored Aug 23, 2024
2 parents a0cc8a5 + 0291d86 commit 6d18731
Show file tree
Hide file tree
Showing 3 changed files with 244 additions and 54 deletions.
5 changes: 4 additions & 1 deletion classes/booking_manager.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,10 @@ public function validate($timenow = null): array {
global $DB;
$errors = [];
$sessioncapacitycache = [];
$timenow ??= time();

if ($timenow == null) {
$timenow = time();
}

// Break into rows and validate the multiple interdependant fields together.
foreach ($this->get_iterator() as $index => $entry) {
Expand Down
77 changes: 24 additions & 53 deletions lib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1123,7 +1123,11 @@ function facetoface_get_future_sessions(int $id): array {
$now = time();
return array_filter(
facetoface_get_sessions($id),
fn (stdClass $s): bool => !empty(array_filter($s->sessiondates, fn (stdClass $d): bool => $d->timestart > $now))
function(stdClass $s) use ($now) {
return !empty(array_filter($s->sessiondates, function(stdClass $d) use ($now) {
return $d->timestart > $now;
}));
}
);
}

Expand Down Expand Up @@ -2238,6 +2242,12 @@ function facetoface_send_notice($postsubject, $posttext, $posttextmgrheading,
}
}

// Define a simple reusable function so we don't have to copy
// and paste a huge function call multiple times.
$substitute = function($text, $session) use ($facetoface, $user) {
return facetoface_email_substitutions($text, format_string($facetoface->name), $facetoface->reminderperiod, $user, $session, $session->id);
};

// Do iCal attachement stuff.
$icalattachments = [];
if ($notificationtype & MDL_F2F_ICAL) {
Expand All @@ -2249,71 +2259,32 @@ function facetoface_send_notice($postsubject, $posttext, $posttextmgrheading,
$session->sessiondates = [$sessiondate]; // One day at a time.

$filename = facetoface_get_ical_attachment($notificationtype, $facetoface, $session, $user);
$subject = facetoface_email_substitutions(
$postsubject,
format_string($facetoface->name),
$facetoface->reminderperiod,
$user,
$session,
$session->id
);
$body = facetoface_email_substitutions($posttext, format_string($facetoface->name), $facetoface->reminderperiod,
$user, $session, $session->id);
$htmlmessage = facetoface_email_substitutions(
$posttext,
$facetoface->name,
$facetoface->reminderperiod,
$user,
$session,
$session->id
);
$htmlbody = $htmlmessage;
$subject = $substitute($postsubject, $session);
$body = $substitute($posttext, $session);
$icalattachments[] = [
'filename' => $filename, 'subject' => $subject,
'body' => $body, 'htmlbody' => $htmlbody,
'body' => $body,
];
}

// Restore session dates.
$session->sessiondates = $sessiondates;
} else {
$filename = facetoface_get_ical_attachment($notificationtype, $facetoface, $session, $user);
$subject = facetoface_email_substitutions($postsubject, format_string($facetoface->name), $facetoface->reminderperiod,
$user, $session, $session->id);
$body = facetoface_email_substitutions($posttext, format_string($facetoface->name), $facetoface->reminderperiod,
$user, $session, $session->id);
$htmlmessage = facetoface_email_substitutions(
$posttext,
$facetoface->name,
$facetoface->reminderperiod,
$user,
$session,
$session->id
);
$htmlbody = $htmlmessage;
$subject = $substitute($postsubject, $session);
$body = $substitute($posttext, $session);
$icalattachments[] = [
'filename' => $filename, 'subject' => $subject,
'body' => $body, 'htmlbody' => $htmlbody,
'body' => $body,
];
}
}

// Fill-in the email placeholders.
$postsubject = facetoface_email_substitutions($postsubject, format_string($facetoface->name), $facetoface->reminderperiod,
$user, $session, $session->id);
$posttext = facetoface_email_substitutions($posttext, format_string($facetoface->name), $facetoface->reminderperiod,
$user, $session, $session->id);

$posttextmgrheading = facetoface_email_substitutions(
$posttextmgrheading,
format_string($facetoface->name),
$facetoface->reminderperiod,
$user,
$session,
$session->id
);
$postsubject = $substitute($postsubject, $session);
$posttext = $substitute($posttext, $session);
$posttextmgrheading = $substitute($posttextmgrheading, $session);

$posthtml = ''; // FIXME.
if ($fromaddress = get_config('facetoface', 'fromaddress')) {
$from = new stdClass();
$from->maildisplay = true;
Expand All @@ -2326,7 +2297,7 @@ function facetoface_send_notice($postsubject, $posttext, $posttextmgrheading,
if ($notificationtype & MDL_F2F_ICAL) {
foreach ($icalattachments as $attachment) {
if (!email_to_user($user, $from, $attachment['subject'], $attachment['body'],
$attachment['htmlbody'], $attachment['filename'], $attachmentfilename)) {
'', $attachment['filename'], $attachmentfilename)) {
return 'error:cannotsendconfirmationuser';
}
unlink($CFG->dataroot . '/' . $attachment['filename']);
Expand All @@ -2335,7 +2306,7 @@ function facetoface_send_notice($postsubject, $posttext, $posttextmgrheading,

// Send plain text email.
if ($notificationtype & MDL_F2F_TEXT
&& !email_to_user($user, $from, $postsubject, $posttext, $posthtml)) {
&& !email_to_user($user, $from, $postsubject, $posttext)) {
return 'error:cannotsendconfirmationuser';
}

Expand All @@ -2347,7 +2318,7 @@ function facetoface_send_notice($postsubject, $posttext, $posttextmgrheading,
$manager->email = $manageremail;

// Leave out the ical attachments in the managers notification.
if (!email_to_user($manager, $from, $postsubject, $managertext, $posthtml)) {
if (!email_to_user($manager, $from, $postsubject, $managertext)) {
return 'error:cannotsendconfirmationmanager';
}
}
Expand All @@ -2361,7 +2332,7 @@ function facetoface_send_notice($postsubject, $posttext, $posttextmgrheading,
$thirdparty->email = trim($recipient);

// Leave out the ical attachments in the 3rd parties notification.
if (!email_to_user($thirdparty, $from, $postsubject, $posttext, $posthtml)) {
if (!email_to_user($thirdparty, $from, $postsubject, $posttext)) {
return 'error:cannotsendconfirmationthirdparty';
}
}
Expand Down
216 changes: 216 additions & 0 deletions tests/session_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@

use core_date;

defined('MOODLE_INTERNAL') || die();
global $CFG;
require_once("$CFG->dirroot/mod/facetoface/lib.php");

/**
* Test the session helper class.
*
Expand Down Expand Up @@ -112,4 +116,216 @@ public function test_get_readable_session_time_with_users_timezone() {
$expectedstring = "1 January 2030, 9:00 AM - 4 January 2030, 5:00 PM (time zone: $expectedtimezone)";
$this->assertEquals($expectedstring, session::get_readable_session_datetime($date));
}

/**
* Provides values to test_email_notification
* @return array
*/
public static function email_notification_provider(): array {
$htmlconfirmmessage = "
<p> This is the confirm message </p>
<p> Details: </p>
[details]
";

$htmldetails = "
<p> This is a html message </p>
<br />
<ul>
<li> Test1 </li>
<li> Test2 </li>
</ul>
";

$expectedhtmlmessage = "<div class=\"text_to_html\"><br />
<p> This is the confirm message </p><p> Details: </p> This is a html message<br />
<br />
* Test1<br />
* Test2<br />
<br />
<br />
</div>";

// Because moodle code standards specify spaces over tabs, editors will automatically insert spaces
// into the string above instead of tabs.
// However we need tabs there, because this is what html_to_text uses for converting <li> to lists with a '\t*'.
$expectedhtmlmessage = str_replace(' *', "\t*", $expectedhtmlmessage);

$plaintextmessage = 'This is a plain text message
It has plain text stuff in it
* This is a fake list
* Another fake list item
(test)[test]{test}!!@@##////
[details]';

$plaintextdetails = "This is a plain text detail
It has plain text stuff in it";

$expectedplaintextmessage = "This is a plain text message<br />
It has plain text stuff in it<br />
* This is a fake list<br />
* Another fake list item<br />
(test)[test]{test}!!@@##////<br />
This is a plain text detail<br />
It has plain text stuff in it";

// phpcs:ignore.
$expectedhtmlandplainmessage = "<p> This is the confirm message </p><p> Details: </p> This is a plain text detail<br />
It has plain text stuff in it<br />";

// Generate a matrix of tests, with all the types, and all the message combinations.
$types = [
'both' => [
'type' => MDL_F2F_BOTH,
'emails' => 2,
'icalemails' => 1,
],
'ical only' => [
'type' => MDL_F2F_ICAL,
'emails' => 1,
'icalemails' => 1,
],
'text only' => [
'type' => MDL_F2F_TEXT,
'emails' => 1,
'icalemails' => 0,
],
];
$messages = [
'html message and html details' => [
'message' => $htmlconfirmmessage,
'details' => $htmldetails,
'expected' => $expectedhtmlmessage,
],
'plain message and plain details' => [
'message' => $plaintextmessage,
'details' => $plaintextdetails,
'expected' => $expectedplaintextmessage,
],
'plain text message and html details' => [
'message' => $htmlconfirmmessage,
'details' => $plaintextdetails,
'expected' => $expectedhtmlandplainmessage,
],
];

$tests = [];
foreach ($types as $typename => $typedata) {
foreach ($messages as $messagename => $messagedata) {
$testname = 'email with message: ' . $messagename . ' with type: ' . $typename;

$tests[$testname] = [
'type' => $typedata['type'],
'confirmmessage' => $messagedata['message'],
'details' => $messagedata['details'],
'expectedcount' => $typedata['emails'],
'expectedicalamount' => $typedata['icalemails'],
'expectedmessage' => $messagedata['expected'],
];
}
}

return $tests;
}

/**
* Tests email notification construction.
* @param int $notifytype type of notification
* @param string $confirmmessage the confirmation message to set
* @param string $details details to set in f2f settings
* @param int $expectedemailcount expected amount of emails that should be sent
* @param int $expectedicalamount expected amount of email with ical attachments that should be sent
* @param string $expectedmessage a string that the output of all emails should contain
* @dataProvider email_notification_provider
*/
public function test_email_notification(int $notifytype, string $confirmmessage, string $details, int $expectedemailcount,
int $expectedicalamount, string $expectedmessage) {

/** @var \mod_facetoface_generator $generator */
$generator = $this->getDataGenerator()->get_plugin_generator('mod_facetoface');

// Setup course, f2f and user.
$course = $this->getDataGenerator()->create_course();
$facetoface = $generator->create_instance([
'course' => $course->id,
'confirmationsubject' => 'Confirmation',
'confirmationmessage' => $confirmmessage,
'confirmationmessageformat' => FORMAT_HTML,
]);
$user = $this->getDataGenerator()->create_user();
$this->getDataGenerator()->create_and_enrol($course, 'student');

// Create a session with start and end times.
$now = time();
$session = $generator->create_session([
'facetoface' => $facetoface->id,
'capacity' => '3',
'allowoverbook' => '0',
'details' => $details,
'duration' => '1.5', // One and half hours.
'normalcost' => '111',
'discountcost' => '11',
'allowcancellations' => '0',
'sessiondates' => [
['timestart' => $now + 1 * DAYSECS, 'timefinish' => $now + 2 * DAYSECS],
],
]);

// Sign user up to session, capturing emails.
$sink = $this->redirectEmails();
facetoface_user_signup($session, $facetoface, $course, '', $notifytype, MDL_F2F_STATUS_BOOKED, $user->id);
$messages = $sink->get_messages();
$this->assertCount($expectedemailcount, $messages);

// Ensure number of ical attachment emails is same as expected.
$icalemails = array_filter($messages, function($message) {
return strpos($message->body, 'Content-Disposition: attachment; filename=invite.ics') != false;
});
$this->assertCount($expectedicalamount, $icalemails);

// Do a very crude form of email multi-mime message parsing.
// to extract the plaintext and html segments of the email.
$messagessections = array_map(function($message) {
// Split on '--' which is the start of the separator in the email html multi-mime message.
$sections = explode('--', $message->body);

// Extract the html section.
$htmlsection = current(array_filter($sections, function($section) {
return strpos($section, 'text/html') != false;
}));
$htmllines = explode("\n", $htmlsection);
unset($htmllines[0]);
$html = implode("\n", $htmllines);
$html = quoted_printable_decode($html);

// Do the same for the plaintext.
$plaintextsection = current(array_filter($sections, function($section) {
return strpos($section, 'text/plain') != false;
}));
$plaintextlines = explode("\n", $plaintextsection);
unset($plaintextlines[0]);
$plaintext = implode("\n", $plaintextlines);
$plaintext = quoted_printable_decode($plaintext);

return [
'html' => $html,
'plaintext' => $plaintext,
];
}, $messages);

$messagehtmls = array_column($messagessections, 'html');
$messageplaintexts = array_column($messagessections, 'plaintext');

// Ensure each message has both html and plaintext.
$this->assertTrue(count($messagehtmls) == count($messageplaintexts));

// Ensure all the HTML messages are the same
// (note this is only applicable for 'both' because it's the only one that sends two emails).
$this->assertCount(1, array_unique($messagehtmls), "Emails should have the same HTML message");

// Ensure each message has the expected html.
foreach ($messagehtmls as $messagehtml) {
$this->assertStringContainsString($expectedmessage, $messagehtml);
}
}
}

0 comments on commit 6d18731

Please sign in to comment.