From 0b9c93e00e3d9a62b1d64d203dff16d6d8844fe3 Mon Sep 17 00:00:00 2001 From: FreeScout Date: Wed, 22 Aug 2018 00:26:42 -0700 Subject: [PATCH] Logging send errors --- app/Jobs/SendNotificationToUsers.php | 75 ++++++++++++------- app/Jobs/SendReplyToCustomer.php | 55 +++++++------- app/Listeners/SendNotificationToUsers.php | 2 +- app/Subscription.php | 12 +-- config/app.php | 8 +- public/js/main.js | 5 ++ .../ajax_html/send_log.blade.php | 6 +- resources/views/conversations/view.blade.php | 10 ++- resources/views/secure/logs.blade.php | 4 +- 9 files changed, 102 insertions(+), 75 deletions(-) diff --git a/app/Jobs/SendNotificationToUsers.php b/app/Jobs/SendNotificationToUsers.php index b9e1acda..c87ef828 100644 --- a/app/Jobs/SendNotificationToUsers.php +++ b/app/Jobs/SendNotificationToUsers.php @@ -59,7 +59,9 @@ class SendNotificationToUsers implements ShouldQueue $headers['In-Reply-To'] = '<'.$prev_message_id.'>'; $headers['References'] = '<'.$prev_message_id.'>'; - $all_failures = []; + // We throw an exception if any of the send attempts throws an exception (connection error, etc) + $global_exception = null; + foreach ($this->users as $user) { $message_id = \App\Mail\Mail::MESSAGE_ID_PREFIX_NOTIFICATION.'-'.$last_thread->id.'-'.$user->id.'-'.time().'@'.$mailbox->getEmailDomain(); $headers['Message-ID'] = $message_id; @@ -77,43 +79,66 @@ class SendNotificationToUsers implements ShouldQueue } $from = ['address' => $mailbox->email, 'name' => $from_name]; - Mail::to([['name' => $user->getFullName(), 'email' => $user->email]]) - ->send(new UserNotification($user, $this->conversation, $this->threads, $headers, $from)); + $exception = null; - $failures = Mail::failures(); + try { + Mail::to([['name' => $user->getFullName(), 'email' => $user->email]]) + ->send(new UserNotification($user, $this->conversation, $this->threads, $headers, $from)); + } catch (\Exception $e) { + // We come here in case SMTP server unavailable for example + activity() + ->causedBy($user) + ->withProperties([ + 'error' => $e->getMessage().'; File: '.$e->getFile().' ('.$e->getLine().')', + ]) + ->useLog(\App\ActivityLog::NAME_EMAILS_SENDING) + ->log(\App\ActivityLog::DESCRIPTION_EMAILS_SENDING_ERROR_TO_USER); - // Save to send log - if (!empty($failures) && in_array($user->email, $failures)) { - $status = SendLog::STATUS_SEND_ERROR; - } else { - $status = SendLog::STATUS_ACCEPTED; + $exception = $e; + $global_exception = $e; } - SendLog::log($last_thread->id, $message_id, $user->email, $status, null, $user->id); - $all_failures = array_merge($all_failures, $failures); + if ($exception) { + $status = SendLog::STATUS_SEND_ERROR; + $status_message = $exception->getMessage(); + } else { + $failures = Mail::failures(); + + // Save to send log + if (!empty($failures) && in_array($user->email, $failures)) { + $status = SendLog::STATUS_SEND_ERROR; + } else { + $status = SendLog::STATUS_ACCEPTED; + } + } + + SendLog::log($last_thread->id, $message_id, $user->email, $status, null, $user->id, $status_message); } - if (!empty($all_failures)) { - throw new \Exception('Could not send email to: '.implode(', ', $all_failures)); + + if ($global_exception) { + throw $global_exception; } } /** * The job failed to process. + * This method is called after attempts had finished. + * At this stage method has access only to variables passed in constructor. * * @param Exception $exception * * @return void */ - public function failed(\Exception $e) - { - // Write to activity log - activity() - //->causedBy($this->customer) - ->withProperties([ - 'error' => $e->getMessage().'; File: '.$e->getFile().' ('.$e->getLine().')', - //'to' => $this->customer->getMainEmail(), - ]) - ->useLog(\App\ActivityLog::NAME_EMAILS_SENDING) - ->log(\App\ActivityLog::DESCRIPTION_EMAILS_SENDING_ERROR_TO_USER); - } + // public function failed(\Exception $e) + // { + // // Write to activity log + // activity() + // //->causedBy($this->customer) + // ->withProperties([ + // 'error' => $e->getMessage().'; File: '.$e->getFile().' ('.$e->getLine().')', + // //'to' => $this->customer->getMainEmail(), + // ]) + // ->useLog(\App\ActivityLog::NAME_EMAILS_SENDING) + // ->log(\App\ActivityLog::DESCRIPTION_EMAILS_SENDING_ERROR_TO_USER); + // } } diff --git a/app/Jobs/SendReplyToCustomer.php b/app/Jobs/SendReplyToCustomer.php index f1b05dc3..c5aef1eb 100644 --- a/app/Jobs/SendReplyToCustomer.php +++ b/app/Jobs/SendReplyToCustomer.php @@ -93,17 +93,21 @@ class SendReplyToCustomer implements ShouldQueue ->bcc($bcc_array) ->send(new ReplyToCustomer($this->conversation, $this->threads, $headers)); } catch (\Exception $e) { + // We come here in case SMTP server unavailable for example activity() - //->causedBy() - ->withProperties([ + ->causedBy($this->customer) + ->withProperties([ 'error' => $e->getMessage().'; File: '.$e->getFile().' ('.$e->getLine().')', - ]) - ->useLog(\App\ActivityLog::NAME_EMAILS_SENDING) - ->log(\App\ActivityLog::DESCRIPTION_EMAILS_SENDING_ERROR_TO_CUSTOMER); + ]) + ->useLog(\App\ActivityLog::NAME_EMAILS_SENDING) + ->log(\App\ActivityLog::DESCRIPTION_EMAILS_SENDING_ERROR_TO_CUSTOMER); - // Failures will be save to send log when retry attempts will finish + // Failures will be saved to send log when retry attempts will finish $this->failures = $this->recipients; + // Save to send log + $this->saveToSendLog($e->getMessage()); + throw $e; } @@ -117,51 +121,50 @@ class SendReplyToCustomer implements ShouldQueue // Save to send log $this->saveToSendLog(); - - if (!empty($failures)) { - throw new \Exception('Could not send email to: '.implode(', ', $failures)); - } } /** * The job failed to process. - * This method is called after number of attempts had finished. + * This method is called after attempts had finished. + * At this stage method has access only to variables passed in constructor. * * @param Exception $exception * * @return void */ - public function failed(\Exception $e) - { - activity() - ->causedBy($this->customer) - ->withProperties([ - 'error' => $e->getMessage().'; File: '.$e->getFile().' ('.$e->getLine().')', - 'to' => $this->customer->getMainEmail(), - ]) - ->useLog(\App\ActivityLog::NAME_EMAILS_SENDING) - ->log(\App\ActivityLog::DESCRIPTION_EMAILS_SENDING_ERROR_TO_CUSTOMER); + // public function failed(\Exception $e) + // { + // activity() + // ->causedBy($this->customer) + // ->withProperties([ + // 'error' => $e->getMessage().'; File: '.$e->getFile().' ('.$e->getLine().')', + // 'to' => $this->customer->getMainEmail(), + // ]) + // ->useLog(\App\ActivityLog::NAME_EMAILS_SENDING) + // ->log(\App\ActivityLog::DESCRIPTION_EMAILS_SENDING_ERROR_TO_CUSTOMER); - $this->saveToSendLog(); - } + // $this->saveToSendLog(); + // } /** - * Save failed email to send log. + * Save emails to send log. */ - public function saveToSendLog() + public function saveToSendLog($error_message = '') { foreach ($this->recipients as $recipient) { if (in_array($recipient, $this->failures)) { $status = SendLog::STATUS_SEND_ERROR; + $status_message = $error_message; } else { $status = SendLog::STATUS_ACCEPTED; + $status_message = ''; } if ($this->customer_email == $recipient) { $customer_id = $this->customer->id; } else { $customer_id = null; } - SendLog::log($this->last_thread->id, $this->message_id, $recipient, $status, $customer_id); + SendLog::log($this->last_thread->id, $this->message_id, $recipient, $status, $customer_id, null, $status_message); } } } diff --git a/app/Listeners/SendNotificationToUsers.php b/app/Listeners/SendNotificationToUsers.php index f972c774..d8695e7b 100644 --- a/app/Listeners/SendNotificationToUsers.php +++ b/app/Listeners/SendNotificationToUsers.php @@ -27,7 +27,7 @@ class SendNotificationToUsers // Detect event type by event class switch (get_class($event)) { case 'App\Events\UserReplied': - $caused_by_user_id = $event->conversation->created_by_user_id; + $caused_by_user_id = $event->thread->created_by_user_id; $event_type = Subscription::EVENT_TYPE_USER_REPLIED; break; case 'App\Events\UserCreatedConversation': diff --git a/app/Subscription.php b/app/Subscription.php index b25969d2..bda84e71 100644 --- a/app/Subscription.php +++ b/app/Subscription.php @@ -281,6 +281,7 @@ class Subscription extends Model if (!$users_to_notify) { continue; } + foreach ($users_to_notify as $medium => $medium_users_to_notify) { // Remove current user from recipients if action caused by current user @@ -291,17 +292,6 @@ class Subscription extends Model } if (count($medium_users_to_notify)) { - - // Remove duplicate users - // $user_ids = []; - // foreach ($users as $i => $user) { - // if (in_array($user->id, $user_ids)) { - // unset($users[$i]); - // } else { - // $user_ids[] = $user->id; - // } - // } - $notify[$medium][$event['conversation']->id] = [ // Users subarray contains all users who need to receive notification for all events 'users' => array_unique(array_merge($users, $medium_users_to_notify)), diff --git a/config/app.php b/config/app.php index f5799ab9..82378de4 100644 --- a/config/app.php +++ b/config/app.php @@ -156,12 +156,12 @@ return [ /* |-------------------------------------------------------------------------- | Parameters used to run queued jobs processing. - | Check for new jobs every 5 seconds. - | Retry failed jobs max 7 times (for example no connection to the mail server) - | Tries happen every minute. + | Checks for new jobs every 5 seconds. + | Do not set more than 1 retry, as it may lead to sending repeated emails if one recipient fails + | and another succeeds. |------------------------------------------------------------------------- */ - 'queue_work_params' => ['--queue' => 'emails', '--sleep' => '5', '--tries' => '7'], + 'queue_work_params' => ['--queue' => 'emails', '--sleep' => '5', '--tries' => '1'], /* |-------------------------------------------------------------------------- diff --git a/public/js/main.js b/public/js/main.js index 627653a7..e2d3901c 100644 --- a/public/js/main.js +++ b/public/js/main.js @@ -732,8 +732,13 @@ function showModal(a, onshow) var modal_class = a.attr('data-modal-class'); // Fit modal body into the screen var fit = a.attr('data-modal-fit'); + var size = a.attr('data-modal-size'); // lg or sm var modal; + if (size) { + modal_class += ' modal-'+size; + } + var html = [ ' diff --git a/resources/views/secure/logs.blade.php b/resources/views/secure/logs.blade.php index 78bfa291..1e43a475 100644 --- a/resources/views/secure/logs.blade.php +++ b/resources/views/secure/logs.blade.php @@ -34,10 +34,12 @@ @foreach ($logs as $row) @foreach ($cols as $col) - + @if (isset($row[$col])) @if ($col == 'user' || $col == 'customer') {{ $row[$col]->getFullName(true) }} + @elseif ($col == 'date') + {{ App\User::dateFormat(new Illuminate\Support\Carbon($row[$col]), 'M j, H:i:s') }} @else {{ $row[$col] }} @endif