From 552ef45f47dfc3ee275778e93de253db3acaac0c Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Thu, 21 May 2020 21:58:48 -0400 Subject: [PATCH] plugin: Be more selective about when ! skips validation. It makes sense to let the bookkeeper skip validations in situations where the metadata requires information that might not be available when entered. It does not make sense to skip validations that *must* be available and affect the structure of the books, like project and entity. This commit ensures every plugin hook has a test for flagged transactions, even for hooks that currently have the desired behavior where no code changes were required for the test to pass. --- conservancy_beancount/plugin/core.py | 6 ++++-- conservancy_beancount/plugin/meta_approval.py | 5 +++-- conservancy_beancount/plugin/meta_entity.py | 2 +- conservancy_beancount/plugin/meta_invoice.py | 1 + .../plugin/meta_payable_documentation.py | 1 + conservancy_beancount/plugin/meta_project.py | 2 +- conservancy_beancount/plugin/meta_receipt.py | 1 + .../plugin/meta_receivable_documentation.py | 1 + conservancy_beancount/plugin/meta_repo_links.py | 1 + conservancy_beancount/plugin/meta_rt_links.py | 1 + conservancy_beancount/plugin/meta_tax_implication.py | 3 +++ tests/test_meta_entity.py | 7 +++++++ tests/test_meta_expense_allocation.py | 10 ++++++++++ tests/test_meta_income_type.py | 10 ++++++++++ tests/test_meta_paypal_id.py | 7 +++++++ tests/test_meta_project.py | 10 ++++++++++ tests/test_meta_tax_implication.py | 8 ++++++++ 17 files changed, 70 insertions(+), 6 deletions(-) diff --git a/conservancy_beancount/plugin/core.py b/conservancy_beancount/plugin/core.py index 7733368..7817859 100644 --- a/conservancy_beancount/plugin/core.py +++ b/conservancy_beancount/plugin/core.py @@ -24,6 +24,7 @@ from .. import errors as errormod from typing import ( Any, + Container, Dict, FrozenSet, Generic, @@ -176,9 +177,10 @@ class MetadataEnum: class TransactionHook(Hook[Transaction]): DIRECTIVE = Transaction + SKIP_FLAGS: Container[str] = frozenset() TXN_DATE_RANGE: _GenericRange = _GenericRange(DEFAULT_START_DATE, DEFAULT_STOP_DATE) - def _run_on_txn(self, txn: Transaction, skip_flags: str='!') -> bool: + def _run_on_txn(self, txn: Transaction) -> bool: """Check whether we should run on a given transaction This method implements our usual checks for whether or not a hook @@ -186,7 +188,7 @@ class TransactionHook(Hook[Transaction]): their own implementations. See _PostingHook below for an example. """ return ( - txn.flag not in skip_flags + txn.flag not in self.SKIP_FLAGS and txn.date in self.TXN_DATE_RANGE and not data.is_opening_balance_txn(txn) ) diff --git a/conservancy_beancount/plugin/meta_approval.py b/conservancy_beancount/plugin/meta_approval.py index 3158e5b..169cc4f 100644 --- a/conservancy_beancount/plugin/meta_approval.py +++ b/conservancy_beancount/plugin/meta_approval.py @@ -26,13 +26,14 @@ from ..beancount_types import ( class MetaApproval(core._RequireLinksPostingMetadataHook): CHECKED_METADATA = ['approval'] + SKIP_FLAGS = '!' def __init__(self, config: configmod.Config) -> None: self.payment_threshold = -config.payment_threshold() - def _run_on_txn(self, txn: Transaction, skip_flags: str='!') -> bool: + def _run_on_txn(self, txn: Transaction) -> bool: return ( - super()._run_on_txn(txn, skip_flags) + super()._run_on_txn(txn) # approval is required when funds leave a cash equivalent asset, # UNLESS that transaction is a transfer to another asset, # or paying off a credit card. diff --git a/conservancy_beancount/plugin/meta_entity.py b/conservancy_beancount/plugin/meta_entity.py index 10827e2..1e4ea02 100644 --- a/conservancy_beancount/plugin/meta_entity.py +++ b/conservancy_beancount/plugin/meta_entity.py @@ -67,7 +67,7 @@ class MetaEntity(core.TransactionHook): return entity, self.ENTITY_RE.match(entity) is not None def run(self, txn: Transaction) -> errormod.Iter: - if not self._run_on_txn(txn, ''): + if not self._run_on_txn(txn): return txn_entity, txn_entity_ok = self._check_entity(txn.meta, txn.payee) if txn_entity_ok is False: diff --git a/conservancy_beancount/plugin/meta_invoice.py b/conservancy_beancount/plugin/meta_invoice.py index 1c1c09c..d7fb6ba 100644 --- a/conservancy_beancount/plugin/meta_invoice.py +++ b/conservancy_beancount/plugin/meta_invoice.py @@ -24,6 +24,7 @@ from ..beancount_types import ( class MetaInvoice(core._RequireLinksPostingMetadataHook): CHECKED_METADATA = ['invoice'] + SKIP_FLAGS = '!' def _run_on_post(self, txn: Transaction, post: data.Posting) -> bool: return post.account.is_under( diff --git a/conservancy_beancount/plugin/meta_payable_documentation.py b/conservancy_beancount/plugin/meta_payable_documentation.py index fd37651..4102b72 100644 --- a/conservancy_beancount/plugin/meta_payable_documentation.py +++ b/conservancy_beancount/plugin/meta_payable_documentation.py @@ -24,6 +24,7 @@ from ..beancount_types import ( class MetaPayableDocumentation(core._RequireLinksPostingMetadataHook): CHECKED_METADATA = ['approval', 'contract'] + SKIP_FLAGS = '!' def _run_on_post(self, txn: Transaction, post: data.Posting) -> bool: if post.account.is_under('Liabilities:Payable:Accounts'): diff --git a/conservancy_beancount/plugin/meta_project.py b/conservancy_beancount/plugin/meta_project.py index 982b893..0014706 100644 --- a/conservancy_beancount/plugin/meta_project.py +++ b/conservancy_beancount/plugin/meta_project.py @@ -102,7 +102,7 @@ class MetaProject(core._NormalizePostingMetadataHook): else: raise errormod.InvalidMetadataError(txn, self.METADATA_KEY, None, post) - def _run_on_txn(self, txn: Transaction, skip_flags: str='') -> bool: + def _run_on_txn(self, txn: Transaction) -> bool: return txn.date in self.TXN_DATE_RANGE def run(self, txn: Transaction) -> errormod.Iter: diff --git a/conservancy_beancount/plugin/meta_receipt.py b/conservancy_beancount/plugin/meta_receipt.py index 4c2c9b9..c679020 100644 --- a/conservancy_beancount/plugin/meta_receipt.py +++ b/conservancy_beancount/plugin/meta_receipt.py @@ -31,6 +31,7 @@ from typing import ( class MetaReceipt(core._RequireLinksPostingMetadataHook): CHECKED_METADATA = ['receipt'] + SKIP_FLAGS = '!' def __init__(self, config: configmod.Config) -> None: self.payment_threshold = abs(config.payment_threshold()) diff --git a/conservancy_beancount/plugin/meta_receivable_documentation.py b/conservancy_beancount/plugin/meta_receivable_documentation.py index df7c21b..02c5a43 100644 --- a/conservancy_beancount/plugin/meta_receivable_documentation.py +++ b/conservancy_beancount/plugin/meta_receivable_documentation.py @@ -41,6 +41,7 @@ class MetaReceivableDocumentation(core._RequireLinksPostingMetadataHook): ISSUED_INVOICE_RE = re.compile( r'[Ii]nvoice[-_ ]*(?:2[0-9]{9,}|30[0-9]+)[A-Za-z]*[-_ .]', ) + SKIP_FLAGS = '!' def __init__(self, config: configmod.Config) -> None: rt_wrapper = config.rt_wrapper() diff --git a/conservancy_beancount/plugin/meta_repo_links.py b/conservancy_beancount/plugin/meta_repo_links.py index 3c68cfc..fdf1fc2 100644 --- a/conservancy_beancount/plugin/meta_repo_links.py +++ b/conservancy_beancount/plugin/meta_repo_links.py @@ -36,6 +36,7 @@ class MetaRepoLinks(core.TransactionHook): HOOK_GROUPS = frozenset(['linkcheck']) LINK_METADATA = data.LINK_METADATA.difference('rt-id') PATH_PUNCT_RE = re.compile(r'[:/]') + SKIP_FLAGS = '!' def __init__(self, config: configmod.Config) -> None: repo_path = config.repository_path() diff --git a/conservancy_beancount/plugin/meta_rt_links.py b/conservancy_beancount/plugin/meta_rt_links.py index 57cc114..95e4fc8 100644 --- a/conservancy_beancount/plugin/meta_rt_links.py +++ b/conservancy_beancount/plugin/meta_rt_links.py @@ -32,6 +32,7 @@ from typing import ( class MetaRTLinks(core.TransactionHook): HOOK_GROUPS = frozenset(['linkcheck', 'network', 'rt']) + SKIP_FLAGS = '!' def __init__(self, config: configmod.Config) -> None: rt_wrapper = config.rt_wrapper() diff --git a/conservancy_beancount/plugin/meta_tax_implication.py b/conservancy_beancount/plugin/meta_tax_implication.py index 3dc5839..b14144d 100644 --- a/conservancy_beancount/plugin/meta_tax_implication.py +++ b/conservancy_beancount/plugin/meta_tax_implication.py @@ -39,6 +39,9 @@ class MetaTaxImplication(core._NormalizePostingMetadataHook): 'USA-Corporation', 'W2', ]) + # Sometimes we accrue a payment before we have determined the recipient's + # tax status. + SKIP_FLAGS = '!' def __init__(self, config: configmod.Config) -> None: self.payment_threshold = -config.payment_threshold() diff --git a/tests/test_meta_entity.py b/tests/test_meta_entity.py index e077a57..cf491b0 100644 --- a/tests/test_meta_entity.py +++ b/tests/test_meta_entity.py @@ -226,3 +226,10 @@ def test_required_by_date(hook, date, need_value): ('Assets:Checking', 10), ]) assert any(hook.run(txn)) == need_value + +def test_still_required_on_flagged(hook): + txn = testutil.Transaction(flag='!', postings=[ + ('Income:Donations', -10), + ('Assets:Checking', 10), + ]) + assert list(hook.run(txn)) diff --git a/tests/test_meta_expense_allocation.py b/tests/test_meta_expense_allocation.py index af081d9..6f712bb 100644 --- a/tests/test_meta_expense_allocation.py +++ b/tests/test_meta_expense_allocation.py @@ -132,3 +132,13 @@ def test_default_value_set_in_date_range(hook, date, set_value): assert not errors expect_meta = None if set_value is None else {TEST_KEY: set_value} testutil.check_post_meta(txn, None, expect_meta) + +@pytest.mark.parametrize('src_value', INVALID_VALUES) +def test_flagged_txn_checked(hook, src_value): + txn = testutil.Transaction(flag='!', postings=[ + ('Assets:Cash', -25), + ('Expenses:General', 25, {TEST_KEY: src_value}), + ]) + errors = list(hook.run(txn)) + assert errors + testutil.check_post_meta(txn, None, {TEST_KEY: src_value}) diff --git a/tests/test_meta_income_type.py b/tests/test_meta_income_type.py index 0260bf0..41bde2a 100644 --- a/tests/test_meta_income_type.py +++ b/tests/test_meta_income_type.py @@ -149,3 +149,13 @@ def test_default_value_set_in_date_range(hook, date, set_value): assert not errors expect_meta = None if set_value is None else {TEST_KEY: set_value} testutil.check_post_meta(txn, None, expect_meta) + +@pytest.mark.parametrize('src_value', INVALID_VALUES) +def test_flagged_txn_checked(hook, src_value): + txn = testutil.Transaction(flag='!', postings=[ + ('Assets:Cash', 25), + ('Income:Other', -25, {TEST_KEY: src_value}), + ]) + errors = list(hook.run(txn)) + assert errors + testutil.check_post_meta(txn, None, {TEST_KEY: src_value}) diff --git a/tests/test_meta_paypal_id.py b/tests/test_meta_paypal_id.py index 49d736e..0860332 100644 --- a/tests/test_meta_paypal_id.py +++ b/tests/test_meta_paypal_id.py @@ -194,3 +194,10 @@ def test_not_required_on_opening(hook): (next(testutil.OPENING_EQUITY_ACCOUNTS), -1000), ]) assert not list(hook.run(txn)) + +def test_still_required_on_flagged_txn(hook): + txn = testutil.Transaction(flag='!', postings=[ + ('Assets:PayPal', 1000), + ('Income:Donations', -1000), + ]) + assert list(hook.run(txn)) diff --git a/tests/test_meta_project.py b/tests/test_meta_project.py index 8c84fcf..6178f68 100644 --- a/tests/test_meta_project.py +++ b/tests/test_meta_project.py @@ -166,3 +166,13 @@ def test_always_required_on_restricted_funds(hook): txn = testutil.OpeningBalance(acct) actual = {error.message for error in hook.run(txn)} assert actual == {f'{acct} missing project'} + +@pytest.mark.parametrize('src_value', INVALID_VALUES) +def test_still_required_on_flagged_txn(hook, src_value): + txn = testutil.Transaction(flag='!', **{TEST_KEY: src_value}, postings=[ + ('Assets:Cash', -25), + ('Expenses:General', 25), + ]) + errors = list(hook.run(txn)) + assert errors + testutil.check_post_meta(txn, None, None) diff --git a/tests/test_meta_tax_implication.py b/tests/test_meta_tax_implication.py index 88aa69f..8d490ca 100644 --- a/tests/test_meta_tax_implication.py +++ b/tests/test_meta_tax_implication.py @@ -135,3 +135,11 @@ def test_validation_only_in_date_range(hook, date, need_value): errors = list(hook.run(txn)) assert bool(errors) == bool(need_value) testutil.check_post_meta(txn, None, None) + +@pytest.mark.parametrize('src_value', INVALID_VALUES) +def test_flagged_txn_skipped(hook, src_value): + txn = testutil.Transaction(flag='!', **{TEST_KEY: src_value}, postings=[ + ('Liabilities:Payable:Accounts', 25), + ('Assets:Cash', -25), + ]) + assert not list(hook.run(txn))