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

Make the URL dynamic, now with a fallback#62852

Merged
vdavid merged 2 commits into
mainfrom
dv/fix-manage-subscription-url
May 23, 2024
Merged

Make the URL dynamic, now with a fallback#62852
vdavid merged 2 commits into
mainfrom
dv/fix-manage-subscription-url

Conversation

@vdavid

@vdavid vdavid commented May 22, 2024

Copy link
Copy Markdown
Contributor

I made this URL dynamic before, here, but that didn't work because the "default" value in the site.schema.json file doesn't do what I thought it did. So @chrsmith did a quick fix in https://github.com/sourcegraph/sourcegraph/pull/62835, but then we lost the ability to make this dynamic.

So this PR makes it dynamic again, this time with a working fallback.

Alternatives considered:

I tried to do the same thing on the back end, but it didn't feel right.

  1. I tried a transformation in computed.go:
// GetCodyProConfig returns the CodyProConfig for the current site config,
// with some handy defaults.
func GetCodyProConfig(siteConfig schema.SiteConfiguration) (c conftypes.CodyProConfig) {
	c.SamsBackendOrigin = "https://accounts.sourcegraph.com"
	c.SscBackendOrigin = "https://accounts.sourcegraph.com"
	c.SscBaseUrl = "https://accounts.sourcegraph.com/cody"
	c.StripePublishableKey = nil

	if siteConfig.Dotcom == nil || siteConfig.Dotcom.CodyProConfig == nil {
		return
	}

	codyProConfig := siteConfig.Dotcom.CodyProConfig
	if codyProConfig != nil {
		c.SamsBackendOrigin = codyProConfig.SamsBackendOrigin
		c.SscBackendOrigin = codyProConfig.SscBackendOrigin
		c.SscBaseUrl = codyProConfig.SscBaseUrl
		c.StripePublishableKey = &codyProConfig.StripePublishableKey
	}
	return
}

But having the defaults there was meh, plus I had to introduce the new type conftypes.CodyProConfig but nothing prevents another eng to just use the config directly, which might become confusing.

  1. I thought about setting the defaults in makeFrontendCodyProConfig, which might be better, but then, on the front end, we already have a single location where we access the SSC base URL, and we already had the default there, so I just went with that.

Test plan

Here is a 1-min Loom where I test it with different settings and demo that it works as intended.

@vdavid vdavid requested review from a team and chrsmith May 22, 2024 11:59
@cla-bot cla-bot Bot added the cla-signed label May 22, 2024

@chrsmith chrsmith left a comment

Copy link
Copy Markdown
Contributor

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, although with a small suggestion.

I agree with your thoughts on other ways to set the "defaults" for the codyProConfig data, and just updating the logic here in the code is probably the right call for now. (And after we ship the embedded UI, we can remove some of the logic here entirely, since it will just be a static route to some other page in the same React app.)

Comment thread client/web/src/cody/util.ts Outdated
Comment thread client/web/src/cody/util.ts
Co-authored-by: Chris Smith <chrsmith@users.noreply.github.com>
@vdavid vdavid enabled auto-merge (squash) May 23, 2024 09:43
@vdavid vdavid merged commit aafce44 into main May 23, 2024
@vdavid vdavid deleted the dv/fix-manage-subscription-url branch May 23, 2024 09:54
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