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 Mollie #976

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

jorisvergeer
Copy link

@jorisvergeer jorisvergeer commented Sep 20, 2021

While I was trying to add Mollie support, I found some bugs and other stuff I wanted to change.

Summarized in this PR:

  • Add support for Mollie
  • Fix bug where request_data and transaction_data is stored as array that gets corrupted when the checkout page is refreshed.
  • Change locale in .env
  • Don't overwrite .env everytime packages are updated or installed
  • Update packages

Fix bug where request_data and transaction_data is stored wrong
Change locale in .env
@quentincaffeino
Copy link
Contributor

quentincaffeino commented Sep 21, 2021

This is really great work, but I think this should be split into multiple prs because merging big changes is usually just pain. Also it is harder to track changes later.

Some changes as refactors or changing methods (warn -> warning) are not worth doing because this project doesn't have that much tests. Which means that repo owners would need to test these changes manually. This in result might decrease chance of your pr getting merged.

I would split your pr into two: bug fixes and new payment gw. Everything else's I would throw out for better. Or keep as a separate refactor pr if you feel that this is really needed.

Edit: Also this pr most probably won't be merged because it seems that you've run composer update (with composer v2, buy I might be wrong). And even if you've tested whole application by hand project maintainers need to find time to do the same which is counterproductive. Updating any package without testing application throughout might break it in the most unexpected places.

@justynpride
Copy link
Contributor

@jorisvergeer Should this create a new payment option in the accounts setting? I only get the standard three when working off your commit?

image

Please let me know if I have missed something. I've not done anything with different payment gateways yet, so this is new, and I'm looking to learn. I'd love there to be something for Braintree, which a client of mine uses.

@jorisvergeer
Copy link
Author

It should create a new one, but I only added it to the existing code that creates the required rows in the database.
I saw that there was one type in one of the seed script. Although I am not sure when and if they are used.

@jorisvergeer
Copy link
Author

@jorisvergeer Should this create a new payment option in the accounts setting? I only get the standard three when working off your commit?

image

Please let me know if I have missed something. I've not done anything with different payment gateways yet, so this is new, and I'm looking to learn. I'd love there to be something for Braintree, which a client of mine uses.

Adding Mollie was basically the same as dummy with some small tweaks. I did not have to change big parts.

@quentincaffeino
Copy link
Contributor

@jorisvergeer Should this create a new payment option in the accounts setting? I only get the standard three when working off your commit?

@justynpride, have you run installation process after applying this pr? Seeds which create payment gateways are only run on installation (or manually).

@justynpride
Copy link
Contributor

justynpride commented Sep 21, 2021

@quentincaffeino You are correct. I've rerun the installation and the seeds have come through. Thanks!

@johannac johannac added the enhancement Improve or enhance an existing feature label Sep 22, 2021
@rozenlicht
Copy link

Is there anything blocking this from getting merged? I'm a new user of Attendize and happy to contribute to the project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve or enhance an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants