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] payment_stripe: regression 98fe5054c2 #32026

Closed

Conversation

Projects
None yet
3 participants
@nle-odoo
Copy link
Contributor

commented Mar 21, 2019

opw-1939323

@nle-odoo nle-odoo added the OE label Mar 21, 2019

nle-odoo referenced this pull request Mar 21, 2019

[FIX] payment_stripe: open>exit>open opened 1 time
If using Stripe we did "Pay Now" -> close modal -> "Pay Now" we would
get 2 modals and possibly be blocked by infinite loading.

With this changeset, we only execute one time our strip.js file so we do
not declare multiple MutationObserver.

We also only open one checkout at a time from stripe when closing and
opening it or multiclicking click (the later one that could block the
interface with several stripe iframe opened and one with a infinite
loading wheel).

opw-1939323
closes #31928

Signed-off-by: Nicolas Lempereur (nle) <nle@odoo.com>

@robodoo robodoo added the seen 🙂 label Mar 21, 2019

@nle-odoo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Test after regression fix: https://www.youtube.com/watch?v=c-7uKp509dU

robodoo r+

@robodoo robodoo added the r+ 👌 label Mar 21, 2019

@wtaferner

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

@nle-odoo
#32025

Hmm, your fix is different, may I ask you to explain? My fix is working and already deployed and tested.

@wtaferner

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

Got it, probably to be backwards compatible?
Anyway, can you please fix the test too in terms of testing a whole Stripe payment?

@nle-odoo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Hi Wolfgang,

Thanks for the quick response, this PR and sorry for the issue.

Hmm, your fix is different, may I ask you to explain? My fix is working and already deployed and tested.

The result is exactly the same.

In the original PR I renamed handler to stripeHandler because this is now a global variable (to the module closure) so I was afraid that in a future change someone using a variable "handler" could have an issue => but the created issue has no comparison.

In my version we have a local variable handler so even if someone changed stripeHandler stripe would still have the correct variable.

In any case this is just for non-existing future code so your PR was as correct.

Regards.

pniederlag pushed a commit to pniederlag/odoo that referenced this pull request Mar 21, 2019

[FIX] payment_stripe: regression 98fe505
opw-1939323

closes odoo#32026

Signed-off-by: Nicolas Lempereur (nle) <nle@odoo.com>
@nle-odoo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Anyway, can you please fix the test too in terms of testing a whole Stripe payment?

The current test we have are only python and are only run at most daily (because stripe would block us because of the number of request) with the number of runbot. But in this instance these test would not catch an issue.

For testing the whole scenario a customer do with e.g. a tour:

  • it would also not be possible to do it on every runbot because we would get the same rate limitation issue

  • in this instance a test on the whole scenario does not seem possible since Stripe adds its payment wizard in an iframe over Odoo. Thanks to browser security a tour in odoo code has no way to set a card number inside the iframe.

edit :

As for test, I tried a JS test, but we need to mock "$.getScript", MutationObserver, stripe reception of request and responses. So the test do not have much meaning.

The best way to test this would be to have a "tour" with a modified browser (eg. https://www.guru99.com/handling-iframes-selenium.html) but I am not sure how this would be implementable with our current testing framework.

@robodoo robodoo added merged 🎉 and removed merging 👷 labels Mar 21, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

Merged, thanks!

@robodoo robodoo closed this Mar 21, 2019

@nle-odoo nle-odoo deleted the odoo-dev:11.0-payment-stripe-opw-1939323-nle branch Mar 21, 2019

@wtaferner

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

@nle-odoo
Alright, then there are only manual tests which can avoid such culprits in the future on your side and I am pretty sure you won't merge payment changes in the future without thoroughly testing in all the browsers a whole payment manually before, right? 😆
This should actually anyway always be the case for these sensible modules, as long there is no 100% test coverage and test framework for all perspectives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.