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 base Subscribtion/recurring payments support #217

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 21 additions & 0 deletions payments/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,27 @@ def get_return_url(
return url + "?" + qs
return url

def autocomplete_with_subscription(self, payment):
"""
Complete the payment with subscription

If the provider uses workflow such that the payments are initiated from
implementer's side.
The users of django-payments will create a payment and call
Payment.autocomplete_with_subscription() right before the subscription end.

Throws RedirectNeeded if there is problem with the payment
that needs to be solved by user.
"""
raise NotImplementedError

def cancel_subscription(self, subscription):
"""
Cancel subscription
Used by providers, that use provider initiated cancellation workflow
"""
raise NotImplementedError

def capture(self, payment, amount=None):
raise NotImplementedError

Expand Down
91 changes: 91 additions & 0 deletions payments/models.py
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
Expand Down Expand Up @@ -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,
)
Comment on lines +44 to +51
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

But adding JSONField for additional fields might be a good idea.

Do you want to add those on this PR, or follow-up later?

Comment on lines +44 to +51

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
)
Comment on lines +52 to +59

Choose a reason for hiding this comment

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

Why not to call it variant (or payment_variant) as in the BasePayment? Is there a particular reason for this inconsistency? I mean, I do not have any good reason for having this consistent but provider usually refers to the particular instance of a Provider so it might be confusing...

Comment on lines +52 to +59

Choose a reason for hiding this comment

The 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 None-checking in the code (which you do not do).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Comment on lines +71 to +76
Copy link
Member

Choose a reason for hiding this comment

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

I guess kwargs here would be assigned to that JSONField...?

self.subscribtion_data = kwargs
Comment on lines +71 to +77

Choose a reason for hiding this comment

The 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 subscription_data should not be provided... I mean, I would bet that sooner or later users will complain about this being too aggressive or not enough flexible...


def get_period(self) -> int:
raise NotImplementedError

def get_unit(self) -> TimeUnit:
raise NotImplementedError
Comment on lines +79 to +83
Copy link

Choose a reason for hiding this comment

The 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):

Choose a reason for hiding this comment

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

Wouldn't it be handy to have a status field indicating whether the subscription is pending/active/canceled or whatever the payment gateways support?

"""
Cancel the subscription by provider
Used by providers, that use provider initiated subscription workflow
Implementer is responsible for cancelling the subscription model
Comment on lines +87 to +89
Copy link

Choose a reason for hiding this comment

The 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:

  1. Cancel the subscription by provider - is there any "by [something]" needed? Isn't "cancel the subscription" enough?
  2. Used by providers, that use provider initiated subscription workflow - cannot it potentially be used by anyone, if the provider supports it?
  3. Implementer is responsible for cancelling the subscription model


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.
Expand Down Expand Up @@ -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 +246 to +251
Copy link

Choose a reason for hiding this comment

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

django-payments refers to payment_details as to the payment handling view. But are they really the same? Would it make sense to call it just a CVV2 URL? Wouldn't provide extra flexibility to the implementations?

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
Comment on lines +261 to +262

Choose a reason for hiding this comment

The 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 is_recurring if get_subscription provides the same (or even more) information?


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)
Comment on lines +264 to +277

Choose a reason for hiding this comment

The 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 status (WAITING) only?


def capture(self, amount=None):
"""Capture a pre-authorized payment.

Expand Down