Skip to content

Settle the :61: RC reversal sign left pending in #127 - #131

Open
DylanMerigaud wants to merge 1 commit into
wolph:developfrom
DylanMerigaud:fix/reversal-mark-sign
Open

Settle the :61: RC reversal sign left pending in #127#131
DylanMerigaud wants to merge 1 commit into
wolph:developfrom
DylanMerigaud:fix/reversal-mark-sign

Conversation

@DylanMerigaud

Copy link
Copy Markdown

Fixes #130, and settles the pending decision behind the xfail you added in 18a6d95.

I filed #130 before spotting #127. Apologies for the duplicate, your test_statement_rc_reversal_amount_is_negative
already has the analysis, and its reason line says exactly what I would have said. So rather than
re-report it, here is the thing that decides it: the fixture already in the repo answers the sign
question on its own.

The arithmetic

mt940_tests/betterplace/sepa_mt9401.sta holds 26 statement blocks, 15 of which declare both a
:60F: opening and a :62F: closing balance. Two of those 15 carry an RC entry of 204,88, and
those two are exactly the two whose entries do not add up to the balance the bank itself states:

before after
statements matching their own :62F: 13 / 15 15 / 15
the two statements holding an RC each 409,76 too high each exact

409,76 is twice 204,88, the signature of an amount counted as arriving instead of leaving. Since
the bank publishes the closing balance in the file, the fixture is its own witness.

The file corroborates it a second way: in block 4 that RC of 204,88 has a C of the same
amount immediately before it, and the RC's :86: subfield ?00 reads SEPA-UEBERW/STORNO.
Block 0's reads SAMMLER/STORNO. The :62F: only closes if that RC nets out.

I mention this because the German wording alone would not settle it: the MultiCash description
gives only "RC = Storno Credit", which is arguable either way. Danske Bank's structured MT940
description is explicit where that is terse, reproducing subfield 3 as "RC for Reversal of credit
(debit entry), RD for Reversal of debit (credit entry)", and Huntington's developer docs say the
same. ING and Rabobank name the marks without giving a direction at all. So the specs are split
on how explicit they are, and the fixture is what actually settles it.

The change

# C = credit, D = debit, RC = reversal of a credit (so money leaves
# the account, like a debit), RD = reversal of a debit (so money
# comes back in, like a credit).
#
# Compared case-insensitively because the tag patterns compile with
# re.IGNORECASE, so a lowercase mark reaches this constructor as-is.

if status.upper() in ('D', 'RC'):
    self.amount = -self.amount

RD needs nothing: 'RD' != 'D', so it was never negated, and positive is the correct sign for
a reversed debit. Of the four MT940 marks, only RC was wrong.

The .upper() is deliberate and matches your ff3d24a. RE_FLAGS includes re.IGNORECASE, so
:61:2001010101rc5,00... parses with status == 'rc', and a plain lowercase d was never
negated either. Two parametrised cases cover both.

Two things you should know before merging

  1. This will break Full audit: fix every confirmed issue #127 if that lands too. test_statement_rc_reversal_amount_is_negative is
    strict=True, so it turns into XPASS(strict). I checked: applying this on
    feature/full-audit-fixes gives 2 failures, that test and the betterplace golden. If you
    would rather I close this and push the same change into Full audit: fix every confirmed issue #127 instead, say the word
    , that is
    probably cleaner for you and I do not mind at all.
  2. sepa_mt9401.yml changes on two amounts and nothing else. I did not use WRITE_YAML_FILES=1,
    because regenerating rewrites 51 of the 52 goldens (anchor renumbering, an added
    transaction_boundary key, refreshed embedded tag patterns) and buries the two lines that
    matter. That churn also appears on a clean develop, so the goldens look stale independently
    of this change. Happy to regenerate the normal way if you would rather have that.

Scope

Not touched: StatementASNB's [A-Z]?[DC] also accepts EC/ED, which parse and stay positive
both before and after this. That is pre-existing and a separate question, and MT942 is where
those marks actually belong.

Checks

163 to 171 passing, coverage still 100%. ruff check, mypy and codespell clean.
ruff format --check flags README.md, which it also does on a clean develop, so I left it
alone.

Found while building a normalisation layer on top of this library, which is a good library and
the reason I did not write my own MT940 parser.

Field 61 has four debit/credit marks, not two: C, D, RC (reversal of a
credit) and RD (reversal of a debit). Amount only negated for D, so RC
came back positive even though a reversed credit takes money out of the
account. RD was already right, since a reversed debit does put money
back in.

This is the decision left pending in wolph#127, where the xfail added in
18a6d95 states the same reasoning. What that test could not settle is
the compatibility cost, so this commit brings the arithmetic that
argues for paying it.

mt940_tests/betterplace/sepa_mt9401.sta holds 26 statement blocks, 15
of which declare both a :60F: opening and a :62F: closing balance. Two
of those 15 carry an RC entry of 204,88, and those two are exactly the
two whose entries do not add up to the balance the bank states in its
own :62F:. Each computes a closing balance 409,76 HIGHER than the
stated one, which is twice an amount counted as arriving instead of
leaving. With RC negated, all 15 reconcile exactly.

The comparison is case-insensitive because the tag patterns compile
with re.IGNORECASE, so a lowercase mark reaches Amount unchanged and a
plain lowercase 'd' was never negated either. ff3d24a on the wolph#127
branch fixes that same class of bug for D, and this is written to sit
on top of it rather than revert it.

The golden sepa_mt9401.yml changes on those two amounts only.
@DylanMerigaud

Copy link
Copy Markdown
Author

Following up with something more useful than a ping.

#127 has been open a month, and besides your own two passes the only reviews on it are bots: Copilot (which hit its quota and bailed), the advanced-security scanner and gemini-code-assist. It is 756 lines through the parser, and it is also what blocks this PR, since my change turns test_statement_rc_reversal_amount_is_negative into an XPASS(strict).

I have that branch checked out and I have been in models.py and the betterplace fixtures while building a corpus of real statement deviations, so I would rather spend the time there than wait here. Two offers, both already scoped:

  1. I review Full audit: fix every confirmed issue #127 properly. File by file, suite running, flagging anything that changes output on a shipped major the way ?31 did. You get a review from someone who is not a bot, and you are free to ignore all of it.
  2. I fold this PR into Full audit: fix every confirmed issue #127 myself. I offered that above and you have not had to answer. Unless you would rather I did not, I will close this one and open the same change against feature/full-audit-fixes with the xfail dropped and the betterplace golden corrected, so the branch stays green as one unit instead of two PRs that break each other.

Separately, #130 is now redundant with this PR, so I will close it and keep your issue list honest.

#132 is the only place I need a word from you, and it is a binary: restore '31': 'applicant_iban', or add the key back alongside the merged name. Either one and the PR follows the same day, goldens included. Pick neither and I will send the restore, since that is the mapping 4.30.0 shipped and the one kefiya#148 is broken against.

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.

:61: reversal mark RC gets the wrong sign

1 participant