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

Add CBIBdySDDReq_00_01_00 SEPA format #664

Merged
merged 9 commits into from
Apr 13, 2023
Merged

Conversation

masetto
Copy link
Contributor

@masetto masetto commented Apr 13, 2023

The CBIBdySDDReq is a XML format used by Italian banks and requires a new setting field called "CUC-code".

@bjendres bjendres added this to the 1.9.x milestone Apr 13, 2023
@bjendres bjendres self-requested a review April 13, 2023 07:44
Copy link
Member

@bjendres bjendres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks pretty solid, thank you!

I'd only ask you to make it (even) more clear in the UI that this value is only used in this file format (so far), so we don't get a lot of questions about what to put there from 99% of the users that don't need it. Maybe a placeholder text saying "only for CBID format" or something would be enough. What do you think?

And, if you're feeling like going the extra mile today, maybe add a unit test for this file format.

@masetto
Copy link
Contributor Author

masetto commented Apr 13, 2023

In the CUC field I removed the default value 'SEPATEST' and added a clarification in the label. Is it ok for you?
I do not know how to add a placeholder....

For unit tests please give me a starting point to take inspiration from?

@bjendres bjendres changed the base branch from master to issue/664 April 13, 2023 08:51
@bjendres
Copy link
Member

For unit tests please give me a starting point to take inspiration from?

You're right. I thought we already had some, but I think I was confusing that with another project. I created #665 to not forget about this. But I don't expect you to do that yourself, unless you're really motivated.

I think the next step would be for me to try this out locally and see if I can spot any more issues.

If everything goes well, I'll merge and tag a 1.8-alpha version for you.

@bjendres bjendres merged commit d00a2db into Project60:issue/664 Apr 13, 2023
bjendres added a commit that referenced this pull request Apr 13, 2023
@bjendres
Copy link
Member

@masetto I have merged your PR and tagged it as 1.9-alpha1.

Could you please confirm that this works? Specifically in the "newly installed" and the "upgrade from 1.8.x" scenarios?

@masetto
Copy link
Contributor Author

masetto commented Apr 13, 2023

@bjendres I confirm that the upgrade from version 1.8.0 to 1.9-alpha1 works well.

@bjendres
Copy link
Member

@bjendres I confirm that the upgrade from version 1.8.0 to 1.9-alpha1 works well.

Great. And your new file format works, too - right?

@masetto
Copy link
Contributor Author

masetto commented Apr 13, 2023

of course 😃 we have already sent contributions to the bank.

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

Successfully merging this pull request may close these issues.

2 participants