search

viafintech / camt_parser

A basic parser for camt.052, camt.053 and camt.054 files
MIT License
20 stars 24 forks source link

Weird behavior for <AmtDtls> with multiple <*Amt>s #25

Closed maxhollmann closed 5 years ago

maxhollmann commented 5 years ago

This commit introduced a bug where a <AmtDtls> with multiple kinds of <*Amt> leads Transaction#parse_currency and #parse_amount to concatenate the values of all <*Amt>s.

E.g., this XML leads to the currency being EUREUREUR and the amount 0.03 (== "0.030.040.03".to_f):

<AmtDtls>
  <InstdAmt>
    <Amt Ccy="EUR">0.03</Amt>
  </InstdAmt>
  <TxAmt>
    <Amt Ccy="EUR">0.04</Amt>
  </TxAmt>
  <PrtryAmt>
    <Tp>IBS</Tp>
    <Amt Ccy="EUR">0.03</Amt>
  </PrtryAmt>
</AmtDtls>
tobischo commented 5 years ago

Oh, damn. Thank you for reporting the issue, I will take a look.

tobischo commented 5 years ago

Fix released as v2.6.3

maxhollmann commented 5 years ago

Thanks for the quick reaction! This changes the behavior from 2.6.1 though, where it always used the TxAmt. I'm assuming the Amts don't have a specified order, so the current behavior is random.

tobischo commented 5 years ago

That's true, but that was already changed in 2.6.2, so 2.6.3 only fixes the error introduced by that change.

The previous behaviour was already "random" also, as it would take the upper level Amt if that was provided. That part hasn't changed.

In 2.6.2/2.6.3 it was changed that also other amounts may be considered and in fact TxAmt may not be the first. While the different amounts are optional, the order should always be the same. In XML schema as far as I am aware you'd otherwise have to go through some lengths to allow a changed order. Therefore it should be in order InstdAmt, TxAmt, CntrValAmt, AnncdPstngAmt, PrtryAmt. Unfortunately not every bank has this implemented the same way, but for the same bank it generally also behaves consistently.

The change in 2.6.2/2.6.3 reflects that it will always take the first amount there. If you need a very specific amount and currency, I would suggest adding specific methods for that, which then also wouldn't consider the upper level Amt