batches: surface batch change feature availability in JS context LicenseInfo#52044
Conversation
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 78d95e6...212906f.
|
| // BatchChangesEnabled is true if: | ||
| // * Batch Changes is NOT disabled by a flag in the site config | ||
| // * Batch Changes is NOT limited to admins-only, or it is, but the user issuing | ||
| // the request is an admin and thus can access batch changes | ||
| // It does NOT reflect whether or not the site license has batch changes available. | ||
| // Use LicenseInfo for that. | ||
| BatchChangesEnabled bool `json:"batchChangesEnabled"` |
There was a problem hiding this comment.
I added this comment to clarify that this other property on the JS Context is NOT related to the license at all, as it's kinda confusing that an instance would have batchChangesEnabled: true if their site license doesn't permit Batch Changes, and yet that's exactly what would happen haha.
We considered modifying the behavior of this field instead, but it's fundamentally trying to achieve something a bit different. This field is false if Batch Changes is explicitly disabled by a site admin with a flag in the site config, or if Batch Changes has been limited to admins only. If it's false, the admin actually is saying they do not want other users on the instance to see Batch Changes. Whereas, what we're going for is essentially product discovery/marketing for a Sourcegraph customer that just hasn't paid for Batch Changes yet. This property should probably be renamed to reflect this usage difference, but it didn't feel in scope to make this change at present.
st0nebreaker
left a comment
There was a problem hiding this comment.
This is great, thank you! While we're exposing more of this license info I'm wondering:
(1) Should we leave window.context.licenseInfo entirely null for OSS? That makes sense to me, but not sure if we want to be more explicit here.
(2) Dotcom is being set with the following. Should we fix this to be more explicit as well since dotcom shouldn't have unrestricted batch changes?
Yeah I guess I feel like this is still the right call because fundamentally there is no concept of a license if you're running Sourcegraph as OSS (you gotta actually talk to us to get a license), so unless we introduce some concept of a default "OSS license" then really
Oh Dotcom. Yeah probably, that's a good call. 🙂 |
|
Updated! Now dotcom will have |
st0nebreaker
left a comment
There was a problem hiding this comment.
🎇 looks great on all use cases ~ thank you!
BolajiOlajide
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks for the fix.
Closes #49714 - Uses new batch change licensing info exposed on the JS Context (from #52044) - Hides Batch Change nav item on OSS - Shows Get Started page for dotcom & enterprise customers without a Batch Changes license ## Test plan - Run `sg start oss` - observe missing batch changes nav item (route is inaccessible too) - Run `sg start dotcom` - observe batch changes page, only the `Get started` content displays (subrotues are inaccessible) - Mock enterprise with limited batch changes - observe limited batch change warning once executing a SSBC --------- Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
Closes #49714 - Uses new batch change licensing info exposed on the JS Context (from #52044) - Hides Batch Change nav item on OSS - Shows Get Started page for dotcom & enterprise customers without a Batch Changes license ## Test plan - Run `sg start oss` - observe missing batch changes nav item (route is inaccessible too) - Run `sg start dotcom` - observe batch changes page, only the `Get started` content displays (subrotues are inaccessible) - Mock enterprise with limited batch changes - observe limited batch change warning once executing a SSBC --------- Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
License info was recently added to the JS Context, which makes a lot of sense given it's pretty much guaranteed to be constant over the lifetime of a user's visit to Sourcegraph and it's useful to make the very earliest of rendering decisions when the web app loads.
One such decision is whether or not to display the Batch Changes tab in the nav bar, or whether or not to show the main Batch Changes list page when visiting that part of the web app. Not properly handling this decision was what led to https://github.com/sourcegraph/sourcegraph/issues/49714, a bug which Becca is working on fixes for. This PR attempts to enable a part of that fix -- namely, that if a site has a license that explicitly does not include Batch Changes in its provisions, we should not show the Batch Changes list page, only the "Getting Started" page. It does so by surfacing the Batch Changes feature configuration in that JS Context
LicenseInfoobject.This PR is just the backend changes; Becca's got the corresponding frontend ones incoming.
What's also notable about this is that it would allow us to refactor away from the somewhat outdated, redundant frontend logic around assessing the license and the
maxUnlicensedChangesetsproperty on the GraphQL schema that we query and rely on in a couple other places in the frontend in order to inform users when they're about to take an action that would exceed their license capabilities. 🙂Test plan
No Batch Changes:
console.log(window.context.licenseInfo)batchChanges: nullDotcom
sg start dotcomconsole.log(window.context.licenseInfo)batchChanges: nullLimited Batch Changes
MaxNumChangesetssetconsole.log(window.context.licenseInfo)batchChanges: { maxNumChangesets: 10, unrestricted: false}Unlimited Batch Changes
Unrestricted: truesetconsole.log(window.context.licenseInfo)batchChanges: { maxNumChangesets: -1, unrestricted: true}