Review Request 128478: Only import OFX requested date range

[Brendan Coupe]

​I think this ​was based on a bug report I filed. While mine might have
been the only report, clearly the person that had already modified the code
and provided the patch had the same problem.

This patch is critical to me and affects anyone with a Citibank credit
card. In my case they took over credit card services for Costco from
American Express and they not only ignore the requested start date but also
send all of the transactions for the past couple of years from the American
Express card. It made OFX useless for me on a credit card that I like to
monitor every day.

I have been compiling the 4.8 branch with this patch for about 4 months and
have not had any problems. I hope the patch will be added to both the 4.8
branch and the main branch soon.



*----Brendan*

On Wed, Oct 26, 2016 at 7:36 AM, Jack Ostroff <
ostroffjh at users.sourceforge.net> wrote:

> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128478/
>
> On July 20th, 2016, 5:55 a.m. UTC, *Thomas Baumgart* wrote:
>
> Your patch in general seems OK if you only have OFX accounts. It will interfere though with other online transaction downloads e.g. HBCI.
>
> Since the problem and the solution are very OFX specific, I suggest to move the logic into the OFX plugin. I would move the calculation of the *startDate* into *bool OfxImporterPlugin::import(const QString& filename)* and the filter logic at the end of *int OfxImporterPlugin::ofxTransactionCallback(struct OfxTransactionData data, void * pv)* where unhandled transaction types are already eliminated.
>
> On July 20th, 2016, 3:43 p.m. UTC, *Jack Ostroff* wrote:
>
> Thomas:  do any of the other import types use the concept of a requested start date for transactions?  (I know csv does not, and have very limited experience with anything except OFX.)  Also, I think this only applies to OFX direct connect, not to OFX file import, in case that affects the implementation.
>
> On July 20th, 2016, 4:56 p.m. UTC, *Thomas Baumgart* wrote:
>
> In fact, KBanking supports this concept from day one and does so pretty well. The German bank servers obey the options in the request (I have not heard otherwise) whereas the Citibank server seems to be ignoring them completely. Your hint about import vs. online download is valid. One could catch this if the calculation for *startDate* is moved to *bool OfxImporterPlugin::updateAccount(const MyMoneyAccount& acc, bool moreAccounts)* which is only called for online updates AFAICS and leave it at 1.1.1900 for all other paths.
> The KBanking logic btw. can be found in *bool KBankingPlugin::updateAccount(const MyMoneyAccount& acc, bool moreAccounts)*.
>
> On July 20th, 2016, 5:18 p.m. UTC, *Jack Ostroff* wrote:
>
> Just because there is only one current example of a server not honoring the start date, and it happens to be OFX, would it hurt to have ALL online imports drop transactions prior to the requested start date?
>
> On July 20th, 2016, 6:14 p.m. UTC, *Thomas Baumgart* wrote:
>
> The KBanking plugin e.g. goes back two or three more days than the last download date. This is because sometimes transactions get delivered on a later date. Having a general filter would eliminate these transactions as well.
>
>   // get last statement request date from application account object
>   // and start from a few days before if the date is valid
>   QDate lastUpdate = QDate::fromString(acc.value("lastImportedTransactionDate"), Qt::ISODate);
>   if (lastUpdate.isValid())
>     lastUpdate = lastUpdate.addDays(-3);
>
> In general, I think, each solution to a specific problem should stay as
> close to the source of the problem. So this OFX specific thing should be
> handled in the OFX specific part. Plus you mention that it only applies to
> OFX download not the file import. There we have the first exception to the
> rule. The MyMoneyStatementReader does not differentiate between file import
> or online download (AFAIR).
>
> On October 25th, 2016, 9:45 p.m. UTC, *Jeff Lundblad* wrote:
>
> Moved the logic to ofximporterplugin. Date filter only applies in updateAccount(), so "File->Import->OFX" files are not affected.
>
> Regarding the lastUpdate.addDays(-3), it has occurred to me before that having a "last update minus X days" in the "Import Details" choices would be nice. Like the current "Today minus X days". X=0 would provide current functionality. I think I occasionally miss investment transactions if I update "just right" between the trade date and the settlement date (I'm not real sure on this).
>
> On October 26th, 2016, 6:55 a.m. UTC, *Thomas Baumgart* wrote:
>
> The changes look good to me, but since I am not an OFX user, I cannot comment on the overall functionality.
>
> If the impact of this change is simply to limit importing transactions to those after the requested start date, then this change will only affect downloads from institutions which do not honor that request.  This reveiw was in response to the only report of that problem I recall ever hearing.
>
>
> - Jack
>
> On October 25th, 2016, 9:44 p.m. UTC, Jeff Lundblad wrote:
> Review request for KMymoney.
> By Jeff Lundblad.
>
> *Updated Oct. 25, 2016, 9:44 p.m.*
> *Bugs: * 365818 <http://bugs.kde.org/show_bug.cgi?id=365818>
> *Repository: * kmymoney
> Description
>
> Only import transactions that are newer than or equal to the "Start date of import" in the account's online settings
>
> Testing
>
> Tested on Citi credit card OFX downloads which nearly always download 2 years worth of transactions regardless of the date range that the OFX request requests.
>
> Diffs
>
>    - kmymoney/plugins/ofximport/ofximporterplugin.cpp (46842ee)
>
> View Diff <https://git.reviewboard.kde.org/r/128478/diff/>
> File Attachments
> - ofx-update-date.patch
> <https://git.reviewboard.kde.org/media/uploaded/files/2016/10/25/7838204d-f10b-41cd-8774-044e17b9e98e__ofx-update-date.patch>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20161030/91a46319/attachment.html>