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

Fix "null" in Ctry, Street and address #64

Merged
merged 1 commit into from
Dec 19, 2023
Merged

Conversation

FreedomNX
Copy link

Fix error "null" in Ctry, Street and Address for Credit.

@FreedomNX FreedomNX changed the title Update sepa.js Fix nu Dec 4, 2023
@FreedomNX FreedomNX changed the title Fix nu Fix "null" in Ctry, Street and address Dec 4, 2023
@lippertto
Copy link
Contributor

Hi @FreedomNX , do you think you can add a test to fix that the behavior for the future? There are already some examples with XPath expressions which you can adapt to your needs.

@FreedomNX
Copy link
Author

I did.

lib/sepa.js Outdated
@@ -598,7 +598,11 @@

if (this[pullFrom + 'Street'] && this[pullFrom + 'City'] && this[pullFrom + 'Country']) {
var pstl = n(reciever, 'PstlAdr');
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like an incomplete merge. Can you resolve this?

Copy link
Author

Choose a reason for hiding this comment

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

Oups, i fixed it.

@kewisch
Copy link
Owner

kewisch commented Dec 8, 2023

@lippertto Thanks for taking an initial look! Would you like to go ahead and complete the review?

Copy link
Contributor

@lippertto lippertto left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests. I have just some minor comments on the test setup. If you have time, it would be great if you could improve the test helpers.

lib/sepa.test.js Outdated

test('ctry and address field not null', () => {
// GIVEN
const doc = validTransferDocument({debtorId: 'FR72ZZZ123456', debtorName: 'debtor-name'});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove debtorId and debtorName here? The values are not relevant to the behavior.
I.e. write validTransferDocument({})

lib/sepa.test.js Outdated
// GIVEN
const doc = validTransferDocument({debtorId: 'FR72ZZZ123456', debtorName: 'debtor-name'});

let info = doc._paymentInfo[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you extend the functions validTransferDocument and addTransaction to take the debtor and creditor values. This will make the test a bit more concise but more importantly will avoid using the private fields _paymentInfo and _payments .

lib/sepa.test.js Outdated
@@ -51,13 +59,22 @@ describe('xml generation for transfer documents', () => {
info.debtorId = debtorId;
info.requestedExecutionDate = new Date();

if(debtorCountry) info.debtorCountry = debtorCountry;
Copy link
Contributor

Choose a reason for hiding this comment

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

The if statements are not necessary

Copy link
Contributor

@lippertto lippertto left a comment

Choose a reason for hiding this comment

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

lgtm

@lippertto
Copy link
Contributor

@kewisch , the PR looks good for me. Can you run the pipeline and merge the branch?

@lippertto
Copy link
Contributor

@kewisch - any thoughts on the PR? 😃

@lippertto
Copy link
Contributor

Friendly reminder @kewisch

@kewisch kewisch merged commit 110399e into kewisch:main Dec 19, 2023
2 checks passed
@kewisch
Copy link
Owner

kewisch commented Dec 19, 2023

Thanks for the review, much appreciated! I've been off the past week, apologies for the delay.

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

Successfully merging this pull request may close these issues.

None yet

3 participants