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

Prevent users from doubleclick on Payment button #9351

Closed
wants to merge 0 commits into from
Closed

Prevent users from doubleclick on Payment button #9351

wants to merge 0 commits into from

Conversation

MathiasReker
Copy link
Contributor

@MathiasReker MathiasReker commented Jul 19, 2018

Questions Answers
Branch? 1.7.4.x
Description? If the payment button is clicked twice, this error will be showed: Cart cannot be loaded or an order has already been placed using this cart prestashop payment. AND the customer will loose his cart.
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? dont know
How to test? Without this fix try to place an order and click multiple times on the payment button on checkout. This will lead to unexpected behavior.

Important guidelines


This change is Reviewable

@prestonBot
Copy link
Collaborator

Hello @mreker!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@prestonBot prestonBot added 1.7.4.x Branch Bug Type: Bug labels Jul 19, 2018
@MathiasReker
Copy link
Contributor Author

My fix simple disable the payment button when it is clicked, to prevent users from double click on it.

I think the check-box should be disabled too, but I can't figure out how to solve this part. If the loading is very slow it is possible to uncheck terms and conditions and the recheck it - and click the now undisabled payment button again.

@prestonBot prestonBot added 1.7.4.x Branch Bug Type: Bug labels Jul 19, 2018
@khouloudbelguith
Copy link
Contributor

Hi Mreker,

Thanks for your PR.
I tested your PR and it is OK for me.

Best regards, Khouloud

@MathiasReker
Copy link
Contributor Author

@khouloudbelguith what do you think about also disable the checkbox?

@Quetzacoalt91
Copy link
Member

Hi @mreker,

You're modifying a generated file in this PR.
Changes should be made in the folder themes/_core/js. Changes in the JS files from this folder will be then applied in themes/core.js

@MathiasReker
Copy link
Contributor Author

MathiasReker commented Jul 19, 2018

Now it is not possible to uncheck the checkbox after click on payment button, I fixed that part too. I think this is the right behavior? wdyt @Quetzacoalt91 @khouloudbelguith

@prestonBot prestonBot added 1.7.4.x Branch Bug Type: Bug labels Jul 19, 2018
@MathiasReker
Copy link
Contributor Author

Can this fix be added to 1.7.4.2 milestone?

@@ -112,6 +112,8 @@ class Payment {
confirm() {
var option = this.getSelectedOption();
if (option) {
$('.custom-checkbox').click(false);
$('.btn-primary').prop('disabled',true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure it's the best idea to disable all .btn-primary button when an error can occurred and block all buttons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you like $('body#checkout #payment-confirmation .btn-primary').prop('disabled',true); instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to chain ids. But you can use exactly the same behavior as above.

$(this.confirmationSelector + ' button').prop('disabled', true);

I think this is the only thing you need for this PR. (but not sure =))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have just tested your solution. I can't reproduce the error 👋 Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks to you for your contribution ;) Don't forget you did all the job!

@@ -112,6 +112,8 @@ class Payment {
confirm() {
var option = this.getSelectedOption();
if (option) {
$('.custom-checkbox').click(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Stopping propagation can crash some modules. a button just need to be disabled after the first click and restored in case of failure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need help on this please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I delete this line

@MathiasReker
Copy link
Contributor Author

Thank you @PierreRambaud and @Quetzacoalt91

@PierreRambaud
Copy link
Contributor

@Quetzacoalt91 is it ok for you?

@Quetzacoalt91
Copy link
Member

Quetzacoalt91 commented Jul 23, 2018

Sounds good to me.

However, assets have to be regenerated before the QA team can review this PR.

@MathiasReker
Copy link
Contributor Author

@Quetzacoalt91 can this be added to 1.7.4.2?

@PierreRambaud
Copy link
Contributor

I'm sorry @mreker , i wasn't able to build assets on your Pull Request, so I tried to push on your repository and it mysteriously clean you branch. I rebuild a PR with assets built #9373.

We can continue on the new PR.

Regards

PierreRambaud added a commit that referenced this pull request Jul 27, 2018
Prevent users from doubleclick on Payment button from #9351
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.4.x Branch Bug Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants