captin411 / ofxclient

Bank transaction downloader and python OFX client libraries
MIT License
261 stars 89 forks source link

TDBank returns "Client up to date" when calling accounts() due to invalid DTACCTUP #69

Open philipsd6 opened 5 years ago

philipsd6 commented 5 years ago

https://github.com/captin411/ofxclient/blob/4da2719f0ecbbf5eee62fb82c1b3b34ec955ee5e/ofxclient/client.py#L125

The default for the date parameter causes TD Bank to return "Client is up to date". If that parameter is set to '19700101' it returns the account list successfully.

How is a user supposed to set that parameter though? After instantiating Institution() here...

inst = Institution(
    id='1002',
    org='CommerceBank',
    url='https://ofx.tdbank.com/eftxweb/td.ofx',
    username=tdbank.username,
    password=tdbank.password,
    client_args={
        'ofx_version': '103',
        'app_version': '2200',
        'id': tdbank.clientid,
    }
)

...calling inst.accounts() should just work, but for TD Bank it doesn't.

https://github.com/captin411/ofxclient/blob/4da2719f0ecbbf5eee62fb82c1b3b34ec955ee5e/ofxclient/institution.py#L134

institution.accounts() doesn't take any parameters, so there's no way to override the default date parameter. Wouldn't it make sense to have that method accept **kwargs and pass them on to account_list_query()?

I will provide a PR to do just that. Afterwards, this call should work fine:

inst.accounts(date='19700101')
jbms commented 5 years ago

It looks like TD Bank is violating the OFX spec here; it should accept the full date/time string as well as the date-only string. See the page with page number 90 at the bottom of http://www.ofx.net/downloads/OFX%202.2.pdf . The best thing would be to send a bug report to TD Bank and have them fix it, but I imagine it is not too likely that it would be fixed in a timely manner, if ever.

It seems likely that changing the code to always send 19700101 would break a different buggy implementation that expects the hour-minutes-seconds form, so that is probably unwise. Probably it would be best to do whatever Quicken does; however, I expect Quicken itself is full of special cases for individual banks.

Supporting parameters for this query makes some sense in general, though I'm not sure in practice there is any use in getting only accounts updated since a given date, or if servers even support that functionality.

However, it would be much better if it just worked for TD Bank by default. One option would be to specifically check for the TD Bank server in client.py --- there is already some special case code for ofx.discovercard.com. Another option would be to initially send the query with 19700101000000 and if it fails, to retry with 19700101. The advantage of this approach is that it is slightly more generic in that if another bank has the same bug as TD Bank, then this would work for that implementation as well. However, I'm not sure how likely it is that another bank's server has the same bug.

philipsd6 commented 5 years ago

First of all, I agree completely: A bug report to TD Bank is unlikely to result in a fix, but I may give that a shot.

For the workaround, I like your idea of automatically retrying -- this will work for TD Bank and any others that may have the same fault, and the worst case is querying twice and still failing.

philipsd6 commented 5 years ago

PR updated and tested OK with TD Bank.