[Kde-pim] Review Request 109914: [IMAP] Use itemsRetrievedIncremental() when fetching missing bodies

[Kevin Ottens]

On Thursday 11 April 2013 14:34:42 Andras Mantia wrote:
> I take the liberty to reply on the list only, as this is not related to the
> review itself imo.

Good point I should have done that instead of a comment.
 
> On Wednesday, April 10, 2013 09:43:24 PM Kevin Ottens wrote:
> > > On April 9, 2013, 6:24 a.m., Kevin Ottens wrote:
> > > > Any chance to get that logic unit tested? The original submitter for
> > > > this behavior didn't unit test and it shows now.>
> > > 
> > > Dan Vrátil wrote:
> > >     The code for retrieving missing bodies uses Akonadi::ItemFetchJob,
> > >     which means the test would have to start a fake Akonadi server. I
> > >     know there's an implementation in kdepimlibs, but I don't know how
> > >     to
> > >     reach it there from kdepim-runtime.>
> > > 
> > > Dan Vrátil wrote:
> > >     It also means that the RetrieveItemsTask test does not work at all
> > >     at
> > >     the moment.>
> > > 
> > > Andras Mantia wrote:
> > >     I didn't write a unit test as the whole tests with a fake server are
> > >     unreliable for me (and cause configuration issues according to
> > >     David). Neverthless we should fix that and have working and reliable
> > >     unit tests. The patch itself looks good for me.
> > 
> > You know that's no good excuse right?
> 
> Not a good one, but it is an excuse. I have a better one though if you want:
> the original request was not rejected because of missing unit test. :)

Right, we screwed up once let's repeat it's gonna work eventually. Interesting 
reasoning.

> That is, blocking this because of that makes no sense.

Sure, that wasn't my intent though (maybe it wasn't clear though).

> Blaming me for not writing unit test for the original feature does.
> That is why I said to commit this fix: without the fix, the original feature
> has bad side-effects for master users.

Side note: If that was the only one... I'm testing another patch locally. 
Which is of course not unit testable (see the trend?).

> We could either remove (disable) that or get this patch in. I find the patch
> in a better solution.

It is *only* if what follows next is the proper refactoring and tests that 
should I have been in place in the original feature commit. I hope it's the 
direction we're heading (that was in fact the point I was trying to make 
earlier even though in a bad way).
 
> > The resource is designed to not talk
> > to the akonadi server at all in test mode. I'm pretty sure it could be
> > unit
> > tested today. I'm really unimpressed at the current layering violation
> > introduced. No Akonadi jobs should be used in the resource tasks, this way
> > it's properly modular and testable. It's not the case anymore for
> > AddCollectionTask and RetrieveItemsTask.
> 
> Well, this is news for me and again, was not mentioned for the original
> patch.

Well it got in before I gave it a go and finished reviewing it... But I was 
not in the best place for such review at the time though, I probably would 
have screwed up and missed the layer violation anyway. :-)

> The missing body checker can be moved out from RetrieveItemsTask (in
> a separate class or code that is in the main imapresource class), although
> the best place is there imo. If you want that, we can discuss.

Yes I do. I'm not clear on the API it should have (that's to be discussed 
indeed) but it should somehow be part of ResourceStateInterface and then 
implemented in ResourceState and DummyResourceState.

Regards.
-- 
Kévin Ottens, http://ervin.ipsquad.net

KDAB - proud supporter of KDE, http://www.kdab.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20130411/1b4b215b/attachment.sig>
-------------- next part --------------
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/