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

fix: update subscription tracking #2461

Merged
merged 6 commits into from
Nov 19, 2024
Merged

Conversation

rebelchris
Copy link
Contributor

We should only fire the event if subscription cycle actually changed, not any of the other changes.
I also decided to update our flags in this case so we have the right setting stored at all times.

AS-744 #done

Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

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

Don't have anything blocking 👀

src/routes/webhooks/paddle.ts Outdated Show resolved Hide resolved
Comment on lines 123 to 131
const userId = await getUserId({
userId: customData?.user_id,
subscriptionId: 'subscriptionId' in data.data && data.data.subscriptionId,
});
const con = await createOrGetConnection();
const flags = await con.getRepository(User).findOne({
where: { id: userId },
select: ['subscriptionFlags'],
});
Copy link
Contributor

Choose a reason for hiding this comment

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

We double query the user, once in getUserId then again in the call in this function?

Copy link
Contributor

@capJavert capJavert Nov 19, 2024

Choose a reason for hiding this comment

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

If we want to abstract anything maybe better that getUserId is just getUser and then we get user object either way which we can use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah was thinking of that as well, just meant more refactors on end type as it's a optional 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.

Also the early return only cares about returning the id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternative could be to have a seperate function for this case? (only 1 user now, but maybe future more)
Let me know if you prefer that

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 just do it inline in both places but if we use util then I would go with getUser. Neither blocking for this PR.

src/routes/webhooks/paddle.ts Outdated Show resolved Hide resolved
@rebelchris rebelchris merged commit 7f77654 into main Nov 19, 2024
8 checks passed
@rebelchris rebelchris deleted the AS-744-update-subscription branch November 19, 2024 13:22
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.

4 participants