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

(WIP) PayPal subscribtions #274

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

PetrDlouhy
Copy link
Contributor

This is partially working implementation of PayPal subscriptions. I was able to manage subscription creation, getting state and cancelling. This is based subscription support drafted in #217.

Although I was able to successfully manage communication with PayPal, I have got stuck and I don't know how to continue at this point. The main problems are:

PayPal requires setting plan_id and having Plan created at the time of creating the Subscription.
This is not required when using Subscription button (like django-paypal does). It brings in whole lot of problems - the price can't be changed per user, it requires implementer to set up the Plans on PayPal and maintain consistency with server settings (especially if django-payments are used with multiple providers).

Getting notification of the payments
I don't know how to capture the recurring payments. With PayPal Subscription buttons PayPal sends notification about the payment. I didn't find how to do that with API. There also might be possibility to check the Subscriptions through the API and check their (possibly changed) expiration.

@PetrDlouhy PetrDlouhy mentioned this pull request Oct 27, 2021
@codecov
Copy link

codecov bot commented Oct 27, 2021

Codecov Report

Merging #274 (b8360f6) into master (10b749a) will decrease coverage by 1.33%.
The diff coverage is 59.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #274      +/-   ##
==========================================
- Coverage   79.92%   78.58%   -1.34%     
==========================================
  Files          28       28              
  Lines        1783     1868      +85     
  Branches      212      223      +11     
==========================================
+ Hits         1425     1468      +43     
- Misses        251      283      +32     
- Partials      107      117      +10     
Impacted Files Coverage Δ
payments/paypal/__init__.py 75.41% <52.94%> (-8.67%) ⬇️
payments/models.py 75.65% <71.42%> (-0.35%) ⬇️
payments/core.py 87.69% <100.00%> (+0.39%) ⬆️

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 463f137...b8360f6. Read the comment docs.

@PetrDlouhy
Copy link
Contributor Author

Now I was at least able to override the plan price.
Although it is still not optimal, as it bothers user with creation of the plans on PayPal.

@PetrDlouhy
Copy link
Contributor Author

I have re-implemented this in django-plans-paypal which uses subscription button from django-paypal.
I have no time nor motivation to finish this PR, but I am leaving it open for volunteers to finish this.

Copy link
Member

@WhyNotHugo WhyNotHugo left a comment

Choose a reason for hiding this comment

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

Sorry for the slowness in feedback here; every time I looked at this PR I couldn't quite figure out a few details... I've questions about it below:

@@ -36,6 +38,60 @@ def __setattr__(self, key, value):
self._payment.extra_data = json.dumps(data)


class BaseSubscription(models.Model):
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, applications need to provide a subclass of this model, correct?

Can you add a brief docstring mentioning this?

Comment on lines +122 to +129
def autocomplete_with_subscription(self, payment):
"""
Complete the payment with subscription
Used by providers, that use server initiated subscription workflow

Throws RedirectNeeded if there is problem with the payment that needs to be solved by user
"""
raise NotImplementedError()
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what this method should do.

What does autocomplete mean in this context?

Comment on lines +74 to +75
def get_period(self) -> int:
raise NotImplementedError()
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this is supposed to return either.

Comment on lines +59 to +62
class TimeUnit(enum.Enum):
year = "year"
month = "month"
day = "day"
Copy link
Member

Choose a reason for hiding this comment

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

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.

2 participants