rusackas commented on PR #32995:
URL: https://github.com/apache/superset/pull/32995#issuecomment-4988336942
Thanks for the continued grind on this one, @fnardin-maystreet, it's really
close. CI is green and the earlier notes are all knocked out. One thing I hit
before I can approve though, and it is a real behavior gap rather than a nit.
The edit/delete gate keys off `isUserEditorOrAdmin(user, original.editors)`,
but the `user` handed in on all three list pages (and the card views) is the
narrow `{ userId }` prop. `isUserAdmin` needs the full user with `roles`, so
with only a `userId` it always returns false and the gate collapses to
editors-only. So an Admin who is not explicitly an editor/owner of a given
chart/dashboard/dataset now sees Edit and Delete disabled, where today they can
edit anything. That is a regression on a pretty common admin workflow.
The fix should be small, since the full user is already right there. In
`ChartList`/`DashboardList` you are pulling `roles` off `state.user` via
`useSelector` a few lines up, so feeding that user (or its roles) into
`isUserEditorOrAdmin` instead of the `{ userId }` prop should restore admin
access. The card props would need the same.
Worth a test for it too. I noticed the DatasetList spec swapped
`mockAdminUser` for `mockEditorUser`, which sidesteps this rather than covering
it. A case with a non-editor admin asserting the buttons stay enabled would
lock it in.
Holler if you want a hand wiring the admin user through, happy to push a
commit. It also needs a rebase on current `master` while you are in there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
