Fix #553 Fix #537 xml-fallback and unbooked-xml transactions - #578
Fix #553 Fix #537 xml-fallback and unbooked-xml transactions#578lukas-staab wants to merge 4 commits into
Conversation
|
Can you not shorten the description to the most important information? It's way too much text to read in my opinion. Please also explain what resources you used to implement this change. How does your AI know the standards? |
|
@lukas-staab I think this PR is doing too many things. For breaking changes, I suggest you explicit mention what breaks and why. These should of course be a separate PR. "I tried to be 100% backwards compatible, but some things did change:" is not very reassuring. |
06f6802 to
7e8cfac
Compare
|
@simonschaufi thanks for your Feedback; i did. I will open the discussions mentioned as future PR or Issues, i will think about it.
About which part of the code are specifically talking? I can only think about the booking flag, i updated the description about it ... The rest of the code does not need to know much about the specification documents; its mostly cleanup and connecting code which was already there but not fully integrated. @leobeal Thanks, i reviewed it as well again, i dropped the php increase, which was not much worth here and quite some noice looking at it after some sleep. I also clarified some things in the text. There is no breaking change which could be seperated; maybe the defaulting to xml, but MT940 is deprecated, so ... If you have anything else in mind which you think i should split up let me know, i am open for suggestions. I could only think of the camt parser bug or xml unbooked, but its only 2 lines each, and the second one split up would introduce some un-symetrical feature sets |
7e8cfac to
c501123
Compare
GetStatementOfAccount fell back to an internal GetStatementOfAccountXML when the bank did not support MT 940, but that field was not part of the serialized form. Persisting the action while the user enters a TAN - as any web application does - restored it without the fallback, so it looked for HIKAZ segments in a response that contained HICAZ and failed with "Only got 0 HIKAZ response segments!". Instead of repairing that field, the decision is now made explicitly from the BPD and each format is a first-class action: - GetStatementOfAccountMT940 (new) holds the MT 940 implementation. - GetStatementOfAccountXML gained getStatement(), so CAMT data can be used through the StatementOfAccount model as well. The conversion itself already existed, it just lived in the fallback code of GetStatementOfAccount. - GetStatementOfAccount picks between them, preferring CAMT XML because MT 940 has been retired by Die Deutsche Kreditwirtschaft, and delegates to the chosen action. The delegate is serialized like any other field, and getDelegate() tells callers which format they ended up with. - AbstractGetStatementOfAccount declares what both formats have in common: getStatement() and getRawResponse(). Pick a class to pin a format; GetStatementOfAccount's MT 940 and CAMT specific getters are deprecated in favour of getRawResponse(). Note that at banks offering both formats it now returns CAMT-derived data, which is why the DKB test pins GetStatementOfAccountMT940. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
camt.052.001.02 through .06 declare the status of an entry as a plain code (EntryStatus2Code, e.g. <Sts>BOOK</Sts>), whereas .08 makes it a choice element (EntryStatus1Choice, e.g. <Sts><Cd>BOOK</Cd></Sts>). The parser only read the plain form, so with .08 - the version German banks have been offering since 2021 and are migrating to exclusively - every transaction was reported as not booked. Read the code element first and fall back to the element's own text, which covers both spellings. Note that the order matters: casting <Sts><Cd>BOOK</Cd> </Sts> to a string yields the element's direct text, which is empty. A pending entry is added to the test fixture, so both the booked and the unbooked case are covered, and the whole test is run a second time against a camt.052.001.08 document. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
HICAZ carries the transactions that the bank has received but not booked yet in a field of its own, next to the booked ones, but GetStatementOfAccountXML never looked at it - so $includeUnbooked had no effect outside of MT 940. The action now collects that document whenever the bank sends one and exposes it through getUnbookedXML(), independent of the flag; $includeUnbooked decides whether those transactions also become part of getStatement() and getRawResponse(), which mirrors how the MT 940 action treats them. GetStatementOfAccount passes the flag on to either format now. Note that the request is the same either way: unlike MT 940 there is no parameter for it, the bank decides whether to send unbooked transactions at all, and it never does for a time range that lies in the past. While at it, HICAZv1::getNichtGebuchteUmsaetze() no longer fatals when the field is absent, which is the normal case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The XML sample still claimed that GetStatementOfAccount "automatically falls back to XML format when MT940 is not available", which is what sent me looking for a fallback that did not work. Describe what the three actions do instead, and point out that $includeUnbooked now applies to both formats. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c501123 to
ac05599
Compare
|
I regrouped the same original code changes in 4 different commits, hopefully this gives better reviewabilty :) |
This PR fixes Issue #553 (xml Fallback did not work properly and was very brittle) and also #537 due to symetry reasons (xml had no unbooked transaction till now)
I am 100% backwards compatible by the exposed Action Methods, but some things did change / are opinonated:
GetStatementOfAccount. The naming is IMHO all over the place and should be restructured in a future fix; therefore i deprecated (not deleted) them. I Introduced getRawResponse() as the new way forward. It might be intrested to differentiate between booked and unbooked there, but that would need further changes. We could discuss this before a new release.src/CAMT/CAMT.php): camt.052.001.02 through .06 spell the booking status as a plain code (<Sts>BOOK</Sts>), while .08 makes it a choice element (<Sts><Cd>BOOK</Cd></Sts>) - and only the plain form was read, so every transaction was reported as not booked. Little exposure so far - CAMT parsing only landed in Add CAMT XML parser with automatic MT940 fallback #533 (released with 4.1 in May) and the converter was reachable only through the broken fallback - but this PR puts that path on the main road, so it should be adressed here imo. Maybe also discussed if we want to use an external lib instead of self implementing (and maintaining) camt parsing?AI-Disclaimer
The changes implementations are planed by me and hand reviewed and tested afterwards, and quite some time was invested during the design Phase but ultimately AI-written with a payed subscription via Claude Code Opus, especially tests and commit messages
Also related to #463, #577. Does NOT fix #572 (only a similar error message; through
GetStatementOfAccountthe delegate now receives the request segment numbers, so that path reports the exception instead of running into a fatal - users ofGetStatementOfAccountXMLdirectly see no change).