code-corps / code-corps/code-corps-ember
Use ember-concurrency in submit buttons throughout the app
- Dominant language
- JavaScript
- Stars
- 120
- Forks
- 75
- PR merge metrics
- No merged PRs in 30d
Description
# 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
- [x] 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.
Contributor guide
Assessment
This issue has not been assessed yet.