Use ember-concurrency in submit buttons throughout the app

[joshsmith]

Problem

Many instances of the app where you're POST-ing to a server you can hit the submit button multiple times. The button is not disabled and the form submission is a repeatable process.

This is bad because the app can get into a weird state, like in onboarding where multiple PATCH requests may come through and the state_transition is outdated by the time later requests hit the server.

Subtasks

• Identify all instances where a button does not become disabled or show proper loading state to the user
• Create a proper async button (or use an external library)
• Replace instances of these buttons with the new behavior
• Test, test, test

Identified instances where we don't handle it:

• start/{hello, interests, skills, expertise}.hbs - can spam click next step, will fail due to trying to transition into invalid state. Should probably bind disable to model.isSaving, or we can just have the continue action return if model is currently saving. Other option might even be better, since short flashes of a disabled state on the button can be perceived badly. The continue action is defined in mixins/onboarding-controller.js
• components/create-comment-form - has just a class binding: class="default small right {{if comment.isSaving "disabled"}}" which is not enough. User can still click, it just looks like they can't. Pretty sure you can end up creating multiple identical comments by spam-clicking here.
• components/editor-with-preview - can spam click the "preview" tab, causing multiple previews to get generated. Doesn't really affect much, but no point in making overhead, so we should fix it. One cleaner way I can think of is to either render different set of tab headers based on current state (editing or previewing), where only one of the headers is bound to an action in each state. The other is to simply return from the action if isLoading (which we already have as a flag) is true. I don't think binding to a disabled state is a great idea, since the button is styled as a tab, not a regular UI button.
• components/login-form bind to a form action, instead of binding the form button directly. There is no model to deal with, so our best bet is a isLoggingIn flag, to disable/enable the button.
• components/member-list-item - organization admins can approve or deny a pending membership here. Neither of the two buttons are being handled. We have the membership model, so we can bind disabled to model.isSaving
• components/organization-settings-form - save button is not handled. It's an update, so there's little danger of doing something wrong, but we should prevent spam none-the-less, probably by binding disabled to organization.isSaving
• components/project-details - non-member can join organization here. The "Join Project" button will quickly switch to "Membership Pending", but there is a small window where the user could conceivably spam-click and issue multiple "create" requests. Since it's a create request, we don't have a model, so an isLoading flag would work. Model could work as well, if we assign is as a component property when initialising.
• components/project-long-description - the "Save" button beneath the markdown editor is not handled. Since it does an update, we have the projects model.isSaving to rely on. This is another one of those where spamming will likely work fine, but better to prevent it anyway.
• components/project-settings-form - same as previous. "Save" button needs handling. Can do it by binding to project.isSaving.
• components/project-tasks-list - most filter buttons aren't being handled properly, but I don't really think it's necessary in this case
• components/signup-form - no handling at all. We need a flag here, since there's no model, or we need to assign the newly created user as a component property and bind disabled to user.isSaving
• components/task-details - "Save" (saveTaskBody) isn't being handled. Not really important since it's an update, but should prevent spam anyway. We have a task model, so we bind to task.isSaving
• components/task-new-form - "Submit" is not handled. Creates a new task, so we either need an isLoading flag, or we need to assign the newly created task to the component and bind to newTask.isSaving
• components/task-title - Save button is not handled. We should bind to task.isSaving to prevent spam, though spam won't cause errors, just unnecessary to have it.
• components/user-settings-form - Save button is not handled. We can bind to user.isSaving to prevent spam, though spam probably won't cause errors.

Where we do handle it

• components/category-item - we have an isLoading flag on the component. Button to add/remove category is disabled if the flag is on
• components/comment-item - (for displaying and editing an existing comment) - Has an {{#if comment.isSaving}} block to hide the "save" and "cancel" buttons while the save is in progress.
• components/role-item - handled by binding disabled to an isLoading flag, which we also use to show a spinner.
• components/skill-button - handled by binding disabled to an isLoading flag, which we also use to show a spinner.

Where it seems like it does, but doesn't need handling

• components/task-status-button - Toggles between closed or open. Doesn't need handling because the state changes before save.

Anything else

This description was written awhile ago, so it's possible there are more places to implement this.

[WenInCode]

We had a similar issue in an app I worked on elsewhere. Their approach was to create a mixin to prevent double saves. It was pretty nice as you could plug it pretty much anywhere. But the saving was pretty consistent everywhere as well. Just wanted to mention it.

[joshsmith]

@WenInCode not a bad idea. If you have some thoughts on whether that's workable here (after we identify those instances so we know our baseline) that would be great.

[sbatson5]

We had a similar issue on our app. A really simple/easy solution we found was binding the disabled attribute of our button to the isSaving property of the model. Our example only had 1 model on the page, but it seemed to work well.

[joshsmith]

@sbatson5 that's a pretty solid approach, too. Thanks for that!

[sbatson5]

Happy to PR something this weekend 👍 . I love this hacktoberfest stuff! I just come across this repo and as an Elixir/Ember dev, this sounds great!

[joshsmith]

@sbatson5 feel free to stop by our Slack and 👋 hello, too!

[begedin]

Identified instances where we don't handle it:

• start/{hello, interests, skills, expertise}.hbs - can spam click next step, will fail due to trying to transition into invalid state. Should probably bind disable to model.isSaving, or we can just have the continue action return if model is currently saving. Other option might even be better, since short flashes of a disabled state on the button can be perceived badly. The continue action is defined in mixins/onboarding-controller.js
• components/create-comment-form - has just a class binding: class="default small right {{if comment.isSaving "disabled"}}" which is not enough. User can still click, it just looks like they can't. Pretty sure you can end up creating multiple identical comments by spam-clicking here.
• components/editor-with-preview - can spam click the "preview" tab, causing multiple previews to get generated. Doesn't really affect much, but no point in making overhead, so we should fix it. One cleaner way I can think of is to either render different set of tab headers based on current state (editing or previewing), where only one of the headers is bound to an action in each state. The other is to simply return from the action if isLoading (which we already have as a flag) is true. I don't think binding to a disabled state is a great idea, since the button is styled as a tab, not a regular UI button.
• components/login-form bind to a form action, instead of binding the form button directly. There is no model to deal with, so our best bet is a isLoggingIn flag, to disable/enable the button.
• components/member-list-item - organization admins can approve or deny a pending membership here. Neither of the two buttons are being handled. We have the membership model, so we can bind disabled to model.isSaving
• components/organization-settings-form - save button is not handled. It's an update, so there's little danger of doing something wrong, but we should prevent spam none-the-less, probably by binding disabled to organization.isSaving
• components/project-details - non-member can join organization here. The "Join Project" button will quickly switch to "Membership Pending", but there is a small window where the user could conceivably spam-click and issue multiple "create" requests. Since it's a create request, we don't have a model, so an isLoading flag would work. Model could work as well, if we assign is as a component property when initialising.
• components/project-long-description - the "Save" button beneath the markdown editor is not handled. Since it does an update, we have the projects model.isSaving to rely on. This is another one of those where spamming will likely work fine, but better to prevent it anyway.
• components/project-settings-form - same as previous. "Save" button needs handling. Can do it by binding to project.isSaving.
• components/project-tasks-list - most filter buttons aren't being handled properly, but I don't really think it's necessary in this case
• components/signup-form - no handling at all. We need a flag here, since there's no model, or we need to assign the newly created user as a component property and bind disabled to user.isSaving
• components/task-details - "Save" (saveTaskBody) isn't being handled. Not really important since it's an update, but should prevent spam anyway. We have a task model, so we bind to task.isSaving
• components/task-new-form - "Submit" is not handled. Creates a new task, so we either need an isLoading flag, or we need to assign the newly created task to the component and bind to newTask.isSaving
• components/task-title - Save button is not handled. We should bind to task.isSaving to prevent spam, though spam won't cause errors, just unnecessary to have it.
• components/user-settings-form - Save button is not handled. We can bind to user.isSaving to prevent spam, though spam probably won't cause errors.

Where we do handle it

• components/category-item - we have an isLoading flag on the component. Button to add/remove category is disabled if the flag is on
• components/comment-item - (for displaying and editing an existing comment) - Has an {{#if comment.isSaving}} block to hide the "save" and "cancel" buttons while the save is in progress.
• components/role-item - handled by binding disabled to an isLoading flag, which we also use to show a spinner.
• components/skill-button - handled by binding disabled to an isLoading flag, which we also use to show a spinner.

Where it seems like it does, but doesn't need handling

• components/task-status-button - Toggles between closed or open. Doesn't need handling because the state changes before save.