From 5d233ba2f48816744f806f1b4684a4546f4d2697 Mon Sep 17 00:00:00 2001 From: Michael Gage Date: Wed, 5 Sep 2012 10:55:15 -0400 Subject: [PATCH] Fix bugs and improve error messages in SendMail.pm It's a wonder that anything worked here. It's clear that errors in mailing would not have been reported. The actual bug seems to be that the value $r needs to be passed explicitly into cleanup_register(). This was not being done. It is not clear why this used to work. The error checking routine has been beefed up and to some extent unit tested. The unit tests can be repeated by uncommenting the warn messages. --- .../ContentGenerator/Instructor/SendMail.pm | 75 +++++++++++-------- 1 file changed, 43 insertions(+), 32 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/Instructor/SendMail.pm b/lib/WeBWorK/ContentGenerator/Instructor/SendMail.pm index 32b8cdcedf..b71ec28f68 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/SendMail.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/SendMail.pm @@ -417,21 +417,21 @@ sub initialize { my $result_message = eval { $self->mail_message_to_recipients() }; if ($@) { # add the die message to the result message - $result_message = "An error occurred while trying to send email.\n" + $result_message .= "An error occurred while trying to send email.\n" . "The error message is:\n\n$@\n\n"; # and also write it to the apache log - $self->r->log->error("An error occurred while trying to send email: $@"); + $r->log->error("An error occurred while trying to send email: $@\n"); } # this could fail too... eval { $self->email_notification($result_message) }; if ($@) { - $self->r->log->error("An error occured while trying to send the email notification: $@"); + $r->log->error("An error occured while trying to send the email notification: $@\n"); } }; if (MP2) { - $r->connection->pool->cleanup_register($post_connection_action); + $r->connection->pool->cleanup_register($post_connection_action, $r); } else { - $r->post_connection($post_connection_action); + $r->post_connection($post_connection_action, $r); } } else { $self->addbadmessage(CGI::p("Didn't recognize button $action")); @@ -781,44 +781,55 @@ sub mail_message_to_recipients { my $merge_file = $self->{merge_file}; my $result_message = ''; my $failed_messages = 0; - + my $error_messages = ''; foreach my $recipient (@recipients) { - # warn "FIXME sending email to $recipient"; - my $error_messages = ''; + $error_messages = ''; + my $ur = $self->{db}->getUser($recipient); #checked unless ($ur) { - $error_messages .= "Record for user $recipient not found\n"; + $error_messages .= "Record for user $recipient not found\n"; next; } - unless ($ur->email_address) { - $error_messages .="User $recipient does not have an email address -- skipping\n"; + unless ($ur->email_address=~/\S/) { #unless address contains a non-blank charachter + $error_messages .="User $recipient does not have an email address -- skipping\n"; next; } + #warn "\nDEBUG: sending email to $recipient with email address ",$ur->email_address,"\n"; + my $msg = eval { $self->process_message($ur,$rh_merge_data) }; $error_messages .= "There were errors in processing user $recipient, merge file $merge_file. \n$@\n" if $@; - my $mailer = Mail::Sender->new({ - from => $ce->{mail}{smtpSender}, - fake_from => $from, - to => $ur->email_address, - smtp => $self->{smtpServer}, - subject => $subject, - headers => "X-Remote-Host: ".$self->{remote_host}, - }); - unless (ref $mailer) { - $error_messages .= "Failed to create a mailer for user $recipient: $Mail::Sender::Error\n"; + #warn "message is ok"; + my $mailer = eval{ Mail::Sender->new({ + from => $ce->{mail}{smtpSender}, + fake_from => $from, + to => $ur->email_address, + smtp => $self->{smtpServer}, + subject => $subject, + headers => "X-Remote-Host: ".$self->{remote_host}, + }) + }; + if ($@) { + $error_messages .= "Failed to create a mailer for user $recipient: $Mail::Sender::Error\n$@\n"; next; } - unless (ref $mailer->Open()) { - $error_messages .= "Failed to open the mailer for user $recipient: $Mail::Sender::Error\n"; + #warn "DEBUG: mailer created as $mailer\n"; + unless (ref($mailer) and $mailer->Open()) { + $error_messages .= "Failed to open the mailer for user $recipient: $@\n $Mail::Sender::Error\n"; next; } - my $MAIL = $mailer->GetHandle() || ($error_messages .= "Couldn't get mailer handle \n"); - print $MAIL $msg || ($error_messages .= "Couldn't print to $MAIL"); - close $MAIL || ($error_messages .= "Couldn't close $MAIL"); - #warn "FIXME mailed to $recipient: ", $ur->email_address, " from $from subject $subject Errors: $error_messages"; - $failed_messages++ if $error_messages; - $result_message .= $error_messages; - } + #warn "DEBUG: mailer opened\n"; + my $MAIL = $mailer->GetHandle() || ($error_messages .= "$recipient: Couldn't get mailer handle \n"); + print $MAIL $msg || ($error_messages .= "$recipient: Couldn't print to mail $MAIL\n"); + close $MAIL || ($error_messages .= "$recipient: Couldn't close mail $MAIL -- possibly a badly formed address: ".$ur->email_address."\n"); + #warn "DEBUG: mailed to $recipient: ", $ur->email_address, " from $from subject $subject. Errors:\n $error_messages\n\n"; + #FIXME -- allow this list to be turned off with a "verbose" flag + $result_message .= "Msg sent to $recipient at ".$ur->email_address."\n" unless $error_messages; + } continue { #update failed messages before continuing loop + if ($error_messages) { + $failed_messages++; + $result_message .= $error_messages; + } + } my $courseName = $self->r->urlpath->arg("courseID"); my $number_of_recipients = scalar(@recipients) - $failed_messages; $result_message = <{defaultFrom}; + warn "\ninstructor message \"". $self->{subject}."\" sent to ", $self->{defaultFrom},"\n"; } sub getRecord {