Add base Subscribtion/recurring payments support #217

PetrDlouhy · 2020-04-06T17:47:40Z

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 · 2020-04-06T17:49:37Z

Codecov Report

Attention: Patch coverage is 75.00000% with 7 lines in your changes are 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 4f9df42. Consider uploading reports for the commit 4f9df42 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.

patrys · 2020-04-07T12:46:35Z

payments/models.py

+    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


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?

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)

But maybe it would be appropriate to throw NotImplementedError instead of returning None and write some note to the comments about thet.

PetrDlouhy · 2020-11-17T07:38:17Z

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.

WhyNotHugo

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

WhyNotHugo · 2020-11-17T09:13:17Z

payments/models.py

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


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?

WhyNotHugo · 2020-11-17T09:13:29Z

payments/models.py

+        """
+        raise NotImplementedError()
+
+    def get_user(self):


What's this supposed to return?

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.

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.

WhyNotHugo · 2020-11-17T09:14:19Z

payments/models.py

+        """ Get the user asociated with this payment """
+        raise NotImplementedError()
+
+    def get_user_email(self):


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

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

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.

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

WhyNotHugo · 2020-11-17T09:16:06Z

payments/models.py

+        except AttributeError:
+            raise NotImplementedError()
+
+    def get_user_last_name(self):


Ditto billing_last_name.

payments/models.py

PetrDlouhy · 2020-11-17T11:49:09Z

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 · 2021-10-23T04:31:20Z

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

WhyNotHugo

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

WhyNotHugo · 2021-10-27T13:48:34Z

payments/core.py

+    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()


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?

+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)

WhyNotHugo · 2021-10-27T13:52:09Z

payments/models.py

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


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?

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.

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

WhyNotHugo · 2021-10-27T13:55:54Z

payments/models.py

+    def get_subscription(self) -> Optional[BaseSubscription]:
+        """
+        Returns subscription object associated with this payment
+        or None if the payment is not recurring
+        """
+        return None


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?

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.

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

WhyNotHugo · 2021-11-29T20:54:30Z

payments/models.py

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


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

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

WhyNotHugo · 2021-11-29T20:55:21Z

payments/models.py

+    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


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

WhyNotHugo · 2021-11-29T20:59:36Z

payments/models.py

+    def get_subscription(self) -> Optional[BaseSubscription]:
+        """
+        Returns subscription object associated with this payment
+        or None if the payment is not recurring
+        """
+        return None


WhyNotHugo

Makes sense to me.

for more information, see https://pre-commit.ci

PetrDlouhy · 2024-04-17T12:46:42Z

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?

radekholy24 · 2024-04-25T11:49:45Z

payments/models.py

+    def is_recurring(self) -> bool:
+        return self.get_subscription() is not None


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?

radekholy24 · 2024-04-25T12:15:43Z

payments/models.py

+    def get_subscription(self) -> Optional[BaseSubscription]:
+        """
+        Returns subscription object associated with this payment
+        or None if the payment is not recurring
+        """
+        return None


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.

radekholy24

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.

radekholy24 · 2024-04-25T12:43:07Z

payments/models.py

+    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
+        """


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?

radekholy24 · 2024-04-25T13:01:10Z

payments/models.py

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


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

radekholy24 · 2024-04-25T13:04:05Z

payments/models.py

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


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

radekholy24 · 2024-04-25T13:08:42Z

payments/models.py

+    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


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

radekholy24 · 2024-04-25T13:13:36Z

payments/models.py

+        Cancel the subscription by provider
+        Used by providers, that use provider initiated subscription workflow
+        Implementer is responsible for cancelling the subscription model


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:

Cancel the subscription by provider - is there any "by [something]" needed? Isn't "cancel the subscription" enough?

Used by providers, that use provider initiated subscription workflow - cannot it potentially be used by anyone, if the provider supports it?

Implementer is responsible for cancelling the subscription model

radekholy24 · 2024-04-25T13:15:11Z

payments/models.py

+    def get_unit(self) -> TimeUnit:
+        raise NotImplementedError
+
+    def cancel(self):


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

radekholy24 · 2024-04-25T13:17:52Z

payments/models.py

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


Any particular reason for allowing blank/null values? I mean, allowing this usually implies a lot of tedious None-checking in the code.

radekholy24 · 2024-04-25T13:33:44Z

payments/models.py

+    def get_period(self) -> int:
+        raise NotImplementedError
+
+    def get_unit(self) -> TimeUnit:
+        raise NotImplementedError


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

radekholy24 · 2024-04-25T13:53:24Z

payments/models.py

+    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)


Oh, and... Would it make sense to check if the payment has an allowed status (WAITING) only?

patrys reviewed Apr 7, 2020

View reviewed changes

PetrDlouhy force-pushed the model-payu branch from a0f5639 to 4d3a848 Compare June 8, 2020 14:22

PetrDlouhy force-pushed the model-payu branch from a10fcfa to 2ef577a Compare November 17, 2020 07:33

PetrDlouhy mentioned this pull request Nov 17, 2020

Add PayU provider #239

Merged

WhyNotHugo reviewed Nov 17, 2020

View reviewed changes

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

PetrDlouhy force-pushed the model-payu branch 5 times, most recently from 244c031 to 6bd1ef1 Compare October 25, 2021 10:08

auvipy approved these changes Oct 27, 2021

View reviewed changes

PetrDlouhy requested review from WhyNotHugo and patrys October 27, 2021 07:23

This was referenced Oct 27, 2021

(WIP) PayPal subscribtions #274

Draft

recurring payments #59

Open

WhyNotHugo reviewed Oct 28, 2021

View reviewed changes

PetrDlouhy force-pushed the model-payu branch from 28c0167 to f3d265a Compare November 20, 2021 06:54

WhyNotHugo reviewed Nov 29, 2021

View reviewed changes

WhyNotHugo approved these changes Nov 29, 2021

View reviewed changes

PetrDlouhy force-pushed the model-payu branch 2 times, most recently from ac776ef to 97ba417 Compare April 17, 2024 12:05

PetrDlouhy added 3 commits April 17, 2024 14:40

add common subscription methods

5f705a6

fixes of WhyNotHugo's remarks

67496a8

fix ruff testing

8b7288f

BaseSubscribtion add subscribtion_data field

c1e2f00

PetrDlouhy force-pushed the model-payu branch from 20bdcf2 to c1e2f00 Compare April 17, 2024 12:40

[pre-commit.ci] auto fixes from pre-commit.com hooks

4f9df42

for more information, see https://pre-commit.ci

PetrDlouhy changed the title ~~Payment model changes for recurring payments and PayU backend~~ Add base Subscribtion/recurring payments support Apr 17, 2024

radekholy24 reviewed Apr 25, 2024

View reviewed changes

radekholy24 approved these changes Apr 25, 2024

View reviewed changes

radekholy24 reviewed Apr 25, 2024

View reviewed changes

		def is_recurring(self) -> bool:
		return self.get_subscription() is not None

Add base Subscribtion/recurring payments support #217

Are you sure you want to change the base?

Add base Subscribtion/recurring payments support #217

Conversation

PetrDlouhy commented Apr 6, 2020

codecov bot commented Apr 6, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetrDlouhy commented Nov 17, 2020

WhyNotHugo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetrDlouhy commented Nov 17, 2020

PetrDlouhy commented Oct 23, 2021

WhyNotHugo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radekholy24 Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WhyNotHugo left a comment

Choose a reason for hiding this comment

PetrDlouhy commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radekholy24 left a comment • edited

Choose a reason for hiding this comment

radekholy24 Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radekholy24 Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radekholy24 Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 6, 2020 •

edited

radekholy24 Apr 25, 2024 •

edited

radekholy24 left a comment •

edited

radekholy24 Apr 25, 2024 •

edited

radekholy24 Apr 25, 2024 •

edited

radekholy24 Apr 25, 2024 •

edited