'Re: [Kde-pim] Review Request 109276: Fixes in IMAP resource folder handling'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    Re: [Kde-pim] Review Request 109276: Fixes in IMAP resource folder handling
From:       "Volker Krause" <vkrause () kde ! org>
Date:       2013-05-06 18:20:27
Message-ID: 20130506182027.3488.38427 () vidsolbach ! de
[Download RAW message or body]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109276/#review32148
-----------------------------------------------------------

resources/imap/addcollectiontask.cpp
<http://git.reviewboard.kde.org/r/109276/#comment23938>

    This looks rather dangerous, what if there's already content in this folder?

- Volker Krause

On March 28, 2013, 9:45 p.m., Andras Mantia wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109276/
> -----------------------------------------------------------
> 
> (Updated March 28, 2013, 9:45 p.m.)
> 
> 
> Review request for KDEPIM and Kevin Ottens.
> 
> 
> Description
> -------
> 
> We discovered several issues with the IMAP resource folder handling. This patch \
> addresses them, although in some cases with a temporarly solution: 
> 1) Creation of toplevel folders created folders with weird remote ids. E.g when \
> creating folder "foo", it set the remoteid to "ifoo". This was because \
> AddCollectionTask sets the first char of the remoteId for new collections based on \
> the first character of the remoteId of the parent collection. For toplevel \
> collections this is "i" from "imap://"... This breaks everything created below this \
> folder as well. Now it is hardcoded to "/", although this will break for IMAP \
> servers using other separators. A nicer solution is needed (finding the right \
> separator is not trivial though). This bug seems to be present also in other tasks \
> as well, but I didn't change as I didn't tested those codepaths. 
> 2) Creating toplevel folders requires a resync of the resource for them to appear. \
> This is actually some bug in the ETM that I don't feel like debugging, so a \
> solution used also in other parts of the code is reused: request a resync of the \
> collection list. 
> 3) If a folder is created, but it fails (e.g lack of rights on the server), the \
> collection for the folder remains in the akonadi database without a remoteId. This \
> blocks creating of new folders with the same name. I consider that failing to \
> create a folder on the resource's server side is something the user must know and \
>                 the akonadi cache should reflect the status on the server, I did \
>                 the following:
> - remove the collection from the cache
> - propagate the error through the status message. This is less then optimal, but \
> afaik there is no other mechanism so far to catch the errors in applications \
> (KMail). Maybe the tracer can be used though? 
> 4) Given two folders starting with the same characters (e.g foo and foobar) and \
> deleting "foo" deleted both "foo" and "foobar" because of the wrong startsWith \
>                 check. Now the folder is deleted, if:
> - it matches the name exactly
> - the found folder is a subolder of the folder that will be deleted (startwith name \
> + separator) 
> 5) Deleting of folder was...broken completely. If we had INBOX, foo, bar and tried \
> to deleted e.g bar,  the code started to list the directories, but aborted on the \
> first time when onMailBoxesReceived was called, but the list of mailbox didn't \
> include what we wanted to delete. E.g if the list emitted INBOX, then we aborted \
> without waiting that the slot is called with the rest of the folders. Now we keep \
> track if we found the right folder and abort only if none of the listed folders is \
> what we were expecting. 
> 6) Dummyresourcestate unconditionally removed the first char from the mailbox name, \
> so for a collection "foo" it returned "oo" messing the tests up. Now it is slightly \
> more intelligent. 
> 7) adapt the test (now the order of deletion calls is different) and fix its \
> synchronization by not using QTest::qWait(), but an event loop to detect when the \
> task finished. This catches (with a deadlock) cases when the task is finished \
> without calling changeProcessed() or cancelTask(). 
> 
> Diffs
> -----
> 
> resources/imap/tests/testremovecollectiontask.cpp ba1780b 
> resources/imap/tests/dummyresourcestate.cpp 518bf8a 
> resources/imap/removecollectionrecursivetask.cpp 50de4d6 
> resources/imap/removecollectionrecursivetask.h 361cb0d 
> resources/imap/imapresource.cpp 8d2332e 
> resources/imap/imapresource.h 130a337 
> resources/imap/changecollectiontask.cpp c5ae674 
> resources/imap/addcollectiontask.cpp d44b98b 
> 
> Diff: http://git.reviewboard.kde.org/r/109276/diff/
> 
> 
> Testing
> -------
> 
> Testing on GMail and the KDAB imap server, running the above unit test (most of the \
> unit tests fail, but I was looking only at this one now). 
> 
> Thanks,
> 
> Andras Mantia
> 
> 

_______________________________________________
KDE PIM mailing list kde-pim@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[prev in list] [next in list] [prev in thread] [next in thread] 