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

Add ability to refresh the user identity in useGetIdentity #8372

Merged
merged 8 commits into from
Nov 14, 2022

Conversation

fzaninotto
Copy link
Member

@fzaninotto fzaninotto commented Nov 8, 2022

Problem

If an application contains a form letting the current user update their name and/or avatar, developers may want to refresh the identity after the form is submitted. It is currently cumbersome as it implies adding a context on top of the app (cf https://marmelab.com/blog/2020/12/14/react-admin-v3-userprofile.html)

Solution

Use react-query to fetch the authProvider in useGetIdentity

Sample code

const IdentityForm = () => {
    const { isLoading, error, identity, refetch } = useGetIdentity();
    const [newIdentity, setNewIdentity] = useState('');

    if (isLoading) return <>Loading</>;
    if (error) return <>Error</>;

    const handleChange = event => {
        setNewIdentity(event.target.value);
    };
    const handleSubmit = (e) => {
        e.preventDefault();
        if (!newIdentity) return;
        fetch('/update_identity', {
            method: 'POST',
            headers: { 'Content-Type': 'application/json' },
            body: JSON.stringify({ identity: newIdentity })
        }).then(() => { 
            // call authProvider.getIdentity() again and notify the listeners of the result,
            // including the UserMenu in the AppBar
            refetch();
         });
    };
    
    return (
        <form onSubmit={handleSubmit}>
            <input defaultValue={identity.fullName} onChange={handleChange} />
            <input type="submit" value="Save" />
        </form>
    );
};

@fzaninotto
Copy link
Member Author

fzaninotto commented Nov 8, 2022

I considered exposing the refetch from react-query in the useGetPermissions response instead. I still hesitate, and I'm open to comments.

Edit: I finally opted in to using refetch, it's simpler to use and explain.

@fzaninotto fzaninotto changed the title Add ability to refresh the user identity Add ability to refresh the user identity in useGetIdentity Nov 9, 2022
docs/useGetIdentity.md Outdated Show resolved Hide resolved
Co-authored-by: Aníbal Svarcas <[email protected]>
Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes requests, otherwise I approve the main changes

docs/useGetIdentity.md Outdated Show resolved Hide resolved
packages/ra-core/src/auth/useGetIdentity.spec.tsx Outdated Show resolved Hide resolved
packages/ra-core/src/auth/useGetIdentity.stories.tsx Outdated Show resolved Hide resolved
@slax57 slax57 added this to the 4.6 milestone Nov 14, 2022
@slax57 slax57 merged commit 7d90cea into next Nov 14, 2022
@slax57 slax57 deleted the useGetIdentity-react-query branch November 14, 2022 09:31
const authProvider = useAuthProvider();
useEffect(() => {
if (authProvider && typeof authProvider.getIdentity === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing typeof authProvider.getIdentity === 'function' check causes a tiny BC break.

We found there are console errors after upgrading pass 4.6 because we have never implemented the getIdentity method since it was introduced in #5180

In the previous pull request it says

The change is backward compatible (if the authProvider.getIdentity() method isn't implemented, the admin shows an anonymous user avatar with no name, as before).

In v4 upgrade guide it did not mention this method is now required to be implemented. I believe we should now add a note about that breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be very easy for us to catch the issue and implement the method.

Just for other users they should be aware when they upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out!
The getIdentity function should remain optional, even in v4, so I believe we should reintroduce this check to avoid errors. I'll open a new PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is: #8509

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants