From e22e63dcca7893e2fa5fdf3c0a17adcebec8fcd1 2020-06-05 14:54:35 From: Brett Smith Date: 2020-06-05 14:54:35 Subject: [PATCH] accrual: Make accruals consistent by entity on the accrual side. It is more common than I realized that we split an invoice by entity on the accrual side, so this supports that better. It still disregards inconsistency between accrual entity and payment entity for reporting purposes, to help keep reporting clean around automatic imports. The changes to BaseReport._report shook out because at this point, the group key is effectively arbitrary and shouldn't be used for any reporting purposes. --- diff --git a/conservancy_beancount/reports/accrual.py b/conservancy_beancount/reports/accrual.py index 584bc42ac704dfb272c81f23be2fae297534bbdb..cc705fae12307283f690d7307ed886bdfc6abb82 100644 --- a/conservancy_beancount/reports/accrual.py +++ b/conservancy_beancount/reports/accrual.py @@ -246,20 +246,26 @@ class AccrualPostings(core.RelatedPostings): def make_consistent(self) -> Iterator[Tuple[MetaValue, 'AccrualPostings']]: account_ok = isinstance(self.account, str) + if len(self.accrued_entities) == 1: + entity = next(iter(self.accrued_entities)) + else: + entity = None # `'/' in self.invoice` is just our heuristic to ensure that the # invoice metadata is "unique enough," and not just a placeholder # value like "FIXME". It can be refined if needed. invoice_ok = isinstance(self.invoice, str) and '/' in self.invoice - if account_ok and invoice_ok: + if account_ok and entity is not None and invoice_ok: yield (self.invoice, self) return groups = collections.defaultdict(list) for post in self: - if invoice_ok: - key = f'{self.invoice} {post.account}' - else: - key = f'{post.account} {post.meta.get("entity")} {post.meta.get("invoice")}' - groups[key].append(post) + post_invoice = self.invoice if invoice_ok else ( + post.meta.get('invoice') or 'BlankInvoice' + ) + post_entity = entity if entity is not None else ( + post.meta.get('entity') or 'BlankEntity' + ) + groups[f'{post.account} {post_invoice} {post_entity}'].append(post) type_self = type(self) for group_key, posts in groups.items(): yield group_key, type_self(posts, _can_own=True) @@ -314,16 +320,12 @@ class BaseReport: self.out_file = out_file self.logger = logger.getChild(type(self).__name__) - def _report(self, - invoice: str, - posts: AccrualPostings, - index: int, - ) -> Iterable[str]: + def _report(self, posts: AccrualPostings, index: int) -> Iterable[str]: raise NotImplementedError("BaseReport._report") def run(self, groups: PostGroups) -> None: for index, invoice in enumerate(groups): - for line in self._report(str(invoice), groups[invoice], index): + for line in self._report(groups[invoice], index): print(line, file=self.out_file) @@ -490,16 +492,12 @@ class AgingReport(BaseReport): class BalanceReport(BaseReport): - def _report(self, - invoice: str, - posts: AccrualPostings, - index: int, - ) -> Iterable[str]: + def _report(self, posts: AccrualPostings, index: int) -> Iterable[str]: posts = posts.since_last_nonzero() date_s = posts[0].meta.date.strftime('%Y-%m-%d') if index: yield "" - yield f"{invoice}:" + yield f"{posts.invoice}:" yield f" {posts.balance_at_cost()} outstanding since {date_s}" @@ -520,11 +518,7 @@ class OutgoingReport(BaseReport): else: return parsed - def _report(self, - invoice: str, - posts: AccrualPostings, - index: int, - ) -> Iterable[str]: + def _report(self, posts: AccrualPostings, index: int) -> Iterable[str]: posts = posts.since_last_nonzero() try: ticket_id, _ = self._primary_rt_id(posts) @@ -537,7 +531,7 @@ class OutgoingReport(BaseReport): if ticket is None: self.logger.error( "can't generate outgoings report for %s because no RT ticket available: %s", - invoice, errmsg, + posts.invoice, errmsg, ) return diff --git a/setup.py b/setup.py index 0afb6bb41251d9a3999c52a85f70d95e68f739f6..d8e2c8e3c19f0b812dfe01e9f5ab01aefce976b8 100755 --- a/setup.py +++ b/setup.py @@ -5,7 +5,7 @@ from setuptools import setup setup( name='conservancy_beancount', description="Plugin, library, and reports for reading Conservancy's books", - version='1.1.2', + version='1.1.3', author='Software Freedom Conservancy', author_email='info@sfconservancy.org', license='GNU AGPLv3+', diff --git a/tests/test_reports_accrual.py b/tests/test_reports_accrual.py index 3a16dfc3105b9f5fb9a3bdca5dca02bcd4a71da2..9b14372a419c3c84d05cf26b00427e12965c73cd 100644 --- a/tests/test_reports_accrual.py +++ b/tests/test_reports_accrual.py @@ -20,6 +20,7 @@ import datetime import io import itertools import logging +import operator import re import babel.numbers @@ -478,18 +479,21 @@ def test_consistency_check_cost(): assert err.source.get('lineno') == post.meta['lineno'] def test_make_consistent_not_needed(): - invoice = 'Invoices/ConsistentDoc.pdf' + main_meta = { + 'entity': 'ConsistentTest', + 'invoice': 'Invoices/ConsistentDoc.pdf', + } other_meta = {key: f'{key}.pdf' for key in CONSISTENT_METADATA} # We intentionally make inconsistencies in "minor" metadata that shouldn't # split out the group. txn = testutil.Transaction(postings=[ - (ACCOUNTS[0], 20, {**other_meta, 'invoice': invoice}), - (ACCOUNTS[0], 25, {'invoice': invoice}), + (ACCOUNTS[0], 20, {**main_meta, **other_meta}), + (ACCOUNTS[0], 25, {**main_meta}), ]) related = accrual.AccrualPostings(data.Posting.from_txn(txn)) consistent = related.make_consistent() actual_key, actual_postings = next(consistent) - assert actual_key == invoice + assert actual_key == main_meta['invoice'] assert actual_postings is related assert next(consistent, None) is None @@ -500,13 +504,17 @@ def test_make_consistent_not_needed(): )) def test_make_consistent_bad_invoice(acct_name, invoice, day): txn = testutil.Transaction(date=datetime.date(2019, 1, day), postings=[ - (acct_name, index * 10, {'invoice': invoice}) + (acct_name, index * 10, {'invoice': invoice, 'entity': f'BadInvoice{day}'}) for index in range(1, 4) ]) related = accrual.AccrualPostings(data.Posting.from_txn(txn)) consistent = dict(related.make_consistent()) assert len(consistent) == 1 - actual = consistent.get(f'{acct_name} None {invoice}') + key = next(iter(consistent)) + assert acct_name in key + if invoice: + assert str(invoice) in key + actual = consistent[key] assert actual assert len(actual) == 3 for act_post, exp_post in zip(actual, txn.postings): @@ -516,16 +524,15 @@ def test_make_consistent_bad_invoice(acct_name, invoice, day): def test_make_consistent_across_accounts(): invoice = 'Invoices/CrossAccount.pdf' txn = testutil.Transaction(date=datetime.date(2019, 2, 1), postings=[ - (acct_name, 100, {'invoice': invoice}) + (acct_name, 100, {'invoice': invoice, 'entity': 'CrossAccount'}) for acct_name in ACCOUNTS ]) related = accrual.AccrualPostings(data.Posting.from_txn(txn)) consistent = dict(related.make_consistent()) assert len(consistent) == len(ACCOUNTS) - for acct_name in ACCOUNTS: - actual = consistent[f'{invoice} {acct_name}'] - assert len(actual) == 1 - assert actual[0].account == acct_name + for key, posts in consistent.items(): + assert len(posts) == 1 + assert posts.account in key def test_make_consistent_both_invoice_and_account(): txn = testutil.Transaction(date=datetime.date(2019, 2, 2), postings=[ @@ -534,10 +541,39 @@ def test_make_consistent_both_invoice_and_account(): related = accrual.AccrualPostings(data.Posting.from_txn(txn)) consistent = dict(related.make_consistent()) assert len(consistent) == len(ACCOUNTS) - for acct_name in ACCOUNTS: - actual = consistent[f'{acct_name} None None'] - assert len(actual) == 1 - assert actual[0].account == acct_name + for key, posts in consistent.items(): + assert len(posts) == 1 + assert posts.account in key + +@pytest.mark.parametrize('acct_name', ACCOUNTS) +def test_make_consistent_across_entity(acct_name): + amt_sign = operator.pos if acct_name.startswith('Assets') else operator.neg + txn = testutil.Transaction(postings=[ + (acct_name, amt_sign(n), {'invoice': 'Inv/1.pdf', 'entity': f'Entity{n}'}) + for n in range(1, 4) + ]) + related = accrual.AccrualPostings(data.Posting.from_txn(txn)) + consistent = dict(related.make_consistent()) + assert len(consistent) == 3 + for key, posts in consistent.items(): + assert len(posts) == 1 + assert len(posts.accrued_entities) == 1 + assert next(posts.entities()) in key + +@pytest.mark.parametrize('acct_name', ACCOUNTS) +def test_make_consistent_entity_differs_accrual_payment(acct_name): + invoice = 'Invoices/DifferPay.pdf' + txn = testutil.Transaction(postings=[ + # Depending on the account, the order of the accrual and payment might + # be swapped here, but that shouldn't matter. + (acct_name, 125, {'invoice': invoice, 'entity': 'Positive'}), + (acct_name, -125, {'invoice': invoice, 'entity': 'Negative'}), + ]) + related = accrual.AccrualPostings(data.Posting.from_txn(txn)) + consistent = related.make_consistent() + _, actual = next(consistent) + assert actual is related + assert next(consistent, None) is None def check_output(output, expect_patterns): output.seek(0) @@ -659,6 +695,17 @@ def test_aging_report_date_cutoffs(accrual_postings, date, recv_end, pay_end): output = run_aging_report(accrual_postings, date) check_aging_ods(output, date, expect_recv, expect_pay) +def test_aging_report_entity_consistency(accrual_postings): + output = run_aging_report(( + post for post in accrual_postings + if post.meta.get('rt-id') == 'rt:480' + and post.units.number < 0 + )) + check_aging_ods(output, None, [], [ + AgingRow.make_simple('2010-04-15', 'MultiPartyA', 125, 'rt:480/4800'), + AgingRow.make_simple('2010-04-15', 'MultiPartyB', 125, 'rt:480/4800'), + ]) + def run_main(arglist, config=None): if config is None: config = testutil.TestConfig(