diff --git a/core/action.class.inc.php b/core/action.class.inc.php index a45529db2..e5ee3ad0b 100644 --- a/core/action.class.inc.php +++ b/core/action.class.inc.php @@ -200,6 +200,17 @@ abstract class ActionNotification extends Action */ class ActionEmail extends ActionNotification { + /** + * @var string + * @since 3.0.1 + */ + const ENUM_HEADER_NAME_MESSAGE_ID = 'Message-ID'; + /** + * @var string + * @since 3.0.1 + */ + const ENUM_HEADER_NAME_REFERENCES = 'References'; + /** * @inheritDoc */ @@ -207,13 +218,13 @@ class ActionEmail extends ActionNotification { $aParams = array ( - "category" => "grant_by_profile,core/cmdb,application", - "key_type" => "autoincrement", - "name_attcode" => "name", - "state_attcode" => "", - "reconc_keys" => array('name'), - "db_table" => "priv_action_email", - "db_key_field" => "id", + "category" => "grant_by_profile,core/cmdb,application", + "key_type" => "autoincrement", + "name_attcode" => "name", + "state_attcode" => "", + "reconc_keys" => array('name'), + "db_table" => "priv_action_email", + "db_key_field" => "id", "db_finalclass_field" => "", ); MetaModel::Init_Params($aParams); @@ -416,9 +427,8 @@ class ActionEmail extends ActionNotification $sBody = MetaModel::ApplyParams($this->Get('body'), $aContextArgs); $oObj = $aContextArgs['this->object()']; - $sMessageId = sprintf('iTop_%s_%d_%f@%s.openitop.org', get_class($oObj), $oObj->GetKey(), microtime(true /* get as float*/), - MetaModel::GetEnvironmentId()); - $sReference = '<'.$sMessageId.'>'; + $sMessageId = $this->GenerateIdentifierForHeaders($oObj, static::ENUM_HEADER_NAME_MESSAGE_ID); + $sReference = $this->GenerateIdentifierForHeaders($oObj, static::ENUM_HEADER_NAME_REFERENCES); } catch (Exception $e) { /** @noinspection PhpUnhandledExceptionInspection */ @@ -456,8 +466,7 @@ class ActionEmail extends ActionNotification $oEmail = new EMail(); - if ($this->IsBeingTested()) - { + if ($this->IsBeingTested()) { $oEmail->SetSubject('TEST['.$sSubject.']'); $sTestBody = $sBody; $sTestBody .= "
\n"; @@ -467,8 +476,8 @@ class ActionEmail extends ActionNotification $sTestBody .= "
  • TO: $sTo
    • \n"; $sTestBody .= "
  • CC: $sCC
    • \n"; $sTestBody .= "
  • BCC: $sBCC
    • \n"; - $sTestBody .= empty($sFromLabel) ? "
  • From: $sFrom
    • \n": "
  • From: $sFromLabel <$sFrom>
    • \n"; - $sTestBody .= empty($sReplyToLabel) ? "
  • Reply-To: $sReplyTo
    • \n": "
  • Reply-To: $sReplyToLabel <$sReplyTo>
    • \n"; + $sTestBody .= empty($sFromLabel) ? "
  • From: $sFrom
    • \n" : "
  • From: $sFromLabel <$sFrom>
    • \n"; + $sTestBody .= empty($sReplyToLabel) ? "
  • Reply-To: $sReplyTo
    • \n" : "
  • Reply-To: $sReplyToLabel <$sReplyTo>
    • \n"; $sTestBody .= "
  • References: $sReference
    • \n"; $sTestBody .= "\n"; $sTestBody .= "

    \n"; @@ -478,9 +487,9 @@ class ActionEmail extends ActionNotification $oEmail->SetRecipientFrom($sFrom, $sFromLabel); $oEmail->SetReferences($sReference); $oEmail->SetMessageId($sMessageId); - } - else - { + // Note: N°4849 We pass the "References" identifier instead of the "Message-ID" on purpose as we want notifications emails to group around the triggering iTop object, not just the users' replies to the notification + $oEmail->SetInReplyTo($sReference); + } else { $oEmail->SetSubject($sSubject); $oEmail->SetBody($sBody, 'text/html', $sStyles); $oEmail->SetRecipientTO($sTo); @@ -490,6 +499,8 @@ class ActionEmail extends ActionNotification $oEmail->SetRecipientReplyTo($sReplyTo, $sReplyToLabel); $oEmail->SetReferences($sReference); $oEmail->SetMessageId($sMessageId); + // Note: N°4849 We pass the "References" identifier instead of the "Message-ID" on purpose as we want notifications emails to group around the triggering iTop object, not just the users' replies to the notification + $oEmail->SetInReplyTo($sReference); } if (isset($aContextArgs['attachments'])) @@ -516,26 +527,64 @@ class ActionEmail extends ActionNotification { case EMAIL_SEND_OK: return "Sent"; - + case EMAIL_SEND_PENDING: return "Pending"; - + case EMAIL_SEND_ERROR: return "Errors: ".implode(', ', $aErrors); } } - } - else - { - if (is_array($this->m_aMailErrors) && count($this->m_aMailErrors) > 0) - { + } else { + if (is_array($this->m_aMailErrors) && count($this->m_aMailErrors) > 0) { $sError = implode(', ', $this->m_aMailErrors); - } - else - { + } else { $sError = 'Unknown reason'; } + return 'Notification was not sent: '.$sError; } } + + /** + * @param \DBObject $oObject + * @param string $sHeaderName {@see \ActionEmail::ENUM_HEADER_NAME_REFERENCES}, {@see \ActionEmail::ENUM_HEADER_NAME_MESSAGE_ID} + * + * @return string The formatted identifier for $sHeaderName based on $oObject + * @throws \Exception + * @since 3.0.1 N°4849 + */ + protected function GenerateIdentifierForHeaders(DBObject $oObject, string $sHeaderName): string + { + $sObjClass = get_class($oObject); + $sObjId = $oObject->GetKey(); + $sAppName = utils::Sanitize(ITOP_APPLICATION_SHORT, '', utils::ENUM_SANITIZATION_FILTER_VARIABLE_NAME); + + switch ($sHeaderName) { + case static::ENUM_HEADER_NAME_MESSAGE_ID: + case static::ENUM_HEADER_NAME_REFERENCES: + // Prefix + $sPrefix = sprintf('%s_%s_%d', $sAppName, $sObjClass, $sObjId); + if ($sHeaderName === static::ENUM_HEADER_NAME_MESSAGE_ID) { + $sPrefix .= sprintf('_%f', microtime(true /* get as float*/)); + } + // Suffix + $sSuffix = sprintf('@%s.openitop.org', MetaModel::GetEnvironmentId()); + // Identifier + $sIdentifier = $sPrefix.$sSuffix; + if ($sHeaderName === static::ENUM_HEADER_NAME_REFERENCES) { + $sIdentifier = "<$sIdentifier>"; + } + + return $sIdentifier; + } + + // Requested header name invalid + $sErrorMessage = sprintf('%s: Could not generate identifier for header "%s", only %s are supported', static::class, $sHeaderName, implode(' / ', [static::ENUM_HEADER_NAME_MESSAGE_ID, static::ENUM_HEADER_NAME_REFERENCES])); + IssueLog::Error($sErrorMessage, LogChannels::NOTIFICATIONS, [ + 'Object' => $sObjClass.'::'.$sObjId.' ('.$oObject->GetRawName().')', + 'Action' => get_class($this).'::'.$this->GetKey().' ('.$this->GetRawName().')', + ]); + throw new Exception($sErrorMessage); + } } diff --git a/core/email.class.inc.php b/core/email.class.inc.php index d265dc97e..1470befaf 100644 --- a/core/email.class.inc.php +++ b/core/email.class.inc.php @@ -325,20 +325,33 @@ class EMail // Note: Swift will add the angle brackets for you // so let's remove the angle brackets if present, for historical reasons $sId = str_replace(array('<', '>'), '', $sId); - + $oMsgId = $this->m_oMessage->getHeaders()->get('Message-ID'); $oMsgId->SetId($sId); } - + public function SetReferences($sReferences) { $this->AddToHeader('References', $sReferences); } + /** + * Set the "In-Reply-To" header to allow emails to group as a conversation in modern mail clients (GMail, Outlook 2016+, ...) + * + * @link https://en.wikipedia.org/wiki/Email#Header_fields + * + * @param string $sMessageId + * + * @since 3.0.1 N°4849 + */ + public function SetInReplyTo(string $sMessageId) + { + $this->AddToHeader('In-Reply-To', $sMessageId); + } + public function SetBody($sBody, $sMimeType = 'text/html', $sCustomStyles = null) { - if (($sMimeType === 'text/html') && ($sCustomStyles !== null)) - { + if (($sMimeType === 'text/html') && ($sCustomStyles !== null)) { $oDomDocument = CssInliner::fromHtml($sBody)->inlineCss($sCustomStyles)->getDomDocument(); HtmlPruner::fromDomDocument($oDomDocument)->removeElementsWithDisplayNone(); $sBody = CssToAttributeConverter::fromDomDocument($oDomDocument)->convertCssToVisualAttributes()->render(); // Adds html/body tags if not already present diff --git a/core/log.class.inc.php b/core/log.class.inc.php index 5d2d457ba..e52eaf3b7 100644 --- a/core/log.class.inc.php +++ b/core/log.class.inc.php @@ -544,7 +544,13 @@ class LogChannels { public const APC = 'apc'; - public const CLI = 'CLI'; + /** + * @var string + * @since 3.0.1 N°4849 + */ + public const NOTIFICATIONS = 'notifications'; + + public const CLI = 'CLI'; /** * @var string diff --git a/test/core/ActionEmailTest.php b/test/core/ActionEmailTest.php new file mode 100644 index 000000000..1149e3b56 --- /dev/null +++ b/test/core/ActionEmailTest.php @@ -0,0 +1,91 @@ + 'Test action', + 'status' => 'disabled', + 'from' => 'unit-test@openitop.org', + 'subject' => 'Test subject', + 'body' => 'Test body', + ]); + static::$oActionEmail->DBInsert(); + } + + /** + * @covers \ActionEmail::GenerateIdentifierForHeaders + * @dataProvider GenerateIdentifierForHeadersProvider + * @throws \Exception + */ + public function testGenerateIdentifierForHeaders(string $sHeaderName) + { + // Retrieve object + $oObject = MetaModel::GetObject('Organization', $this->getTestOrgId(), true, true); + $sObjClass = get_class($oObject); + $sObjId = $oObject->GetKey(); + + try { + $sTestedIdentifier = $this->InvokeNonPublicMethod('\ActionEmail', 'GenerateIdentifierForHeaders', static::$oActionEmail, [$oObject, $sHeaderName]); + } catch (Exception $oException) { + $sTestedIdentifier = null; + } + + $sAppName = utils::Sanitize(ITOP_APPLICATION_SHORT, '', utils::ENUM_SANITIZATION_FILTER_VARIABLE_NAME); + $sEnvironmentHash = MetaModel::GetEnvironmentId(); + + switch ($sHeaderName) { + case ActionEmail::ENUM_HEADER_NAME_MESSAGE_ID: + // Note: For this test we can't use the more readable sprintf test as the generated timestamp will never be the same as the one generated during the call of the tested method + // $sTimestamp = microtime(true /* get as float*/); + // $sExpectedIdentifier = sprintf('%s_%s_%d_%f@%s.openitop.org', $sAppName, $sObjClass, $sObjId, $sTimestamp, $sEnvironmentHash); + $this->assertEquals(1, preg_match('/'.$sAppName.'_'.$sObjClass.'_'.$sObjId.'_[\d]+\.[\d]+@'.$sEnvironmentHash.'.openitop.org/', $sTestedIdentifier), "Identifier doesn't match regexp for header $sHeaderName, got $sTestedIdentifier"); + break; + + case ActionEmail::ENUM_HEADER_NAME_REFERENCES: + $sExpectedIdentifier = '<'.sprintf('%s_%s_%d@%s.openitop.org', $sAppName, $sObjClass, $sObjId, $sEnvironmentHash).'>'; + $this->assertEquals($sExpectedIdentifier, $sTestedIdentifier); + break; + + default: + $sExpectedIdentifier = null; + $this->assertEquals($sExpectedIdentifier, $sTestedIdentifier); + break; + } + + } + + public function GenerateIdentifierForHeadersProvider() + { + return [ + 'Message-ID' => ['Message-ID'], + 'References' => ['References'], + 'IncorrectHeaderName' => ['IncorrectHeaderName'], + ]; + } +} \ No newline at end of file