GODT-966 Append internal message to AllMail should be no action

GODT-966 Fixed CI issues (WIP)

WIP GODT-966 modified behavior of APPEND.

WIP GODT-966 implemented test for possible order of operations.

WIP GODT-966: code cleanup and refactoring.

WIP GODT-966 fix for linter.

Other: Minor refactoring.

internal/imap/mailbox_append.go

@@ -116,11 +116,10 @@ func (im *imapMailbox) createMessage(imapFlags []string, date time.Time, r imap.
 	if internalID != "" {
 		if msg, err := im.storeMailbox.GetMessage(internalID); err == nil {
 			if im.user.user.IsCombinedAddressMode() || im.storeAddress.AddressID() == msg.Message().AddressID {
-				return im.labelExistingMessage(msg.ID(), msg.IsMarkedDeleted())
+				return im.labelExistingMessage(msg)
 			}
 		}
 	}
-
 	return im.importMessage(kr, hdr, body, imapFlags, date)
 }
 
@@ -146,7 +145,7 @@ func (im *imapMailbox) createDraftMessage(kr *crypto.KeyRing, email string, body
 	return uidplus.AppendResponse(im.storeMailbox.UIDValidity(), im.storeMailbox.GetUIDList([]string{draft.ID}))
 }
 
-func (im *imapMailbox) labelExistingMessage(messageID string, isDeleted bool) error {
+func (im *imapMailbox) labelExistingMessage(msg storeMessageProvider) error { //nolint[funlen]
 	im.log.Info("Labelling existing message")
 
 	// IMAP clients can move message to local folder (setting \Deleted flag)
@@ -156,17 +155,37 @@ func (im *imapMailbox) labelExistingMessage(messageID string, isDeleted bool) er
 	// not delete the message (EXPUNGE would delete the original message and
 	// the new duplicate one would stay). API detects duplicates; therefore
 	// we need to remove \Deleted flag if IMAP client re-imports.
-	if isDeleted {
+	if msg.IsMarkedDeleted() {
-		if err := im.storeMailbox.MarkMessagesUndeleted([]string{messageID}); err != nil {
+		if err := im.storeMailbox.MarkMessagesUndeleted([]string{msg.ID()}); err != nil {
 			log.WithError(err).Error("Failed to undelete re-imported message")
 		}
 	}
 
-	if err := im.storeMailbox.LabelMessages([]string{messageID}); err != nil {
+	storeMBox := im.storeMailbox
+
+	// Outlook Uses APPEND instead of COPY. There is no need to copy to All Mail because messages are already there.
+	// if the message is copied from Spam or Trash, it should stay as is.  Return error.
+	// Otherwise the message is already in All mail, Return OK.
+	if pmapi.AllMailLabel == storeMBox.LabelID() {
+		foundArchive := false
+		for _, mBox := range im.storeAddress.ListMailboxes() {
+			if mBox.LabelID() == pmapi.ArchiveLabel {
+				foundArchive = true
+				storeMBox = mBox
+				break
+			}
+		}
+
+		if !foundArchive {
+			return errors.New("could not find Archive folder")
+		}
+	}
+
+	if err := storeMBox.LabelMessages([]string{msg.ID()}); err != nil {
 		return err
 	}
 
-	return uidplus.AppendResponse(im.storeMailbox.UIDValidity(), im.storeMailbox.GetUIDList([]string{messageID}))
+	return uidplus.AppendResponse(storeMBox.UIDValidity(), storeMBox.GetUIDList([]string{msg.ID()}))
 }
 
 func (im *imapMailbox) importMessage(kr *crypto.KeyRing, hdr textproto.Header, body []byte, imapFlags []string, date time.Time) error {

test/features/bridge/imap/message/move_without_support.feature

@@ -12,7 +12,7 @@ Feature: IMAP move messages by append and delete (without MOVE support, e.g., Ou
       | 2  | jane.doe@mail.com | name@pm.me | bar     | world |
     And there is IMAP client "source" selected in "INBOX"
     And there is IMAP client "target" selected in "<mailbox>"
-    When IMAP clients "source" and "target" move message seq "2" of "user" from "INBOX" to "<mailbox>" by append and delete
+    When IMAP clients "source" and "target" move message seq "2" of "user" from "INBOX" to "<mailbox>"
     Then IMAP response to "source" is "OK"
     Then IMAP response to "target" is "OK"
     When IMAP client "source" sends expunge
@@ -38,7 +38,7 @@ Feature: IMAP move messages by append and delete (without MOVE support, e.g., Ou
       | 2  | jane.doe@mail.com | name@pm.me | bar     | world |
     And there is IMAP client "source" selected in "<mailbox>"
     And there is IMAP client "target" selected in "INBOX"
-    When IMAP clients "source" and "target" move message seq "2" of "user" from "<mailbox>" to "INBOX" by append and delete
+    When IMAP clients "source" and "target" move message seq "2" of "user" from "<mailbox>" to "INBOX"
     Then IMAP response to "source" is "OK"
     Then IMAP response to "target" is "OK"
     When IMAP client "source" sends expunge
@@ -54,3 +54,35 @@ Feature: IMAP move messages by append and delete (without MOVE support, e.g., Ou
       | mailbox |
       | Spam    |
       | Trash   |
+
+  Scenario Outline: Move message from <mailbox> to All Mail by <order>
+    Given there are messages in mailbox "<mailbox>" for "user"
+      | id | from              | to         | subject | body  |
+      | 1  | john.doe@mail.com | user@pm.me | subj1   | body1 |
+      | 2  | john.doe@mail.com | name@pm.me | subj2   | body2 |
+    And there is IMAP client "source" selected in "<mailbox>"
+    And there is IMAP client "target" selected in "All Mail"
+    When IMAP clients "source" and "target" move message seq "2" of "user" to "All Mail" by <order>
+    Then IMAP response to "source" is "OK"
+    Then IMAP response to "target" is "OK"
+    And mailbox "<mailbox>" for "user" has messages
+      | from              | to         | subject |
+      | john.doe@mail.com | user@pm.me | subj1   |
+    And mailbox "All Mail" for "user" has messages
+       | from              | to         | subject |
+       | john.doe@mail.com | user@pm.me | subj1   |
+       | john.doe@mail.com | name@pm.me | subj2   |
+    Examples:
+      | mailbox | order                 |
+      | INBOX   | APPEND DELETE EXPUNGE |
+      | Archive | APPEND DELETE EXPUNGE |
+      | Trash   | APPEND DELETE EXPUNGE |
+      | Spam    | APPEND DELETE EXPUNGE |
+      | INBOX   | DELETE APPEND EXPUNGE |
+      | Archive | DELETE APPEND EXPUNGE |
+      | Trash   | DELETE APPEND EXPUNGE |
+      | Spam    | DELETE APPEND EXPUNGE |
+      | INBOX   | DELETE EXPUNGE APPEND |
+      | Archive | DELETE EXPUNGE APPEND |
+      | Trash   | DELETE EXPUNGE APPEND |
+      | Spam    | DELETE EXPUNGE APPEND |

test/imap_actions_messages_test.go

@@ -18,10 +18,14 @@
 package tests
 
 import (
+	"errors"
 	"fmt"
 	"strconv"
+	"strings"
 	"sync"
+	"time"
 
+	"github.com/ProtonMail/proton-bridge/test/mocks"
 	"github.com/cucumber/godog"
 	"golang.org/x/net/html/charset"
 )
@@ -35,7 +39,8 @@ func IMAPActionsMessagesFeatureContext(s *godog.ScenarioContext) {
 	s.Step(`^IMAP client searches for "([^"]*)"$`, imapClientSearchesFor)
 	s.Step(`^IMAP client copies message seq "([^"]*)" to "([^"]*)"$`, imapClientCopiesMessagesTo)
 	s.Step(`^IMAP client moves message seq "([^"]*)" to "([^"]*)"$`, imapClientMovesMessagesTo)
-	s.Step(`^IMAP clients "([^"]*)" and "([^"]*)" move message seq "([^"]*)" of "([^"]*)" from "([^"]*)" to "([^"]*)" by append and delete$`, imapClientsMoveMessageSeqOfUserFromToByAppendAndDelete)
+	s.Step(`^IMAP clients "([^"]*)" and "([^"]*)" move message seq "([^"]*)" of "([^"]*)" from "([^"]*)" to "([^"]*)"$`, imapClientsMoveMessageSeqOfUserFromTo)
+	s.Step(`^IMAP clients "([^"]*)" and "([^"]*)" move message seq "([^"]*)" of "([^"]*)" to "([^"]*)" by ([^"]*) ([^"]*) ([^"]*)`, imapClientsMoveMessageSeqOfUserFromToByOrederedOperations)
 	s.Step(`^IMAP client imports message to "([^"]*)"$`, imapClientCreatesMessage)
 	s.Step(`^IMAP client imports message to "([^"]*)" with encoding "([^"]*)"$`, imapClientCreatesMessageWithEncoding)
 	s.Step(`^IMAP client creates message "([^"]*)" from "([^"]*)" to "([^"]*)" with body "([^"]*)" in "([^"]*)"$`, imapClientCreatesMessageFromToWithBody)
@@ -113,7 +118,7 @@ func imapClientMovesMessagesTo(messageSeq, newMailboxName string) error {
 	return nil
 }
 
-func imapClientsMoveMessageSeqOfUserFromToByAppendAndDelete(sourceIMAPClient, targetIMAPClient, messageSeq, bddUserID, sourceMailboxName, targetMailboxName string) error {
+func imapClientsMoveMessageSeqOfUserFromTo(sourceIMAPClient, targetIMAPClient, messageSeq, bddUserID, sourceMailboxName, targetMailboxName string) error {
 	account := ctx.GetTestAccount(bddUserID)
 	if account == nil {
 		return godog.ErrPending
@@ -160,6 +165,45 @@ func imapClientsMoveMessageSeqOfUserFromToByAppendAndDelete(sourceIMAPClient, ta
 	return nil
 }
 
+func extractMessageBodyFromImapResponse(response *mocks.IMAPResponse) (string, error) {
+	sections := response.Sections()
+	if len(sections) != 1 {
+		return "", internalError(errors.New("unexpected result from FETCH"), "retrieving message body using FETCH")
+	}
+	sections = strings.Split(sections[0], "\n")
+	if len(sections) < 2 {
+		return "", internalError(errors.New("failed to parse FETCH result"), "extraction body from FETCH reply")
+	}
+	return strings.Join(sections[1:], "\n"), nil
+}
+
+func imapClientsMoveMessageSeqOfUserFromToByOrederedOperations(sourceIMAPClient, targetIMAPClient, messageSeq, bddUserID, targetMailboxName, op1, op2, op3 string) error {
+	account := ctx.GetTestAccount(bddUserID)
+	if account == nil {
+		return godog.ErrPending
+	}
+	msgStr, err := extractMessageBodyFromImapResponse(ctx.GetIMAPClient(sourceIMAPClient).Fetch(messageSeq, "BODY.PEEK[]").AssertOK())
+	if err != nil {
+		return err
+	}
+
+	for _, op := range []string{op1, op2, op3} {
+		switch op {
+		case "APPEND":
+			res := ctx.GetIMAPClient(targetIMAPClient).Append(targetMailboxName, msgStr)
+			ctx.SetIMAPLastResponse(targetIMAPClient, res)
+		case "DELETE":
+			_ = imapClientNamedMarksMessageSeqAsDeleted(sourceIMAPClient, messageSeq)
+		case "EXPUNGE":
+			_ = imapClientNamedExpunge(sourceIMAPClient)
+		default:
+			return errors.New("unknow IMAP operation " + op)
+		}
+		time.Sleep(100 * time.Millisecond)
+	}
+	return nil
+}
+
 func imapClientCreatesMessage(mailboxName string, message *godog.DocString) error {
 	return imapClientCreatesMessageWithEncoding(mailboxName, "utf8", message)
 }

test/mocks/imap_response.go

@@ -107,6 +107,11 @@ func (ir *IMAPResponse) AssertOK() *IMAPResponse {
 	return ir
 }
 
+func (ir *IMAPResponse) Sections() []string {
+	ir.wait()
+	return ir.sections
+}
+
 func (ir *IMAPResponse) AssertResult(wantResult string) *IMAPResponse {
 	ir.wait()
 	a.NoError(ir.t, ir.err)