Add modal to remind users to connect external accounts#61721
Conversation
…/60616-account-connection-ux
…/60616-account-connection-ux
…/60616-account-connection-ux
eseliger
left a comment
There was a problem hiding this comment.
left a few questions inline, will continue review tmr
| const [seenAuthzProviders, setSeenAuthzProviders] = useTemporarySetting('user.seenAuthProviders') | ||
|
|
||
| useEffect(() => { | ||
| const externalAccountsModalVisible = shouldShowExternalAccountsModal( | ||
| window.context.authProviders, | ||
| seenAuthzProviders | ||
| ) | ||
| setExternalAccountsModalVisible(externalAccountsModalVisible) | ||
| }, [seenAuthzProviders]) |
There was a problem hiding this comment.
this logic split seems slightly weird to me, we import the shouldShow function from the modal into here, to then not render the modal.
Could we instead move this into the modal and do a !show return null ?
There was a problem hiding this comment.
So I don't like this either, but because the Apollo lib uses React hooks, and React hooks can't be called conditionally, it leads to extra GraphQL queries being made when we could determine earlier that we're not going to render the modal.
Perhaps we can pull out the inner component of the modal into a sub-component? And then at least encapsulate all the logic into one file.
There was a problem hiding this comment.
Yep I think that's the react-y way to go :)
| type AuthzProviderResolver interface { | ||
| URN() string | ||
| ServiceType() string | ||
| ServiceID() string | ||
| } |
There was a problem hiding this comment.
the external accounts page uses this query here: https://sourcegraph.sourcegraph.com/-/editor?remote_url=github.com%2Fsourcegraph%2Fsourcegraph&branch=es%2Fhoney-ms&file=client%2Fweb%2Fsrc%2Fuser%2Fsettings%2Fbackend.tsx&editor=VSCode&version=2.2.16&start_row=49&start_col=0&end_row=68&end_col=0
Why do we need a new resolver here, or what can that one not provide?
There was a problem hiding this comment.
The external accounts page lists all auth providers. We're interested in only Authz providers, i.e. auth providers for which there is also a corresponding code host connection that has authorization enabled.
There was a problem hiding this comment.
Aha!
I wonder if we could perhaps add a field to the resolver that's linked that indicates "required for authz" or so 🤔 That way, we could also add an info icon on the account security page or so, and it's mostly a backend concern what actually "is required for authz"
| userHistory={userHistory} | ||
| /> | ||
| )} | ||
| {props.authenticatedUser && externalAccountsModalVisible && ( |
There was a problem hiding this comment.
IMO this new modal is a net-positive improvement, but to be absolutely sure, should we add something to disable the feature?
There was a problem hiding this comment.
I'm not in favour of adding yet another "but have you enabled this setting" thing. I can be convinced otherwise, but my gut feeling is that I don't think it is necessary.
There was a problem hiding this comment.
I totally hear you :/ I'm just scared we might be missing some edge-case in the initial implementation and that some customer will run into issues with it where we unnecessarily bother every user. For example, what happens with explicit perms?
It could be some temporary stopgap kinda thing?
…/60616-account-connection-ux
| userHistory={userHistory} | ||
| /> | ||
| )} | ||
| {props.authenticatedUser && ( |
There was a problem hiding this comment.
Do we need to do anything with the new svelte app? Or even let code search team know?
…/60616-account-connection-ux
eseliger
left a comment
There was a problem hiding this comment.
Approving to unblock for release cut today, I’ll play with it a little more later
Co-authored-by: Erik Seliger <erikseliger@me.com>
Closes #60616
Adds a modal that reminds users to connect external accounts for permissions syncing.
The modal does not show accounts that the user has already connected, and once dismissed, the modal won't be shown again unless a new auth provider is added.
The modal also only shows auth providers that would have an effect on permissions syncing. If no authorization is configured for any code host connections, then no modal will be shown.
See the attached Loom for a demo
https://www.loom.com/share/cb5f219bb9f3436fa1bcc1f70d603e67
Corresponding docs entry: sourcegraph/docs#282
Test plan
Lots of manual testing and added a unit test for the resolver. Probably needs a few more tests