Review Request 128478: Only import OFX requested date range

[Jack Ostroff]

> On July 20, 2016, 5:55 a.m., 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.
> 
> 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.
> 
> 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)**.
> 
> 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?
> 
> 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.
>     ```C++
>       // 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).
> 
> 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).
> 
> 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


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128478/#review97637
-----------------------------------------------------------


On Oct. 25, 2016, 9:44 p.m., Jeff Lundblad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128478/
> -----------------------------------------------------------
> 
> (Updated Oct. 25, 2016, 9:44 p.m.)
> 
> 
> Review request for KMymoney.
> 
> 
> 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
> 
> 
> Diffs
> -----
> 
>   kmymoney/plugins/ofximport/ofximporterplugin.cpp 46842ee 
> 
> Diff: https://git.reviewboard.kde.org/r/128478/diff/
> 
> 
> 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.
> 
> 
> 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
> 
> 
> Thanks,
> 
> Jeff Lundblad
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20161026/07a8dc78/attachment.html>