Comments

miro_dietiker created an issue. See original summary.

miro_dietiker’s picture

Priority: Normal » Major

Promoting to major since this could be a great value from inmail and removes complexity from custom solutions.

miro_dietiker’s picture

Sure, we need to discuss if this is debug only, if handlers could modify / enrich the response, or suppress / replace it at all.

mbovan’s picture

Yes, it seems it can be a setting in Inmail settings... Becuase of the mentioned reasons (bounce messages), I think handlers should be able to disable replies too.

There are log messages on the processor result object. Those messages are used to create event arguments. We could think to use them for reply reports.

miro_dietiker’s picture

Yeah a simple first version could to send all the log lines to the source.

Still not sure about the exact additional interface we should define.
The response could have some response object on it.
And the response could have a flag if it should include the log.

Again this is related to the standard result. #2754253: Lack of standard result in collaboration of analyzers
Or we could again introduce another partial interface.

toncic’s picture

Status: Active » Needs review
FileSize
1.98 KB

I trying to send message, but we want to send message from admin@example.com or from custom email address? The test are failing, just first step on this issue. After discussing with @miro_dietiker, we also want to add checkbox in deliverer settings to ask user if he want the whole log message in message body or not, and test of course.

Status: Needs review » Needs work

The last submitted patch, 6: send_processing_report-2757421-6.patch, failed testing.

miro_dietiker’s picture

  1. +++ b/inmail.module
    +++ b/inmail.module
    @@ -318,6 +318,34 @@ function inmail_mail($key, &$message, $params) {
    
    @@ -318,6 +318,34 @@ function inmail_mail($key, &$message, $params) {
           $message['raw_headers'] = $header;
    ...
    +  $result = $params['original'];
    

    Oh we already have some other mail case...

    So we need a switch statement for $key.

  2. +++ b/inmail.module
    @@ -318,6 +318,34 @@ function inmail_mail($key, &$message, $params) {
    +  $status = $result->getStatusLabel();
    +  $subject = strtoupper($status) . ': ' . $result->getValue() . ' ' . $sensor_config->getLabel();
    ...
    +  $message_id = '<' . $result->getSensorId() . '.' . time() . '@' . \Drupal::request()->getHost() . '>';
    

    Remove those lines related to $result or $sensor_* That's more related to a Monitoring result where we copied the code from.

  3. +++ b/inmail.module
    @@ -318,6 +318,34 @@ function inmail_mail($key, &$message, $params) {
    +  $message['subject'] = $subject;
    

    We could simply answer with the original subject + "Re: ".

  4. +++ b/inmail.module
    @@ -318,6 +318,34 @@ function inmail_mail($key, &$message, $params) {
    +  // Set the mail sender.
    +  $site_config = \Drupal::config('system.site');
    +  $site_mail = $site_config->get('mail');
    +  $site_name = $site_config->get('name');
    +  $message['from'] = 'INMAIL ' . $site_name . ' <' . $site_mail . '>';
    

    Simply remove. The standard from is OK.

  5. +++ b/inmail.module
    @@ -318,6 +318,34 @@ function inmail_mail($key, &$message, $params) {
    +  $message['headers']['message-id'] = $message_id;
    

    We should add a reference to the original message-id here.

  6. +++ b/src/MessageProcessor.php
    @@ -168,6 +168,14 @@ class MessageProcessor implements MessageProcessorInterface {
    +      $mail_manager->mail('inmail_success', 'Success', $recipient, \Drupal::languageManager()->getDefaultLanguage()->getId(), $params);
    

    It's $module and $key. So it's "success" and not "Success".

And as discussed, make sending this mail a configuration setting of the deliverer.

toncic’s picture

Status: Needs work » Needs review
FileSize
5.25 KB
6.25 KB

Added test coverage and try to add checkbox, but I am not sure where is the best to save that state.

Status: Needs review » Needs work

The last submitted patch, 9: send_processing_report-2757421-9.patch, failed testing.

toncic’s picture

Review by Miro:

  1. +++ b/inmail.module
    @@ -317,6 +317,15 @@ function inmail_mail($key, &$message, $params) {
    +      $message['from'] = $original->getFrom();
    +      $message['id'] = $original->getMessageId();
    
    +++ b/src/MessageProcessor.php
    @@ -168,6 +168,13 @@ class MessageProcessor implements MessageProcessorInterface {
    +      $recipient = $message->getFrom();
    
    +++ b/tests/src/Kernel/ModeratorForwardTest.php
    @@ -107,7 +107,16 @@ class ModeratorForwardTest extends KernelTestBase {
    +           ->getEditable('system.site')
    +           ->set('mail', 'mail@example.com')
    

    The from is the site.

  2. +++ b/src/Form/PluginConfigurationForm.php
    @@ -29,11 +29,19 @@ class PluginConfigurationForm extends EntityForm {
    +  protected $sendLog;
    ...
    +    $this->sendLog = FALSE;
    

    Initialise with default value, not constructor.

  3. +++ b/inmail.module
    @@ -317,6 +317,15 @@ function inmail_mail($key, &$message, $params) {
    +      $message['id'] = $original->getMessageId();
    

    This is messing with the principle of unique id by reusing it. Use the references field: https://tools.ietf.org/html/rfc2822#section-3.6.4

  4. +++ b/inmail.module
    @@ -317,6 +317,15 @@ function inmail_mail($key, &$message, $params) {
    +      $message['cc'] = $original->getCc();
    

    No.

  5. +++ b/inmail.module
    @@ -317,6 +317,15 @@ function inmail_mail($key, &$message, $params) {
    +      $message['body'] = 'Reply.';
    

    Consider the code that we previously wrote to fill the body with the result log. Start the message with something like:
    The message has been processed successfully.

  6. +++ b/src/Form/PluginConfigurationForm.php
    @@ -66,6 +74,12 @@ class PluginConfigurationForm extends EntityForm {
    +    $form['log'] = [
    +      '#title' => $this->t('Attache log'),
    

    The key is about mail, not log. Say 'mail'.
    Label: Mail processing report to sender.

  7. +++ b/src/Form/PluginConfigurationForm.php
    @@ -66,6 +74,12 @@ class PluginConfigurationForm extends EntityForm {
    +      '#default_value' => $this->sendLog,
    

    Use the same CamelCase member var name.

  8. +++ b/src/MessageProcessor.php
    @@ -168,6 +168,13 @@ class MessageProcessor implements MessageProcessorInterface {
    +      $mail_manager->mail('inmail_success', 'success', $recipient, \Drupal::languageManager()->getDefaultLanguage()->getId(), $params);
    
    +++ b/tests/src/Kernel/ModeratorForwardTest.php
    @@ -54,7 +54,7 @@ class ModeratorForwardTest extends KernelTestBase {
    -    $this->assertMailCount(0);
    +    $this->assertMailCount(1);
    

    The proper signature is:
    public function mail($module, $key, $to, $langcode, $params = array(), $reply = NULL, $send = TRUE) {
    inmail_success is not a module name...

    Also the setting is not considered.

    By default the setting should be disabled and you should check in tests that no mail is sent. Only after enabling, you should retrigger and check count.

  9. +++ b/tests/src/Kernel/ModeratorForwardTest.php
    @@ -107,7 +107,16 @@ class ModeratorForwardTest extends KernelTestBase {
    +    $this->assertMailString('body', 'Reply', 1);
    

    Assert all mail body and header by example.

toncic’s picture

Issue summary: View changes
Status: Needs work » Needs review

Added checkbox in deliverer settings and test coverage for that. Also sending mail and test for that.
Only local images are allowed.

toncic’s picture

Status: Needs review » Needs work

The last submitted patch, 13: send_processing_report-2757421-12.patch, failed testing.

miro_dietiker’s picture

  1. +++ b/src/MessageProcessor.php
    @@ -168,6 +168,17 @@ class MessageProcessor implements MessageProcessorInterface {
    +      if ($deliverer->isMessageReport()) {
    

    Check this first and only do the lines above inside the if().

  2. +++ b/tests/src/Kernel/ModeratorForwardTest.php
    @@ -94,6 +94,17 @@ class ModeratorForwardTest extends KernelTestBase {
    +    $mail = $this->getMails();
    

    mail => mails. I would recommend that you assign the mail in question:
    $mail = $mails[0]

  3. +++ b/tests/src/Kernel/ModeratorForwardTest.php
    @@ -94,6 +94,17 @@ class ModeratorForwardTest extends KernelTestBase {
    +    $this->assertEquals('Re: BMH testing sample', $mail[0]['subject']);
    

    That's an awesome example...
    Because it reminds me that we will for sure NOT send a mail to the sender in case the mail is classified as a bounce...
    Otherwise we will create mail loops.

  4. +++ b/inmail.module
    @@ -317,6 +317,23 @@ function inmail_mail($key, &$message, $params) {
    +      $body [] = 'The message has been processed successfully.';
    

    Let's add some more data such as:
    - Original message date
    Anything else to mention in the body?

  5. +++ b/inmail.module
    @@ -317,6 +317,23 @@ function inmail_mail($key, &$message, $params) {
    +      // We can use here Kerims function 'inmail_get_log_message'
    

    OK we can do this in a follow-up. But please remove the garbage comment lines. Just add a clean @todo and a link to the issue where the method is added.

  6. +++ b/src/Entity/DelivererConfig.php
    @@ -32,6 +32,7 @@ use Drupal\inmail\DelivererConfigInterface;
      *     "configuration",
    + *     "message_report" = "message_report",
    

    There's no "= ..."

toncic’s picture

Implemented stuff from #15.
Moved creating checkbox in DelivererConfigurationForm.
Created follow -up..
Do we want to create new follow up for skipping bounce mail?

toncic’s picture

Assigned: Unassigned » toncic
yongt9412’s picture

Status: Needs review » Needs work
  1. +++ b/src/MessageProcessor.php
    @@ -168,6 +168,19 @@ class MessageProcessor implements MessageProcessorInterface {
    +      //TODO: Skip bounce imail.s
    

    Typo (?)

  2. +++ b/src/MessageProcessor.php
    @@ -168,6 +168,19 @@ class MessageProcessor implements MessageProcessorInterface {
    +        // Check if message report
    

    . missing at the end.

  3. +++ b/src/MessageProcessor.php
    @@ -168,6 +168,19 @@ class MessageProcessor implements MessageProcessorInterface {
    +          $mail_manager->mail('inmail', 'success', $recipient, \Drupal::languageManager()->getDefaultLanguage()->getId(), $params);
    

    Wrong indentation? :)

  4. +++ b/tests/src/Kernel/ModeratorForwardTest.php
    @@ -94,6 +94,18 @@ class ModeratorForwardTest extends KernelTestBase {
    +    $this->assertEquals('The message has been processed successfully.', $last_mail['body']);
    +
     
    

    Extra white line. :)

toncic’s picture

Status: Needs work » Needs review
FileSize
5.47 KB
1.65 KB

Implemented comm #18.

miro_dietiker’s picture

Status: Needs review » Needs work
  1. +++ b/inmail.module
    @@ -319,6 +319,17 @@ function inmail_mail($key, &$message, $params) {
    +      $message['from'] = \Drupal::config('system.site')->get('mail');
    

    The site is the from by default. You don't need to state that. Also, it is improved over your code because it's the site name as display name.

  2. +++ b/inmail.module
    @@ -319,6 +319,17 @@ function inmail_mail($key, &$message, $params) {
    +      $message['headers']['message-id'] = $original->getMessageId();
    

    still unchanged. see my comments above.

  3. +++ b/src/MessageProcessor.php
    @@ -168,6 +168,18 @@ class MessageProcessor implements MessageProcessorInterface {
    +      if ($deliverer->isMessageReport()) {
    
    +++ b/tests/src/Kernel/ModeratorForwardTest.php
    @@ -94,6 +94,17 @@ class ModeratorForwardTest extends KernelTestBase {
    +    $this->assertMailCount(1);
    ...
    +    $this->assertEquals('bounces@example.com', $last_mail['from']);
    +    $this->assertEquals('Re: BMH testing sample', $last_mail['subject']);
    

    You still need to make sure that messages classified as bounce are not triggering a mail.

    Something like

    +      if ($deliverer->isMessageReport() && !$message->isBounce()) {
    

    And even if "send report" is enabled, you want to make sure in tests that when processing a bounce, no mail went out.

toncic’s picture

Implemented stuff from #20.

toncic’s picture

FileSize
5.76 KB

Wrong extension for interdiff.

Status: Needs review » Needs work

The last submitted patch, 21: interdiff-2757421-19-21.patch, failed testing.

toncic’s picture

Status: Needs work » Needs review
miro_dietiker’s picture

Status: Needs review » Needs work
+++ b/src/MessageProcessor.php
@@ -195,6 +197,37 @@ class MessageProcessor implements MessageProcessorInterface {
+    if ( $default_result->hasContext('bounce')) {
+      $bounce_data = $default_result->getContext('bounce')->getContextData();
+      if ($deliverer->isMessageReport() && !$bounce_data->isBounce()) {

This is not correct.

Even if the bounce context doesn't exist, such as when the BounceAnalyzer is disabled, the mail should be sent.
It should only be skipped if the context explicitly exists and confirms isBounce().

toncic’s picture

Status: Needs work » Needs review
FileSize
7.14 KB
1.33 KB

Changed the if logic.

  • miro_dietiker committed f232ee3 on 8.x-1.x authored by toncic
    Issue #2757421 by toncic, miro_dietiker: Send processing report as reply
    
miro_dietiker’s picture

Status: Needs review » Fixed
+++ b/tests/src/Kernel/ModeratorForwardTest.php
@@ -108,6 +109,31 @@ class ModeratorForwardTest extends KernelTestBase {
+    $this->assertEquals('bounces@example.com', $last_mail['from']);

Where is this from...This is misleading, but it's already defined in setUp(). :-)

Committed with lots of small fixes.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.