[qmf] IMAP: handle errors for Create/Rename/Delete online folder actions

Problem description: classes ImapCreate(Delete/Rename)FolderStrategy have internal
'_inProgress' counters to support multiple requests processing.
However, when some action is completed with error - instances of those classes are not notified at all.
Thus, counters are never become zero and actions are never completed on client side

Additional fixes: don't allow create/rename folder if new name contains IMAP delimiter. Even if IMAP RFC allows this, it will create multiple folders instead of 1,
and it contradicts with user expectations (and with IMAP plugin expectations as well)

Testing:
=======
Use IMAP Gmail account

Delete Folder:
--------------
- Have some folder 'A' and folder 'B' in Gmail structure
- From Gmail Web UI, delete folder 'A', don't sync with device (I used jolla-email)
- On device try to delete this folder 'A'
- Action is completed with error
- Delete folder 'B' on device
- Without the fix: action will not be completed (even if folder is really deleted)
- With the fix: action is completed successfully

Rename folder 1:
- Have some folder 'A' and folder 'B' in Gmail structure
- From Gmail Web UI, delete folder 'A', don't sync with device (I used jolla-email)
- On device try to rename this folder 'A'
- Action is completed with error
- Rename folder 'B' on device
- Without the fix: action will not be completed (even if folder is really renamed)
- With the fix: action is completed successfully

Rename folder 2:
- Have some folder 'A' in Gmail structure
- Try to rename it to 'A/B/C' (where / is IMAP delimiter)
- Without the fix: folder will be created incorrectly (and there will be some empty name folders on device)
- With the fix: action will be completed with error. Log will indicate that folder name contains IMAP delimiter
- The same scenario is applicable to 'Create' folder action

Change-Id: I126f60d38a068a7ffd6cd86b5293866c31bd7633
Reviewed-by: Christopher Adams <chris.adams@jollamobile.com>
Reviewed-by: Matthew Vogt <matthew.vogt@qinetic.com.au>

6 files changed, 126 insertions, 61 deletions

diff --git a/src/plugins/messageservices/imap/imapclient.cpp b/src/plugins/messageservices/imap/imapclient.cpp
index 87475603..0ff17f82 100644
--- a/src/plugins/messageservices/imap/imapclient.cpp
+++ b/src/plugins/messageservices/imap/imapclient.cpp
@@ -424,12 +424,12 @@ ImapClient::ImapClient(QObject* parent)
             this, SLOT(downloadSize(QString, int)) );
     connect(&_protocol, SIGNAL(urlAuthorized(QString)),
             this, SLOT(urlAuthorized(QString)) );
-    connect(&_protocol, SIGNAL(folderCreated(QString)),
-            this, SLOT(folderCreated(QString)));
-    connect(&_protocol, SIGNAL(folderDeleted(QMailFolder)),
-            this, SLOT(folderDeleted(QMailFolder)));
-    connect(&_protocol, SIGNAL(folderRenamed(QMailFolder, QString)),
-            this, SLOT(folderRenamed(QMailFolder, QString)));
+    connect(&_protocol, SIGNAL(folderCreated(QString, bool)),
+            this, SLOT(folderCreated(QString, bool)));
+    connect(&_protocol, SIGNAL(folderDeleted(QMailFolder, bool)),
+            this, SLOT(folderDeleted(QMailFolder, bool)));
+    connect(&_protocol, SIGNAL(folderRenamed(QMailFolder, QString, bool)),
+            this, SLOT(folderRenamed(QMailFolder, QString, bool)));
     connect(&_protocol, SIGNAL(updateStatus(QString)),
             this, SLOT(transportStatus(QString)) );
     connect(&_protocol, SIGNAL(connectionError(int,QString)),
@@ -964,20 +964,22 @@ void ImapClient::messageFetched(QMailMessage& mail, const QString &detachedFilen
 }
 
 
-void ImapClient::folderCreated(const QString &folder)
+void ImapClient::folderCreated(const QString &folder, bool success)
 {
-    mailboxListed(QString(), folder);
-    _strategyContext->folderCreated(folder);
+    if (success) {
+        mailboxListed(QString(), folder);
+    }
+    _strategyContext->folderCreated(folder, success);
 }
 
-void ImapClient::folderDeleted(const QMailFolder &folder)
+void ImapClient::folderDeleted(const QMailFolder &folder, bool success)
 {
-    _strategyContext->folderDeleted(folder);
+    _strategyContext->folderDeleted(folder, success);
 }
 
-void ImapClient::folderRenamed(const QMailFolder &folder, const QString &newPath)
+void ImapClient::folderRenamed(const QMailFolder &folder, const QString &newPath, bool success)
 {
-    _strategyContext->folderRenamed(folder, newPath);
+    _strategyContext->folderRenamed(folder, newPath, success);
 }
 
 static bool updateParts(QMailMessagePart &part, const QByteArray &bodyData)
diff --git a/src/plugins/messageservices/imap/imapclient.h b/src/plugins/messageservices/imap/imapclient.h
index b4dd4fdb..c3e5c1fd 100644
--- a/src/plugins/messageservices/imap/imapclient.h
+++ b/src/plugins/messageservices/imap/imapclient.h
@@ -128,9 +128,9 @@ public slots:
     void messageCreated(const QMailMessageId &, const QString &);
     void downloadSize(const QString &uid, int);
     void urlAuthorized(const QString &url);
-    void folderDeleted(const QMailFolder &folder);
-    void folderCreated(const QString &folder);
-    void folderRenamed(const QMailFolder &folder, const QString &newName);
+    void folderDeleted(const QMailFolder &folder, bool success);
+    void folderCreated(const QString &folder, bool success);
+    void folderRenamed(const QMailFolder &folder, const QString &newName, bool success);
 
 protected slots:
     void connectionInactive();
diff --git a/src/plugins/messageservices/imap/imapprotocol.cpp b/src/plugins/messageservices/imap/imapprotocol.cpp
index 6f2f47f7..573019a9 100644
--- a/src/plugins/messageservices/imap/imapprotocol.cpp
+++ b/src/plugins/messageservices/imap/imapprotocol.cpp
@@ -656,8 +656,9 @@ public:
     virtual QString transmit(ImapContext *c);
     virtual void leave(ImapContext *c);
     virtual void taggedResponse(ImapContext *c, const QString &line);
+    virtual QString error(const QString &line);
 signals:
-    void folderCreated(const QString &name);
+    void folderCreated(const QString &name, bool success);
 
 private:
     QString makePath(ImapContext *c, const QMailFolderId &parent, const QString &name);
@@ -685,6 +686,13 @@ QString CreateState::transmit(ImapContext *c)
         return QString();
     }
 
+    if (name.contains(c->protocol()->delimiter())) {
+        qWarning() << "Unsupported: folder name contains IMAP delimiter" << name << c->protocol()->delimiter();
+        emit folderCreated(makePath(c, parent, name), false);
+        c->operationCompleted(command(), OpFailed);
+        return QString();
+    }
+
     QString path = makePath(c, parent, name);
 
     QString cmd("CREATE " + ImapProtocol::quoteString(path));
@@ -699,12 +707,17 @@ void CreateState::leave(ImapContext *)
 
 void CreateState::taggedResponse(ImapContext *c, const QString &line)
 {
-    if (status() == OpOk) {
-        emit folderCreated(makePath(c, _mailboxes.first().first, _mailboxes.first().second));
-    }
+    emit folderCreated(makePath(c, _mailboxes.first().first, _mailboxes.first().second), status() == OpOk);
     ImapState::taggedResponse(c, line);
 }
 
+QString CreateState::error(const QString &line)
+{
+    qWarning() << "CreateState::error:" << line;
+    emit folderCreated(_mailboxes.first().second, false);
+    return ImapState::error(line);
+}
+
 QString CreateState::makePath(ImapContext *c, const QMailFolderId &parent, const QString &name)
 {
     QString path;
@@ -733,8 +746,9 @@ public:
     virtual QString transmit(ImapContext *c);
     virtual void leave(ImapContext *c);
     virtual void taggedResponse(ImapContext *c, const QString &line);
+    virtual QString error(const QString &line);
 signals:
-    void folderDeleted(const QMailFolder &name);
+    void folderDeleted(const QMailFolder &name, bool success);
 
 private:
     QList<QMailFolder> _mailboxList;
@@ -765,12 +779,17 @@ void DeleteState::leave(ImapContext *)
 
 void DeleteState::taggedResponse(ImapContext *c, const QString &line)
 {
-    if (status() == OpOk) {
-        emit folderDeleted(_mailboxList.first());
-    }
+    emit folderDeleted(_mailboxList.first(), status() == OpOk);
     ImapState::taggedResponse(c, line);
 }
 
+QString DeleteState::error(const QString &line)
+{
+    qWarning() << "DeleteState::error:" << line;
+    emit folderDeleted(_mailboxList.first(), false);
+    return ImapState::error(line);
+}
+
 class RenameState : public ImapState
 {
     Q_OBJECT
@@ -785,8 +804,9 @@ public:
     virtual QString transmit(ImapContext *c);
     virtual void leave(ImapContext *c);
     virtual void taggedResponse(ImapContext *c, const QString &line);
+    virtual QString error(const QString &line);
 signals:
-    void folderRenamed(const QMailFolder &folder, const QString &newPath);
+    void folderRenamed(const QMailFolder &folder, const QString &newPath, bool success);
 
 private:
     QString buildNewPath(ImapContext *c , const QMailFolder &folder, QString &newName);
@@ -813,6 +833,14 @@ QString RenameState::transmit(ImapContext *c)
 
     QString from = _mailboxNames.last().first.path();
     QString to =  buildNewPath(c, _mailboxNames.last().first, _mailboxNames.last().second);
+    if (_mailboxNames.last().second.contains(c->protocol()->delimiter())) {
+        qWarning() << "Unsupported: new name contains IMAP delimiter" << _mailboxNames.last().second
+                   << c->protocol()->delimiter();
+        emit folderRenamed(from, to, false);
+        c->operationCompleted(command(), OpFailed);
+        return QString();
+    }
+
     QString cmd(QString("RENAME %1 %2").arg(ImapProtocol::quoteString(from)).arg( ImapProtocol::quoteString(to)));
     return c->sendCommand(cmd);
 }
@@ -825,13 +853,18 @@ void RenameState::leave(ImapContext *)
 
 void RenameState::taggedResponse(ImapContext *c, const QString &line)
 {
-    if (status() == OpOk) {
-        QString path = buildNewPath(c, _mailboxNames.first().first, _mailboxNames.first().second);
-        emit folderRenamed(_mailboxNames.first().first, path);
-    }
+    QString path = buildNewPath(c, _mailboxNames.first().first, _mailboxNames.first().second);
+    emit folderRenamed(_mailboxNames.first().first, path, status() == OpOk);
     ImapState::taggedResponse(c, line);
 }
 
+QString RenameState::error(const QString &line)
+{
+    qWarning() << "RenameState::error:" << line;
+    emit folderRenamed(_mailboxNames.first().first, _mailboxNames.first().second, false);
+    return ImapState::error(line);
+}
+
 QString RenameState::buildNewPath(ImapContext *c , const QMailFolder &folder, QString &newName)
 {
     QString path;
@@ -2867,12 +2900,12 @@ ImapProtocol::ImapProtocol()
             this, SIGNAL(messageStored(QString)));
     connect(&_fsm->uidCopyState, SIGNAL(messageCopied(QString, QString)), 
             this, SIGNAL(messageCopied(QString, QString)));
-    connect(&_fsm->createState, SIGNAL(folderCreated(QString)),
-            this, SIGNAL(folderCreated(QString)));
-    connect(&_fsm->deleteState, SIGNAL(folderDeleted(QMailFolder)),
-            this, SIGNAL(folderDeleted(QMailFolder)));
-    connect(&_fsm->renameState, SIGNAL(folderRenamed(QMailFolder, QString)),
-            this, SIGNAL(folderRenamed(QMailFolder, QString)));
+    connect(&_fsm->createState, SIGNAL(folderCreated(QString, bool)),
+            this, SIGNAL(folderCreated(QString, bool)));
+    connect(&_fsm->deleteState, SIGNAL(folderDeleted(QMailFolder, bool)),
+            this, SIGNAL(folderDeleted(QMailFolder, bool)));
+    connect(&_fsm->renameState, SIGNAL(folderRenamed(QMailFolder, QString, bool)),
+            this, SIGNAL(folderRenamed(QMailFolder, QString, bool)));
 }
 
 ImapProtocol::~ImapProtocol()
diff --git a/src/plugins/messageservices/imap/imapprotocol.h b/src/plugins/messageservices/imap/imapprotocol.h
index fcb9d360..5cb46523 100644
--- a/src/plugins/messageservices/imap/imapprotocol.h
+++ b/src/plugins/messageservices/imap/imapprotocol.h
@@ -226,9 +226,9 @@ signals:
     void messageCreated(const QMailMessageId& id, const QString& uid);
     void urlAuthorized(const QString& url);
 
-    void folderCreated(const QString &folder);
-    void folderDeleted(const QMailFolder &name);
-    void folderRenamed(const QMailFolder &folder, const QString &newPath);
+    void folderCreated(const QString &folder, bool success);
+    void folderDeleted(const QMailFolder &name, bool success);
+    void folderRenamed(const QMailFolder &folder, const QString &newPath, bool success);
 
     void continuationRequired(ImapCommand, const QString &);
     void completed(ImapCommand, OperationStatus);
diff --git a/src/plugins/messageservices/imap/imapstrategy.cpp b/src/plugins/messageservices/imap/imapstrategy.cpp
index 7380818f..e0d4ec73 100644
--- a/src/plugins/messageservices/imap/imapstrategy.cpp
+++ b/src/plugins/messageservices/imap/imapstrategy.cpp
@@ -636,23 +636,26 @@ void ImapStrategy::urlAuthorized(ImapStrategyContextBase *context, const QString
     Q_UNUSED(url)
 }
 
-void ImapStrategy::folderCreated(ImapStrategyContextBase *context, const QString &folder)
+void ImapStrategy::folderCreated(ImapStrategyContextBase *context, const QString &folder, bool success)
 {
     Q_UNUSED(context)
     Q_UNUSED(folder)
+    Q_UNUSED(success)
 }
 
-void ImapStrategy::folderDeleted(ImapStrategyContextBase *context, const QMailFolder &folder)
+void ImapStrategy::folderDeleted(ImapStrategyContextBase *context, const QMailFolder &folder, bool success)
 {
     Q_UNUSED(context)
     Q_UNUSED(folder)
+    Q_UNUSED(success)
 }
 
-void ImapStrategy::folderRenamed(ImapStrategyContextBase *context, const QMailFolder &folder, const QString &newPath)
+void ImapStrategy::folderRenamed(ImapStrategyContextBase *context, const QMailFolder &folder, const QString &newPath, bool success)
 {
     Q_UNUSED(context)
     Q_UNUSED(folder)
     Q_UNUSED(newPath)
+    Q_UNUSED(success)
 }
 
 void ImapStrategy::selectFolder(ImapStrategyContextBase *context, const QMailFolder &folder)
@@ -699,14 +702,22 @@ void ImapCreateFolderStrategy::process(ImapStrategyContextBase *context)
 {
     while(_folders.count() > 0) {
         QPair<QMailFolderId, QString> folder = _folders.takeFirst();
-        context->protocol().sendCreate(folder.first, folder.second);
         _inProgress++;
+        context->protocol().sendCreate(folder.first, folder.second);
     }
 }
 
-void ImapCreateFolderStrategy::folderCreated(ImapStrategyContextBase *context, const QString &folder)
+void ImapCreateFolderStrategy::folderCreated(ImapStrategyContextBase *context, const QString &folder, bool success)
 {
-    if (--_inProgress == 0) {
+    if (_inProgress > 0) {
+        _inProgress--;
+    }
+    if (!success) {
+        _inProgress = 0; // in case of error, subsequent responses may not be received
+        qWarning() << "IMAP folder creation failed";
+        return; // don't call context->operationCompleted in case of error
+    }
+    if (_inProgress == 0) {
         if (_matchFoldersRequired) {
             QMailAccountId accountId = context->config().id();
             QMail::detectStandardFolders(accountId);
@@ -769,16 +780,24 @@ void ImapDeleteFolderStrategy::deleteFolder(const QMailFolderId &folderId, ImapS
     }
 
     //now the parent is safe to delete
-    context->protocol().sendDelete(QMailFolder(folderId));
     _inProgress++;
+    context->protocol().sendDelete(QMailFolder(folderId));
 }
 
-void ImapDeleteFolderStrategy::folderDeleted(ImapStrategyContextBase *context, const QMailFolder &folder)
+void ImapDeleteFolderStrategy::folderDeleted(ImapStrategyContextBase *context, const QMailFolder &folder, bool success)
 {
-    if(!QMailStore::instance()->removeFolder(folder.id()))
+    if (_inProgress > 0) {
+        _inProgress--;
+    }
+    if (!success) {
+        _inProgress = 0; // in case of error, subsequent responses may not be received
+        qWarning() << "IMAP folder deletion failed";
+        return; // don't call context->operationCompleted in case of error
+    }
+    if (!QMailStore::instance()->removeFolder(folder.id()))
         qWarning() << "Unable to remove folder id: " << folder.id();
 
-    if(--_inProgress == 0)
+    if (_inProgress == 0)
        context->operationCompleted();
 }
 
@@ -820,15 +839,23 @@ void ImapRenameFolderStrategy::process(ImapStrategyContextBase *context)
 {
     while(_folderNewNames.count() > 0) {
         const QPair<QMailFolderId, QString> &folderId_name =  _folderNewNames.takeFirst();
-        context->protocol().sendRename(QMailFolder(folderId_name.first), folderId_name.second);
         _inProgress++;
+        context->protocol().sendRename(QMailFolder(folderId_name.first), folderId_name.second);
     }
 }
 
-void ImapRenameFolderStrategy::folderRenamed(ImapStrategyContextBase *context, const QMailFolder &folder, const QString &newPath)
+void ImapRenameFolderStrategy::folderRenamed(ImapStrategyContextBase *context, const QMailFolder &folder,
+                                             const QString &newPath, bool success)
 {
     QString name;
-
+    if (_inProgress > 0) {
+        _inProgress--;
+    }
+    if (!success) {
+        _inProgress = 0; // in case of error, subsequent responses may not be received
+        qWarning() << "IMAP folder rename failed";
+        return; // don't call context->operationCompleted in case of error
+    }
     if(!context->protocol().delimiter().isNull()) {
         //only update if we're dealing with a hierarchical system
         QChar delimiter = context->protocol().delimiter();
@@ -859,7 +886,7 @@ void ImapRenameFolderStrategy::folderRenamed(ImapStrategyContextBase *context, c
 
     if(!QMailStore::instance()->updateFolder(&newFolder))
         qWarning() << "Unable to locally rename folder";
-    if(--_inProgress == 0)
+    if (_inProgress == 0)
         context->operationCompleted();
 }
 
diff --git a/src/plugins/messageservices/imap/imapstrategy.h b/src/plugins/messageservices/imap/imapstrategy.h
index a909010a..83a21efb 100644
--- a/src/plugins/messageservices/imap/imapstrategy.h
+++ b/src/plugins/messageservices/imap/imapstrategy.h
@@ -141,9 +141,9 @@ public:
     virtual void messageCreated(ImapStrategyContextBase *context, const QMailMessageId &id, const QString &uid);
     virtual void downloadSize(ImapStrategyContextBase *context, const QString &uid, int length);
     virtual void urlAuthorized(ImapStrategyContextBase *context, const QString &url);
-    virtual void folderCreated(ImapStrategyContextBase *context, const QString &folder);
-    virtual void folderDeleted(ImapStrategyContextBase *context, const QMailFolder &folder);
-    virtual void folderRenamed(ImapStrategyContextBase *context, const QMailFolder &folder, const QString &newName);
+    virtual void folderCreated(ImapStrategyContextBase *context, const QString &folder, bool success);
+    virtual void folderDeleted(ImapStrategyContextBase *context, const QMailFolder &folder, bool success);
+    virtual void folderRenamed(ImapStrategyContextBase *context, const QMailFolder &folder, const QString &newName, bool success);
     virtual void selectFolder(ImapStrategyContextBase *context, const QMailFolder &folder);
 
     void clearError() { _error = false; }
@@ -169,7 +169,7 @@ public:
 
     virtual void transition(ImapStrategyContextBase *, const ImapCommand, const OperationStatus);
     virtual void createFolder(const QMailFolderId &folder, const QString &name, bool matchFoldersRequired);
-    virtual void folderCreated(ImapStrategyContextBase *context, const QString &folder);
+    virtual void folderCreated(ImapStrategyContextBase *context, const QString &folder, bool success);
 protected:
     virtual void handleCreate(ImapStrategyContextBase *context);
     virtual void handleLogin(ImapStrategyContextBase *context);
@@ -191,7 +191,7 @@ public:
 
     virtual void transition(ImapStrategyContextBase *, const ImapCommand, const OperationStatus);
     virtual void deleteFolder(const QMailFolderId &folderId);
-    virtual void folderDeleted(ImapStrategyContextBase *context, const QMailFolder &folder);
+    virtual void folderDeleted(ImapStrategyContextBase *context, const QMailFolder &folder, bool success);
 protected:
     virtual void handleLogin(ImapStrategyContextBase *context);
     virtual void handleDelete(ImapStrategyContextBase *context);
@@ -209,7 +209,8 @@ public:
 
     virtual void transition(ImapStrategyContextBase *, const ImapCommand, const OperationStatus);
     virtual void renameFolder(const QMailFolderId &folderId, const QString &newName);
-    virtual void folderRenamed(ImapStrategyContextBase *context, const QMailFolder &folder, const QString &name);
+    virtual void folderRenamed(ImapStrategyContextBase *context, const QMailFolder &folder,
+                               const QString &name, bool success);
 protected:
     virtual void handleLogin(ImapStrategyContextBase *context);
     virtual void handleRename(ImapStrategyContextBase *context);
@@ -852,9 +853,11 @@ public:
     void messageCreated(const QMailMessageId &id, const QString &uid) { _strategy->messageCreated(this, id, uid); }
     void downloadSize(const QString &uid, int length) { _strategy->downloadSize(this, uid, length); }
     void urlAuthorized(const QString &url) { _strategy->urlAuthorized(this, url); }
-    void folderCreated(const QString &folder) { _strategy->folderCreated(this, folder); }
-    void folderDeleted(const QMailFolder &folder) { _strategy->folderDeleted(this, folder); }
-    void folderRenamed(const QMailFolder &folder, const QString &name) { _strategy->folderRenamed(this, folder, name); }
+    void folderCreated(const QString &folder, bool success) { _strategy->folderCreated(this, folder, success); }
+    void folderDeleted(const QMailFolder &folder, bool success) { _strategy->folderDeleted(this, folder, success); }
+    void folderRenamed(const QMailFolder &folder, const QString &name, bool success) {
+        _strategy->folderRenamed(this, folder, name, success);
+    }
     QString baseFolder() { return _strategy->baseFolder(); }
 
     ImapStrategy *strategy() const { return _strategy; }