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

make disable_with default in submit_tag #21135

Merged
merged 1 commit into from Aug 17, 2015

Conversation

Projects
None yet
7 participants
@DropsOfSerenity
Contributor

DropsOfSerenity commented Aug 5, 2015

I heard on the "Bike Shed" podcast that the idea of possibly making disable_with a default for submit_tag to prevent double submission by default, thought I would draft up a pull request for it and take a stab at it.

Show outdated Hide outdated actionview/CHANGELOG.md
@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 6, 2015

Contributor

@kaspth updated with your style changes sir :)

Contributor

DropsOfSerenity commented Aug 6, 2015

@kaspth updated with your style changes sir :)

@rafaelfranca

View changes

Show outdated Hide outdated actionview/test/template/form_helper_test.rb
@rafaelfranca

View changes

Show outdated Hide outdated actionview/lib/action_view/helpers/form_tag_helper.rb
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 6, 2015

Member

thank you for the work. It makes sense but I think we may have problems with with the hash update. Could you add tests that it is working with the inputs that I pointed?

Member

rafaelfranca commented Aug 6, 2015

thank you for the work. It makes sense but I think we may have problems with with the hash update. Could you add tests that it is working with the inputs that I pointed?

@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 6, 2015

Contributor

Ok yeah, now that I fixed the tests in form_tag_helper_test I got a failing test when any other data attributes were passed in. I fixed by using deep_merge! instead of update and wrote a test to cover the base case as well.

Contributor

DropsOfSerenity commented Aug 6, 2015

Ok yeah, now that I fixed the tests in form_tag_helper_test I got a failing test when any other data attributes were passed in. I fixed by using deep_merge! instead of update and wrote a test to cover the base case as well.

@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 6, 2015

Contributor

ah I see that passing "data-disable-with" to the options has will also result in problems, writing a test and working on a solution.

Contributor

DropsOfSerenity commented Aug 6, 2015

ah I see that passing "data-disable-with" to the options has will also result in problems, writing a test and working on a solution.

@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 6, 2015

Contributor

Alright I've updated the code and written some tests to ensure that a user specified option is not overridden.

Contributor

DropsOfSerenity commented Aug 6, 2015

Alright I've updated the code and written some tests to ensure that a user specified option is not overridden.

@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 7, 2015

Contributor

@kaspth So I've done some benchmarking and applied a fix.

I found that deep_stringify_keys was causing a drop of roughly 30,000 iterations per second, a loss of 37% performance approx.

After changing it to expect the symbol key for the nested hash (which ruby does anyway so it's not necessary to deep stringify) the performance normalized back to 80k ip/s.

# Rails#master
Calculating -------------------------------------
      submit_tag old     7.988k i/100ms
-------------------------------------------------
      submit_tag old     83.042k (± 2.8%) i/s -    415.376k

# Rails with updated submit_tag code
Calculating -------------------------------------
      submit_tag new     7.711k i/100ms
-------------------------------------------------
      submit_tag new     80.468k (± 2.2%) i/s -    408.683k

I think the last thing i'm trying to wrap my head around is that nested hashes by default right now are being stringified only on the base level, and not on the nested level.

e.g.

irb(main):004:0> { "data" => { "disable_with": "Please wait..." } }.stringify_keys
=> {"data"=>{:disable_with=>"Please wait..."}}

irb(main):007:0> {}.update({ "data" => { "disable_with": "Please wait..." } }.stringify_keys)
=> {"data"=>{:disable_with=>"Please wait..."}}

This would mean this is happening for other data attributes being passed in already as well. Am i missing something here?

Contributor

DropsOfSerenity commented Aug 7, 2015

@kaspth So I've done some benchmarking and applied a fix.

I found that deep_stringify_keys was causing a drop of roughly 30,000 iterations per second, a loss of 37% performance approx.

After changing it to expect the symbol key for the nested hash (which ruby does anyway so it's not necessary to deep stringify) the performance normalized back to 80k ip/s.

# Rails#master
Calculating -------------------------------------
      submit_tag old     7.988k i/100ms
-------------------------------------------------
      submit_tag old     83.042k (± 2.8%) i/s -    415.376k

# Rails with updated submit_tag code
Calculating -------------------------------------
      submit_tag new     7.711k i/100ms
-------------------------------------------------
      submit_tag new     80.468k (± 2.2%) i/s -    408.683k

I think the last thing i'm trying to wrap my head around is that nested hashes by default right now are being stringified only on the base level, and not on the nested level.

e.g.

irb(main):004:0> { "data" => { "disable_with": "Please wait..." } }.stringify_keys
=> {"data"=>{:disable_with=>"Please wait..."}}

irb(main):007:0> {}.update({ "data" => { "disable_with": "Please wait..." } }.stringify_keys)
=> {"data"=>{:disable_with=>"Please wait..."}}

This would mean this is happening for other data attributes being passed in already as well. Am i missing something here?

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Aug 7, 2015

Contributor
  1. The default string can be longer then original label of submit. It will take extra space when change the label. It can be a problem in tough designs where there is no empty space.
  2. It needs to be I18n-ed by default.
Contributor

bogdan commented Aug 7, 2015

  1. The default string can be longer then original label of submit. It will take extra space when change the label. It can be a problem in tough designs where there is no empty space.
  2. It needs to be I18n-ed by default.
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 7, 2015

Member

it needs to be I18n-ed by default.

It doesn't. If you need i18n you can just explicitly set disable_with.

Member

rafaelfranca commented Aug 7, 2015

it needs to be I18n-ed by default.

It doesn't. If you need i18n you can just explicitly set disable_with.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 7, 2015

Member

The default string can be longer then original label of submit. It will take extra space when change the label. It can be a problem in tough designs where there is no empty space.

If that is the case they can just override the default.

Member

rafaelfranca commented Aug 7, 2015

The default string can be longer then original label of submit. It will take extra space when change the label. It can be a problem in tough designs where there is no empty space.

If that is the case they can just override the default.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 7, 2015

Member

A problem that I think is: is it possible to remove disable_with now that we are setting a default?

Member

rafaelfranca commented Aug 7, 2015

A problem that I think is: is it possible to remove disable_with now that we are setting a default?

@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 7, 2015

Contributor

Maybe using "Saving..." instead of "Please wait..." would be a little more device friendly?

Contributor

DropsOfSerenity commented Aug 7, 2015

Maybe using "Saving..." instead of "Please wait..." would be a little more device friendly?

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Aug 7, 2015

Member

Based on this implementation, no it doesn't look like there's a way to opt-out. Adding disable_with: false, "data-disable-with": false, or data: { "disable-with": false } should all opt-out of this behavior.

Member

sgrif commented Aug 7, 2015

Based on this implementation, no it doesn't look like there's a way to opt-out. Adding disable_with: false, "data-disable-with": false, or data: { "disable-with": false } should all opt-out of this behavior.

@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 7, 2015

Contributor

Alright, I'll add this functionality :)

Contributor

DropsOfSerenity commented Aug 7, 2015

Alright, I'll add this functionality :)

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Aug 7, 2015

Contributor

No I18n makes the feature only available for en apps. Others will be forced to specify something everywhere in a project. Also I18n makes it possible to disable the feature and make upgrading easier.

Contributor

bogdan commented Aug 7, 2015

No I18n makes the feature only available for en apps. Others will be forced to specify something everywhere in a project. Also I18n makes it possible to disable the feature and make upgrading easier.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 7, 2015

Member

We don't have I18n support right now and I want to keep it out. I18n by default will slow down a lot submit_tag for applications that only use english (I bet it is something like 80% of the apps)

Member

rafaelfranca commented Aug 7, 2015

We don't have I18n support right now and I want to keep it out. I18n by default will slow down a lot submit_tag for applications that only use english (I bet it is something like 80% of the apps)

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Aug 7, 2015

Contributor

@rafaelfranca so what should I do after upgrading? Go through my app and specify disable-with: false everywhere because now it shows unreadable symbols to all my users.

Did you include multi-language apps into those 80%? They would also experience problems.

What about people that don't want a change? How would they disable feature globally without modifying entire app?

I think 80% is overestimated: exclude people that don't want this feature and multi-language apps (<- most successful ones by the way).

It looks bad without ability to globally disable. We should have a way of doing it. Also we need somehow tell people in the internationalisation guide:

Please go to your AppController and extend submit_tag to support I18n because we don't support it out of the box.

I hardly image it will work without I18n. It will be top1 change request.

Contributor

bogdan commented Aug 7, 2015

@rafaelfranca so what should I do after upgrading? Go through my app and specify disable-with: false everywhere because now it shows unreadable symbols to all my users.

Did you include multi-language apps into those 80%? They would also experience problems.

What about people that don't want a change? How would they disable feature globally without modifying entire app?

I think 80% is overestimated: exclude people that don't want this feature and multi-language apps (<- most successful ones by the way).

It looks bad without ability to globally disable. We should have a way of doing it. Also we need somehow tell people in the internationalisation guide:

Please go to your AppController and extend submit_tag to support I18n because we don't support it out of the box.

I hardly image it will work without I18n. It will be top1 change request.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 7, 2015

Member

It looks bad without ability to globally disable.

Agree, good point.. I prefer to have it disabled globally without using i18n, but I don't want to introduce other global switch just for that, so I agree that I18n would solve this and also enable other features. 👍

Member

rafaelfranca commented Aug 7, 2015

It looks bad without ability to globally disable.

Agree, good point.. I prefer to have it disabled globally without using i18n, but I don't want to introduce other global switch just for that, so I agree that I18n would solve this and also enable other features. 👍

@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Aug 7, 2015

Contributor

What about using the button text and just disabling the button? If you need something specific, you can override with a string or i18n otherwise the button is just disabled? Not enough user feedback?

Contributor

derekprior commented Aug 7, 2015

What about using the button text and just disabling the button? If you need something specific, you can override with a string or i18n otherwise the button is just disabled? Not enough user feedback?

@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 7, 2015

Contributor

Since our submit button defaults to "Save Changes" e.g

def submit_tag(value = "Save changes", options = {})

It would seem like it sense to default the text to "Saving..." and allow the user to mess with i18n or define there own disable_with as needed.

Will have some i18n changes coming through soon.

Contributor

DropsOfSerenity commented Aug 7, 2015

Since our submit button defaults to "Save Changes" e.g

def submit_tag(value = "Save changes", options = {})

It would seem like it sense to default the text to "Saving..." and allow the user to mess with i18n or define there own disable_with as needed.

Will have some i18n changes coming through soon.

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Aug 7, 2015

Contributor

I like @derekprior's idea. We should not aim to make a good interface - there is no consensus on what label is the best. If we just disable the button, we fix the technical issue that all of agree that needs to be addressed.

Contributor

bogdan commented Aug 7, 2015

I like @derekprior's idea. We should not aim to make a good interface - there is no consensus on what label is the best. If we just disable the button, we fix the technical issue that all of agree that needs to be addressed.

@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Aug 7, 2015

Contributor

Don't get me wrong - having a conventional place to go to override the
label without code would be great but if it really is a performance concern
then I think disabling only is a fine compromise.

That's in line with what users see today except that the button is
disabled.

On Friday, August 7, 2015, Bogdan Gusiev notifications@github.com wrote:

I like @derekprior https://github.com/derekprior's idea. We should not
aim to make a good interface - there is no consensus on what label is the
best. If we just disable the button, we fix the technical issue that all of
agree that needs to be addressed.


Reply to this email directly or view it on GitHub
#21135 (comment).

Contributor

derekprior commented Aug 7, 2015

Don't get me wrong - having a conventional place to go to override the
label without code would be great but if it really is a performance concern
then I think disabling only is a fine compromise.

That's in line with what users see today except that the button is
disabled.

On Friday, August 7, 2015, Bogdan Gusiev notifications@github.com wrote:

I like @derekprior https://github.com/derekprior's idea. We should not
aim to make a good interface - there is no consensus on what label is the
best. If we just disable the button, we fix the technical issue that all of
agree that needs to be addressed.


Reply to this email directly or view it on GitHub
#21135 (comment).

@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 7, 2015

Contributor

Makes sense, in that case we don't want the i18n anymore since it will just be using the value of the submit tag anyway?

Contributor

DropsOfSerenity commented Aug 7, 2015

Makes sense, in that case we don't want the i18n anymore since it will just be using the value of the submit tag anyway?

@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 7, 2015

Contributor

Alright the feature can now be disabled on a per tag basis and uses the value passed in as it's default value.

The only thing missing now is if/how we want to be able to disable this feature globally.

Contributor

DropsOfSerenity commented Aug 7, 2015

Alright the feature can now be disabled on a per tag basis and uses the value passed in as it's default value.

The only thing missing now is if/how we want to be able to disable this feature globally.

@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 8, 2015

Contributor

What do you guys think about adding a globally configurable action_view settings like so?

# Specify whether submit_tag should automatically disable on click
cattr_accessor :automatically_disable_submit_tag
@@automatically_disable_submit_tag = true

So then users that want to disable the feature for whatever reason can set the preference in application.rb

config.action_view.automatically_disable_submit_tag = false
Contributor

DropsOfSerenity commented Aug 8, 2015

What do you guys think about adding a globally configurable action_view settings like so?

# Specify whether submit_tag should automatically disable on click
cattr_accessor :automatically_disable_submit_tag
@@automatically_disable_submit_tag = true

So then users that want to disable the feature for whatever reason can set the preference in application.rb

config.action_view.automatically_disable_submit_tag = false
@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Aug 8, 2015

Member

I'm 👍 for adding a config to opt out, if only to aid migration.

On Fri, Aug 7, 2015, 8:26 PM Justin notifications@github.com wrote:

What do you guys think about adding a globally configurable action_view
settings like so?

Specify whether submit_tag should automatically disable on click

cattr_accessor :automatically_disable_submit_tag@@automatically_disable_submit_tag = true

So then users that want to disable the feature for whatever reason can set
the preference in application.rb

config.action_view.automatically_disable_submit_tag = false


Reply to this email directly or view it on GitHub
#21135 (comment).

Member

sgrif commented Aug 8, 2015

I'm 👍 for adding a config to opt out, if only to aid migration.

On Fri, Aug 7, 2015, 8:26 PM Justin notifications@github.com wrote:

What do you guys think about adding a globally configurable action_view
settings like so?

Specify whether submit_tag should automatically disable on click

cattr_accessor :automatically_disable_submit_tag@@automatically_disable_submit_tag = true

So then users that want to disable the feature for whatever reason can set
the preference in application.rb

config.action_view.automatically_disable_submit_tag = false


Reply to this email directly or view it on GitHub
#21135 (comment).

@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 8, 2015

Contributor

Added opt out settings + a test for it.

Contributor

DropsOfSerenity commented Aug 8, 2015

Added opt out settings + a test for it.

@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 11, 2015

Contributor

Do I need to do anything further for this?

Contributor

DropsOfSerenity commented Aug 11, 2015

Do I need to do anything further for this?

@kaspth

View changes

Show outdated Hide outdated actionview/lib/action_view/helpers/form_tag_helper.rb
@kaspth

View changes

Show outdated Hide outdated actionview/lib/action_view/helpers/form_tag_helper.rb
@kaspth

View changes

Show outdated Hide outdated actionview/lib/action_view/helpers/form_tag_helper.rb
@kaspth

View changes

Show outdated Hide outdated actionview/lib/action_view/helpers/form_tag_helper.rb
@kaspth

View changes

Show outdated Hide outdated actionview/lib/action_view/helpers/form_tag_helper.rb
@kaspth

View changes

Show outdated Hide outdated actionview/lib/action_view/helpers/form_tag_helper.rb
@kaspth

View changes

Show outdated Hide outdated actionview/lib/action_view/helpers/form_tag_helper.rb
Make disable_with default in submit_tag
Prevents double submission by making disable_with the default.

Default disable_with option will only be applied if user has not
specified her/his own disable_with option, whether that is in the
`data-disable-with` string form or the
`:data => { :disable_with => "Saving..." }` hash form. disable_with
will default to the value attribute.

A configuration option was added to opt out of this functionality if
the user so desires.
`config.action_view.automatically_disable_submit_tag = false`
@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 11, 2015

Contributor

Ok so in summary of recent changes:

  • Used new hash syntax in comment on how to disable.
  • Made an initial check to see if the user has disabled the feature on this specific submit tag before doing work.
  • Updated the logic using ||= now that the case of the user disabling the feature has been taken care of beforehand.
  • Used Hash#delete instead of Hash#extract!
  • Moved explanation of what disable_with defaults to at the end of the doc.
  • Removed unnecessary braces.
Contributor

DropsOfSerenity commented Aug 11, 2015

Ok so in summary of recent changes:

  • Used new hash syntax in comment on how to disable.
  • Made an initial check to see if the user has disabled the feature on this specific submit tag before doing work.
  • Updated the logic using ||= now that the case of the user disabling the feature has been taken care of beforehand.
  • Used Hash#delete instead of Hash#extract!
  • Moved explanation of what disable_with defaults to at the end of the doc.
  • Removed unnecessary braces.
@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 14, 2015

Contributor

Any other ideas guys?

Contributor

DropsOfSerenity commented Aug 14, 2015

Any other ideas guys?

sgrif added a commit that referenced this pull request Aug 17, 2015

Merge pull request #21135 from DropsOfSerenity/master
make disable_with default in submit_tag

@sgrif sgrif merged commit 249a06d into rails:master Aug 17, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Aug 17, 2015

Member

Can you add this issue number to the upgrade guide meta issue?

Member

sgrif commented Aug 17, 2015

Can you add this issue number to the upgrade guide meta issue?

@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 19, 2015

Contributor

@sgrif Where can I find that?

Contributor

DropsOfSerenity commented Aug 19, 2015

@sgrif Where can I find that?

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Aug 19, 2015

Member

Just did it. Was on my phone earlier and didn't have time to search.

Member

sgrif commented Aug 19, 2015

Just did it. Was on my phone earlier and didn't have time to search.

@DropsOfSerenity

This comment has been minimized.

Show comment
Hide comment
@DropsOfSerenity

DropsOfSerenity Aug 19, 2015

Contributor

Ah thankyou I kind of had no idea what you were talking about since this is my first time contributing to rails :P but thanks :D

Contributor

DropsOfSerenity commented Aug 19, 2015

Ah thankyou I kind of had no idea what you were talking about since this is my first time contributing to rails :P but thanks :D

tjgrathwell added a commit to railsbridge/bridge_troll that referenced this pull request Sep 22, 2015

Add "disable_with" to most form submits to prevent double submission
Should be able to remove when upgrading to Rails 5 thanks to
rails/rails#21135
@claudiob

This comment has been minimized.

Show comment
Hide comment
@claudiob

claudiob Mar 18, 2016

Member

It looks like this does not work with the button_to helper, where the generated code is something on the line of:

<form class="button_to" method="post" action="">
  <button type="submit">…</button>
</form>

@DropsOfSerenity @sgrif I think we should make that happen… what do you think?

Member

claudiob commented Mar 18, 2016

It looks like this does not work with the button_to helper, where the generated code is something on the line of:

<form class="button_to" method="post" action="">
  <button type="submit">…</button>
</form>

@DropsOfSerenity @sgrif I think we should make that happen… what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment