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

[backend] Disable submit buttons after first click #3342

Merged

Conversation

spaghetticode
Copy link
Member

Description
It may happen that admin users click multiple times (purposely or not) on submit buttons in the backend area.

This can be a problem as at times it may silently generate unwanted duplicate records, or show an error page due to duplicate records. This is much more likely to happen on real shops when unanticipated server load makes the application unresponsive (at first click nothing seems to happen, so then why not click again?) and where some controller create actions may be patched and be slower.

By changing button_tag to f.submit or submit_tag the button gets disabled after the first click, thanks to the automatically added attribute data-disable-with="Create"

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change (if needed)

@spaghetticode spaghetticode added the changelog:solidus_backend Changes to the solidus_backend gem label Sep 20, 2019
@spaghetticode spaghetticode self-assigned this Sep 20, 2019
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

This is a nice feature. Thanks.

I only have some notes about the specs for this. We should use the existing specs to test this feature and should not add multiple additional it blocks

select Spree::PaymentMethod::Check.model_name.human, from: "Type"
click_button "Create"

expect(find('input[type="submit"]')).to be_disabled
Copy link
Member

Choose a reason for hiding this comment

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

Could we move this into the other spec instead? We should avoid to have multiple it blocks in feature specs. This unnecessarily slows down our test suite


click_on "Create"

expect(find('input[type="submit"]')).to be_disabled
Copy link
Member

Choose a reason for hiding this comment

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

Here as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

@tvdeyen I updated the files according to your suggestion. I think they don't read as good as before, so we're loosing something here. But I agree that saving some seconds is a good thing, so why not 👍


create_code_batch

expect(find('input[type="submit"]')).to be_disabled
Copy link
Member

Choose a reason for hiding this comment

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

We don’t need multiple it blocks to test this feature.

@seandawes
Copy link

This is also an issue in areas where double click occurs on refunds for example. It will double refund. We solved this on our store and you ( @spaghetticode ) actually fixed the issue we spec'd out for you . Dec 2018 Modded Euros issue 870 in our codebase for reference

Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

This a good idea. I agree with @tvdeyen, but I'd also be okay with merging this as it is -- it is a pretty big UX improvement in my opinion.

Thanks @spaghetticode ❤🍝

@spaghetticode spaghetticode force-pushed the spaghetticode/backend-disable-submit branch 4 times, most recently from 5ca9607 to 4c30bd5 Compare September 27, 2019 08:58
Disabling the button prevents multiple form submissions that may result in
errors, as it generally does not make any sense creating multiple customer
returns starting from the same data.
Disabling the button prevents multiple form submissions that would result
in unwanted duplicated records.
Some modifications to `click_nav` have been done in order to make
that helper method work with Selenium JS tests as well.
Disabling the button prevents multiple form submissions that would result
in unwanted duplicated records.
Disabling the button prevents multiple form submissions that would result
in unwanted duplicated records.
Disabling the button prevents multiple form submissions that would result
in duplicate error messages.
Disabling the button prevents multiple form submission that would result
in unwanted duplicated records.
This partial is used by many `create` forms in the admin. Disabling
the button prevents multiple form submissions that would result in
unwanted duplicated records or errors.
@spaghetticode spaghetticode force-pushed the spaghetticode/backend-disable-submit branch from 4c30bd5 to 192cac5 Compare September 27, 2019 09:07
Copy link
Contributor

@ericsaupe ericsaupe left a comment

Choose a reason for hiding this comment

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

Good change! We can keep iterating going forward

@kennyadsl kennyadsl merged commit a9294a2 into solidusio:master Oct 17, 2019
@kennyadsl kennyadsl deleted the spaghetticode/backend-disable-submit branch October 17, 2019 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_backend Changes to the solidus_backend gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants