-
-
Notifications
You must be signed in to change notification settings - Fork 517
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
8.0 FIX bug #111: crash when generating SEPA credit transfer file #112
Conversation
Note that, without this patch, we CANNOT EVEN GENERATE A SEPA CREDIT TRANSFER FILE on v8 !!! We shouldn't be proud of that, so let's patch this rapidly ! |
👍 |
This is delicate the revert is not a full revert of This commit will allows the generate file but will also probably break branch code that depends on it like the 'ES' one. That probably means the fields is required by many module and should be declared else where. I did not find the bug related to the commit so I want to have @pedrobaesa advice on this one. An other point is that this modification greenify Travis. That odd and maybe means a test case is missing. I would be in favour of merging it. But I would like in parallel to have a related bug report with corrective steps or a confirmation that code is fine as is. |
As I read it, the version before 791370e was broken in the same way for ES but for ES only. |
Yeah, that's correct. This is inherited code from 7.0 that I assume correct and make it general taking "partial information". I have to investigate if SEPA creditor identifier is also required for SEPA credit transfer. If so, solution is to move the field from account_direct_debit to account_banking_pain_base. I'll tell you ASAP. |
@pedrobaeza Could we move forward on this ? This bug is really critical, because we can't generate a SEPA credit transfer on v8... |
I agree with @alexis-via, we must merge this now, and provide a more complete in another PR if necessary. We can't leave SEPA credit transfer in a broken state in this repo. If nobody objects, I'll merge tomorrow. |
Sorry for the delay, but I query one expert, and I was waiting for the answer. Let me check tonight by myself, and I'll give a solution. |
Thanks, Pedro! |
As I can see in https://businessbanking.bankofireland.com/fs/doc/wysiwyg/sepa-credit-transfer-pain-001-001-03-xml-file-structure-july-2013.pdf (page 8), section is also mandatory for credit transfer, so we have to make it another way. I'm going to prepare another PR with the following changes:
I expect to have it tonight. Stay tuned. |
I have made #131 for solving properly this issue. Please check. |
Dear All, I am a new joiner, My name is David. Please correct me if this comment is not at the right place. https://www.febelfin.be/en/standards-distance-banking Perhaps is my understanding not complete, then please let me know |
@pedrobaeza Should we close this one? @dgouthiere Thanks for your input, the code was patched have you tried with the last version? I don't know if your remark was taken into account though. |
Yeah, I close it |
Please refer to the bug report #111 for the explainations. In fact, the commit just goes back to the code of v7.0, which worked well.