XWiki Platform
  1. XWiki Platform
  2. XWIKI-12165

Mailing list and newsletter use case no more covered by the Mail API

    Details

    • Difficulty:
      Unknown
    • Documentation:
      N/A
    • Similar issues:

      Description

      With IterableMimeMessageFactory, there is two use case to be covered:

      1. multiple different mails are created from a template (ie: SecureTemplateMimeMessageFactory)
      2. the same message is sent to multiple recipients (ie: MessageMimeMessageFactory)

      In the first case, all message SHOULD and will have a different `Message-ID` headers, but in the second case, all message MUST have the exact same `Message-ID` headers, since this is the exact same message being sent. This the use case of the mailing list, where a user should be able to reply to message sent by the list, and reference that message for all other subscribers properly.

      Since XWIKI-12128, we use the `Message-ID` for tracking mail sending, and XWIKI-12140 cause message from MessageMimeMessageFactory to have different `Message-ID`. Both of them should be somehow reverted.

        Issue Links

          Activity

          Hide
          Denis Gervalle added a comment -

          In place of just using the `Message-ID` header for identification, we would use an SHA1 of the `Message-ID`and the mail recipients.
          Benefits:

          • the resulting tracking ID (the SHA1) will really identify the unicity of the mail in a thread in term of end-user expectation, including the case where an iterator produce twice the same message.
          • while not included in the message (no need for a custom header), the information will be implicitly present.
          Show
          Denis Gervalle added a comment - In place of just using the `Message-ID` header for identification, we would use an SHA1 of the `Message-ID`and the mail recipients. Benefits: the resulting tracking ID (the SHA1) will really identify the unicity of the mail in a thread in term of end-user expectation, including the case where an iterator produce twice the same message. while not included in the message (no need for a custom header), the information will be implicitly present.
          Hide
          Guillaume Delhumeau added a comment -

          Putting as "blocker" because a regression is always a blocker.

          Show
          Guillaume Delhumeau added a comment - Putting as "blocker" because a regression is always a blocker.
          Hide
          Vincent Massol added a comment -

          AFAIK Denis Gervalle won't have the time to work on it in the coming 1-2 days which is what we would need for the 7.1.1 timeframe.

          Also this issue's title is misleading. These use cases will still work. The only thing that will not work perfectly well is the mail thread organisation in your mail client and only if recipients reply to each other. In that case, the reply will not be listed in the same thread in the mail client.

          I agree it should be fixed in 7.1.x ideally but I think it could be in 7.1.2 if we need to release 7.1.1 quickly.

          Show
          Vincent Massol added a comment - AFAIK Denis Gervalle won't have the time to work on it in the coming 1-2 days which is what we would need for the 7.1.1 timeframe. Also this issue's title is misleading. These use cases will still work. The only thing that will not work perfectly well is the mail thread organisation in your mail client and only if recipients reply to each other. In that case, the reply will not be listed in the same thread in the mail client. I agree it should be fixed in 7.1.x ideally but I think it could be in 7.1.2 if we need to release 7.1.1 quickly.
          Hide
          Vincent Massol added a comment -

          Removing "fix version" since Denis doesn't have the time to work on it ATM.

          Show
          Vincent Massol added a comment - Removing "fix version" since Denis doesn't have the time to work on it ATM.
          Hide
          Denis Gervalle added a comment -

          I have just push the following on master for approval:

          • Added a MessageIdComputer to compute unique messageId based on actual mimemessage messagId and the message receipients
          • Changed MessageId used in the API to use that new messageId at all levels
          • Reverted the change on the MessageMimeMessageFactory to keep the same mimemessage messageId on all generated messages
          • Fix unit tests affected by those changes

          This does not affect the API at the binary level, since this constitue mainly internal changes.
          However, the meaning of MailStatus#getMessageId() has changed, as well as all messageId argument of the API, since now this messageId is no more equivalent to the MimeMessage#getMessageId() value, like it has been between 7.1rc1 and 7.4.

          I will push that back on 7.4.x stable branch as soon as I receive positive feedback about this implementation.

          Show
          Denis Gervalle added a comment - I have just push the following on master for approval: Added a MessageIdComputer to compute unique messageId based on actual mimemessage messagId and the message receipients Changed MessageId used in the API to use that new messageId at all levels Reverted the change on the MessageMimeMessageFactory to keep the same mimemessage messageId on all generated messages Fix unit tests affected by those changes This does not affect the API at the binary level, since this constitue mainly internal changes. However, the meaning of MailStatus#getMessageId() has changed, as well as all messageId argument of the API, since now this messageId is no more equivalent to the MimeMessage#getMessageId() value, like it has been between 7.1rc1 and 7.4. I will push that back on 7.4.x stable branch as soon as I receive positive feedback about this implementation.
          Hide
          Denis Gervalle added a comment -

          After discussion with Vincent, we devised a better proposal that benefit the fact that the API is still considered unstable so far, and so we can still change it. The new fix that replace the previous one is based on the introduction of publicly exposed ExtendedMimeMessage that provide a getUniqueMessageId() method.

          While at it, I have also introduce in this new class helpers that were previously internal to

          • manage the Message type
          • use custom Message-ID for message without the risk of having them replaced by a generated one

          The original API has been kept as unchanged as possible, except for:

          • MailContentStore which now save/load ExtendedMimeMessage only, which ensure it has a way to identify them uniquely
          • MailListener which now receive ExtendedMimeMessage, so they can create MailStatus that can identify the message uniquely
          • MailStatus constructor which is linked with the above

          While affecting the original API, this new fix looks better to me, I am waiting for a review before pushing it back to the 7.4.x stable branch.

          Show
          Denis Gervalle added a comment - After discussion with Vincent, we devised a better proposal that benefit the fact that the API is still considered unstable so far, and so we can still change it. The new fix that replace the previous one is based on the introduction of publicly exposed ExtendedMimeMessage that provide a getUniqueMessageId() method. While at it, I have also introduce in this new class helpers that were previously internal to manage the Message type use custom Message-ID for message without the risk of having them replaced by a generated one The original API has been kept as unchanged as possible, except for: MailContentStore which now save/load ExtendedMimeMessage only, which ensure it has a way to identify them uniquely MailListener which now receive ExtendedMimeMessage, so they can create MailStatus that can identify the message uniquely MailStatus constructor which is linked with the above While affecting the original API, this new fix looks better to me, I am waiting for a review before pushing it back to the 7.4.x stable branch.

            People

            • Assignee:
              Denis Gervalle
              Reporter:
              Denis Gervalle
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Date of First Response: