From 5859421a15cefa02906a8ddb395f84d36f43eafb 2020-06-10 20:03:08 From: Brett Smith Date: 2020-06-10 20:03:08 Subject: [PATCH] accrual: Remove the consistency checker. Everything it said was a problem has been done legitimately in our books at one point or another. * Variation in contract can happen in different line items of an invoice or "group of contractor" situations. * Variation in cost can happen because one invoice spans a period of time, like donation matching programs. There is probably still value in a tool that checks to make sure we use consistent rates each day, but that affects all kinds of transactions, not just accruals, so it would be done better in a separate tool. * Variation in account happens because invoices legitimately span accrual accounts, like donation matching programs with fees payable. So: it's gone, good riddance. --- diff --git a/conservancy_beancount/reports/accrual.py b/conservancy_beancount/reports/accrual.py index f2c65bd7588f4896ee6c7ab9a86aa9bbc2547f70..1649d90ac518bc4556a2fd6b4ddb26e1ab8f83e4 100644 --- a/conservancy_beancount/reports/accrual.py +++ b/conservancy_beancount/reports/accrual.py @@ -169,32 +169,14 @@ class AccrualAccount(enum.Enum): class AccrualPostings(core.RelatedPostings): - def _meta_getter(key: MetaKey) -> Callable[[data.Posting], MetaValue]: # type:ignore[misc] - def meta_getter(post: data.Posting) -> MetaValue: - return post.meta.get(key) - return meta_getter - - _FIELDS: Dict[str, Callable[[data.Posting], MetaValue]] = { - 'account': operator.attrgetter('account'), - 'contract': _meta_getter('contract'), - 'invoice': _meta_getter('invoice'), - 'purchase_order': _meta_getter('purchase-order'), - } - INCONSISTENT = Sentinel() __slots__ = ( 'accrual_type', - 'accrued_entities', 'end_balance', - 'paid_entities', 'account', - 'accounts', - 'contract', - 'contracts', + 'entity', 'invoice', - 'invoices', - 'purchase_order', - 'purchase_orders', ) + INCONSISTENT = Sentinel() def __init__(self, source: Iterable[data.Posting]=(), @@ -204,56 +186,57 @@ class AccrualPostings(core.RelatedPostings): super().__init__(source, _can_own=_can_own) # The following type declarations tell mypy about values set in the for # loop that are important enough to be referenced directly elsewhere. - self.account: Union[data.Account, Sentinel] - self.invoice: Union[MetaValue, Sentinel] - for name, get_func in self._FIELDS.items(): - values = frozenset(get_func(post) for post in self) - setattr(self, f'{name}s', values) - if len(values) == 1: - one_value = next(iter(values)) - else: - one_value = self.INCONSISTENT - setattr(self, name, one_value) - if self.account is self.INCONSISTENT: + self.account = self._single_item(post.account for post in self) + if isinstance(self.account, Sentinel): self.accrual_type: Optional[AccrualAccount] = None - self.end_balance = self.balance_at_cost() - self.accrued_entities = self._collect_entities() - self.paid_entities = self.accrued_entities + norm_func: Callable[[T], T] = lambda x: x + entity_pred: Callable[[data.Posting], bool] = bool else: - self.accrual_type = AccrualAccount.classify(self) + self.accrual_type = AccrualAccount.by_account(self.account) norm_func = self.accrual_type.normalize_amount - self.end_balance = norm_func(self.balance_at_cost()) - self.accrued_entities = self._collect_entities( - lambda post: norm_func(post.units).number > 0, - ) - self.paid_entities = self._collect_entities( - lambda post: norm_func(post.units).number < 0, - ) + entity_pred = lambda post: norm_func(post.units).number > 0 + self.entity = self._single_item(self.entities(entity_pred)) + self.invoice = self._single_item(self.first_links('invoice')) + self.end_balance = norm_func(self.balance_at_cost()) - def _collect_entities(self, - pred: Callable[[data.Posting], bool]=bool, - default: str='', - ) -> FrozenSet[MetaValue]: - return frozenset( - post.meta.get('entity') or default - for post in self if pred(post) - ) + def _single_item(self, seq: Iterable[T]) -> Union[T, Sentinel]: + items = iter(seq) + try: + item1 = next(items) + except StopIteration: + all_same = False + else: + all_same = all(item == item1 for item in items) + return item1 if all_same else self.INCONSISTENT - def entities(self) -> Iterator[MetaValue]: - yield from self.accrued_entities - yield from self.paid_entities.difference(self.accrued_entities) + def entities(self, pred: Callable[[data.Posting], bool]=bool) -> Iterator[MetaValue]: + seen: Set[MetaValue] = set() + for post in self: + if pred(post): + try: + entity = post.meta['entity'] + except KeyError: + pass + else: + if entity not in seen: + yield entity + seen.add(entity) + + def first_links(self, key: MetaKey, default: Optional[str]=None) -> Iterator[Optional[str]]: + for post in self: + try: + yield post.meta.get_links(key)[0] + except (IndexError, TypeError): + yield default 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 + entity_ok = isinstance(self.entity, str) # `'/' 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 entity is not None and invoice_ok: + if account_ok and entity_ok and invoice_ok: yield (self.invoice, self) return groups = collections.defaultdict(list) @@ -261,7 +244,7 @@ class AccrualPostings(core.RelatedPostings): post_invoice = self.invoice if invoice_ok else ( post.meta.get('invoice') or 'BlankInvoice' ) - post_entity = entity if entity is not None else ( + post_entity = self.entity if entity_ok else ( post.meta.get('entity') or 'BlankEntity' ) groups[f'{post.account} {post_invoice} {post_entity}'].append(post) @@ -269,28 +252,6 @@ class AccrualPostings(core.RelatedPostings): for group_key, posts in groups.items(): yield group_key, type_self(posts, _can_own=True) - def report_inconsistencies(self) -> Iterable[Error]: - for field_name, get_func in self._FIELDS.items(): - if getattr(self, field_name) is self.INCONSISTENT: - for post in self: - errmsg = 'inconsistent {} for invoice {}: {}'.format( - field_name.replace('_', '-'), - self.invoice or "", - get_func(post), - ) - yield Error(post.meta, errmsg, post.meta.txn) - costs = collections.defaultdict(set) - for post in self: - costs[post.units.currency].add(post.cost) - for code, currency_costs in costs.items(): - if len(currency_costs) > 1: - for post in self: - if post.units.currency == code: - errmsg = 'inconsistent cost for invoice {}: {}'.format( - self.invoice or "", post.cost, - ) - yield Error(post.meta, errmsg, post.meta.txn) - def is_paid(self, default: Optional[bool]=None) -> Optional[bool]: if self.accrual_type is None: return default @@ -533,7 +494,9 @@ class AgingReport(BaseReport): rows.sort(key=lambda related: ( related.account, related[0].meta.date, - min(related.entities()) if related.accrued_entities else '', + ('\0'.join(related.entities()) + if related.entity is related.INCONSISTENT + else related.entity), )) self.ods.write(rows) self.ods.save_file(self.out_bin) @@ -556,12 +519,7 @@ class OutgoingReport(BaseReport): self.rt_wrapper = rtutil.RT(rt_client) def _primary_rt_id(self, posts: AccrualPostings) -> rtutil.TicketAttachmentIds: - rt_ids: Set[str] = set() - for post in posts: - try: - rt_ids.add(post.meta.get_links('rt-id')[0]) - except (IndexError, TypeError): - pass + rt_ids = {url for url in posts.first_links('rt-id') if url is not None} rt_ids_count = len(rt_ids) if rt_ids_count != 1: raise ValueError(f"{rt_ids_count} rt-id links found") @@ -666,10 +624,10 @@ class ReportType(enum.Enum): class ReturnFlag(enum.IntFlag): LOAD_ERRORS = 1 - CONSISTENCY_ERRORS = 2 REPORT_ERRORS = 4 NOTHING_TO_REPORT = 8 + def filter_search(postings: Iterable[data.Posting], search_terms: Iterable[cliutil.SearchTerm], ) -> Iterable[data.Posting]: @@ -756,10 +714,6 @@ def main(arglist: Optional[Sequence[str]]=None, for error in load_errors: bc_printer.print_error(error, file=stderr) returncode |= ReturnFlag.LOAD_ERRORS - for related in groups.values(): - for error in related.report_inconsistencies(): - bc_printer.print_error(error, file=stderr) - returncode |= ReturnFlag.CONSISTENCY_ERRORS if not groups: logger.warning("no matching entries found to report") returncode |= ReturnFlag.NOTHING_TO_REPORT diff --git a/setup.py b/setup.py index 93549bd8743ab5b0bf168fb3ccf34a9f7c372a19..348372729da0d6f09e4258b0a0d9e9e23370e26e 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.7', + version='1.1.8', 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 147b479802e1b07055c0e616966da4317429ffa6..c4392fc165cb5283570b40fb52adce2cc85d568d 100644 --- a/tests/test_reports_accrual.py +++ b/tests/test_reports_accrual.py @@ -63,11 +63,6 @@ ACCOUNTS = [ 'Liabilities:Payable:Vacation', ] -CONSISTENT_METADATA = [ - 'contract', - 'purchase-order', -] - class AgingRow(NamedTuple): date: datetime.date entity: Sequence[str] @@ -271,26 +266,6 @@ def test_accrual_postings_consistent_account(acct_name): ]) related = accrual.AccrualPostings(data.Posting.from_txn(txn)) assert related.account == acct_name - assert related.accounts == {acct_name} - -@pytest.mark.parametrize('meta_key,acct_name', testutil.combine_values( - CONSISTENT_METADATA, - ACCOUNTS, -)) -def test_accrual_postings_consistent_metadata(meta_key, acct_name): - meta_value = f'{meta_key}.pdf' - meta = { - meta_key: meta_value, - 'invoice': f'invoice with {meta_key}.pdf', - } - txn = testutil.Transaction(postings=[ - (acct_name, 70, meta), - (acct_name, 35, meta), - ]) - related = accrual.AccrualPostings(data.Posting.from_txn(txn)) - attr_name = meta_key.replace('-', '_') - assert getattr(related, attr_name) == meta_value - assert getattr(related, f'{attr_name}s') == {meta_value} def test_accrual_postings_entity(): txn = testutil.Transaction(postings=[ @@ -299,8 +274,8 @@ def test_accrual_postings_entity(): (ACCOUNTS[0], -10, {'entity': 'Payee10'}), ]) related = accrual.AccrualPostings(data.Posting.from_txn(txn)) - assert related.accrued_entities == {'Accruee'} - assert related.paid_entities == {'Payee10', 'Payee15'} + assert related.entity == 'Accruee' + assert set(related.entities()) == {'Accruee', 'Payee10', 'Payee15'} def test_accrual_postings_entities(): txn = testutil.Transaction(postings=[ @@ -333,107 +308,6 @@ def test_accrual_postings_inconsistent_account(): ]) related = accrual.AccrualPostings(data.Posting.from_txn(txn)) assert related.account is related.INCONSISTENT - assert related.accounts == set(ACCOUNTS) - -@pytest.mark.parametrize('meta_key,acct_name', testutil.combine_values( - CONSISTENT_METADATA, - ACCOUNTS, -)) -def test_accrual_postings_inconsistent_metadata(meta_key, acct_name): - invoice = 'invoice with {meta_key}.pdf' - meta_value = f'{meta_key}.pdf' - txn = testutil.Transaction(postings=[ - (acct_name, 20, {'invoice': invoice, meta_key: meta_value}), - (acct_name, 35, {'invoice': invoice}), - ]) - related = accrual.AccrualPostings(data.Posting.from_txn(txn)) - attr_name = meta_key.replace('-', '_') - assert getattr(related, attr_name) is related.INCONSISTENT - assert getattr(related, f'{attr_name}s') == {meta_value, None} - -@pytest.mark.parametrize('meta_key,account', testutil.combine_values( - CONSISTENT_METADATA, - ACCOUNTS, -)) -def test_consistency_check_when_consistent(meta_key, account): - invoice = f'test-{meta_key}-invoice' - meta_value = f'test-{meta_key}-value' - meta = { - 'invoice': invoice, - meta_key: meta_value, - } - txn = testutil.Transaction(postings=[ - (account, 100, meta), - (account, -100, meta), - ]) - related = accrual.AccrualPostings(data.Posting.from_txn(txn)) - assert not list(related.report_inconsistencies()) - -@pytest.mark.parametrize('meta_key,account', testutil.combine_values( - ['approval', 'entity', 'fx-rate', 'statement'], - ACCOUNTS, -)) -def test_consistency_check_ignored_metadata(meta_key, account): - invoice = f'test-{meta_key}-invoice' - txn = testutil.Transaction(postings=[ - (account, 100, {'invoice': invoice, meta_key: 'credit'}), - (account, -100, {'invoice': invoice, meta_key: 'debit'}), - ]) - related = accrual.AccrualPostings(data.Posting.from_txn(txn)) - assert not list(related.report_inconsistencies()) - -@pytest.mark.parametrize('meta_key,account', testutil.combine_values( - CONSISTENT_METADATA, - ACCOUNTS, -)) -def test_consistency_check_when_inconsistent(meta_key, account): - invoice = f'test-{meta_key}-invoice' - txn = testutil.Transaction(postings=[ - (account, 100, {'invoice': invoice, meta_key: 'credit', 'lineno': 1}), - (account, -100, {'invoice': invoice, meta_key: 'debit', 'lineno': 2}), - ]) - related = accrual.AccrualPostings(data.Posting.from_txn(txn)) - errors = list(related.report_inconsistencies()) - for exp_lineno, (actual, exp_msg) in enumerate(itertools.zip_longest(errors, [ - f'inconsistent {meta_key} for invoice {invoice}: credit', - f'inconsistent {meta_key} for invoice {invoice}: debit', - ]), 1): - assert actual.message == exp_msg - assert actual.entry is txn - assert actual.source.get('lineno') == exp_lineno - -def test_consistency_check_cost(): - account = ACCOUNTS[0] - invoice = 'test-cost-invoice' - txn = testutil.Transaction(postings=[ - (account, 100, 'EUR', ('1.1251', 'USD'), {'invoice': invoice, 'lineno': 1}), - (account, -100, 'EUR', ('1.125', 'USD'), {'invoice': invoice, 'lineno': 2}), - ]) - related = accrual.AccrualPostings(data.Posting.from_txn(txn)) - errors = list(related.report_inconsistencies()) - for post, err in itertools.zip_longest(txn.postings, errors): - assert err.message == f'inconsistent cost for invoice {invoice}: {post.cost}' - assert err.entry is txn - assert err.source.get('lineno') == post.meta['lineno'] - -def test_make_consistent_not_needed(): - 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, {**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 == main_meta['invoice'] - assert actual_postings is related - assert next(consistent, None) is None @pytest.mark.parametrize('acct_name,invoice,day', testutil.combine_values( ACCOUNTS, @@ -495,8 +369,10 @@ def test_make_consistent_across_entity(acct_name): 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 + entities = posts.entities() + assert next(entities, None) == posts.entity + assert next(entities, None) is None + assert posts.entity in key @pytest.mark.parametrize('acct_name', ACCOUNTS) def test_make_consistent_entity_differs_accrual_payment(acct_name):