Skip to content
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

issue for transactions having different sequence number details #18

Open
pdevemy opened this issue Apr 16, 2020 · 13 comments
Open

issue for transactions having different sequence number details #18

pdevemy opened this issue Apr 16, 2020 · 13 comments

Comments

@pdevemy
Copy link

pdevemy commented Apr 16, 2020

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:

  • the transaction number 00010000 having in his message a concatenation of the info of all the transaction(s) 0001xxxx.
  • 2 transactions for sequence number 0002 and 0003.

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
Copy link
Owner

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
Copy link
Author

pdevemy commented Apr 16, 2020

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
Copy link
Author

pdevemy commented Apr 16, 2020

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
Copy link
Owner

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
Copy link
Author

pdevemy commented Apr 17, 2020

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
Copy link
Author

pdevemy commented Apr 17, 2020

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
Copy link
Owner

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
Copy link
Author

pdevemy commented Apr 18, 2020

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
Copy link
Owner

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

@pdevemy
Copy link
Author

pdevemy commented Apr 21, 2020

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 :

  • check if I don't find corner cases,
  • review the code,
  • prepare unit test
  • provide you the modified code.

Regards,

Patrice

@pdevemy
Copy link
Author

pdevemy commented Apr 24, 2020

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
Copy link
Owner

Hi Patrice,

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

Wim

@wimverstuyf
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants