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 support for the custom_info object as OrderRequest argument #9

Conversation

DaanDeSmedt
Copy link
Contributor

@DaanDeSmedt DaanDeSmedt commented Oct 20, 2023

Description

As per the documentation provided by MultiSafepay (https://docs.multisafepay.com/reference/createorder), the custom_info object allows the inclusion of extra order-related detail fields.

I've sourced the php-sdk but have not come across any indication of this feature.

To address this gap, I've taken the liberty in creating this PR to suggest some code changes.
Which addresses an approach for reported Support custom_info when creating a payment page/link #8

Type of change

  • New feature (non-breaking change which adds functionality)

Added Unit tests

Added basis unit tests to check correct implementation.

  • Added test testRequestOrderRequestWithCustomInfoon the test class OrderRequestTest.php
  • Added Testcase CustomInfoTest to validate setting & retrieving custom info based on the custom_1, custom_2 & custom_3 approach described in the docs.

@danielcivit
Copy link
Member

Hi @DaanDeSmedt

Thanks for submitting this contribution. We just assigned two developers as reviewers of this PR.
They will help and work to pass the tests before merging this PR.

@DaanDeSmedt
Copy link
Contributor Author

@Daniel-MultiSafepay ,

Thanks for the heads-up. 👍

I've fixed the Code sniffer / Check coding conventions on my added code.

I suppose some extra pair of eyes on the failing unit tests would be welcome, since these failing unit tests don't seem related to my additions(?).

Have a great day!

Copy link
Contributor

@mikededecker1 mikededecker1 left a comment

Choose a reason for hiding this comment

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

Hi @DaanDeSmedt

After reviewing your code we can confirm that the changes are working as intended. There is only an issue with the code sniffer. If you could solve that issue we can merge the PR.

Regarding the tests failing, this is due to not having certain payment methods available on your test account. I've added an internal ticket to resolve this issue.

So for the failing tests, you don't have to do anything at this moment

'custom_3' => $this->getCustom3(),
];
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Expected 1 newline at end of file; 0 found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mike-MultiSafepay , great feedback, thanks! Just pushed a fix for the code sniffer issue mentioned.

@danielcivit danielcivit merged commit b0ce0eb into MultiSafepay:master Oct 25, 2023
@danielcivit
Copy link
Member

Thanks for this contribution @DaanDeSmedt. !!

We just merged the PR into the master branch.

All tests are passing, but some of them fail here, on GitHub, because the API Key set as a secret of this repository is not properly configured. I will check and fix that in the next couple of days.

We will be happy to send you a present as proof of our appreciation for your contribution. Could you share with us your email address by sending us an email to [email protected] with a reference to this PR; so we can stay in contact and get your address, and details required?

Thanks again.

@DaanDeSmedt
Copy link
Contributor Author

You're welcome, and thank you for merging the PR into the master branch.

I appreciate your offer of a present as a token of appreciation for my contribution.
I'll send an email to [email protected], referencing this PR.

Great day!

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.

3 participants