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

Fix useCanAccess causes extra rerender + use useCanAccess in the simple demo #10278

Merged
merged 9 commits into from
Oct 17, 2024

Conversation

slax57
Copy link
Contributor

@slax57 slax57 commented Oct 15, 2024

Problems

  1. The React Admin simple blog demo uses usePermissions. But React Admin v5.3 will bring a better replacement feature called useCanAccess, which we should demonstrate in this demo.
  2. useCanAccess causes an unnecessary extra rerender to components using it (e.g. EditButton or ShowButton) when there is no authProvider or no authProvider.canAccess implementation.

Solution

  1. Replace usePermissions by useCanAccess / <CanAccess> in the simple demo.
  2. Enable the react-query query in useCanAccess only when there is an actual authProvider.canAccess implementation (otherwise it creates an extra rerender because of the query status update).

How To Test

  1. Run the simple demo (make run).
    Try signing in as:
  • login
  • user
  • admin
  1. Run the useCanAccess unit tests

Todo

  • Disable the useCanAccess query when there is no authProvider.canAccess as it causes unnecessary rerenders!
  • In the simple example, change the way authProvider.canAccess is implemented to return true by default, and call a fn stored in memory that will actually implement the canAccess for a given role

Additional Checks

  • The PR targets master for a bugfix, or next for a feature
  • The PR includes unit tests (if not possible, describe why)
  • The PR includes one or several stories (if not possible, describe why)
  • The documentation is up to date

Also, please make sure to read the contributing guidelines.

Additional Notes

Current use of the permissions in the demo

role

  • login
    • none
  • user
    • <Resource name="users" {...users} />
  • admin
    • <Resource name="users" {...users} />
    • PostCreate: <ArrayInput source="authors">
    • PostEdit: <ArrayInput source="authors">
    • UserCreate: <SaveButton label="user.action.save_and_add" />
    • UserCreate: <TabbedForm.Tab label="user.form.security" path="security">
    • UserEdit: <TextInput source="id" InputProps={{ disabled: true }} />
    • UserEdit: <TabbedForm.Tab label="user.form.security" path="security">
    • UserList: filters.push(<TextInput source="role" />)
    • UserList: rowClick = 'edit'
    • UserList: <SimpleList secondaryText={record.role}>
    • UserList: Datagrid: <TextField source="role" />
    • UserShow: <TabbedShowLayout.Tab label="user.form.security">

Issues found while testing

  • UserCreate: <TabbedForm.Tab label="user.form.security" path="security">
    • error: the value prop is required at runtime
    • TabbedForm does not support <CanAccess>, because it still clones children
    • => No choice but to use useCanAccess instead
  • UserCreate: Toolbar is misplaced
    • Already exists on master

@slax57 slax57 added the WIP Work In Progress label Oct 15, 2024
@slax57 slax57 changed the title [Demo] Use useCanAccess in the simple demo Fix useCanAccess causes extra rerender + use useCanAccess in the simple demo Oct 17, 2024
@slax57 slax57 added RFR Ready For Review and removed WIP Work In Progress labels Oct 17, 2024
@@ -70,7 +76,7 @@ const UserCreate = () => {
validate={[required(), isValidName, unique()]}
/>
</TabbedForm.Tab>
{permissions === 'admin' && (
{canManageUsers ? (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could not use <CanAccess> here because <TabbedForm> still scans its children, so having <CanAccess> as one of its child couldn't work.

@@ -153,7 +152,7 @@ const PostCreate = () => {
<TextInput source="url" defaultValue="" />
</SimpleFormIterator>
</ArrayInput>
{permissions === 'admin' && (
<CanAccess action="manage_post_authors">
Copy link
Member

Choose a reason for hiding this comment

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

This action name is actually an action and a resource... Why not use action="edit" resource="users"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my first implementation I did this, but then I realized that for such simple access rules we don't need that much complexity, and it allows me to omit the resource prop completely, making the code easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up implementing a mixed approach: keeping a 'custom' action for things like 'batch_create', and using resource for fields like 'posts.authors' and 'users.role'.

@slax57
Copy link
Contributor Author

slax57 commented Oct 17, 2024

Seems I need to fix the E2E tests. Back to WIP.

@slax57 slax57 added WIP Work In Progress and removed RFR Ready For Review labels Oct 17, 2024
@slax57
Copy link
Contributor Author

slax57 commented Oct 17, 2024

All green, back to RFR!

@slax57 slax57 added RFR Ready For Review and removed WIP Work In Progress labels Oct 17, 2024
@djhi djhi requested a review from fzaninotto October 17, 2024 10:35
Comment on lines 90 to 92
getPermissions: () => {
const role = localStorage.getItem('role');
return Promise.resolve(role ?? '');
return Promise.resolve();
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove this now, right? The getPermissions method is optional now

@djhi djhi merged commit 4842169 into next Oct 17, 2024
14 checks passed
@djhi djhi deleted the canAccess-demos branch October 17, 2024 12:30
@fzaninotto fzaninotto added this to the 5.3.0 milestone Oct 18, 2024
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.

3 participants