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

SSC: Fix checkout links#63190

Merged
vdavid merged 3 commits intomainfrom
dv/fix-checkout-link
Jun 10, 2024
Merged

SSC: Fix checkout links#63190
vdavid merged 3 commits intomainfrom
dv/fix-checkout-link

Conversation

@vdavid
Copy link
Contributor

@vdavid vdavid commented Jun 10, 2024

The "Create team" and "Upgrade to Pro" links currently point to the old SSC site.

CleanShot 2024-06-10 at 16 39 44@2x

This PR makes both of them point to the new PLG checkout experience, behind the feature flag.

Test plan

Tested it manually, both buttons work now as they should

Changelog

fix(plg): make upgrade links point to the right pages for Teams & Invites

@vdavid vdavid requested a review from a team June 10, 2024 14:41
@cla-bot cla-bot bot added the cla-signed label Jun 10, 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.

I don't believe this is correct, and will break things if we merge it.

The subscription/CodySubscriptionPage.tsx is shown on sourcegraph.com (here), and is live. So we have to have this continue to redirect the user to https://accounts.sourcegraph.com/cody, right? Otherwise if we merge this it will essentially break things for all users?

When we go live with the new UI, we will need to replace all of these links. But shouldn't we just update the manageSubscriptionRedirectURL value to (depending on the site configuration) point to the new location?

That way, it will continue to work for Sourcegraph.com, but for the cody-services-dev.sgdev.dev it will do the right thing.

Or am I just mistaken, and the CodySubscriptionPage is part of our "new UI". (Which in that case, yes, we can just link directly to the page and not need to bother with manageSubscriptionRedirectURL.)

@vdavid
Copy link
Contributor Author

vdavid commented Jun 10, 2024

@chrsmith, take a closer look: if you extend the code right above my changes, you'll see it's all within a useEmbeddedCodyUI ? ... condition. I've only changed the URL, but we don't even render the "Create Cody Pro team" button without the feature flag.

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.

I see. I absolutely did not notice that!

... but taking a closer look at the code, this is (at least to me) confusing enough that we should rewrite it.

I'll aprove this so we can just move on and start testing things. But we need to have a firm definition of what is and is not acceptable for the React codebase.

In this case, I'd argue this definitely isn't acceptable. In that it's super confusing and easy to miss really important details... such as me being concerned this will break things, but it is actually just fine.

     {isProUser ? (
          <ManyLinesOfReactComponents>                              
         ) : useEmbeddedCodyUI ? (
               <ManyLinesOfReactComponents>
             ) : (
                <ManyLinesOfReactComponents>
          )}

When written like this it is impossible to reason about the context some code renders in without carefully walking back N-lines of the source tree, and paying super-close attention to elements like ) : ( or ) : useEmbeddedCodyUI ? (.

@vdavid vdavid merged commit 8971855 into main Jun 10, 2024
@vdavid vdavid deleted the dv/fix-checkout-link branch June 10, 2024 18:34
@vdavid
Copy link
Contributor Author

vdavid commented Jun 10, 2024

@chrsmith, yup, let's get back to this and make it an org-wide discussion.

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