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

cody vs code: check whether user has verified email #51870

Merged
merged 9 commits into from
May 17, 2023

Conversation

mrnugget
Copy link
Contributor

@mrnugget mrnugget commented May 12, 2023

This fixes #51992 and is a follow-up to #51706 to show a nicer message in VS Code in case user hasn't confirmed their email yet.

It changes how we fetch the authentication status:

  • if we're connected to dotcom, we check whether the user has a verified email
  • if we're not connected to dotcom, we skip that

If the user requires and doesn't have a verified email, we show a different error on the Login page.

There rest of the changes are essentially there to handle config changes, handle errors that happen at runtime, and login/logout.

This needs a thorough review

Test plan

@sourcegraph-buildkite
Copy link
Collaborator

sourcegraph-buildkite commented May 12, 2023

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (0.00 kb) 0.00% (+0.34 kb) 0.00% (+0.34 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits ae937b3 and f7a0bd0 or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@mrnugget mrnugget force-pushed the mrn/verified-email-cody-vscode branch 2 times, most recently from e5d1589 to 1282e91 Compare May 15, 2023 11:22
@mrnugget
Copy link
Contributor Author

@abeatrix I pulled in some of the changes in your PR, but not all of them (because my rebase -- which I did too early -- made it hard to see the changes).

@abeatrix @philipp-spiess @novoselrok can you take a look whether the introspection query here makes sense? I essentially want to check whether I can query the field and if so, I query it. Otherwise I ignore it.

I also changed a lot of the checks in other places to make sure that isLoggedIn() only returns true if the user is logged in and has a verified email if that is required.

(I haven't marked as ready for review, because it's still very rough and I need to test it, but wanted to get early eyes on the approach)

@philipp-spiess
Copy link
Contributor

@abeatrix @philipp-spiess @novoselrok can you take a look whether the introspection query here makes sense? I essentially want to check whether I can query the field and if so, I query it. Otherwise I ignore it.

I've seen introspection for other parts of the product already but if this only affects dotcom which we fully control, can't we just roll out the server changes first? 🤔 I’m a bit confused why we need to introspect at all

@@ -51,12 +53,13 @@ export const App: React.FunctionComponent<{ vscodeAPI: VSCodeWrapper }> = ({ vsc
break
}
case 'config':
console.log('are we here?')
Copy link
Contributor

Choose a reason for hiding this comment

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

right now this log user in as long as the user has entered a token, even if the token in not valid

@mrnugget
Copy link
Contributor Author

I've seen introspection for other parts of the product already but if this only affects dotcom which we fully control, can't we just roll out the server changes first? 🤔 I’m a bit confused why we need to introspect at all

You mean, detect whether the user connects to sourcegraph.com on the client and then check? I assumed that was too brittle so added the check on the server-side and also because, well, we might remove that check on dotcom in the future (i can see someone saying "we noticed that our DAUs drop when we require verified emails, so let's stop this") But yeah, looks like that might be easier...

(Why we need introspect: I want to query site.requiresVerifiedEmailForCody, but if that field doesn't exist - which is the case for every <5.1 version of SG - it would error. So we need to check whether we can check the field...)


General update: @abeatrix I ported a lot of your changes, and the check works when logging in (it shows error message). But right now it's broken when a request fails after the user is already logged in. I think it might have to do with serialization of AuthStatus.

I'll take another look tomorrow, but I'm not sure I'll get it done before the sprint planning. If someone has capacity and wants to hop on this branch, be my guest.

@mrnugget
Copy link
Contributor Author

Update after pairing session with @eseliger and @philipp-spiess on this: it works as it should now and we made the code simpler than what I had before.

But the state machine in App.tsx still needs a fix. As of now, when Cody is starting we show the login page, even if we're currently logging the user in and we have valid credentials. That's because we removed the view === 'login' thing and authStatus can be undefined.

That needs to be fixed.

@mrnugget mrnugget changed the title WIP: Check whether user has verified email in VS Code Cody Check whether user has verified email in VS Code Cody May 17, 2023
@mrnugget mrnugget marked this pull request as ready for review May 17, 2023 08:32
@mrnugget
Copy link
Contributor Author

@sourcegraph/cody can you review? This needs a thorough review. I'll do more manual testing (and Philip is working on adding integration test) but I really want us all to understand this code.

@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented May 17, 2023

📖 Storybook live preview

@mrnugget mrnugget force-pushed the mrn/verified-email-cody-vscode branch from 1ce369c to 35e2061 Compare May 17, 2023 08:49
@mrnugget mrnugget requested a review from marekweb May 17, 2023 08:50
@mrnugget mrnugget force-pushed the mrn/verified-email-cody-vscode branch from 35e2061 to c64fbb8 Compare May 17, 2023 09:02
@mrnugget
Copy link
Contributor Author

I added a Loom to the PR description that shows how I tested this and what the cases are: https://www.loom.com/share/f95a19cd9fd0419798fc888b49245ca0

@mrnugget mrnugget changed the title Check whether user has verified email in VS Code Cody cody vs code: check whether user has verified email May 17, 2023
@philipp-spiess philipp-spiess self-requested a review May 17, 2023 10:02
Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

This looks good to me now! I think requiring a view to be set for any case other than the initial loading is a good idea.

client/cody-shared/src/sourcegraph-api/graphql/client.ts Outdated Show resolved Hide resolved
client/cody-shared/src/sourcegraph-api/graphql/queries.ts Outdated Show resolved Hide resolved
client/cody/webviews/App.tsx Outdated Show resolved Hide resolved
@mrnugget mrnugget enabled auto-merge (squash) May 17, 2023 12:24
@mrnugget mrnugget merged commit 8e27b90 into main May 17, 2023
13 of 16 checks passed
@mrnugget mrnugget deleted the mrn/verified-email-cody-vscode branch May 17, 2023 12:34
@marekweb
Copy link
Contributor

Since we now have the ability to connect the Cody extension to App as a server endpoint as well, I'm going to add an isApp check in addition to the isDotCom check introduced here. So that we can make the email-verified check work similarly for App (since App also uses a dotcom user). If there's no objection to that approach I'll follow up shortly with a PR with that.

@mrnugget
Copy link
Contributor Author

Since we now have the ability to connect the Cody extension to App as a server endpoint as well, I'm going to add an isApp check in addition to the isDotCom check introduced here. So that we can make the email-verified check work similarly for App (since App also uses a dotcom user). If there's no objection to that approach I'll follow up shortly with a PR with that.

Do you need that? Wouldn't Cody-through-App go up to dotcom anyway?

Anyway, yeah, if it makes sense, open a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show warning to VS Code users if email is not verified on dotcom
7 participants