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

Admin UI: fixed no-access indicators not showing up in list table #2939

Merged
merged 1 commit into from
May 13, 2020

Conversation

Vultraz
Copy link
Contributor

@Vultraz Vultraz commented May 11, 2020

The list query doesn't include an error argument at all. It looks something like this:

{
      
      allTodos(first: 20 orderBy: "name_ASC") {
        id _label_
        name updatedAt
      }
      _allTodosMeta(search: "") { count }
    }

Plus, I cannot actually find any other place where errors are part of the query data result instead of in query.errors. This looks like it's meant to gray out specific items from the list table you don't have access to, but in my testing you simply just don't see those items at all.

@changeset-bot
Copy link

changeset-bot bot commented May 11, 2020

🦋 Changeset is good to go

Latest commit: 4cfedbb

We got this.

This PR includes changesets to release 1 package
Name Type
@keystonejs/app-admin-ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jesstelford
Copy link
Contributor

There are two ways to restrict access to a field:

keystone.createList('Things', {
  fields: {
    name: { type: Text, access: false },
    title: { type: Text, access: () => false },
  }
});

In the first form (for name), the field should simple disappear from the Admin UI.

In the second form (for title), should field should still be visible, but greyed out in the Admin UI? Or should it also disappear completely?

If it's to disappear completely, this change seems reasonable. Otherwise, we might need to keep those errors around to catch the case where access control is determined dynamically based on the function passed to access.

@Vultraz
Copy link
Contributor Author

Vultraz commented May 12, 2020

That's field access, though. The code in question here was part of the list query and applies to the list table.

Fields to which you do not have access are indeed already not editable as you say (though they're not actually grayed out/disabled, you just can't - there's #2258 for that)

@gautamsi
Copy link
Member

gautamsi commented May 12, 2020

@jesstelford my thoughts on improving over the functional access control.

there is two way to use this functional access thing.

  1. use GraphQL to provide access control information to admin ui, I see there is already plan in - Return meta data for Select field from GraphQL API #2679 (comment), caveat
    • it would not help evaluate access based on current item, only authedItem would be accessible.
    • can be very complex to evaluate the current Item or related item in the access control function.
  2. use UI hooks api to provide admin-ui way to evaluate access control. caveats:
    • can be tedious as you have to create access control separately for admin-ui.
  3. use expression-match or similar lib to use templating to determine access control in backend as well as server. I did that with dependsOn PR add dependsOn option in field which is same as v4 #2260

@Vultraz Vultraz force-pushed the admin-ui-remove-queryerrors branch from 32a2938 to 4cfedbb Compare May 13, 2020 03:47
@Vultraz Vultraz changed the title Admin UI: removed non-functional use of errors in the list data result Admin UI: fixed no-access indicators not showing up in list table May 13, 2020
@Vultraz
Copy link
Contributor Author

Vultraz commented May 13, 2020

Update: turns out I just needed to actually grab the errors from the right source and format them properly and everything worked again!
image

@jesstelford jesstelford merged commit 15943a1 into keystonejs:master May 13, 2020
@github-actions github-actions bot mentioned this pull request May 13, 2020
@Vultraz Vultraz deleted the admin-ui-remove-queryerrors branch May 13, 2020 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants