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

refactor: payment context to follow the new flow of fetching from API #4356

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

sshanzel
Copy link
Member

@sshanzel sshanzel commented Apr 7, 2025

Changes

  • Refactor the interface being used based on the new structure of the new query as agreed upon by the RFC.
  • Refactor all captions to use the same component to make it easier to maintain.
    • Removed all the extra conditions that check if isTrial ? 'success' : 'help', etc.
  • Make every request of pricing preview to be coming from API (cached every hour).
  • Introduced a BasePaymentProvider that will handle common PaymentContextProviders like the one for Paddle and Extension. Most logics are stored from the base provider then any requirements based on platform will be hosted on their respective contexts.
    • Also removed the earlyAdopterId prop as it is not being used anymore.
    • Made the StoreKit provider to fetch the metadata for easier maintenance of metadata.

To test this locally, you can simply pull this PR for API until it is merged: dailydotdev/daily-api#2741

After that, everything should work out of the box.

Events

Did you introduce any new tracking events?

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

Jira ticket

MI-853

Preview domain

https://mi-853.preview.app.daily.dev

Copy link

vercel bot commented Apr 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
daily-webapp ✅ Ready (Inspect) Visit Preview Apr 7, 2025 5:04pm
1 Skipped Deployment
Name Status Preview Updated (UTC)
storybook ⬜️ Ignored (Inspect) Apr 7, 2025 5:04pm

Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

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

Nice work!
Big PR and very critical so we might need to define some new critical flows for purchases, can you maybe set a confluence page up for this? (CC: @idoshamun )

Also would like to have @omBratteng look at it for the IAP stuff.

In general looks ok, minor question around the extension layer.

return duration === PlusPriceType.Yearly ? 0 : 1;
})
.at(0),
productOptions.find(
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we should just expose this early adopter package in the context already so it's uniform?

staleTime: StaleTime.Default,
});

const giftOneYear = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

I mean we could expose the earlyAdopted like this :)

@@ -64,25 +42,25 @@ export const PaddleSubProvider = ({
eventCallback: (event: PaddleEventData) => {
switch (event?.name) {
case CheckoutEventNames.CHECKOUT_PAYMENT_INITIATED:
logRef.current({
logRef.current?.({
Copy link
Contributor

Choose a reason for hiding this comment

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

Why these conditionals?

Comment on lines +20 to +22
if (checkIsExtension()) {
return <ChromeExtensionProvider>{children}</ChromeExtensionProvider>;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct though?
I can actually show prices now right, it just can't handle checkout.

@idoshamun
Copy link
Member

I agree with @rebelchris, this will have to be thoroughly tested and we should define critical flows for payment. We will need it for web funnels as well

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.

3 participants