wimverstuyf / php-coda-parser

PHP parser for Belgian CODA banking files
GNU Lesser General Public License v3.0
42 stars 25 forks source link

issue for transactions having different sequence number details #18

Open pdevemy opened 4 years ago

pdevemy commented 4 years ago

Hello,

I started yesterday to use your library, it works very well except for records using sequence number details. All the transactions are linked to the sequence numbers and the sequence number details are not taken into account.

so if I have in the coda file 4 transactions, the 2 first ones having the same sequence number but different sequence number details and the 2 other ones having different sequence numbers: 2100010000... 2200010000... 2300010000... 3100010000... 2100010001... 2200010001... 2300010001... 3100010001... 2100020000... 2200020000... 2300020000... 3100020000... 2100030000... 2200030000... 2300030000... 3100030000...

When I parse the file, only 3 transactions are found:

Are you already aware of this issue?

This evening I tried to quickly fix it by adding a transactionSequenceDetail to the Transaction class. Then in StatementParser class in function groupTransactions I use it to increment $idx. The result looks good but I still have to check if there is no side effect. Do you think it's a good way to fix it?

Best Regards,

Patrice Devemy

wimverstuyf commented 4 years ago

Hi Patrice

I can't see the full details of your example but in the information you provided I can only see 3 transactions (0001, 0002 and 0003) (so the parser should return 3 transactions). As far as I'm aware all information from lines with the same transactionnumber belong to the same transaction and thus should be concatenated. The sequence detail should not be exposed as it is only a technical detail.

If you can provide a complete example I could take a deeper look at it.

Wim

pdevemy commented 4 years ago

Wim,

Thanks for you answer.

I'll send you a file having the issue but before I have to anonymize data, I will try to do it this evening.

Yesterday, I read the CODA v2.6 document, especially the chapter 6 and 7 and I found this sentence that match the current issue I'm facing:

All "movement records" relating to a particular transaction have the same sequence number, but the detail number varies.

So indeed the transaction sequence number is correct but a transaction can contain multiple movements defined by a sequence number detail.

Patrice

.

pdevemy commented 4 years ago

Wim,

Please find attached the coda file, I stripped lines because there are more than 8000 movements in that file and data is anonymized.

example.txt

Let me know if you need more info.

Regards,

Patrice

wimverstuyf commented 4 years ago

Hi Patrice

Thanks for the example. I haven't yet come across this type of data in a coda-file. It does seem the multiple movements should each be categorized as a transaction (in the output). Do you know in which circumstances these kind of files are produced?

Can you provide the changes to the code you made? Maybe as a PR or a fork. Then I will take a look at it. Could you also add the example-file as a unittest? Thx.

Wim

pdevemy commented 4 years ago

Hello Wim,

This example is related to recurrent SEPA, a SEPA file is sent to the bank to request payment of monthly subscriptions. In the CODA the movements related to the SEPA file are grouped.

The changes that I made 2 days ago are not correct. I had a closer look to CODA files and to the documentation v2.6 yesterday. In chapter 5.5 next code and 5.6 link code, they are in position 126 and 128 of line types 21,22,23, 31, 32, 33. In the coming days, I will try to split movement based on those 2 fields.

I'll keep you informed and I will provide you a PR ou a fork as soon as I have something running.

Have a nice weekend,

Patrice

pdevemy commented 4 years ago

Wim,

I updated the example file, there was an error in line 3, I didn't update correctly the amount. I started to use values in position 126 and 128, it helps to define partially the next type of record.

I have also noticed that I have to take into account the "transaction type".

In the case of the example file: line 3 2100010000 is of type 1, it's the total amount of SEPA collected, the bank put on the bank account the total all the SEPA in one time, so it's the main transaction.

Amount as totalised by the customer; e.g. a file regrouping payments of wages or payments made to suppliers or a file regrouping collections for whic the customer is debited or credited with one single amount

The following records 210001xxxx detail the main transaction, it's the list of all the transactions that compose the main one. Regarding the documentation for transaction types, a transaction can have up to 2 levels of details (for type 2).

A solution to solve this issue is to add to the transaction object an array of transactions, so when a transaction is detailed by other transaction we add them to the array. I will try to implement it.

Regards,

Patrice

wimverstuyf commented 4 years ago

Hi Patrice

Great! A transaction containing a list of transactions (or maybe a new class "Movement"?) could be a good solution. If you can give it a try to implement this change I'll take a look at it.

Wim

pdevemy commented 4 years ago

Hi Wim,

Yes, a class Movement a good idea but it will have almost the same properties as the class Transaction. I will start with a Transaction class and then see if I have to specialize it I will try to work on it this weekend.

Are your test coda are real Coda file that you anonymized? I found unexpected next codes and link codes in the 5th one.

Patrice

wimverstuyf commented 4 years ago

The CODA-samples are anonymized (real) CODA files but of course it is possible an error has been added.

pdevemy commented 4 years ago

Hi Wim,

I think I have something more or less ok now. I split the transactions on the next & link code then I follow the normal process to create final transaction objects at the last moment the transaction details are move to their parent transaction. The parser should be backward compatible.

In the coming days, I will :

Regards,

Patrice

pdevemy commented 4 years ago

Wim,

I pushed my change in a fork: pdevemy/php-coda-parser

I still have to add tests.

I did some changes to sample5.cod and sample6.cod, next and link codes weren't correct. for sample 2 and 3, the test are not working because the merge message is seen as part of detailed transactions.

I will try to add tests this weekend.

Regards,

Patrice

wimverstuyf commented 4 years ago

Hi Patrice,

Looks good. I'll take a deeper look at it in the coming days. Some new tests would be welcome ;-).

Wim

wimverstuyf commented 4 years ago

Hi Patrice

I've taken a better look at it but I'm not really sure how the changes work out. Could you add some tests to verify the result? If you create a PR I'll comment where I'm not sure about the code.

Wim