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

Numerous changes in the Parsian driver to make it work #50

Merged
merged 1 commit into from Jun 4, 2023

Conversation

mhemrg
Copy link
Collaborator

@mhemrg mhemrg commented Jun 4, 2023

I've had access to real Parsian credentials and done these changes based on that.

And also noticed that soap usage throughout the project has some issues. For instance, we should add the Async suffix to all method calls.

@Keivan-sf
Copy link
Collaborator

Thanks for your contribution.
The project been untouched for a long time since we were amid of updating to v2. So there might be a lot of bugs in the code base for now.
I'm ready to get back to it, see where we left it and take over the job here.

@alitnk do you have free time for it or should I proceed on my own?

@Keivan-sf
Copy link
Collaborator

Also I will see to your PR @mhemrg , Meanwhile since you've worked with actual Parsian API, can you checkout our Parsian driver and open up issues for the problems you see? That'd be fantastic for us since we couldn't test it irl.

Also please report the issue about the soup agent in issues section and we will take it from there.

Cheers

@mhemrg
Copy link
Collaborator Author

mhemrg commented Jun 4, 2023

Thanks for your quick response @Keivan-sf.
I didn't know you're working on a v2 branch.

The same issues with Parsian driver exist in the v2 branch as well. Do you want me to apply these changes to the v2 branch?

@alitnk
Copy link
Owner

alitnk commented Jun 4, 2023

Thank you for this @mhemrg, and @Keivan-sf for responding. Really nice to test these drivers with actual credentials. We would have gone for it but the process of even getting a PDF documentation is a hassle, let alone the credentials.

And also noticed that soap usage throughout the project has some issues. For instance, we should add the Async suffix to all method calls.

Great call, soap is one of those things that remained untested. A follow-up PR adding the Async suffix to all drivers would be great. Let me know if you guys wanna do it, otherwise I'll get on that.

Do you want me to apply these changes to the v2 branch?

That would be nice. We can also just merge this and resolve it when merging v2 to main.

Also we might as well just get v2 out of the door. I think just gotta:

  • If we're going with new errors, here are some of my thoughts:
    • 1 Error class (MonopayError, or PayError perhaps) to keep things simple. We can't really put errors into different categories and also cover 10 different drivers with different conventions - We could have an additional error for config errors (thrown by zod), or just throw standard zod errors and keep it simple.
    • The default message field, and a userFriendlyMessage (open to alternative names) that exposes a message that is intended for the end user to see that can default to just "Internal Error" for most cases where user isn't concerned.
  • v2 docs
  • lmk if you think i'm missing anything

P.S. If you're interested in contributing @mhemrg, I can add you as a collaborator. Having to work from a fork can get annoying.

@mhemrg
Copy link
Collaborator Author

mhemrg commented Jun 4, 2023

Awesome. I would love to join as a contributor. I have access to a few more gateway credentials and plan to try them all with Monopay's drivers. I will submit more PRs for that and the soap stuff.

And, it'd be great if you merge this on the main (v1) branch for now.

@alitnk alitnk merged commit e417a4f into alitnk:main Jun 4, 2023
@alitnk
Copy link
Owner

alitnk commented Jun 4, 2023

Invited you.
I'll probably have to move this repo into an org at some point but this'll do for now.

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