Citi fix and some other fixes - fixes #38

[jantman]

I was hit by #38, Citi Credit Cards not working, like the other people who commented there. It turns out that, after trying a bunch of things and doing some investigation online, Citi is sensitive to the User-Agent header. If I totally omit it, things work right.

I did a bit of a refactor here. Previously (from this commit), client.py itself had a separate branch for Discover, which omitted the Accept and User-Agent headers and ensured the headers were ordered correctly for Discover.

I've removed the alternate code path and made this logic a bit more configurable... Client() now takes two additional arguments on the constructor, user_agent and accept, corresponding to those HTTP headers. They can either be set to None (use the default values, as previously), a specific string, or False in which case the header will be omitted completely. I also did a bit of work to make serializing the Client arguments a bit easier.

To expose this to the user, I added a client_args_for_bank() function to cli.py. When we fetch data for a bank from ofxhome and then instantiate an Institution for it, instead of passing a hard-coded dict of client_args (which just contained ofx_version), this method builds the client args. It provides one simple place for us to define institution-specific client arguments. In addition, instead of being hard-coded into Client directly these will be serialized as part of the config... so things should work right all the way from the CLI for institutions that we've explicitly set args for, but if problems arise users can add or alter the client args in their ofxclient.ini without having to dig into the Python.

The one down side of this is that, while it's probably more robust in the long run, it means that existing Discover accounts will need a manual update to ofxclient.ini to get them working again, specifically:

institution.client_args.user_agent = !!False!!
institution.client_args.accept = !!False!!

To get previously-working Citi accounts working again, use only the user_agent line above.

Note that this also included some changes to SecurableConfigParser to set and get boolean False values.

[jhaggard11]

That is awesome, thank you. This is what I need for Citi cards, I hope this gets merged soon.

[sneilan]

This project seems dead :(

[jantman]

Some stuff was committed 2 months ago, so I'd give the author the benefit of the doubt, though this PR has been open for over a month. I've occasionally let some PRs sit that long on my own projects when life was really busy, but perhaps @captin411 would consider giving someone else the commit bit?

[captin411]

I'll take a look at this one when I have time.

My biggest concern with this PR is the risk of fixing one bank and breaking others.

Ideally there would be some unit tests included here to ensure that the HTTP information remains unchanged for all other banks except the one in question.

Adding those would help ease some of the mental blockage I have in just accepting this PR without a deeper review.

As an aside it looks like the Travis build didn't run right for this PR.. another thing for me to look into.

[captin411]

@jantman I forgot to mention, I like the refactor of the client args into a function. This simultaneously adds flexibility and testability. Thank you.

[jhaggard11]

I really need @jantman fix for Chase, if there is anything I can do to help I'd gladly do it. I am not sure how to do unit test but if there is doc to point me to on how I'd gladly do it. I predicate everyone's work on this project.

[jantman]

@captin411 I can give a try at adding some tests around this maybe this weekend... unfortunately I've been pretty busy lately. I'm not sure what the quality will be, as I'm not really familiar with unittest/nose, there aren't any existing tests for client or cli (so I'm not sure I can promise much coverage), and given all these refactors I'm not sure how much adding unit tests would prove...

[captin411]

@jantman you're totally right. The commandline script is not written in a way that is easily testable. A lot of its business logic probably needs to be pushed into a module better suited for unit testing.

[jhaggard11]

I downloaded @jantman pr and tested it. Works great for Citi and my needs. Let me know if there is anything else I can do to help.

[captin411]

Thanks for all the hard work @jantman . :+1:

[jhaggard11]

Awesome!