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

Prevent duplicate form submission in Create realm dialog in admin ui #28256

Open
thomasdarimont opened this issue Mar 27, 2024 · 6 comments · May be fixed by #28701
Open

Prevent duplicate form submission in Create realm dialog in admin ui #28256

thomasdarimont opened this issue Mar 27, 2024 · 6 comments · May be fixed by #28701

Comments

@thomasdarimont
Copy link
Contributor

Description

The current "Create realm" dialog in the admin-ui does allow clicking the "Create" button multiple times, if the server response is slow. This yields error messages like the following, where the first "Create realm" request succeeds, but the second attempt failes with a conflict error.
image

Discussion

No response

Motivation

This problem happens quite often in practice, especially during Keycloak demos when one wants to create a realm quickly.

Details

We should disable the "Create" button while the request is pending.

@thomasdarimont thomasdarimont added kind/enhancement Categorizes a PR related to an enhancement status/triage labels Mar 27, 2024
@thomasdarimont thomasdarimont changed the title Prevent double post form submission in Create realm dialog in admin ui Prevent duplicate form submission in Create realm dialog in admin ui Mar 27, 2024
@jonkoops
Copy link
Contributor

Yeah, ideally we'd do this consistently across UIs. We should have some kind of re-usable component or function to handle all this stuff. It looks like React Form Hook accepts a Promise return value in the handleSubmit() method, which leads me to believe it has some kind of state around this we could use.

@jonkoops
Copy link
Contributor

Took a quick look, and it seems we should take several of the fields in the FormState into account. I whipped up this example:

import { type SubmitHandler, useForm, FormState } from 'react-hook-form';

const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));

const isSubmittable = (formState: FormState<any>) => {
  return (
    formState.isDirty &&
    formState.isValid &&
    !formState.isLoading &&
    !formState.isValidating &&
    !formState.isSubmitting
  );
};

interface FormData {
  username: string;
}

export default function App() {
  const {
    register,
    handleSubmit,
    formState,
  } = useForm<FormData>();

  const onSubmit: SubmitHandler<FormData> = async (data) => {
    await sleep(2000);
    console.log(data);
  };

  return (
    <form onSubmit={handleSubmit(onSubmit)}>
      <label htmlFor="username">User Name</label>
      <input {...register('username')} placeholder="Enter a username" />
      <button type="submit" disabled={!isSubmittable(formState)}>Submit</button>
    </form>
  );
}

We should extract the isSubmittable() function here into a separate utility, so that it can be used in all forms to handle the disabling of the submit button. It might perhaps even make sense to introduce a PR upstream to React Form Hook to add a isSubmittable field to the FormState, not sure how amicable they would be to that.

@thomasdarimont
Copy link
Contributor Author

That looks good! I think many forms are affected by this and that this is a common "error" that puzzles users.

It seems just like a minor improvement, but making this more robust can help improve the trust of users in Keycloak.

@jonkoops
Copy link
Contributor

Agreed, should also not be too hard to put this in a utility of ui-shared and use if everywhere, if someone wants to go ahead and pick this up that would be a nice quality of life enhancement.

@jonkoops jonkoops added this to the Backlog milestone Mar 28, 2024
@jhchong92
Copy link
Contributor

Hi, can I try to take this issue?

Thank you for the sample code provided @jonkoops. Instead of creating a utility method isSubmittable#formState, I would opt for creating a reusable component FormSubmitButton.

import { Button, ButtonProps } from "@patternfly/react-core"
import { PropsWithChildren } from "react"
import { FieldValues, FormState } from "react-hook-form"

export type FormSubmitButtonProps = ButtonProps & {
  formState: FormState<FieldValues>
}

const isSubmittable = (formState: FormState<FieldValues>) => {
  return (
    formState.isDirty &&
    formState.isValid &&
    !formState.isLoading &&
    !formState.isValidating &&
    !formState.isSubmitting
  );
};

export const FormSubmitButton = ({ formState, children, ...rest }
  : PropsWithChildren<FormSubmitButtonProps>) => {
    
  return <Button {...rest} type="submit" isDisabled={!isSubmittable(formState)}>
    {children}
  </Button>
}

I think this provides more flexibility and reusability compared to a utility method.

@jonkoops
Copy link
Contributor

jonkoops commented Apr 2, 2024

That's a great idea @jhchong92. We're trying to reduce the amount of repetition in our form code, so that would certainly be beneficial.

jhchong92 added a commit to jhchong92/keycloak that referenced this issue Apr 2, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
jhchong92 added a commit to jhchong92/keycloak that referenced this issue Apr 2, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
jhchong92 added a commit to jhchong92/keycloak that referenced this issue Apr 14, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
@jhchong92 jhchong92 linked a pull request Apr 14, 2024 that will close this issue
jhchong92 added a commit to jhchong92/keycloak that referenced this issue Apr 14, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
jhchong92 added a commit to jhchong92/keycloak that referenced this issue Apr 14, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
jhchong92 added a commit to jhchong92/keycloak that referenced this issue Apr 14, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
jhchong92 added a commit to jhchong92/keycloak that referenced this issue Apr 16, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
jonkoops pushed a commit to jhchong92/keycloak that referenced this issue Apr 16, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
jhchong92 added a commit to jhchong92/keycloak that referenced this issue Apr 16, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
jhchong92 added a commit to jhchong92/keycloak that referenced this issue Apr 16, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
jhchong92 added a commit to jhchong92/keycloak that referenced this issue Apr 18, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
jhchong92 added a commit to jhchong92/keycloak that referenced this issue May 1, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
jhchong92 added a commit to jhchong92/keycloak that referenced this issue May 7, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
jhchong92 added a commit to jhchong92/keycloak that referenced this issue May 9, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
jhchong92 added a commit to jhchong92/keycloak that referenced this issue May 9, 2024
closes keycloak#28256

Signed-off-by: jchong <jhchong92@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants