This repository was archived by the owner on Sep 30, 2024. It is now read-only.
SSC: Handle "Accept invite" calls#62888
Merged
Merged
Conversation
Contributor
|
Thanks for writing this up so detailed and clearly @vdavid .
|
taras-yemets
approved these changes
May 24, 2024
taras-yemets
left a comment
Contributor
There was a problem hiding this comment.
Overall the code LGTM. Left a few comments inline. Approving to unblock.
739bdcd to
79f1730
Compare
Contributor
Author
|
@chenkc805, I've added the next actions to new ticket https://github.com/sourcegraph/self-serve-cody/issues/785. For the back-end/front-end question, the back-end solution is a clear winner so marked that one as the next action. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before this PR, we had no "Accept invite" functionality.
A few days ago, @chrsmith added the email-sending feature in https://github.com/sourcegraph/self-serve-cody/pull/777, and I tweaked it a bit in https://github.com/sourcegraph/self-serve-cody/pull/781. The current PR wires up the rest of the flow, and lets the user actually use the invite.
Three changes happen in this PR:
CodyAlertandCodyContainerbecause I wanted to reuse them on pages other thanCodyManageTeamPage.CodyManageTeamPageintoInviteUsersandTeamMemberList./cody/invites/accept. It makes a back-end call toPOST /team/${teamId}/invites/${inviteId}/accept, and if it succeeds, redirects the user to/cody/team/manage, otherwise displays an error message.For easier review, no. 1 and no. 2 are in one commit, and no. 3 is in a different commit.
There are a few problematic parts, but first, to have a common understanding of the process, I'll describe the user flow and how the user interacts with our system when using invites.
This is a common happy path:
https://cody-services-dev.sgdev.dev/cody/subscription, clicks the "Create a Cody Pro team" button, and buys two seats.https://cody-services-dev.sgdev.dev/cody/team/manageand invites a colleague. This action sends an email to the colleaguehttps://cody-services-dev.sgdev.dev/cody/invites/acceptpage, through a link that looks like this:https://cody-services-dev.sgdev.dev/cody/invites/accept?inviteID=018f9b13-ddd1-7f1f-b030-29b6f8fc77dd&signin=OpenIDConnect&teamID=018f9b13-5bf7-77cc-b192-c2fcac17ad94(has aninviteID, and ateamIDargument).inviteId+teamIdcombo.https://cody-services-dev.sgdev.dev/cody/team/manageand sees a list of the admin and themself.Problems I'm seeing:
https://cody-services-dev.sgdev.dev/cody/invites/accept?inviteID=018f9b13-ddd1-7f1f-b030-29b6f8fc77dd&signin=OpenIDConnect&teamID=018f9b13-5bf7-77cc-b192-c2fcac17ad94&email=invited.person@company.com(notice the last part), and then check it against the logged in user's email address on the front end. Fail if they don't match.https://cody-services-dev.sgdev.dev/cody/invites/accept, which straight away accepts the invite. The button text is misleading.That said, the current PR could be fine as-is; we just need to be mindful of these two caveats and fix them (or consciously ignore them) as a follow-up.
Test plan
I've tested it manually. The flow is not perfect (A "Bad gateway" response displays on the "Accept" page for a split-second AND a "Forbidden" response is returned on the "Manage" page), but it accepts the invite correctly and the user shows up in the list:
Same on the admin's side:
I'll fix these problems in a follow-up PR.