Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

[bug] Fix user permissions page with no external accounts#57372

Merged
pjlast merged 6 commits into
mainfrom
pjlast/fix-unsynced-user-permissions-page
Oct 5, 2023
Merged

[bug] Fix user permissions page with no external accounts#57372
pjlast merged 6 commits into
mainfrom
pjlast/fix-unsynced-user-permissions-page

Conversation

@pjlast

@pjlast pjlast commented Oct 5, 2023

Copy link
Copy Markdown
Contributor

If a user has had no effective permission syncs (like having no external accounts connected), the user permissions page would not work. This PR adds a case for having had no permission syncs.

Before:

Screenshot 2023-10-05 at 12 01 55

After:

Screenshot 2023-10-05 at 12 12 43

Test plan

Manual verification + tests should still pass.

@cla-bot cla-bot Bot added the cla-signed label Oct 5, 2023
@pjlast pjlast requested a review from a team October 5, 2023 10:19
@pjlast pjlast enabled auto-merge (squash) October 5, 2023 10:20
@sourcegraph-bot

sourcegraph-bot commented Oct 5, 2023

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 3a9aff1...0d78024.

Notify File(s)
@unknwon cmd/frontend/internal/authz/resolvers/permissions_info.go
cmd/frontend/internal/authz/resolvers/resolver.go

@sourcegraph-bot

sourcegraph-bot commented Oct 5, 2023

Copy link
Copy Markdown
Contributor

📖 Storybook live preview

@eseliger eseliger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Was the page erroring before because there was no match in the permsSourceMap or what was the problem? The fix isn't immediately obvious to me 🤔

@eseliger eseliger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copying in the root cause that Petri identified:

Basically, even though r.source is a pointer, it's never nil. So the resolver tried to "unmarshal" the empty string to one of the enum options, instead of setting it to null like it should be

Also, this reveals that we don't render error messages on this page at all and just keep spinning - which we should fix separately :)

@pjlast pjlast disabled auto-merge October 5, 2023 12:50
@pjlast pjlast merged commit 52be312 into main Oct 5, 2023
@pjlast pjlast deleted the pjlast/fix-unsynced-user-permissions-page branch October 5, 2023 13:14
sourcegraph-release-bot pushed a commit that referenced this pull request Oct 5, 2023
keegancsmith added a commit that referenced this pull request Oct 16, 2023
…nts (#57379)

[bug] Fix user permissions page with no external accounts (#57372)

(cherry picked from commit 52be312)

Co-authored-by: Petri-Johan Last <petri.last@sourcegraph.com>
Co-authored-by: Keegan Carruthers-Smith <keegan.csmith@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants