Repository navigation
Keep the XML fallback of GetStatementOfAccount across serialization (#553) #591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,11 +5,15 @@ | |
| use Fhp\Action\GetStatementOfAccount; | ||
| use Fhp\Model\SEPAAccount; | ||
| use Fhp\Protocol\BPD; | ||
| use Fhp\Protocol\Message; | ||
| use Fhp\Protocol\UPD; | ||
| use Fhp\Segment\BaseSegment; | ||
| use Fhp\Segment\CAZ\HKCAZv1; | ||
| use Fhp\Segment\KAZ\HIKAZSv5; | ||
| use Fhp\Segment\KAZ\HIKAZSv6; | ||
| use Fhp\Segment\KAZ\HIKAZSv7; | ||
| use Fhp\Segment\KAZ\HKKAZv5; | ||
| use Fhp\Segment\KAZ\HKKAZv6; | ||
| use Fhp\Segment\KAZ\HKKAZv7; | ||
| use Fhp\Segment\KAZ\ParameterKontoumsaetzeV2; | ||
| use Fhp\Segment\SPA\HISPASv1; | ||
|
|
@@ -85,6 +89,89 @@ public function testCreateRequestRejectsAllAccountsWhenNotSupportedByBank() | |
| $action->getNextRequest(self::createBpd(self::createHikazsSegment(6, false)), null); | ||
| } | ||
|
|
||
| /** | ||
| * Banks without HIKAZS (e.g. Atruvia/Volksbank) make the action fall back to HKCAZ. Those banks also tend to | ||
| * require a TAN for HKCAZ, so applications serialize the action while waiting for it and then feed the response | ||
| * to the unserialized copy. The fallback must survive that roundtrip, otherwise the copy expects HIKAZ segments. | ||
| */ | ||
| public function testXmlFallbackSurvivesSerialization() | ||
| { | ||
| $action = GetStatementOfAccount::create(self::account(), new \DateTimeImmutable('2026-04-20'), new \DateTimeImmutable('2026-07-19')); | ||
|
|
||
| $requestSegments = $action->getNextRequest(self::createCamtOnlyBpd(), self::createUpdWithHkcaz()); | ||
| self::assertCount(1, $requestSegments); | ||
| self::assertInstanceOf(HKCAZv1::class, $requestSegments[0]); | ||
| // FinTs::execute() numbers the segments before sending, which serialization of the request relies on. | ||
| $requestSegments[0]->setSegmentNumber(3); | ||
| $action->setRequestSegmentNumbers([3]); | ||
|
|
||
| /** @var GetStatementOfAccount $action */ | ||
| $action = unserialize(serialize($action)); | ||
|
|
||
| $action->processResponse(Message::parse(self::camtResponse())); | ||
|
|
||
| $statements = $action->getStatement()->getStatements(); | ||
| self::assertCount(1, $statements); | ||
| $transactions = $statements[0]->getTransactions(); | ||
| self::assertCount(1, $transactions); | ||
| self::assertSame(12.5, $transactions[0]->getAmount()); | ||
| self::assertSame('Max Mustermann', $transactions[0]->getName()); | ||
| } | ||
|
|
||
| /** Actions serialized by earlier versions of this library carry no XML fallback and must still unserialize. */ | ||
| public function testUnserializesActionsWithoutXmlFallback() | ||
| { | ||
| $action = GetStatementOfAccount::create(self::account(), new \DateTimeImmutable('2026-04-20'), new \DateTimeImmutable('2026-07-19')); | ||
| $serialized = $action->__serialize(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't call the magic functions as member functions. Use PHP's global
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's good, hard-coding the specific string we want to be able to unserialize, as it would sit persisted like this in some database too. |
||
| self::assertNull(array_pop($serialized)); // Drop the trailing XML fallback entry to emulate the old format. | ||
|
|
||
| $unserialized = new GetStatementOfAccount(); | ||
| $unserialized->__unserialize($serialized); | ||
|
|
||
| $requestSegments = $unserialized->getNextRequest(self::createBpd(self::createHikazsSegment(6, true)), null); | ||
| self::assertCount(1, $requestSegments); | ||
| self::assertInstanceOf(HKKAZv6::class, $requestSegments[0]); | ||
| self::assertSame('20260420', $requestSegments[0]->vonDatum); | ||
| } | ||
|
|
||
| /** A BPD that offers HKCAZ but no HKKAZ at all, like Atruvia banks do. */ | ||
| private static function createCamtOnlyBpd(): BPD | ||
| { | ||
| $hicazs = BaseSegment::parse("HICAZS:58:1:3+1+1+1+90:N:N:urn?:iso?:std?:iso?:20022?:tech?:xsd?:camt.052.001.02'"); | ||
| $bpd = self::createBpd($hicazs, self::createHispasSegment(true)); | ||
| unset($bpd->parameters['HIKAZS']); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This Optionally,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 162cd79: |
||
| $bpd->parameters['HICAZS'][$hicazs->getVersion()] = $hicazs; | ||
| return $bpd; | ||
| } | ||
|
|
||
| private static function createUpdWithHkcaz(): UPD | ||
| { | ||
| $upd = new UPD(); | ||
| $upd->hiupd = [BaseSegment::parse( | ||
| "HIUPD:7:6:4+5407324931::280:50010517+DE44500105175407324931+9999999999+1+EUR+Mustermann+Max+Girokonto++HKSAK:1+HKCAZ:1+HKSPA:1'" | ||
| )]; | ||
| return $upd; | ||
| } | ||
|
|
||
| private static function camtResponse(): string | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should return
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 162cd79. |
||
| { | ||
| $xml = '<?xml version="1.0" encoding="UTF-8"?>' | ||
| . '<Document xmlns="urn:iso:std:iso:20022:tech:xsd:camt.052.001.02">' | ||
| . '<BkToCstmrAcctRpt><GrpHdr><MsgId>1</MsgId><CreDtTm>2026-07-19T10:00:00+02:00</CreDtTm></GrpHdr>' | ||
| . '<Rpt><Id>1</Id><Acct><Id><IBAN>DE44500105175407324931</IBAN></Id></Acct>' | ||
| . '<Ntry><Amt Ccy="EUR">12.50</Amt><CdtDbtInd>CRDT</CdtDbtInd><Sts>BOOK</Sts>' | ||
| . '<BookgDt><Dt>2026-07-17</Dt></BookgDt><ValDt><Dt>2026-07-17</Dt></ValDt>' | ||
| . '<NtryDtls><TxDtls><RltdPties><Dbtr><Nm>Max Mustermann</Nm></Dbtr>' | ||
| . '<DbtrAcct><Id><IBAN>DE89370400440532013000</IBAN></Id></DbtrAcct></RltdPties>' | ||
| . '<RmtInf><Ustrd>Rechnung 4711</Ustrd></RmtInf></TxDtls></NtryDtls></Ntry>' | ||
| . '</Rpt></BkToCstmrAcctRpt></Document>'; | ||
|
|
||
| return "HNHBK:1:3+000000000000+300+0+1+0:1'" | ||
| . "HIRMG:2:2+0020::Auftrag ausgeführt.'" | ||
| . 'HICAZ:3:1:3+DE44500105175407324931:INGDDEFFXXX:5407324931::280:50010517+urn?:iso?:std?:iso?:20022?:tech?:xsd?:camt.052.001.02+@' . strlen($xml) . '@' . $xml . "'" | ||
| . "HNHBS:4:1+1'"; | ||
| } | ||
|
|
||
| private static function account(): SEPAAccount | ||
| { | ||
| return (new SEPAAccount()) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,6 +93,10 @@ public function __serialize(): array | |
| parent::__serialize(), | ||
| $this->account, $this->from, $this->to, $this->allAccounts, $this->includeUnbooked, | ||
| $this->bankName, | ||
| // The XML fallback action is created in createRequest(), so it must survive the serialization roundtrip | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure this comment is necessary. But I also don't mind it being here. A nested action should intuitively be treated like the parent action for most operations like serialization. Seems like a basic implication of "composition" (versus inheritance, where we'd equally intuitively call the parent function). Hence no need to especially justify it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trimmed to one line in 162cd79 – it only notes why the entry exists at all. |
||
| // that applications do while waiting for a TAN (banks like Atruvia/Volksbank offer only HKCAZ and require | ||
| // a TAN for it). Without it, processResponse() would look for HIKAZ segments in an HICAZ response. | ||
| $this->xmlAction, | ||
| ]; | ||
| } | ||
|
|
||
|
|
@@ -113,7 +117,8 @@ public function __unserialize(array $serialized): void | |
| $parentSerialized, | ||
| $this->account, $this->from, $this->to, $this->allAccounts, $this->includeUnbooked, | ||
| $this->bankName, | ||
| ) = $serialized; | ||
| $this->xmlAction, | ||
| ) = array_pad($serialized, 8, null); // Actions serialized before the XML fallback was persisted have 7 entries. | ||
|
|
||
| is_array($parentSerialized) ? | ||
| parent::__unserialize($parentSerialized) : | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of hard-coding these magic numbers here, reuse
GetCreditCardStatementTest::nextRequest()by moving it to a helper class / super test class and giving it a?UPDarg.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 162cd79:
nextRequest()moved fromGetCreditCardStatementTestto the newActionTestCasebase class, takes anyBaseActionplus an optional?UPD, andGetCreditCardStatementTestuses the shared copy. (#594 has the same pattern in its new test and will switch to the helper once one of the two lands.)