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

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

    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.

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?

WhyNotHugo · 2025-03-04T09:16:50Z

I've tried to review this a few times, but I don't understand what this BaseWallet type represents. When you say "wallet", to what are you referring?

PetrDlouhy · 2025-05-21T13:36:53Z

@WhyNotHugo The Wallet model represents type of recurring payments model used by PayU and other payment providers.
In this model the provider let the service provider charge the users directly with various charges. This is opposed to the model used by PayPal where the recurring subscription is managed by PayPal which only sends periodical confirmations that the payment succeeded.

…7 (comment)

Add core interface for server-initiated payments with stored payment methods. Components: - BaseWallet model with lifecycle (PENDING → ACTIVE → ERASED) - BasePayment methods: get_renew_token, set_renew_token, get_renew_data, autocomplete_with_wallet - BasicProvider methods: autocomplete_with_wallet, _finalize_wallet_payment, erase_wallet - WalletStatus constants (PENDING, ACTIVE, ERASED) This enables variable-amount recurring payments where the application controls when to charge and how much, unlike subscription-based systems where the provider manages both schedule and amount.

Implement autocomplete_with_wallet in DummyProvider as reference implementation showing: - Token retrieval and validation - Simulated server-initiated charge - Status updates and captured_amount - Wallet activation via _finalize_wallet_payment Serves as template for other provider implementations.

Add 14 mock-based tests verifying wallet interface following the established pattern from test_core.py. Tests cover: - WalletStatus constants and lifecycle - BaseWallet payment_completed, activate, erase - BasePayment token management (get/set_renew_token, get_renew_data) - DummyProvider autocomplete_with_wallet implementation - Provider helper methods (_finalize_wallet_payment) - Error handling (missing token)

Add comprehensive documentation (569 lines) covering: - Overview emphasizing variable amount capability - Architecture with flow diagrams - Amount flexibility section with examples - Step-by-step implementation guide - Multiple storage patterns - Provider implementation guide - Security best practices - Troubleshooting - Complete API reference Integrated into docs/index.rst and cross-referenced from docs/usage.rst.

Add working example showing: - Wallet model extending BaseWallet - Payment model with wallet FK - Token management implementation (get/set_renew_token) - Migration for wallet support Demonstrates the simple FK-based pattern for wallet integration.

Document v4.1.0 additions: - Wallet-based recurring payments interface - Components and use cases - Provider support status - Backward compatibility

- Add testmain to INSTALLED_APPS in test_settings.py - Update PYTHONPATH to testapp/testapp for testmain module discovery - Configure pytest testpaths to include testapp/testapp for integration tests - Fix PYTHONPATH order in tox.ini (root first to avoid symlink issues) - Add --ignore-glob for testapp/payments symlink This infrastructure supports both mock-based unit tests (in payments/) and integration tests with real DB (in testapp/testapp/testmain/).

Split wallet tests into mock-based unit tests and integration tests: Mock tests (payments/test_wallet.py - 17 tests): - Interface contracts and simple logic without database - Test wallet status choices, default values, token retrieval logic - Test provider delegation and error handling with mocks - Document usage patterns (FK pattern, custom storage) Integration tests (testapp/testapp/testmain/test_wallet.py - 12 tests): - Real database operations and model lifecycle - Wallet state transitions (activate, erase, payment_completed) - DummyProvider full workflow with real Payment/Wallet instances - End-to-end scenarios (first payment, recurring, variable amounts) DummyProvider enhancements (payments/dummy/__init__.py): - Add WalletStatus import - Add wallet status check before charging (security best practice) - Fix captured_amount and transaction_id persistence Total: 29 tests, all passing independently.

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

PetrDlouhy · 2025-12-15T16:07:51Z

I have significantly improved this PR in past two weeks. I added more documentation, updated the interface a bit etc.
This was all motivated by recurring Stripe payments implementation in PR #467. I am still working on that PR and I would like to wait until I have some more experience with recurring Stripe payments from my server. But I think this one (the interface) is ready to be merged

Now this interface is somehow confirmed to be viable by 3 providers: PayU, Stripe and Dummy.

@WhyNotHugo What do you think now. Can we proceed with this or are there any more issues to be addressed?

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 3 times, most recently from 20bdcf2 to c1e2f00 Compare April 17, 2024 12:40

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

PetrDlouhy force-pushed the model-payu branch from d95c757 to 249c784 Compare February 21, 2025 10:50

PetrDlouhy pushed a commit to PetrDlouhy/django-payments that referenced this pull request May 29, 2025

Replace the subscription logic with a wallet logic as per jazzband#21…

6d63805

…7 (comment)

PetrDlouhy force-pushed the model-payu branch from 249c784 to 09d3a70 Compare May 29, 2025 15:43

PetrDlouhy pushed a commit to PetrDlouhy/django-payments that referenced this pull request Dec 4, 2025

Replace the subscription logic with a wallet logic as per jazzband#21…

1915df0

…7 (comment)

PetrDlouhy force-pushed the model-payu branch 3 times, most recently from 34e7c45 to d6beb42 Compare December 8, 2025 07:59

PetrDlouhy mentioned this pull request Dec 8, 2025

Implement recurring (wallet) payments to Stripe provider #467

Draft

PetrDlouhy force-pushed the model-payu branch 4 times, most recently from f4df13a to 80f9828 Compare December 8, 2025 11:10

PetrDlouhy marked this pull request as draft December 8, 2025 11:33

PetrDlouhy added 2 commits December 8, 2025 12:48

PetrDlouhy force-pushed the model-payu branch 3 times, most recently from 6b422d1 to ad876c7 Compare December 8, 2025 12:34

PetrDlouhy added 6 commits December 8, 2025 13:50

docs: Add CHANGELOG entry for wallet interface

9b4da1b

Document v4.1.0 additions: - Wallet-based recurring payments interface - Components and use cases - Provider support status - Backward compatibility

PetrDlouhy force-pushed the model-payu branch from 22815d7 to d589fbc Compare December 8, 2025 13:06

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

ea99855

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

PetrDlouhy marked this pull request as ready for review December 8, 2025 17:10

Uh oh!

Add base Subscribtion/recurring payments support #217

Are you sure you want to change the base?

Add base Subscribtion/recurring payments support #217

Uh oh!

Conversation

PetrDlouhy commented Apr 6, 2020

Uh oh!

codecov bot commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetrDlouhy commented Nov 17, 2020

Uh oh!

WhyNotHugo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PetrDlouhy commented Nov 17, 2020

Uh oh!

PetrDlouhy commented Oct 23, 2021

Uh oh!

WhyNotHugo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radekholy24 Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WhyNotHugo left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 6, 2020 •

edited

Loading

radekholy24 Apr 25, 2024 •

edited

Loading