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 external Payments (BankTransfer, DirectPayment) #152

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

devkral
Copy link
Contributor

@devkral devkral commented Oct 7, 2017

Sometimes it is useful to have placeholders for certain payments (BankTransfer and manual cashing process).
I provide here two Providers for this purpose:
DirectPaymentProvider: versatile Provider, for e.g. voucher systems or cash on delivery.
BankTransferProvider: for normal bank transfers. Some other logic has to validate payments

This pull request depends on the pull requests: #148, #149

@codecov-io
Copy link

codecov-io commented Oct 7, 2017

Codecov Report

Merging #152 into master will increase coverage by 1.83%.
The diff coverage is 87.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #152      +/-   ##
==========================================
+ Coverage   79.13%   80.97%   +1.83%     
==========================================
  Files          27       30       +3     
  Lines        1649     1819     +170     
  Branches      190      204      +14     
==========================================
+ Hits         1305     1473     +168     
+ Misses        245      239       -6     
- Partials       99      107       +8
Impacted Files Coverage Δ
payments/sofort/__init__.py 87.71% <ø> (-1.17%) ⬇️
payments/braintree/forms.py 93.1% <100%> (+0.51%) ⬆️
payments/stripe/__init__.py 60% <100%> (ø) ⬆️
payments/stripe/forms.py 96.92% <100%> (+0.09%) ⬆️
payments/paypal/__init__.py 84.58% <100%> (ø) ⬆️
payments/testcommon.py 100% <100%> (ø)
payments/dummy/__init__.py 86.79% <100%> (ø) ⬆️
payments/cybersource/__init__.py 66.56% <100%> (+0.1%) ⬆️
payments/authorizenet/__init__.py 86.11% <100%> (+6.56%) ⬆️
payments/core.py 87.69% <100%> (ø) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49c93aa...b613ba0. Read the comment docs.

@iraycd
Copy link

iraycd commented Oct 20, 2017

I have been waiting for this. @devkral thank you.

@xponrails
Copy link

@devkral , great job. Can you provide an external repository with this PR yet merged in the latest version of django_payments so I can clone it as is and use in my saleor project?
I would like to wait an "official" merge by mirumee but I don't think it happens in the next few days :-)

@devkral
Copy link
Contributor Author

devkral commented Mar 2, 2018

the state is also very unsatisfying for me. I solved all problems but no feedback (may I changed too much). I can do and think about a fork.

@patrys
Copy link
Contributor

patrys commented Mar 2, 2018

Would you mind submitting the API changes as a separate pull request? We generally encourage third-party payment gateway to release separate packages (as we usually have no way to test and maintain them) but it seems that you've also refactored parts of the existing API which we'll be happy to review.

See https://github.com/esistgut/django-payments-bnlepos for an example of a payment implementation that lives in a separate package.

@devkral
Copy link
Contributor Author

devkral commented Mar 2, 2018

I really would like to but I depend on some pull requests which were not integrated:
see devkral in prs

@devkral
Copy link
Contributor Author

devkral commented Mar 3, 2018

I started my fork. I think it is the best thing because I can eliminate bad design decisions this way.
@patrys I can understand your decision as you represent a company. You want most probably not so many big changes to a system which is in production.
anyway here the link:
https://github.com/devkral/web-payments-connector

@devkral
Copy link
Contributor Author

devkral commented Mar 3, 2018

@patrys I pushed the PRs I required separately and waited 6 months now with no response.

@devkral
Copy link
Contributor Author

devkral commented Mar 3, 2018

And anyway the changes I would really like to integrate are too heavy weight. It is not realistic that you will ever integrate them (breaking api into small chunks, renaming payments to web_payments because of name conflicts, non-django framework support)

@zchking
Copy link

zchking commented Apr 25, 2018

HI, any update?

@devkral
Copy link
Contributor Author

devkral commented Apr 25, 2018

yes, integration will unlikely happen, so I put all my efforts into my fork (see above), which is stable now.
Still need to port backends.

@patrys
Copy link
Contributor

patrys commented Apr 25, 2018

Just to reiterate: there is no reason for all backends to live in this repository. The code was built in a way that makes loading third party backends no different from loading the first party ones. You don't need to fork to maintain a custom backend. If the core needs to change for a backend to work then we're looking forward to patches but we can't possibly take over maintenance of all possible bank and payment gateway integrations as we're only humans, I hope that does not surprise anyone 😄

@devkral
Copy link
Contributor Author

devkral commented Apr 26, 2018

that's absolutely true but I was too headstrong with my ideas :).
And anyway, flask support and a test project to develop backends with is cool.

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.

6 participants