Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi account OFX support #63

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Multi account OFX support #63

wants to merge 3 commits into from

Conversation

fabrepe
Copy link

@fabrepe fabrepe commented Feb 28, 2019

* Fix egh#32
* Allows specify format of account in case of multiple accounts when
  importing OFX
@egh egh self-assigned this Feb 28, 2019
* FIX print_results wrong number of arguments error when called by sync
* fix typos in --account argument to allow forcing the account name when
  importing OFX files with multiple accounts
@fabrepe
Copy link
Author

fabrepe commented Mar 1, 2019

New commit (f49b743) fix some errors when syncing instead of importing from an OFX file. I don't know if I need to cancel this pull request and create another one.

@egh
Copy link
Owner

egh commented Mar 17, 2019

Closing & re-opening to trigger travis build.

@egh egh closed this Mar 17, 2019
@egh egh reopened this Mar 17, 2019
@egh
Copy link
Owner

egh commented Mar 17, 2019

Sorry, I had broken tests and for some reason the travis-ci integration had stopped working. Can you merge in master to this branch to fix the tests? Thank you!

dest='account_format',
help="""Format string to use for generating the account line.
Substitutions can be written using {account_id}, {routing_number},
{branch_id}, {account_type} for OFX.""")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

@fabrepe
Copy link
Author

fabrepe commented Apr 4, 2019

Sorry for the delay, master branch has just been synced in the fabrepe/ledger-autosync

@501st-alpha1
Copy link
Contributor

This branch has been working for me. 👍

One unfortunate thing about the way my bank builds my OFX files is that if a given account doesn't have any transactions for the selected period, it will not be included in the OFX file. Thus I have some files with e.g. all 5 accounts, some with only 3, some with another set of 3, and so on. (The relative ordering appears to be constant though.)

So another useful feature would be the ability to link account names to account numbers, and just ignore any extra account arguments. I'd be fine with leaving that for another PR though.

@@ -55,7 +55,7 @@ def find_ledger_file(ledgerrcpath=None):
return None


def print_results(converter, ofx, ledger, txns, args):
def print_results(converter, ofx, ledger, txns, args, account_idx=0):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function would be better if we passed in account instead of ofx and account_idx.

diff --git a/ledgerautosync/cli.py b/ledgerautosync/cli.py
index a007c59..806e798 100755
--- a/ledgerautosync/cli.py
+++ b/ledgerautosync/cli.py
@@ -55,7 +55,7 @@ found."""
         return None
 
 
-def print_results(converter, ofx, ledger, txns, args, account_idx=0):
+def print_results(converter, account, ledger, txns, args):
     """
     This function is the final common pathway of program:
 
@@ -69,18 +69,18 @@ def print_results(converter, ofx, ledger, txns, args, account_idx=0):
         if (not(ledger.check_transaction_by_id
                 ("ofxid", converter.mk_ofxid(AUTOSYNC_INITIAL))) and
                 not(ledger.check_transaction_by_id("ofxid", ALL_AUTOSYNC_INITIAL))):
-            print(converter.format_initial_balance(ofx.accounts[account_idx].statement))
+            print(converter.format_initial_balance(account.statement))
     for txn in txns:
         print(converter.convert(txn).format(args.indent))
     if args.assertions:
-        print(converter.format_balance(ofx.accounts[account_idx].statement))
+        print(converter.format_balance(account.statement))
 
     # if OFX has positions use these to obtain commodity prices
     # and print "P" records to provide dated/timed valuations
     # Note that this outputs only the commodity price,
     # not your position (e.g. # shares), even though this is in the OFX record
-    if hasattr(ofx.accounts[account_idx].statement, 'positions'):
-        for pos in ofx.accounts[account_idx].statement.positions:
+    if hasattr(account.statement, 'positions'):
+        for pos in account.statement.positions:
             print(converter.format_position(pos))
 
 def make_ofx_converter(account,
@@ -138,7 +138,7 @@ def sync(ledger, accounts, args):
                                                hardcodeaccount=None,
                                                shortenaccount=args.shortenaccount,
                                                security_list=SecurityList(ofx))
-                print_results(converter, ofx, ledger, txns, args)
+                print_results(converter, acct, ledger, txns, args)
         except KeyboardInterrupt:
             raise
         except BaseException:
@@ -156,8 +156,7 @@ def import_ofx(ledger, args):
         sys.stderr.write("number of account name (--account) does not match the number of accounts in the OFX file\n")
         exit(1)
 
-    account_idx = 0
-    for account in ofx.accounts:
+    for account_idx, account in enumerate(ofx.accounts):
         txns = sync.filter(
             account.statement.transactions,
             account.account_id)
@@ -191,8 +190,7 @@ def import_ofx(ledger, args):
                                  hardcodeaccount=args.hardcodeaccount, #TODO
                                  shortenaccount=args.shortenaccount, #TODO
                                  security_list=security_list)
-        print_results(converter, ofx, ledger, txns, args, account_idx)
-        account_idx += 1
+        print_results(converter, account, ledger, txns, args)
 
 
 def import_csv(ledger, args):

@501st-alpha1
Copy link
Contributor

One unfortunate thing about the way my bank builds my OFX files is that if a given account doesn't have any transactions for the selected period, it will not be included in the OFX file. Thus I have some files with e.g. all 5 accounts, some with only 3, some with another set of 3, and so on. (The relative ordering appears to be constant though.)

So another useful feature would be the ability to link account names to account numbers, and just ignore any extra account arguments. I'd be fine with leaving that for another PR though.

My bank actually just updated their transaction downloading interface, and transactions are now limited to a single account, so this is no longer an issue for me. (Not to say that this might not still be a useful feature, but just that I no longer have immediate need of it.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only reads one account from OFX file
3 participants