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 VSCode: Add support for verified email check message when connected to app; and improve connected status message #52075

Merged
merged 2 commits into from
May 17, 2023

Conversation

marekweb
Copy link
Contributor

@marekweb marekweb commented May 17, 2023

Show verified email message when connecting Cody extension to App

When connecting the Cody extension to App, it's expected to act similarly to dotcom in that requires an email address and if the user doesn't have a verified email address, it should show the same message.

This PR checks if we are connecting to App, and if so it will treat the email verification logic the same as if we were connecting to sourcegraph.com, so we also get the same error message for an unverified email.

Background info: App will basically proxy the request to dotcom so the response to currentUser { id, hasVerifiedEmail } will be similar to a dotcom response. The only difference, then, is that Cody extension holds an access token to App and App holds an access token to dotcom.

Video

Screen.Recording.2023-05-17.at.11.16.53.AM.mov

Update "Connected to" display

This is a cosmetic change to show "Connected to Sourcegraph App" instead of "Connected to http://localhost:3080":

Screenshot 2023-05-17 at 10 58 04 AM

Test plan

  • Connect App to a dotcom account
  • Test Cody extension with an unverified email address in the dotcom account
  • Test Cody extension after verifying the email address in the dotcom account

@cla-bot cla-bot bot added the cla-signed label May 17, 2023
@marekweb marekweb changed the title Add support for app verified email check and show connected Cody VSCode: Add support for app verified email check; and show that we're connected to App May 17, 2023
@marekweb marekweb changed the title Cody VSCode: Add support for app verified email check; and show that we're connected to App Cody VSCode: Add support for verified email check message when connected to app; and improve connected status message May 17, 2023
@cla-bot cla-bot bot added the cla-signed label May 17, 2023
serverEndpoint: DOTCOM_URL.href,
serverEndpoint,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes a regression; serverEndpoint is already set above to DOTCOM_URL if we're connecting to dotcom, or it's set to LOCAL_APP_URL if we're connecting to App so we don't want to overwrite it here.

@marekweb marekweb marked this pull request as ready for review May 17, 2023 15:23
@marekweb marekweb requested review from abeatrix and a team May 17, 2023 15:26
@sourcegraph-bot
Copy link
Contributor

📖 Storybook live preview

@marekweb marekweb linked an issue May 17, 2023 that may be closed by this pull request
@abeatrix
Copy link
Contributor

Is there a setting i'd need to change to test? I have the old feature flag set and app opened, but I don't see the button to login with app:
image

Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

I don't have the release build of App to test the button but the code looks good to me

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.

Sweet!

@marekweb marekweb merged commit a95e69b into main May 17, 2023
31 of 32 checks passed
@marekweb marekweb deleted the marek/cody-app-verified-email branch May 17, 2023 16:40
Copy link
Contributor

@limitedmage limitedmage left a comment

Choose a reason for hiding this comment

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

Can we change the error message to "Sourcegraph.com account"? (instead of instance)
image

@mrnugget
Copy link
Contributor

@limitedmage yup, makes sense: #52131

mrnugget added a commit that referenced this pull request May 19, 2023
…graph.com" (#52131)

Follow-up to this comment:
#52075 (review)

## Test plan

- N/A
VolhaBakanouskaya pushed a commit to VolhaBakanouskaya/sourcegraph-public that referenced this pull request Jun 30, 2023
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.

App: Test App connection with Cody with unverified email
7 participants