Add new admin toggle for turning Enterprise on and off #1095

jordigh · 2024-07-09T13:12:47Z

Original commit messages:

9c3f9de Dockerfile: silence a warning

Docker doesn't like mixed case in keywords.
8a1d124 config: end the file with a newline

Small cosmetic change, POSIX requires final newlines in text files.

https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline
2060416 configCore: default to enterprise edition if TEST_ENABLE_ACTIVATION is truthy

This will ensure that the grist-ee image will have a consistent config
setting when created from the default value.
cbf15af config: new API endpoint

This adds PATCH and GET endpoints to handle config.json.
531f6d1 FlexServer: remove restart endpoint

The restart logic is now handled by the config endpoint, which is
where it's needed.
314962a supervisor: remove config stub

The configuration is now handled by the config API, so we no longer
need the stub function here.
733677b ConfigAPI: new class to handle frontend requests to config backend

This new API is somewhat patterned after the InstallAPI, but simpler
whenever possible.
262838f ToggleEnterpriseModel: new GrainJS model to handle changes to config API

Patterned after TelemetryModel.ts
9c5e669 AdminToggleCss: factor out CSS from SupportGristPage

We will create a new enterprise toggle, so we will need to share the same CSS.
8559b99 ToggleEnterpriseWidget: new frontend toggle for the admin

Strongly patterned after SupportGristPage. In fact, it has almost the
same structure.

Perhaps one day it would be possible to synchronise the logic between
the two toggles even further, but I couldn't see a simple way to do so
now. For now, some code structure duplication seemed easiest in lieau
of more abstractions.
466211e AdminPanel: add the toggle for enterprise

Final ingredient. This surfaces the work in creating the backend
config API, the frontend model, the grainjs observable, and the
grainjs DOM and CSS components.

github-actions · 2024-07-09T13:19:19Z

Deployed commit 2afddcd95d665fd9708392f5314d6d3ce823f951 as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-08T13:17:41.962Z)

github-actions · 2024-07-09T20:24:44Z

Deployed commit 7542c93690b3c49a852670cf7bb6c077fde9160b as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-08T20:23:07.504Z)

github-actions · 2024-07-16T01:45:55Z

Deployed commit 3c3abfaaae923b60974b387933bf416ea560ab75 as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-15T01:44:12.920Z)

github-actions · 2024-07-16T01:57:40Z

Deployed commit 3139328e82b8b976470156be69c23465693aa1c2 as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-15T01:55:54.827Z)

app/server/lib/ConfigBackendAPI.ts

Docker doesn't like mixed case in keywords.

github-actions · 2024-07-18T21:39:54Z

Deployed commit 4476820022a9bc2ab5429f340e0edfa3098a4aad as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-17T21:38:16.042Z)

github-actions · 2024-07-18T21:44:53Z

Deployed commit 8febbaf9b8f3b11a922d4ff58d2ef0563890e95e as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-17T21:43:17.853Z)

github-actions · 2024-07-19T20:47:54Z

Deployed commit 529e160d82bb555094ec3d209a7fd0474ab9a6df as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-18T20:46:13.834Z)

Small cosmetic change, POSIX requires final newlines in text files. https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline

…s truthy This will ensure that the grist-ee image will have a consistent config setting when created from the default value.

This adds PATCH and GET endpoints to handle `config.json`.

Patterned after TelemetryModel.ts

github-actions · 2024-07-22T14:46:13Z

Deployed commit 5770c436d5317cc8f10cd180ce780dd8535f6ec4 as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-21T14:44:35.220Z)

I have made the requested changes

jordigh · 2024-07-28T23:22:37Z

I believe I've addressed most of Jarek's concerns, save for some module separations I'm stubbornly keeping.

Current round of commit messages:

eded090 Dockerfile: silence a warning

Docker doesn't like mixed case in keywords.
566d873 config: end the file with a newline

Small cosmetic change, POSIX requires final newlines in text files.

https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline
6562988 configCore: default to enterprise edition if TEST_ENABLE_ACTIVATION is truthy

This will ensure that the grist-ee image will have a consistent config
setting when created from the default value.
089b37f config: new API endpoint

This adds PATCH and GET endpoints to handle config.json.
6bc4e67 FlexServer: remove config from restart endpoint

The config endpoint now handles changing config values, so we only
need to handle restarts here.
57e50ca supervisor: remove config stub

The configuration is now handled by the config API, so we no longer
need the stub function here.
d3f8573 ConfigAPI: new class to handle frontend requests to config backend

This new API is somewhat patterned after the InstallAPI, but simpler
whenever possible.
392c40f ToggleEnterpriseModel: new GrainJS model to handle changes to config API

Patterned after TelemetryModel.ts
cebb847 AdminToggleCss: factor out CSS from SupportGristPage

We will create a new enterprise toggle, so we will need to share the same CSS.
85a1338 ToggleEnterpriseWidget: new frontend toggle for the admin

Strongly patterned after SupportGristPage. In fact, it has almost the
same structure.

Perhaps one day it would be possible to synchronise the logic between
the two toggles even further, but I couldn't see a simple way to do so
now. For now, some code structure duplication seemed easiest in lieau
of more abstractions.
eede7d5 AdminPanel: add the toggle for enterprise

Final ingredient. This surfaces the work in creating the backend
config API, the frontend model, the grainjs observable, and the
grainjs DOM and CSS components.

github-actions · 2024-07-28T23:26:27Z

Deployed commit 175eb9c3574448c2887ff167dffd9ec542a239bb as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-27T23:24:35.830Z)

github-actions · 2024-07-29T14:05:00Z

Deployed commit eede7d568f89fde35a5e807b39230590ed6cf5b9 as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-28T14:03:05.718Z)

paulfitz

This looks pretty good. Testing locally, with the supervisor I can see restarts. But in enterprise mode, and with a reloaded page, I don't see signs of enterprise activity, e.g. an Activation page in menu, or a banner. What could I be doing wrong? I have included an ext directory from grist-ee and it is built I believe.

app/client/models/ToggleEnterpriseModel.ts

app/common/ConfigAPI.ts

nbush · 2024-07-29T20:06:44Z

Some content notes:

For "Grist Enterprise Edition is enabled.", is there a reason it's below the description? I'd place it above if it's not a big task, and instead of bolding the whole thing, just emphasize enabled for better legibility.
We should aim to be consistent between "Grist Enterprise Edition" and "Grist Enterprise". The docs don't seem to use "Edition", and since there are no other "editions" I think we're OK to drop it from the button. I'd also remove "features" from the subheading, and just have it read "Activate Grist Enterprise".
We should also be consistent with "enable" vs. "activate". If we're calling them "activation keys", I think it makes sense to use "activate/deactivate", even if it's a bit less casual.
Related: if it's only one key, suggest changing to "An activation key is used to run [...]"
Suggest linking to the pricing page to give users a path if the trial has ended, like it is in the docs: Get an activation key by signing up for Grist Enterprise.
This looks like a bigger docs task, but we need a way to explain to users what Grist Core is, and I don't think this link is super helpful. I'm not sure linking to the repo is helpful either. Thoughts?

In case Grist isn't running with the supervisor (e.g. it's running under nodemon instead via `yarn start`), surface the problem to the frontend.

This new API is somewhat patterned after the InstallAPI, but simpler whenever possible.

github-actions · 2024-07-29T20:43:29Z

Deployed commit fd866e6d8273f3e6780ac76dfa2de858c6738074 as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-28T20:41:58.893Z)

Since we've started using Markdown, why not a simple utility function to start using it?

Strongly patterned after SupportGristPage. In fact, it has almost the same structure. Perhaps one day it would be possible to synchronise the logic between the two toggles even further, but I couldn't see a simple way to do so now. For now, some code structure duplication seemed easiest in lieau of more abstractions.

github-actions · 2024-07-29T23:34:51Z

Deployed commit 2e8f8125f732f533543fa3ffea12055862a78011 as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-28T23:33:01.125Z)

jordigh · 2024-07-29T23:36:13Z

Thanks for the review, @nbush

1. For "Grist Enterprise Edition is enabled.", is there a reason it's below the description? I'd place it above if it's not a big task, and instead of bolding the whole thing, just emphasize **enabled** for better legibility.

Done.

2. We should aim to be consistent between "Grist Enterprise Edition" and "Grist Enterprise". The docs don't seem to use "Edition", and since there are no other "editions" I think we're OK to drop it from the button. I'd also remove "features" from the subheading, and just have it read "Activate Grist Enterprise".

Made those changes.

3. We should also be consistent with "enable" vs. "activate". If we're calling them "activation keys", I think it makes sense to use "activate/deactivate", even if it's a bit less casual.

This one is tricky. There are two separate actions: you use the toggle to turn on enterprise, with a 30 day trial that will expire. You use the activation key to keep enterprise on after your trial period is over. I figured the first action was called "enabling" and the second action was called "activation".

Are you telling me to just replace all instances of "enable" to "activate"?

4. Related: if it's only one key, suggest changing to "An activation key is used to run [...]"

Done.

5. Suggest linking to the pricing page to give users a path if the trial has ended, like it is in the docs: Get an activation key by [signing up for Grist Enterprise](https://www.getgrist.com/pricing/).

Done.

6. This looks like a bigger docs task, but we need a way to explain to users what Grist Core is, and I don't think [this link](https://support.getgrist.com/self-managed/#what-is-self-managed-grist) is super helpful. I'm not sure linking to the [repo](https://github.com/gristlabs/grist-core/) is helpful either. Thoughts?

I wrote some verbiage to explain this, but I put it in our README. Should it also or instead live in our docs website? It might correspond nicely with the docs that Spoffy is writing.

github-actions · 2024-07-29T23:43:09Z

Deployed commit 4e930412f3ed9d621c759d7d6dba7f009b4bb6eb as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-28T23:41:44.859Z)

github-actions · 2024-07-29T23:46:31Z

Deployed commit cc1ebb4b620aee19fea54ca0102ba01ca9fd2705 as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-28T23:44:57.990Z)

nbush · 2024-07-30T05:06:49Z

Thanks @jordigh !

3. We should also be consistent with "enable" vs. "activate". If we're calling them "activation keys", I think it makes sense to use "activate/deactivate", even if it's a bit less casual.
This one is tricky. There are two separate actions: you use the toggle to turn on enterprise, with a 30 day trial that will expire. You use the activation key to keep enterprise on after your trial period is over. I figured the first action was called "enabling" and the second action was called "activation".

Are you telling me to just replace all instances of "enable" to "activate"?

Ah, I understand. That is a bit confusing, but I get why it's the case. How is the user notified that the trial has lapsed? I would assume two things: the "trial" banner would change somehow, and the toggle would be auto-disabled. But then I get a bit lost – since the activation key is a file/env var, does Grist check it's been properly detected and enable Enterprise automatically? Can Enterprise still be enabled/disabled with a detected activation key?

If the states are independent I'd leave the text as-is. If Grist doesn't auto-enable Enterprise if the activation key is set up correctly, we should mention how to enable it in the docs.

Additionally (and this is outside the scope of this review) it might be nice if this section mentioned if an activation key was properly set up, like: "Grist Enterprise has been activated with a key and is now enabled." In this case, we could also hide the explanatory text around activation and signing up for Enterprise. Always nice to confirm things we know.

6. This looks like a bigger docs task, but we need a way to explain to users what Grist Core is, and I don't think [this link](https://support.getgrist.com/self-managed/#what-is-self-managed-grist) is super helpful. I'm not sure linking to the [repo](https://github.com/gristlabs/grist-core/) is helpful either. Thoughts?
I wrote some verbiage to explain this, but I put it in our README. Should it also or instead live in our docs website? It might correspond nicely with the docs that Spoffy is writing.

Your explanation is clear, but will be a bit disorienting to link to from here. Spoffy's section is helpful too but with no easy permalink. On second thought, linking "Grist Core" to here is probably the best option for now, but there are already too many links in this part that are more important, so I wouldn't worry about it.

paulfitz · 2024-07-30T15:20:45Z

@jordigh the check for running over supervisor looks good, but the error message doesn't propagate to the user if this happens in admin panel. They just get this:

Edit: another time I got this:

paulfitz · 2024-07-30T15:25:56Z

Very strange. If I hit the toggle too quickly in succession, I get a NetworkError.

Final ingredient. This surfaces the work in creating the backend config API, the frontend model, the grainjs observable, and the grainjs DOM and CSS components.

jordigh · 2024-07-30T16:56:00Z

I've made some changes to make the error messages clearer. How's that?

github-actions · 2024-07-30T17:00:49Z

Deployed commit ee716aefdc48e459304123a879fbc0ea42cf0a27 as https://grist-gristlabs-grist-core-jordigh-ee-toggle.fly.dev (until 2024-08-29T16:59:17.915Z)

Done

paulfitz

Thanks @jordigh, let's go for it.

Suggested TODOs to file for a follow-on:

Tests.
Count time of enterprise time from when it is first activated, rather than when Grist is first installed.
Clean up retry (common code maybe? cancellation on retoggle?)

paulfitz · 2024-07-30T17:30:55Z

app/client/lib/markdown.ts

+import { BindableValue, DomElementMethod, subscribeElem } from 'grainjs';
+import { marked } from 'marked';
+
+export function markdown(markdownObs: BindableValue<string>): DomElementMethod {


Some comments around purpose would be useful for these two methods.

paulfitz · 2024-07-30T17:32:36Z

app/client/models/ToggleEnterpriseModel.ts

+
+// Copied from DocPageModel.ts
+const reconnectIntervals = [1000, 1000, 2000, 5000, 10000];
+async function retryOnNetworkError<R>(func: () => Promise<R>): Promise<R> {


Not great to have copies of retry code that get independent fixes, but also ok given schedule.

Hmm might be a bit weird if multiple requests overlap, there's no cancellation.

It is acceptable, given what it is.

jordigh added the preview Launch preview deployment of this PR label Jul 9, 2024

jordigh force-pushed the jordigh/ee-toggle branch from 2afddcd to 7542c93 Compare July 9, 2024 20:17

jordigh force-pushed the jordigh/ee-toggle branch from 7542c93 to 3c3abfa Compare July 16, 2024 01:34

jordigh force-pushed the jordigh/ee-toggle branch from 3c3abfa to 3139328 Compare July 16, 2024 01:50

Spoffy previously requested changes Jul 16, 2024

View reviewed changes

app/server/lib/ConfigBackendAPI.ts Outdated Show resolved Hide resolved

jordigh force-pushed the jordigh/ee-toggle branch from 3139328 to 4476820 Compare July 18, 2024 21:33

Dockerfile: silence a warning

eded090

Docker doesn't like mixed case in keywords.

jordigh force-pushed the jordigh/ee-toggle branch from 4476820 to 8febbaf Compare July 18, 2024 21:38

jordigh force-pushed the jordigh/ee-toggle branch from 8febbaf to 529e160 Compare July 19, 2024 20:41

jordigh added 4 commits July 22, 2024 09:43

config: end the file with a newline

566d873

Small cosmetic change, POSIX requires final newlines in text files. https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline

configCore: default to enterprise edition if TEST_ENABLE_ACTIVATION i…

6562988

…s truthy This will ensure that the grist-ee image will have a consistent config setting when created from the default value.

config: new API endpoint

089b37f

This adds PATCH and GET endpoints to handle `config.json`.

ToggleEnterpriseModel: new GrainJS model to handle changes to config API

d8a4d32

Patterned after TelemetryModel.ts

jordigh force-pushed the jordigh/ee-toggle branch from 529e160 to 5770c43 Compare July 22, 2024 14:39

jordigh changed the title ~~wip~~ Add new admin toggle for turning Enterprise on and off Jul 22, 2024

jordigh marked this pull request as ready for review July 22, 2024 16:41

berhalak self-requested a review July 23, 2024 21:02

jordigh force-pushed the jordigh/ee-toggle branch 2 times, most recently from 2793b24 to 08f87f8 Compare July 23, 2024 21:18

jordigh added preview Launch preview deployment of this PR and removed preview Launch preview deployment of this PR labels Jul 23, 2024

jordigh force-pushed the jordigh/ee-toggle branch from 175eb9c to eede7d5 Compare July 29, 2024 13:56

paulfitz reviewed Jul 29, 2024

View reviewed changes

app/client/models/ToggleEnterpriseModel.ts Outdated Show resolved Hide resolved

app/common/ConfigAPI.ts Show resolved Hide resolved

jordigh added 2 commits July 29, 2024 16:28

restart: gracefully handle restart failure

48c872f

In case Grist isn't running with the supervisor (e.g. it's running under nodemon instead via `yarn start`), surface the problem to the frontend.

ConfigAPI: new class to handle frontend requests to config backend

35b2516

This new API is somewhat patterned after the InstallAPI, but simpler whenever possible.

jordigh added 2 commits July 29, 2024 19:13

markdown: new utility module

d0d9150

Since we've started using Markdown, why not a simple utility function to start using it?

jordigh force-pushed the jordigh/ee-toggle branch from fd866e6 to 2e8f812 Compare July 29, 2024 23:27

jordigh force-pushed the jordigh/ee-toggle branch 2 times, most recently from 4e93041 to cc1ebb4 Compare July 29, 2024 23:39

AdminPanel: add the toggle for enterprise

ee716ae

Final ingredient. This surfaces the work in creating the backend config API, the frontend model, the grainjs observable, and the grainjs DOM and CSS components.

jordigh force-pushed the jordigh/ee-toggle branch from cc1ebb4 to ee716ae Compare July 30, 2024 16:53

paulfitz approved these changes Jul 30, 2024

View reviewed changes

jordigh merged commit 1b6a803 into main Jul 30, 2024
11 checks passed

jordigh deleted the jordigh/ee-toggle branch July 30, 2024 17:41

jordigh mentioned this pull request Aug 5, 2024

Activations: add an enabled_at column #1145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new admin toggle for turning Enterprise on and off #1095

Add new admin toggle for turning Enterprise on and off #1095

jordigh commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024

github-actions bot commented Jul 9, 2024

github-actions bot commented Jul 16, 2024

github-actions bot commented Jul 16, 2024

github-actions bot commented Jul 18, 2024

github-actions bot commented Jul 18, 2024

github-actions bot commented Jul 19, 2024

github-actions bot commented Jul 22, 2024

jordigh commented Jul 28, 2024 •

edited

Loading

github-actions bot commented Jul 28, 2024

github-actions bot commented Jul 29, 2024

paulfitz left a comment

nbush commented Jul 29, 2024

github-actions bot commented Jul 29, 2024

github-actions bot commented Jul 29, 2024

jordigh commented Jul 29, 2024

github-actions bot commented Jul 29, 2024

github-actions bot commented Jul 29, 2024

nbush commented Jul 30, 2024

paulfitz commented Jul 30, 2024 •

edited

Loading

paulfitz commented Jul 30, 2024

jordigh commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

paulfitz left a comment

paulfitz Jul 30, 2024

paulfitz Jul 30, 2024

Add new admin toggle for turning Enterprise on and off #1095

Add new admin toggle for turning Enterprise on and off #1095

Conversation

jordigh commented Jul 9, 2024 • edited Loading

github-actions bot commented Jul 9, 2024

github-actions bot commented Jul 9, 2024

github-actions bot commented Jul 16, 2024

github-actions bot commented Jul 16, 2024

github-actions bot commented Jul 18, 2024

github-actions bot commented Jul 18, 2024

github-actions bot commented Jul 19, 2024

github-actions bot commented Jul 22, 2024

jordigh commented Jul 28, 2024 • edited Loading

github-actions bot commented Jul 28, 2024

github-actions bot commented Jul 29, 2024

paulfitz left a comment

Choose a reason for hiding this comment

nbush commented Jul 29, 2024

github-actions bot commented Jul 29, 2024

github-actions bot commented Jul 29, 2024

jordigh commented Jul 29, 2024

github-actions bot commented Jul 29, 2024

github-actions bot commented Jul 29, 2024

nbush commented Jul 30, 2024

paulfitz commented Jul 30, 2024 • edited Loading

paulfitz commented Jul 30, 2024

jordigh commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

paulfitz left a comment

Choose a reason for hiding this comment

paulfitz Jul 30, 2024

Choose a reason for hiding this comment

paulfitz Jul 30, 2024

Choose a reason for hiding this comment

jordigh commented Jul 9, 2024 •

edited

Loading

jordigh commented Jul 28, 2024 •

edited

Loading

paulfitz commented Jul 30, 2024 •

edited

Loading