Skip to content

Queued emails: Handle non-submission saving webforms and cleanup #420

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.

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

Merged
merged 5 commits into from
May 20, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ Nedenfor ses dato for release og beskrivelse af opgaver som er implementeret.

## [4.3.0] XXXX-XX-XX

* OS2Forms kø emails
* Håndterede formularer der ikke gemmer indsendelser.
* Opdaterede fejlbeskeder.
* Opdaterede til nyeste version af kø-systemet,
[advancedqueue](https://www.drupal.org/project/advancedqueue).

## [4.2.1] 2025-05-06

* Sikrede at OS2Forms attachment elementer bliver detekteret korrekt
Expand Down
19 changes: 11 additions & 8 deletions composer.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
final class QueuedEmail extends JobTypeBase implements ContainerFactoryPluginInterface {

public const OS2FORMS_QUEUED_EMAIL_LOGGER_CHANNEL = 'os2forms_queued_email_info';
public const OS2FORMS_QUEUED_EMAIL_IS_STATIC_FILE = 'OS2FORMS_QUEUED_EMAIL_IS_STATIC_FILE';
public const OS2FORMS_QUEUED_EMAIL_FILE_PATH = 'private://queued-email-files';
public const OS2FORMS_QUEUED_EMAIL_CONFIG_NAME = 'os2forms_queued_email_file_path';
Expand All @@ -36,6 +37,13 @@ final class QueuedEmail extends JobTypeBase implements ContainerFactoryPluginInt
*/
protected LoggerChannelInterface $submissionLogger;

/**
* The submission logger.
*
* @var \Drupal\Core\Logger\LoggerChannelInterface
*/
protected LoggerChannelInterface $queuedEmailLogger;

/**
* {@inheritdoc}
*
Expand All @@ -51,6 +59,7 @@ public function __construct(
) {
parent::__construct($configuration, $plugin_id, $plugin_definition);
$this->submissionLogger = $loggerFactory->get('webform_submission');
$this->queuedEmailLogger = $loggerFactory->get(self::OS2FORMS_QUEUED_EMAIL_LOGGER_CHANNEL);
}

/**
Expand All @@ -77,9 +86,6 @@ public function process(Job $job): JobResult {
$payload = $job->getPayload();
$message = json_decode($payload['message'], TRUE);

// Load the Webform submission entity by ID.
$submission = WebformSubmission::load($payload['submissionId']);

// Gather filenames for os2forms attachments for deletion later.
$os2formsAttachmentFilenames = [];

Expand All @@ -88,10 +94,15 @@ public function process(Job $job): JobResult {
// Handle OS2Forms attachments.
if (isset($attachment[self::OS2FORMS_QUEUED_EMAIL_CONFIG_NAME])) {
$os2formsAttachmentFilenames[] = $attachment[self::OS2FORMS_QUEUED_EMAIL_CONFIG_NAME];

if (!file_exists($attachment[self::OS2FORMS_QUEUED_EMAIL_CONFIG_NAME])) {
throw new \Exception('OS2Forms attachment file not found: ' . $attachment[self::OS2FORMS_QUEUED_EMAIL_CONFIG_NAME]);
}

$attachment[self::FILECONTENT] = file_get_contents($attachment[self::OS2FORMS_QUEUED_EMAIL_CONFIG_NAME]);

if (FALSE === $attachment[self::FILECONTENT]) {
throw new \Exception('OS2Forms attachment file not found: ' . $attachment[self::OS2FORMS_QUEUED_EMAIL_CONFIG_NAME]);
throw new \Exception('OS2Forms attachment file cannot be read: ' . $attachment[self::OS2FORMS_QUEUED_EMAIL_CONFIG_NAME]);
}

unset($attachment[self::OS2FORMS_QUEUED_EMAIL_CONFIG_NAME]);
Expand All @@ -110,42 +121,68 @@ public function process(Job $job): JobResult {

}

$this->mailManager->createInstance('SMTPMailSystem')->mail($message);
$result = $this->mailManager->createInstance('SMTPMailSystem')->mail($message);

$logger_context = [
'handler_id' => 'os2forms_queued_email',
'channel' => 'webform_submission',
'webform_submission' => $submission,
'operation' => 'email sent',
];
// Logging of failed mail is handled in catch.
if (!$result) {
throw new \Exception('Failed sending email');
}

$this->submissionLogger->notice($this->t('The submission #@serial was successfully delivered', ['@serial' => $submission->serial()]), $logger_context);
try {
// Load the Webform submission entity by ID.
// Beware that some webforms may be configured to NOT save submissions,
// submission may therefore be null.
$submission = WebformSubmission::load($payload['submissionId']);

if ($submission) {
$logger_context = [
'handler_id' => 'os2forms_queued_email',
'channel' => 'webform_submission',
'webform_submission' => $submission,
'operation' => 'email sent',
];

$this->submissionLogger->notice($this->t('The submission #@serial was successfully delivered', ['@serial' => $submission->serial()]), $logger_context);
}

}
catch (\Exception $e) {
$this->queuedEmailLogger->notice(sprintf('Failed logging to webform_submission logger: %s', $e->getMessage()));
}

// Remove OS2Forms attachments.
foreach ($os2formsAttachmentFilenames as $os2formsAttachmentFilename) {
unlink($os2formsAttachmentFilename);
}

$msg = sprintf('Email, %s, sent to %s. Webform id: %s.', $message['subject'], $message['to'], $submission->getWebform()->id());
$msg = sprintf('Email, %s, sent to %s. Webform id: %s.', $payload['subject'] ?? NULL, $payload['to'] ?? NULL, $payload['webformId'] ?? NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be worth moving this code before the file deletion to make sure that you only delete files if the job is succesful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With our current knowledge we assume that it is exactly these two lines that cause the sporadic issue, i.e. the 'stuckness' in the processing state and subsequent re-processings. Moving this above deletion would result in subsequent job processes also sending emails.

Currently we have:

  • Sporadic false-positive failed jobs, i.e. the mail is sent.

If we move the code we would have:

  • Sporadic multiple emails sent

I'm not sure which is better.

$this->auditLogger->info('Email', $msg);

return JobResult::success();
}
catch (\Exception $e) {

$submission = $payload['submissionId'] ? WebformSubmission::load($payload['submissionId']) : NULL;

$logger_context = [
'handler_id' => 'os2forms_queued_email',
'channel' => 'webform_submission',
'webform_submission' => $submission,
'operation' => 'email failed',
];
try {
$submission = WebformSubmission::load($payload['submissionId']);

if ($submission) {
$logger_context = [
'handler_id' => 'os2forms_queued_email',
'channel' => 'webform_submission',
'webform_submission' => $submission,
'operation' => 'email failed',
];

$this->submissionLogger->error($this->t('The submission #@serial failed (@message)', [
'@serial' => $submission->serial(),
'@message' => $e->getMessage(),
]), $logger_context);
}

$this->submissionLogger->error($this->t('The submission #@serial failed (@message)', [
'@serial' => $submission?->serial(),
'@message' => $e->getMessage(),
]), $logger_context);
}
catch (\Exception $e) {
$this->queuedEmailLogger->notice(sprintf('Failed logging to webform_submission logger: %s', $e->getMessage()));
}

return JobResult::failure($e->getMessage());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@ public function mail(array $message) {
'key' => $message['key'],
'to' => $message['to'],
'subject' => $message['subject'],
// Note that submissions may not exist later when job is processed.
'submissionId' => $submission->id(),
'webformId' => $submission->getWebform()->id(),
'message' => json_encode($message),
]);

Expand Down
Loading