Correct bank account parsing#5
Conversation
| * | ||
| * @return array{prefix: ?string, number: ?string, bankCode: ?string} | ||
| */ | ||
| public function getParsedAccountNumber(): array |
There was a problem hiding this comment.
Previously this method was constructing $parsedAccountNumber on every transactions - this can lead to bad performance - constructing is done in setAccountNumber.
|
@pionl I will get real export from Česká spořitelna. Export allows to select internal or normal account format so I would stick with normal format in this case. |
|
We need to just fix Would be great to get other bank statements to ensure correct support. Don't know how to solve RB - potentially could be the "default" bank account (or add ability to set it as default to BankAccountParser). |
|
Přidána podpora CREDITAS bank |
|
na PHP 8.1 mě to hlásí warning:
|
pionl
force-pushed
the
bank-account-detection
branch
from
4c20769 to
c53c242
Compare
pionl
changed the title
|
I've finalized all proposed changes, we are missing tests for other banks, but it should work and we have new fallbacks to customize "failures". I've tested the changes on my import usage and now all looks well. |
| } | ||
|
|
||
| // Example: 7204021470000115 | ||
| $internalAccountNumber = substr($line, 3, 16); |
There was a problem hiding this comment.
I would move transformation of internal to normal format to separate utility function in ABOParser
There was a problem hiding this comment.
I disagree. At this moment ABOParser is huge and the testability of the code is thank to it degreased. It is sensible to use the "service" class to handle bank account transformation so we can test it in detail in its own test case and ABOParser test will not care about implementation detail.
|
|
||
| # Counter account number | ||
| $counterAccountNumber = substr($line, 19, 6) . '-' . substr($line, 25, 10); | ||
| $counterAccountNumber = $this->bankAccountParser->formatBankAccountNumber(substr($line, 19, 6), substr($line, 25, 10)); |
There was a problem hiding this comment.
Conversion from internal format should be used here too for banks that use internal format
There was a problem hiding this comment.
Well I've not found an example of bank account. I would add it if we have an example. Otherwise we should leave it as it is?
- Move bank related const to own file: based on feedback - expose construct properties: makes testing easier (via DI)
|
|
Bank code in bank number of const BANKS_WITH_ALT_POSTING_CODE = [
'0300', // Česká spořitelna
];Also the bank code is not |
|
Hi @maryo , thank your for the catch, i will update the comment (CSOB is the expected name). Also thank you for CS export. It is sad that we are unable to detect the bank :( |
|
I think it should be Česká spořitelna because the "alt" posting codes were previously set in Also according to https://www.csas.cz/static_internet/cs/Obchodni_informace-Produkty/Prime_bankovnictvi/Spolecne/Prilohy/ABO_format.pdf
BTW it is actually not a new problem caused by your MR. With your MR, you can at least pass an instance of In master branch there is no way. It is even not possible to override the constants with custom (I'd report it elsewhere or create a PR but the issues are closed and this PR partially solves the problem by providing the |
|
Thanks for the summary :) I would recommend using this (if you have multiple statements from different banks). When the account number is 973789052 then 5500 bank code is added. Other bank statements contains bank code so it works for me as needed :) return new BankAccountParser(bankAccountNumberToBankCodeMap: [
'973789052' => '5500',
]); |
|
@nedvajz any chance of merging? :) |
4 participants
I've found that bank account parsing is not working. I've used by real exports from my client.
At this moment I'm in proof of concept stage, code structure can change.
Commit d2380654fcf89722aff85f612b582de053a70a25 introduced bank number in bank account number but the format is incorrectly interpreted
This PR tries to construct it correct format if possible - real exports are needed.