Implement subscriber sync from Paddle API #19

matteing · 2020-04-10T01:07:13Z

Hi, I decided to implement subscriber sync.

A few things to note.

Some fields aren't returned - see lines marked with question marks. Some are even returned conditionally depending on the state of the subscription.
There is a serious problem - we can't get_or_create users (get_subscriber_model). The User model does not allow creating users without usernames, etc.
I had to change a few DB fields to accommodate the API returns.
No tests implemented yet. I did test with a prod workload and it seems to sync just fine, but nothing automated.

As a side note, I implemented a partial fix to bullet point 2: this seemed very worrying.

Code review welcome. So far it seems to be working fine, but it certainly isn't perfect.

Django 2 requires an app_name variable to be set on urls.py when using include(). https://stackoverflow.com/questions/48608894/impropyconfigurederror-about-app-name-when-using-namespace-in-include

Next bill date can be null if a subscription != active

Sub sync

matteing · 2020-04-10T02:15:21Z

Apologies for the messy commit history - it seems GH automatically merged another commit into my pull.

matteing · 2020-04-10T02:41:43Z

Added a few other bugfixes:

Fix mistyped parameter in update_or_create: this was breaking functionality from_update.
Fix extra 'created' unpack: this fixed a mistake I made.

Fix mistyped parameter in update_or_create

matteing · 2020-04-10T05:00:42Z

More bugfixes:

Made max_length on IDs TextFields rather than 32, for some reason Paddle is returning IDs of length 35 and this broke webhooks completely for me (on production).

kennell · 2020-04-10T20:27:30Z

@matteing A potential problem i see here is that the email used for Paddle checkout may not match the local user's email. A clean way of doing this would be to send a unique local user id as a passthrough argument when initializing the Paddle JS snippet (see https://paddle.com/docs/paddle-checkout-web/ - section "Sending additional user data"), this will then be included in the subscription_created webook and can be used to match a local user, even if a different email was used during the Paddle checkout.

However, Paddle currently only includes this passthrough string in the Webhook, not in the List-Subscriptions API endpoint*. So if anything goes wrong during the webhook, it will not be possible to manually sync via Django command later (or we will have to fallback to email matching).

*I've already contacted Paddle about including the passthrough argument is the API endpoint, was told they see that it would be useful but they can't provide a ETA.

Just something to consider.

matteing · 2020-04-10T20:30:01Z

I actually absolutely agree with @kennell on this subject (I face this problem on production), however I didn't find that the current code did this linking using the user_id argument - so I didn't implement it as to keep feature parity (see here).

If given a green light I think I can take a shot at adding user_id linking.

Edit: Reading more carefully, noted on the endpoint issue not returning webhook data.

FPurchess · 2020-04-13T08:39:02Z

djpaddle/management/commands/djpaddle_sync_subscribers_from_paddle.py

+        """Call sync_from_paddle_data for each subscriber returned by api_list."""
+        for sub_data in Subscription.api_list():
+            sub = Subscription.sync_from_paddle_data(sub_data)
+            print("Synchronized {0}".format(str(sub)))


since it can become a lot of subscriptions, this statement might be too verbose. Please make it a summary or add a parameter for verbose output

let's switch to self.stdout.write(self.style.SUCCESS('...')) - I'll change this in the plan-command as well

FPurchess · 2020-04-13T08:39:21Z

djpaddle/migrations/0002_auto_20200410_0056.py

@@ -0,0 +1,18 @@
+# Generated by Django 3.0.5 on 2020-04-10 00:56


please squash migrations

FPurchess · 2020-04-13T08:41:33Z

djpaddle/management/commands/djpaddle_sync_subscribers_from_paddle.py

+
+    def handle(self, *args, **options):
+        """Call sync_from_paddle_data for each subscriber returned by api_list."""
+        for sub_data in Subscription.api_list():


let's call it subscription_data

FPurchess · 2020-04-13T08:48:35Z

djpaddle/models.py

+    def sync_from_paddle_data(cls, data):
+        pk = data.get('subscription_id', None)
+        # First, find and drop current sub with this data.
+        cls.objects.filter(pk=pk).delete()


please initialize kwargs with all parameters needed to create/update the Subscription except pk, then use update_or_create instead of deleting the Subscription right away.

[ } use update_or_create

FPurchess · 2020-04-13T08:49:10Z

djpaddle/models.py

+            update_url = data.get('update_url'),
+            **kwargs
+        )
+        return sub


please rename to instance or subscription

FPurchess · 2020-04-13T08:51:39Z

djpaddle/models.py

-            data["subscriber"], created = settings.get_subscriber_model().objects.get_or_create(
-                email=payload["email"]
-            )
+        try:


Subscriber = settings.get_subscriber_model() if subscriber_id not in ["", None]: try: data[ "subscriber"] = Subscriber.objects.get(email=payload["email"]) except settings.get_subscriber_model().DoesNotExist: data[ "subscriber"] = None

FPurchess · 2020-04-13T08:54:39Z

djpaddle/models.py

+        try:
+            kwargs['plan'] = Plan.objects.get(pk=data.get("plan_id"))
+        except Plan.DoesNotExist:
+            print("Skipping, plan not found.")


please use log.warn or make it a comment

please return None

FPurchess · 2020-04-13T08:58:51Z

djpaddle/models.py

+            quantity=data.get("last_payment", {}).get("amount", 0),
+            source="", # ???
+            status=data.get("state"),
+            unit_price=0.00, # ???


@matteing shall we rather make this a default in the model field?

FPurchess · 2020-04-13T09:10:56Z

@matteing thanks for your good work! I added some comments. However, it would be great to have a test for it. I'd like to see our test-coverage reach a reasonable level soon.

FPurchess · 2020-04-13T09:16:33Z

@matteing A potential problem i see here is that the email used for Paddle checkout may not match the local user's email. A clean way of doing this would be to send a unique local user id as a passthrough argument when initializing the Paddle JS snippet (see https://paddle.com/docs/paddle-checkout-web/ - section "Sending additional user data"), this will then be included in the subscription_created webook and can be used to match a local user, even if a different email was used during the Paddle checkout.

However, Paddle currently only includes this passthrough string in the Webhook, not in the List-Subscriptions API endpoint*. So if anything goes wrong during the webhook, it will not be possible to manually sync via Django command later (or we will have to fallback to email matching).

*I've already contacted Paddle about including the passthrough argument is the API endpoint, was told they see that it would be useful but they can't provide a ETA.

Just something to consider.

Agree. This is another good argument to make the matching of subscribers configurable with a sane default (e.g. email-matching). So the developer that implements dj-paddle into his project can decide how to handle it for his respective use-case.

j0lvera · 2023-05-26T19:49:57Z

I need this feature. Are the owners of the repo active lately? I don't want to put in the work if they aren't reviewing pull requests.

matteing · 2023-05-26T19:53:53Z

Package seems long unmaintained. I'm unsure of any alternatives. It's been a while since this thread! :)

j0lvera · 2023-05-26T19:56:21Z

@matteing thanks for the reply and the initial work!

Package seems long unmaintained.

ugh, forking it is then.

matteing · 2023-05-26T20:14:30Z

@matteing thanks for the reply and the initial work!

Package seems long unmaintained.

ugh, forking it is then.

That's a good approach! Although no shade on the original authors -- they did great work pushing support for Paddle with Django.

I hope the Paddle API primitives haven't changed since then!

j0lvera · 2023-05-26T20:21:53Z

That's a good approach! Although no shade on the original authors -- they did great work pushing support for Paddle with Django.

Totally true. My comment was rude. The authors don't owe nobody their time and the project is of great value as it is.

matteing · 2023-05-26T21:08:45Z

No worries homie! Hope my work helps as well.

…

Sent from my iPhone On May 26, 2023, at 4:22 PM, Juan Olvera ***@***.***> wrote: That's a good approach! Although no shade on the original authors -- they did great work pushing support for Paddle with Django. Totally true. My comment was rude. The authors don't owe nobody their time and the project is of great value as it is. — Reply to this email directly, view it on GitHub<#19 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AB4XGZNWZJ366LPPPRQ6QP3XIEGGZANCNFSM4MFENADA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

saschwarz · 2023-05-26T21:56:00Z

@j0lv3r4 I ended up forking for a fix/features I needed semi-recently. Don't know if you are interested in any of those changes: https://github.com/saschwarz/dj-paddle/commits/master

j0lvera · 2023-05-30T18:23:56Z

@saschwarz awesome, thank you. I will check it out.

kennell · 2023-05-31T12:45:49Z

Just FYI i started a alternative implementation a while ago:

https://github.com/kennell/django-paddle

Far from feature complete, but perhaps a alternative to some -- or at least a inspiration ;)

matteing and others added 5 commits April 9, 2020 18:30

Fix a deprecation warning in Django 2.0

68d9f03

Django 2 requires an app_name variable to be set on urls.py when using include(). https://stackoverflow.com/questions/48608894/impropyconfigurederror-about-app-name-when-using-namespace-in-include

Make a basic version of subscriber_sync

14d1882

Make next_bill_date nullable

1e8b2cd

Next bill date can be null if a subscription != active

Make a basic version of subscriber_sync

444dbdc

Make next_bill_date nullable

e371caf

Next bill date can be null if a subscription != active

matteing mentioned this pull request Apr 10, 2020

Management Command to synchronize subscriptions from paddle #3

Open

matteing force-pushed the sub-sync branch from 5e9f68c to e371caf Compare April 10, 2020 02:10

diettam and others added 3 commits April 9, 2020 22:11

Fix extra 'created' unpack

ffef1a6

Merge branch 'master' into sub-sync

4ccee16

Merge pull request #1 from matteing/sub-sync

c68fe29

Sub sync

Fix mistyped parameter in update_or_create

ff86a94

matteing and others added 3 commits April 9, 2020 22:42

Merge pull request #2 from matteing/sub-sync

8102da8

Fix mistyped parameter in update_or_create

Make fields with length=32 TextFields

a72e46f

Merge branch 'fix-length' into sub-sync

0d2ac0d

FPurchess requested changes Apr 13, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement subscriber sync from Paddle API #19

Implement subscriber sync from Paddle API #19

matteing commented Apr 10, 2020 •

edited

Loading

matteing commented Apr 10, 2020

matteing commented Apr 10, 2020

matteing commented Apr 10, 2020 •

edited

Loading

kennell commented Apr 10, 2020

matteing commented Apr 10, 2020 •

edited

Loading

FPurchess Apr 13, 2020

FPurchess Apr 13, 2020

FPurchess Apr 13, 2020

FPurchess Apr 13, 2020

FPurchess Apr 13, 2020

FPurchess Apr 13, 2020

FPurchess Apr 13, 2020

FPurchess Apr 13, 2020

FPurchess commented Apr 13, 2020

FPurchess commented Apr 13, 2020

j0lvera commented May 26, 2023

matteing commented May 26, 2023

j0lvera commented May 26, 2023

matteing commented May 26, 2023

j0lvera commented May 26, 2023

matteing commented May 26, 2023 via email

saschwarz commented May 26, 2023

j0lvera commented May 30, 2023

kennell commented May 31, 2023

		@@ -0,0 +1,18 @@
		# Generated by Django 3.0.5 on 2020-04-10 00:56

Implement subscriber sync from Paddle API #19

Are you sure you want to change the base?

Implement subscriber sync from Paddle API #19

Conversation

matteing commented Apr 10, 2020 • edited Loading

matteing commented Apr 10, 2020

matteing commented Apr 10, 2020

matteing commented Apr 10, 2020 • edited Loading

kennell commented Apr 10, 2020

matteing commented Apr 10, 2020 • edited Loading

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

FPurchess commented Apr 13, 2020

FPurchess commented Apr 13, 2020

j0lvera commented May 26, 2023

matteing commented May 26, 2023

j0lvera commented May 26, 2023

matteing commented May 26, 2023

j0lvera commented May 26, 2023

matteing commented May 26, 2023 via email

saschwarz commented May 26, 2023

j0lvera commented May 30, 2023

kennell commented May 31, 2023

matteing commented Apr 10, 2020 •

edited

Loading

matteing commented Apr 10, 2020 •

edited

Loading

matteing commented Apr 10, 2020 •

edited

Loading