-
-
Notifications
You must be signed in to change notification settings - Fork 282
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 base Subscribtion/recurring payments support #217
base: main
Are you sure you want to change the base?
Changes from 5 commits
5f705a6
67496a8
8b7288f
c1e2f00
4f9df42
e4585a5
9e5e0c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
from __future__ import annotations | ||
|
||
import enum | ||
import json | ||
from typing import Iterable | ||
from uuid import uuid4 | ||
|
@@ -39,6 +40,63 @@ def __setattr__(self, key, value): | |
return None | ||
|
||
|
||
class BaseSubscription(models.Model): | ||
token = models.CharField( | ||
_("subscription token/id"), | ||
help_text=_("Token/id used to identify subscription by provider"), | ||
max_length=255, | ||
default=None, | ||
null=True, | ||
blank=True, | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any particular reason for allowing blank/null values? I mean, allowing this usually implies a lot of tedious None-checking in the code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But could we expect, that all providers will have tokens/IDs? |
||
payment_provider = models.CharField( | ||
_("payment provider"), | ||
help_text=_("Provider variant, that will be used for payment renewal"), | ||
max_length=255, | ||
default=None, | ||
null=True, | ||
blank=True, | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not to call it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, is there any particular reason for allowing blank/null values? I mean, allowing this usually implies a lot of tedious There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here probably no. |
||
subscribtion_data = models.JSONField( | ||
_("subscription data"), | ||
help_text=_("Provider-specific data associated with subscription"), | ||
default=dict, | ||
) | ||
|
||
class TimeUnit(enum.Enum): | ||
year = "year" | ||
month = "month" | ||
day = "day" | ||
|
||
def set_recurrence(self, token: str, **kwargs): | ||
""" | ||
Sets token and other values associated with subscription recurrence | ||
Kwargs can contain provider-specific values | ||
""" | ||
self.token = token | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess |
||
self.subscribtion_data = kwargs | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any particular reason for using this method if one can modify the attributes directly? I mean, if yes, would it make sense to make the attributes private? Also, if yes, I am not sure if some extra care regarding (un)intentional merging/overwriting |
||
|
||
def get_period(self) -> int: | ||
raise NotImplementedError | ||
|
||
def get_unit(self) -> TimeUnit: | ||
raise NotImplementedError | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about leaving these up to the subclasses? I mean, I can imagine that this interface might be too flexible for someone (e.g. PayU uses only days so it might be ambiguous how to convert from e.g. years to days) and not enough flexible for others (irregular subscriptions, minute-based subscriptions, day-in-month subscriptions, ...) |
||
|
||
def cancel(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it be handy to have a |
||
""" | ||
Cancel the subscription by provider | ||
Used by providers, that use provider initiated subscription workflow | ||
Implementer is responsible for cancelling the subscription model | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please clarify this docstring a bit? I am not sure if it really says what it is supposed to do. I mean, what do you mean by:
|
||
|
||
Raises PaymentError if the cancellation didn't pass through | ||
""" | ||
provider = provider_factory(self.variant) | ||
provider.cancel_subscription(self) | ||
|
||
class Meta: | ||
abstract = True | ||
|
||
|
||
class BasePayment(models.Model): | ||
""" | ||
Represents a single transaction. Each instance has one or more PaymentItem. | ||
|
@@ -185,6 +243,39 @@ def get_success_url(self) -> str: | |
def get_process_url(self) -> str: | ||
return reverse("process_payment", kwargs={"token": self.token}) | ||
|
||
def get_payment_url(self) -> str: | ||
""" | ||
Get the url the view that handles the payment | ||
(payment_details() in documentation) | ||
For now used only by PayU provider to redirect users back to CVV2 form | ||
""" | ||
Comment on lines
+217
to
+222
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. django-payments refers to |
||
raise NotImplementedError | ||
|
||
def get_subscription(self) -> BaseSubscription | None: | ||
""" | ||
Returns subscription object associated with this payment | ||
or None if the payment is not recurring | ||
""" | ||
return None | ||
|
||
def is_recurring(self) -> bool: | ||
return self.get_subscription() is not None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you think of a use case when someone would want to overload this method? Otherwise, why would one ever call |
||
|
||
def autocomplete_with_subscription(self): | ||
""" | ||
Complete the payment with subscription | ||
|
||
If the provider uses workflow such that the payments are initiated from | ||
implementer's side. | ||
Call this function right before the subscription end to | ||
make a new subscription payment. | ||
|
||
Throws RedirectNeeded if there is problem with the payment | ||
that needs to be solved by user | ||
""" | ||
provider = provider_factory(self.variant) | ||
provider.autocomplete_with_subscription(self) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, and... Would it make sense to check if the payment has an allowed |
||
|
||
def capture(self, amount=None): | ||
"""Capture a pre-authorized payment. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that (in previous versions) you needed additional fields to store the provider-specific data. Did you move those to your subclass of this model? What do you think of this being a
JSONField
so each provider can store all the data it may need?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, that some kind of token or payment identification would be needed for most implementation.
I used this is for Subscription ID in case of PayPal and for Card token in case of PayU. Accessing that through JSONField would be quite unflexible.
But adding JSONField for additional fields might be a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to add those on this PR, or follow-up later?