[bug] Display user permissions even if the GQL resolver has errors#57375
Conversation
There was a problem hiding this comment.
Can we write a test for this?
There was a problem hiding this comment.
very nice changelog entry!
There was a problem hiding this comment.
Should we get a query plan for this on scaletesting or so, to avoid regressions? Just to be safe.
There was a problem hiding this comment.
Also, I wish a test had to be adjusted after this change 😢 Can we at least add a new test that verifies this new behavior? :)
There was a problem hiding this comment.
In an ideal world, we would check for the error at this nodes path and make sure it is actually a no access error - otherwise errors from loading the repo or so could bubble up here as a "no access error"
that said, I don't think we are generally following a ton of great practices around apollo so don't feel obligated to be the first one to care 😬
5ad8e7e to
ff154de
Compare
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 52be312...38c3fef.
|
| repoID, err := graphqlbackend.UnmarshalRepositoryID(graphql.ID(cursor)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| cursorSQL := strconv.Itoa(int(repoID)) |
There was a problem hiding this comment.
Don't know why this was based on repo name instead of ID, as that is what you'd have access to for the cursor
| const debouncedQuery = useDebounce(query, 300) | ||
|
|
||
| const { connection, data, loading, error, refetch, variables, ...paginationProps } = usePageSwitcherPagination< | ||
| const { connection, data, loading, refetch, variables, ...paginationProps } = usePageSwitcherPagination< |
There was a problem hiding this comment.
was error unused? I don't see where a reference to it was removed in this diff 🤔
| type PermissionsInfoRepositoryResolver interface { | ||
| ID() graphql.ID | ||
| Repository() *RepositoryResolver | ||
| Repository(ctx context.Context) (*RepositoryResolver, error) |
There was a problem hiding this comment.
The fact that it was already nullable kind of convinced me that this is the right change :D
Fixes an issue where admins can't view a user's permissions if they don't have access to all of the repos a user has access to.
Previously the page would just load forever, now admins are told they can't see the repo:
Test plan
Manual tests