code-corps / code-corps/code-corps-ember

Use ember-concurrency in submit buttons throughout the app

Offen
#507 7 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
bug Difficulty: Medium Priority: High Skill: ember-cli-qunit Skill: Ember.js Skill: JavaScript Skill: Testing
Vorherrschende Sprache
JavaScript
Sterne
120
Forks
75
PR-Merge-Kennzahlen
Keine gemergten PRs in 30 T.

Beschreibung

# 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.

Beitragsleitfaden

Beitragsleitfaden öffnen

Rechercherichtung

Beginne mit mixins/onboarding-controller.js und den aufgeführten Komponenten und vergleiche die bestehende Handhabung von isSaving und isLoading mit den submit- und form-Aktionen. Entscheide, wie das async-Schaltflächenverhalten auf die identifizierten Instanzen angewendet werden soll, und überprüfe anschließend, dass wiederholte Übermittlungen verhindert werden und die Lade- oder deaktivierten Zustände während der gesamten betroffenen Abläufe korrekt bleiben.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Bewertung

Tech-Stack
javascript
Bereich
frontend
Issue-Typ
Refactoring
Schwierigkeit
5/5
Geschätzter Aufwand
Über eine Woche
Aktivitätsstatus
Veraltet
Klarheit
Größtenteils klar
Anfängerfreundlichkeit
25/100

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.