reubano / csv2ofx

A Python library and command line tool for converting csv to ofx and qif files
MIT License
199 stars 113 forks source link

Add Schwab bank mapping, LEDGERBAL tag support, MS Money compatible output #93

Closed tstabrawa closed 9 months ago

tstabrawa commented 2 years ago

This PR adds the ability to convert CSV files downloaded from Charles Schwab checking accounts to OFX files supported by MS Money.

For convenience, I broke the changes down into the following logically-distinct commits:

Confirmed no new errors from manage lint. Confirmed all manage test tests pass.

Tested with >18 months of real-world data and manually verified transaction sanity in MS Money.

(Please let me know if anything doesn't meet coding standards, etc. Unfortunately, I'm not a native Python speaker. 😃)

nieder commented 2 years ago

I don't have Charles Schwab, but my CC stopped creating OFX files and now just provides CSV files. So I locally patched with 69140f3, and the resulting OFX file does seem to generally match what I historically had for my CC account OFX files before, especially the header. The biggest difference is that the new csv2ofx output closes all tags, while the CC original leaves many tags open. However, I'm not able to import the generated OFX into MS Money because it says it contains invalid or corrupt data. I'm not sure if it's a problem with csv2ofx or my mapping file.

This is the original CSV from my CC:

Trans. Date,Post Date,Description,Amount,Category
12/18/2021,12/18/2021,"ATMOS ENERGY 888-286-6700",151.75,"Services"
12/11/2021,12/13/2021,"KOHL'S",-66.22,"Payments and Credits"
Trans. Date Post Date Description Amount Category
12/18/2021 12/18/2021 ATMOS ENERGY 888-286-6700 151.75 Services
12/11/2021 12/13/2021 KOHL'S -66.22 Payments and Credits

I luckily still had the original OFX from the CC, and comparing it to the generated OFX, the transaction type seems backward. All purchases are listed as CREDIT with a positive value, while payments are DEBIT with a negative value. This is backwards from the original. Original OFX

<STMTTRN>
<TRNTYPE>DEBIT
<DTPOSTED>20211218170000.000
<TRNAMT>-151.75
<FITID>FITID20211219-151.75Q65G3
<NAME>ATMOS ENERGY 888-286-6700 TX
</STMTTRN>

csv2ofx OFX

<STMTTRN>
<TRNTYPE>CREDIT</TRNTYPE>
<DTPOSTED>20211218120000</DTPOSTED>
<TRNAMT>151.75</TRNAMT>
<FITID>5ff442c1347782c556acb7fc52c477cc</FITID>
<NAME>ATMOS ENERGY 888-286-6700 TXATMO</NAME>
</STMTTRN>

The wrapping tags for the whole OFX are also different. The original uses <CREDITCARDMSGSRSV1><CCSTMTTRNRS>, while the new one uses <BANKMSGSRSV1><STMTTRNRS>.

So I'm not sure if the import failure is specific to the MS money patch from here or general to csv2ofx. Will happily create a new issue if it's general so as to not hijack this PR.

tstabrawa commented 2 years ago

@nieder You'll also want to grab commit 893e6a8531ed1f2aa2f57bddb174eda08ccb08c5 if you want to produce an OFX file that MS Money is happy with. Also, since it looks like your CSV doesn't have a column with balances in it, you'll need to specify an ending balance at the command line (with the -B or --ending-balance option) when running csv2ofx. Without an ending balance in the OFX file, MS Money rejects the file (or at least, it does for bank statements).

This is a tool that I used to check the OFX files for errors while attempting to produce something that will satisfy MS Money: http://moneymvps.org/faq/article/366.aspx. Unfortunately, the tool doesn't appear to check everything, but it was a good start. I think the ledger-balance requirement wasn't actually checked by the tool, IIRC.

As for the incorrect transaction types, it looks like by default, csv2ofx treats negative values as debits and positive values as credits. If I'm interpreting things correctly, you should be able to correct both the <TRNTYPE> and the sign in <TRNAMT> by adding a "type" line to your mapping. For example:

    'type': lambda tr: 'debit' if tr.get('Amount') >= 0 '' else 'credit',

It looks like the <BANKMSGSRSV1> tags may be hard-coded. If MS Money refuses to accept the OFX as a credit card statement with the <BANKMSGSRSV1> tags, you may need to open a new issue or PR for changing that.

nieder commented 2 years ago

Thanks. The extra commit for the balances does help in passing validation. I was able to previously fix the debit/credit issue with this line in the mapping:

    "amount": lambda r: convert_amount(r["Amount"]) * -1,

Now, purchases show up as DEBIT, with a negative value in the OFX. This is clearly a problem with the CSV from Discover that uses logic backwards from what Money expects, but so be it (as long as they stay consistent).

For the BANKMSGSRSV1 tags, etc, if I keep them as is, the OFX analyzer complains. Money takes the file but asks me to identify the account during import (which it didn't with the original files before they switched to CSV). But if I do the following changes on the OFX:

1) Change:
STMTTRNRS -> CCSTMTTRNRS
BANKMSGSRSV1 -> CREDITCARDMSGSRSV1
BANKACCTFROM -> CCACCTFROM
STMTRS -> CCSTMTRS

then Money imports fine. The OFX analyzer also complains about these lines

2) BANKID in CCACCTFROM
3) ACCTTYPE in CCACCTFROM

But I can leave them in and Money seems happy.

Lastly, csv2ofx converted literal ampersands (&) to &amp (missing ending semicolon), and Money complained about that. If I edit them to &amp; everything is fine. Yay! Now to confirm the import imported everything correctly...

reubano commented 9 months ago

Just getting a chance to look this over. Can you sep it into two PRs? I'd like to avoid having library changes and new mappings in the same PR.

tstabrawa commented 9 months ago

@reubano Thanks for considering this PR. It looks like when split, there will still be a dependency between the two PRs. Namely, the tests for the ending-balance code (commit e0143e9695c2bd880ade20a24973364a7d09dbe9) use the new Schwab Bank mapping. (There is also a test in the new-mapping commit (8a4e7b40a8df79cbaafd51cce52a5d74de2f885d) that uses the new MS-Money mode, but that test could easily be split-out / moved to the new-features PR.)

Should I just create the new-mapping PR first (minus the one test that uses MS-Money mode), wait for that to merge, then create another for the other features? Or is there a specific way you'd like me to structure the two PRs so that they could both be created at once?

reubano commented 9 months ago

@tstabrawa if you don't need any new lib features for the mapping then I suggest a PR just for the mapping. So what you said in your second to last sentence. That will be very easy to review and approve. Then new PR(s) with the features so we can discuss the problems and proposed solutions. Thanks!

tstabrawa commented 9 months ago

Replaced by #114 and #115. Closing this PR in favor of those.