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

OWA-85: New payment flow #593

Open
wants to merge 192 commits into
base: develop
Choose a base branch
from
Open

Conversation

mirovladimitrovski
Copy link
Collaborator

@mirovladimitrovski mirovladimitrovski commented Aug 13, 2024

Description

This PR solves #OWA-85.

Steps completed:

According to our definition of done, I have completed the following steps:

  • Acceptance criteria met
  • Unit tests added
  • Docs updated (including config and env variables)
  • Translations added
  • UX tested
  • Browsers / platforms tested
  • Rebased & ready to merge without conflicts
  • Reviewed own code

kiremitrov123 and others added 3 commits September 23, 2024 13:09
* fix: choose-plan-form redirects to /subscription and preselected offer bug fixed

* fix: add 0 as port for access bridge tests

* chore: revert port for tests

---------

Co-authored-by: “Kire <[email protected]>
Base automatically changed from IDM-174/use-passport to develop September 25, 2024 10:59
@ChristiaanScheermeijer
Copy link
Collaborator

@kiremitrov123 @mirovladimitrovski thanks for your work on this feature! I'm currently reviewing and testing and I noticed that the JWP integration without access bridge doesn't work anymore. Is this expected or should it still be possible to use InPlayer (JWP) directly?

If not, it means that we can't upgrade when this is merged for existing customers + InPlayer.

image

@kiremitrov123
Copy link
Collaborator

@kiremitrov123 @mirovladimitrovski thanks for your work on this feature! I'm currently reviewing and testing and I noticed that the JWP integration without access bridge doesn't work anymore. Is this expected or should it still be possible to use InPlayer (JWP) directly?

If not, it means that we can't upgrade when this is merged for existing customers + InPlayer.

image

@ChristiaanScheermeijer correct, we are eliminating the old flow for JW. It will be only possible with using the Access Bridge.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would make it colorless and use the Icon + CSS#fill property for colouring instead.

Should we use material-ui icons? Now we have two icons that don't match the icon set anymore 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good suggestion, but I wouldn't delay merging this PR any further and would suggest leaving this for later.

Copy link
Collaborator Author

@mirovladimitrovski mirovladimitrovski Sep 27, 2024

Choose a reason for hiding this comment

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

Also, which icon set are you referring to? These are icons I got from our design team specifically to use in the new modals introduced here, and material-ui is not in place as a dependency.

import AccountService from './integrations/AccountService';

@injectable()
export default class PaymentService {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be renamed 🤔? This is actually the AccessBridgeService integration and not a payment service. It might be confusing with the CheckoutService.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The Access related methods are in the AccessService, these are the payment related definitions that will be the one source of truth hopefully :D. It is confirmed with @dbudzins for the naming.

@@ -31,7 +31,7 @@ const cleengProps: ProviderProps = {
cardInfo: Array.of(['Card number', '•••• •••• •••• 1115'], ['Expiry date', '03/2030'], ['Security code', '******']),
};

runTestSuite(jwProps, 'JW Player');
// runTestSuite(jwProps, 'JW Player');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not finished yet? Will it be possible to do e2e testing with the access bridge? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually we don't want to have exposed a deployed version.. We will need to come up with some mocks probably.

Copy link
Collaborator

@AntonLantukh AntonLantukh left a comment

Choose a reason for hiding this comment

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

LGTM. Still suggest to address url things.

@mirovladimitrovski
Copy link
Collaborator Author

LGTM. Still suggest to address url things.

If I'm correct about what you're referring to, I've already addressed them.

Comment on lines 5 to 12
const useBillingPortalURL = () => {
const checkoutController = getModule(CheckoutController);

return useMutation({
mutationKey: 'billing-portal',
mutationFn: () => checkoutController.generateBillingPortalUrl(window.location.href),
});
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

The current URL should be forwarded (now it is still using window.location):

const useBillingPortalURL = (returnUrl: string) => {
  // ...
 mutationFn: () => checkoutController.generateBillingPortalUrl(returnUrl),

Copy link
Collaborator Author

@mirovladimitrovski mirovladimitrovski Sep 27, 2024

Choose a reason for hiding this comment

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

Done. I did it a little bit differently, but I think you'll approve of it.

@@ -61,7 +75,7 @@ const Subscription: React.FC<Props> = ({ subscription }) => {
) : (
<>
<p>{t('user:payment.no_subscription_new_payment')}</p>
<Button toModal="choose-offer" label={t('user:payment.view_plans')} />
<Button onClick={onClick} label={t('user:payment.view_plans')} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

onClick shouldn't be needed, but it's fine for now:

<Button to={modalURLFromLocation(location, 'choose-offer')} label={t('user:payment.view_plans')} />

Copy link
Collaborator Author

@mirovladimitrovski mirovladimitrovski Sep 27, 2024

Choose a reason for hiding this comment

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

The onClick handler is there for the isLoading state, because we don't redirect right away, but only after we get a redirection URL from an API request.

Copy link
Collaborator Author

@mirovladimitrovski mirovladimitrovski Sep 27, 2024

Choose a reason for hiding this comment

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

Sorry, my previous answer to this is incorrect for this use case, I actually had a different instance of using Button in mind. You're actually right.

Done.

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.

7 participants