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 7 commits into
base: main
Choose a base branch
from

Conversation

PetrDlouhy
Copy link
Contributor

Changes of Payment model, that are required to implement PayU backend and recurring payments.

I could implement these changes through some mixin mandatory only for PayU backend, but I think, that those methods might be common to more backends and it would be highly usefull to have same implementation for all backends. I am willing to rewrite them bit more general shape, if requested.

@codecov
Copy link

codecov bot commented Apr 6, 2020

Codecov Report

Attention: Patch coverage is 75.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 78.37%. Comparing base (8303e09) to head (20bdcf2).
Report is 2 commits behind head on main.

Current head 20bdcf2 differs from pull request most recent head 9e5e0c0

Please upload reports for the commit 9e5e0c0 to get more accurate results.

Files Patch % Lines
payments/models.py 73.07% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #217      +/-   ##
==========================================
- Coverage   78.42%   78.37%   -0.05%     
==========================================
  Files          29       29              
  Lines        1979     2007      +28     
  Branches      244      244              
==========================================
+ Hits         1552     1573      +21     
- Misses        310      317       +7     
  Partials      117      117              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 147 to 172
def get_user_email(self):
""" Get user email """
try:
return self.get_user().email
except AttributeError:
return None

def get_user_first_name(self):
"""
Get user first name
Used only by PayU provider for now
"""
try:
return self.get_user().first_name
except AttributeError:
return None

def get_user_last_name(self):
"""
Get user last name
Used only by PayU provider for now
"""
try:
return self.get_user().last_name
except AttributeError:
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

This ties the package to a particular shape of the user object, it would not work in at least some of our projects. Could those fields not be extracted from billing information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the implementations for the most common case with intention, that users will override it in any other cases.

The billing information is info about the merchant, not the user, which is required by PayU (or PayU will ask that information in next step, which degrades UX)

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 maybe it would be appropriate to throw NotImplementedError instead of returning None and write some note to the comments about thet.

@PetrDlouhy
Copy link
Contributor Author

Hi @patrys, I rebased this to current master, and the methods are throwing NotImplementedError, if they can't figure out the variables from user.

Can you please give a new review? I think, it would be handy to unify these methods among providers.

@PetrDlouhy PetrDlouhy mentioned this pull request Nov 17, 2020
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.

I've a few questions to further understand the needs behind this.

@@ -135,6 +135,71 @@ 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):
Copy link
Member

Choose a reason for hiding this comment

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

You can raise RedirectNeeded in you provider code. Any application using this lib should catch these and redirect as needed.

Is there a reason that won't work for you?

"""
raise NotImplementedError()

def get_user(self):
Copy link
Member

Choose a reason for hiding this comment

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

What's this supposed to return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is simple helper method used by the other get_user_* methods, so it is the only method, that needs to be overridden in case of the Django django.contrib.auth.models.User model.

Copy link
Member

Choose a reason for hiding this comment

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

You're assuming that all applications have a User for each payment made.

I'm not a fan of imposing such a big restriction, especially without the need for one. You probably want a user in your own Payment instance, not in the abstract one in this package.

""" Get the user asociated with this payment """
raise NotImplementedError()

def get_user_email(self):
Copy link
Member

Choose a reason for hiding this comment

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

What's wrong with billing_email? What's expected of this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The billing_* fields hold information about the issuer (Me), PayU (and potentially other providers) needs information about the user.

Copy link
Member

Choose a reason for hiding this comment

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

The billing_ fields all hold billing details, that is: the details of the person paying (this is what payment providers usually ask for, and the thing that actually varies on a per-payment basis).

This is the assumption made for other payment implementations, so it's the assumption that any third-party providers should make two.

Using billing_ fields as something elsemeans that applications cannot use PayU and another provider, since the give different semantic meaning to the same fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, this is something, I got totally wrong. Sorry for loss of your time and thank you for clearing that out.
I used this for storing issuer info when I was implementing PayPal (which doesn't use it at all), and then continued with the wrong implementation for PayU and recurring payments.

Maybe it would be useful to put some info about those fields into docs, I will try to write something.

payments/models.py Outdated Show resolved Hide resolved
except AttributeError:
raise NotImplementedError()

def get_user_last_name(self):
Copy link
Member

Choose a reason for hiding this comment

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

Ditto billing_last_name.

payments/models.py Outdated Show resolved Hide resolved
@PetrDlouhy
Copy link
Contributor Author

Here is sample implementation of the methods that connects django-payments to django-plans: https://github.com/PetrDlouhy/django-plans-payments/blob/feature/recurring-payments/plans_payments/models.py

@PetrDlouhy PetrDlouhy force-pushed the model-payu branch 2 times, most recently from 6e63114 to b6fad03 Compare October 23, 2021 04:25
@PetrDlouhy
Copy link
Contributor Author

@WhyNotHugo @patrys Recently I was exploring PayPal subscription, which has completely different workflow (PayU payments are initiated from server, PayPal from the provider side).

I have reworked this completely based on your comments and also I tried to make this compatible with the provider initiated subscription, so it could be used for providers like PayPal.

This is still more proposal than done think, so I am open to suggestions how to make the whole recurring interface better.

@PetrDlouhy PetrDlouhy force-pushed the model-payu branch 5 times, most recently from 244c031 to 6bd1ef1 Compare October 25, 2021 10:08
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.

Apologies for the delay, each time I re-reviewed this I had different questions.

I think this looks good, I only have a few doubts on the current implementation...

payments/core.py Outdated
Comment on lines 122 to 158
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 fully understand what this function is supposed to do or return. I'm also thinking this as a "person implementing a Payment Provider subclass", and I wouldn't know what logic is expected here.

What do you mean by server initiated here? The payment provider's server, or the current application? #274 has answered this question for me. Maybe clarify the docstring a bit so it's more obvious?

Copy link
Contributor

@radekholy24 radekholy24 Apr 25, 2024

Choose a reason for hiding this comment

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

+1

It took me several hours to understand what the function is supposed to do. I guess that django-payments rather refers to this as to "making a payment". I would even not hesitate to call it pay_with_subscription() (ditto for the BasePayment class).

To clarify the docstring even more, I would say that this method is used by payments not providers and that it is used in an application-initiated workflow rather than in server-initiated workflow as it is not clear what server is meant (I believe I have seen somewhere that django-payments refers to the users of this library as to applications). (ditto for cancel_subscription and BasePayment class)

Comment on lines 42 to 51
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,
)
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?

payments/models.py Outdated Show resolved Hide resolved
Comment on lines 210 to 259
def get_subscription(self) -> Optional[BaseSubscription]:
"""
Returns subscription object associated with this payment
or None if the payment is not recurring
"""
return None
Copy link
Member

Choose a reason for hiding this comment

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

Do you think that using a foreign key to settings.PAYMENT_SUBSCRIPTION_MODEL makes sense, or would that be more trouble that what it's worth?

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 this enables more loose implementation.

I am using django-plans to which I added the recurring functionality with RecurringUserPlan model. Now I can do it without any ties to django-payments (it works even if the model is not based on BaseSubscription, it just has the same fields).

I am not sure, what would the foreign key would bring.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would go even further and change the autocomplete_with_subscription's signature to autocomplete_with_subscription(self, subscription). Then we would not need get_subscription (and is_recurring) at all and enable implementations to couple payments and subscriptions any way they want.

payments/core.py Outdated Show resolved Hide resolved
Comment on lines 42 to 51
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,
)
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 64 to 76
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
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...?

Comment on lines 210 to 259
def get_subscription(self) -> Optional[BaseSubscription]:
"""
Returns subscription object associated with this payment
or None if the payment is not recurring
"""
return None
Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

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.

Makes sense to me.

@PetrDlouhy PetrDlouhy force-pushed the model-payu branch 2 times, most recently from ac776ef to 97ba417 Compare April 17, 2024 12:05
@PetrDlouhy PetrDlouhy changed the title Payment model changes for recurring payments and PayU backend Add base Subscribtion/recurring payments support Apr 17, 2024
@PetrDlouhy
Copy link
Contributor Author

I have rebased this PR and fixed testing. I also added the requested JSONField.

@WhyNotHugo @patrys Could you please take a look? What needs to be done to make this merged?

Comment on lines 261 to 262
def is_recurring(self) -> bool:
return self.get_subscription() is not None
Copy link
Contributor

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?

Comment on lines 210 to 259
def get_subscription(self) -> Optional[BaseSubscription]:
"""
Returns subscription object associated with this payment
or None if the payment is not recurring
"""
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

I would go even further and change the autocomplete_with_subscription's signature to autocomplete_with_subscription(self, subscription). Then we would not need get_subscription (and is_recurring) at all and enable implementations to couple payments and subscriptions any way they want.

Copy link
Contributor

@radekholy24 radekholy24 left a comment

Choose a reason for hiding this comment

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

Hi, here is my review as you requested...

I also added comments to a couple of existing threads:
#217 (comment)
#217 (comment)

Also, I expect that the documentation and tests are going to be added soon.

All of my comments are things to consider rather than change requests. Looks good to me; approving then.

Comment on lines +246 to +251
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
"""
Copy link
Contributor

@radekholy24 radekholy24 Apr 25, 2024

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?

Comment on lines 52 to 59
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,
)
Copy link
Contributor

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
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,
)
Copy link
Contributor

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.

Comment on lines 71 to 77
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
self.subscribtion_data = kwargs
Copy link
Contributor

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

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

@radekholy24 radekholy24 Apr 25, 2024

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

def get_unit(self) -> TimeUnit:
raise NotImplementedError

def cancel(self):
Copy link
Contributor

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?

Comment on lines 44 to 51
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,
)
Copy link
Contributor

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?

Comment on lines 79 to 83
def get_period(self) -> int:
raise NotImplementedError

def get_unit(self) -> TimeUnit:
raise NotImplementedError
Copy link
Contributor

@radekholy24 radekholy24 Apr 25, 2024

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, ...)

Comment on lines 264 to 277
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)
Copy link
Contributor

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?

@PetrDlouhy
Copy link
Contributor Author

After long discussion with @radekholy24 we realized, that this interface might not be used only for Subscriptions. The logic of PayU and other providers with server initiated payments offers possibility to make subsequent payments regardless of time frame.
I think it should be rather called a Walllet (other suggestions?).

note: This is in opposition of provider initiated payments, like PayPal, where it probably IS a subscription. But I think, that logic is too different to be implemented in one interface.

If we are implementing a Wallet, then we can simplify the logic, because we don't have to deal with time units, periods etc.

Then the workflow will be following:

  1. Implementator of django-payments creates a payment and calls Payment.autocomplete_with_wallet().
  2. The function Provider.autocomplete_with_wallet(payment) is called
  3. The provider will implement the logic, complete the payment and set up BaseWallet.token and BaseWallet.extra_data and BaseWallet.status
  4. The provider will call BaseWallet.payment_completed(payment)
  5. Implementator can override BaseWallet.payment_completed and place own logic there

The basic models are:

BaseWallet:
    - token
    - status
    - extra_data
    - erase()
    - payment_completed()
    
Payment:
   - autocomplete_with_wallet()

@patrys @WhyNotHugo What do you think about this change? Any ideas?

@PetrDlouhy
Copy link
Contributor Author

Now we changed the logic to the wallet logic described above.
@WhyNotHugo @patrys @auvipy Any thoughts?

@patrys
Copy link
Contributor

patrys commented Jun 4, 2024

Please don't tag me, I have not worked on this package in years

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.

5 participants