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

SSC: Three small fixes for Teams & invites#62818

Merged
vdavid merged 4 commits intomainfrom
dv/ssc-small-fixes
May 22, 2024
Merged

SSC: Three small fixes for Teams & invites#62818
vdavid merged 4 commits intomainfrom
dv/ssc-small-fixes

Conversation

@vdavid
Copy link
Contributor

@vdavid vdavid commented May 21, 2024

Finally, I could start testing the experience end-to-end.
I found the first few mistakes during that.

Three changes:

  1. Fixed a few copy-paste mistakes in the team member list action logging
  2. The URL in the case of single users was wrong—now it works correctly. Plus the logging was likely suppressed by the "to" part because event.preventDefault() was not called.
  3. Made the number of invites dynamic in the notification, pluralized:
    CleanShot 2024-05-21 at 14 19 54@2x

The three changes are three commits in this PR to make the review easier.

Test plan

Tested manually.

@vdavid vdavid requested a review from a team May 21, 2024 12:23
@cla-bot cla-bot bot added the cla-signed label May 21, 2024
Copy link
Contributor

@chrsmith chrsmith left a comment

Choose a reason for hiding this comment

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

Just one thing that may be A-OK and I'm just confused about, but otherwise all good things!

<Link
className="text-center"
to="https://sourcegraph.com/contact/request-info?utm_source=cody_subscription_page"
to={manageSubscriptionRedirectURL}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ I'm not sure this is right, and just want to double check my understanding:

export const manageSubscriptionRedirectURL = `${window.context.frontendCodyProConfig?.sscBaseUrl}/subscription`

So the current way this is configured, this will redirect users to https://accounts.sourcegraph.com/cody/subscription. ( ✅ )

But down the road, do we plan on setting sscBaseUrl to https://sourcegraph.com/cody/manage? And then expect the /cody/manage/subscription page to redirect to /cody/manage/subscription/new if the user doesn't have an active Cody Pro subscription? Since (IIRC) we don't have a /cody/manage/subscription page wired up yet, that seems like maybe a dangerous assumption to make?

So for clarity, would we want to export a new createNewCodyProSubscriptionRedirectUrl just to remove any ambiguity? Since in this case, we expect the user to create a new subscription/Cody Pro team, not manage an existing one.

Copy link
Contributor

@chrsmith chrsmith May 21, 2024

Choose a reason for hiding this comment

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

JFYI, and perhaps this will fix it, it looks like this is broken. (At least in our dev environment.) It's very likely I have misconfigured something.

https://cody-services-dev.sgdev.dev/subscription shows this link:
image

And the "Upgrade yourself to Pro" points to: https://cody-services-dev.sgdev.dev/subscription which 404s. (Which is surprising, since I didn't set the sscBackendUrl value in the site config yet. So it should have defaulted to an entirely different domain, accounts.sourcegraph.com...)

image

Update: After setting the sscBackendUrl the link now works, but it redirects to the exact same page (with the behavior that is checked-in now).

https://cody-services-dev.sgdev.dev/cody/subscription

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But down the road, do we plan on setting sscBaseUrl to https://sourcegraph.com/cody/manage?

I mean, if we want to migrate this to some other URL, we can change the definition of manageSubscriptionRedirectURL, right? So I'd not be too forward-looking with this.

Update: After setting the sscBackendUrl the link now works, but it redirects to the exact same page (with the behavior that is checked-in now).

Did you figure out what was the problem? TBH I didn't fully get what you meant was wrong here.
Your PR https://github.com/sourcegraph/sourcegraph/pull/62835 fixes the behavior when sscBackedUrl is not defined, but when it is defined, the link worked in my tests.

@vdavid vdavid enabled auto-merge (squash) May 22, 2024 08:45
@vdavid vdavid merged commit 1652ed0 into main May 22, 2024
@vdavid vdavid deleted the dv/ssc-small-fixes branch May 22, 2024 08:58
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.

2 participants