[core] [docs] Add warnings, docs and gating for paid features #3156

bharatkashyap · 2024-02-01T18:36:53Z

I've read and followed the contributing guide on how to create great pull requests.
https://deploy-preview-3156--mui-toolpad-docs.netlify.app/toolpad/getting-started/roadmap/#paid-plan
Closes Implement pro features flag with license key #3113

Scenario	Warning
Using paid features on a free plan
Using paid features on a paid plan
Adding roles while on a free plan

… paid-plan

Janpot · 2024-02-02T11:43:03Z

packages/toolpad-app/src/server/localMode.ts

+  'role permissions'?: boolean;
+}
+
+export function detectPaidFeatures(application: Application): PaidFeaturesConfig | null {


These don't seem to be used outside of this module, why export them?

Suggested change

export function detectPaidFeatures(application: Application): PaidFeaturesConfig | null {

function detectPaidFeatures(application: Application): PaidFeaturesConfig | null {

Janpot · 2024-02-02T11:47:11Z

packages/toolpad-app/src/server/localMode.ts

+  }
+
+  const { authorization } = application.spec;
+  const hasRoles = authorization.roles && authorization.roles.length > 0;


Shouldn't have the roles property at all IMO.

Suggested change

const hasRoles = authorization.roles && authorization.roles.length > 0;

const hasRoles = !!application?.spec?.authorization?.roles

Janpot · 2024-02-02T11:48:16Z

packages/toolpad-core/src/appDom.ts

@@ -1184,3 +1187,8 @@ export function getPageTitle(node: PageNode): string {
 export function isCodePage(node: PageNode): boolean {
  return !!node.attributes.codeFile;
 }
+
+export function getPlan(dom: AppDom): ToolpadPlan | undefined {


Why not make this apply the default?

Suggested change

export function getPlan(dom: AppDom): ToolpadPlan | undefined {

export function getPlan(dom: AppDom): ToolpadPlan {

Janpot

A few remarks, otherwise this is good 👍

apedroferreira · 2024-02-02T15:47:48Z

docs/data/toolpad/getting-started/roadmap.md

+
+- ### Authorization
+
+  Features allowing you to grant conditional access to pages based on user roles are part of this proposed paid plan. Read more about this feature on [authorisation](/toolpad/concepts/rbac/).


Suggested change

Features allowing you to grant conditional access to pages based on user roles are part of this proposed paid plan. Read more about this feature on [authorisation](/toolpad/concepts/rbac/).

Features allowing you to grant conditional access to pages based on user roles are part of this proposed paid plan. Read more about this feature on [authorization](/toolpad/concepts/rbac/).

apedroferreira

Looks good, thanks Bharat! Added some feedback but should all be simple to address!

apedroferreira · 2024-02-02T15:52:44Z

packages/toolpad-app/src/server/localMode.ts

+
+  const { authorization } = application.spec;
+  const hasRoles = authorization.roles && authorization.roles.length > 0;
+  const hasPaidFeatures = hasRoles;


In my docs PR (#3067) I got the understanding that the whole Azure AD provider, even if only used for authentication, would also be a paid feature. Should it not be?
If it is we would need to check application.authentication.providers for if azure-ad is there.
If not I can adjust my docs PR.

apedroferreira · 2024-02-02T15:54:23Z

packages/toolpad-app/src/server/localMode.ts

@@ -964,6 +966,22 @@ export function getRequiredEnvVars(dom: appDom.AppDom): Set<string> {
  return new Set(allVars);
 }

+export interface PaidFeaturesConfig {
+  roles?: boolean;
+  'role permissions'?: boolean;


Why is this property needed, I guess just roles should cover it? Also if it's needed I guess it could have a camel-case name.

apedroferreira · 2024-02-02T15:55:55Z

packages/toolpad-app/src/server/localMode.ts

+      const paidFeatures = detectPaidFeatures(application);
+      if (paidFeatures) {
+        throw new Error(
+          `You are using ${chalk.bgBlue(Object.keys(paidFeatures))} which ${Object.keys(paidFeatures).length > 1 ? 'are paid features' : 'is a paid feature'}. To continue using Toolpad, upgrade your plan or remove this feature.`,


Not a big deal, but it might be worth it to use some object in the properties so that the object keys aren't necessarily the name of the feature we want to display.

apedroferreira · 2024-02-02T16:03:48Z

packages/toolpad-app/src/toolpad/AppEditor/AppAuthorizationEditor.tsx

+                )}
+              </TabPanel>
+              <TabPanel disableGutters value="roleMappings">
+                {isPaidPlan ? (


The role mappings tab only shows if at least one authentication provider that supports roles is selected. If we end up not letting users use even just authentication in any provider that supports roles, we wouldn't need the check in the role mappings tab. Anyway it's not very important probably, just mentioning it!

Makes sense! I've added a disabling paid authentication types from the Authentication selection section, but I feel we can keep as a way to keep the knowledge that role mappings is a paid feature in the codebase.

apedroferreira · 2024-02-02T16:08:09Z

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/PageOptionsPanel.tsx

-              <TextField {...params} label="Allowed roles" placeholder="Roles" />
-            )}
-          />
+          {isPaidPlan ? (


Would the upgrade message always appear to anyone who is using the free plan?
If so I guess it's better to just hide the controls here as this panel is always open?

Agreed with the fact that it could be a little disruptive and annoying. To me its okay to advertise the fact that there is an upgrade possible as much as we can to enable feature discovery - so we can potentially keep it for now

apedroferreira · 2024-02-02T16:15:10Z

packages/toolpad-app/src/toolpad/AppEditor/ErrorUpgrade.tsx

+        )
+      }
+    >
+      {`${feature ?? 'This feature'} is only available on paid plans.`}


Oh another nit, the error message right now says "Roles is only available on paid plans." instead of "Roles are". Also not sure if worth correcting though, just thought I'd mention it for completeness.

Agreed! For now this is using singular nouns everywhere but we can enhance this to detect pluralization

apedroferreira

Looks great Bharat, thanks for the changes!

apedroferreira · 2024-02-05T18:40:11Z

packages/toolpad-app/src/server/localMode.ts

+  const paidFeatures = [
+    hasRoles ? { id: 'roles', label: 'Role based access control' } : undefined,
+    hasAzureActiveDirectory
+      ? { id: 'azure-ad', label: 'Azure Active Directory authentication' }


Suggested change

? { id: 'azure-ad', label: 'Azure Active Directory authentication' }

? { id: 'azure-ad', label: 'Azure Active Directory authentication provider' }

I would add the word provider, maybe "Azure AD authentication provider" not to be too long.

Or maybe it's fine, whatever you prefer :D

apedroferreira · 2024-02-05T18:48:24Z

packages/toolpad-app/src/toolpad/AppEditor/AppAuthorizationEditor.tsx

@@ -177,6 +182,20 @@ export function AppAuthenticationEditor() {
          placeholder="example.com"
        />
      ))}
+      {!isPaidPlan ? (
+        <UpgradeAlert


Maybe this alert for authentication providers would look better just below the provider input (below the blue alert)? Anyway I'll approve it already, not a big deal.

I did try that, to me it made more sense at the end of the dialog (so that it would be non-obstructive)

Ok, that's fine then!

bharatkashyap added 3 commits February 1, 2024 23:33

docs: Add upgrade guide

7f2d532

feat: Add plan check

26b9d2a

fix: Add upgrade to Roadmap instead

7bc90d7

bharatkashyap added docs Improvements or additions to the documentation enhancement This is not a bug, nor a new feature dx Related to developers' experience labels Feb 1, 2024

bharatkashyap added 3 commits February 2, 2024 03:36

Merge branch 'master' into paid-plan

d20347d

fix: jsonSchemas

9b8948e

Merge branch 'paid-plan' of github.com:bharatkashyap/mui-toolpad into…

0a1bfad

… paid-plan

bharatkashyap requested a review from Janpot February 2, 2024 05:14

Janpot reviewed Feb 2, 2024

View reviewed changes

Janpot approved these changes Feb 2, 2024

View reviewed changes

apedroferreira reviewed Feb 2, 2024

View reviewed changes

apedroferreira requested changes Feb 2, 2024

View reviewed changes

apedroferreira reviewed Feb 2, 2024

View reviewed changes

bharatkashyap added 4 commits February 5, 2024 12:16

fix: Review nits

5affff0

feat: More gating, reusable Upgrade alert

185b5d6

docs: Make upgrading instructions clearer

8f6a3eb

Merge branch 'master' into paid-plan

8219732

bharatkashyap requested a review from apedroferreira February 5, 2024 09:06

apedroferreira approved these changes Feb 5, 2024

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 5, 2024

bharatkashyap added 2 commits February 6, 2024 14:37

Merge branch 'master' of github.com:mui/mui-toolpad into paid-plan

64f41d5

fix: Pedro review

b78dc52

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 6, 2024

fix: Don't gate auth fixture should

da82e4f

bharatkashyap merged commit 6e07cac into mui:master Feb 6, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] [docs] Add warnings, docs and gating for paid features #3156

[core] [docs] Add warnings, docs and gating for paid features #3156

bharatkashyap commented Feb 1, 2024 •

edited

Loading

Janpot Feb 2, 2024 •

edited

Loading

Janpot Feb 2, 2024

Janpot Feb 2, 2024

Janpot left a comment

apedroferreira Feb 2, 2024

apedroferreira left a comment •

edited

Loading

apedroferreira Feb 2, 2024

apedroferreira Feb 2, 2024

apedroferreira Feb 2, 2024

apedroferreira Feb 2, 2024

bharatkashyap Feb 5, 2024

apedroferreira Feb 2, 2024

bharatkashyap Feb 5, 2024

apedroferreira Feb 2, 2024 •

edited

Loading

bharatkashyap Feb 5, 2024

apedroferreira left a comment

apedroferreira Feb 5, 2024

apedroferreira Feb 5, 2024

apedroferreira Feb 5, 2024

bharatkashyap Feb 6, 2024

apedroferreira Feb 6, 2024

	export function detectPaidFeatures(application: Application): PaidFeaturesConfig \| null {
	function detectPaidFeatures(application: Application): PaidFeaturesConfig \| null {

	const hasRoles = authorization.roles && authorization.roles.length > 0;
	const hasRoles = !!application?.spec?.authorization?.roles

	export function getPlan(dom: AppDom): ToolpadPlan \| undefined {
	export function getPlan(dom: AppDom): ToolpadPlan {


		- ### Authorization

		Features allowing you to grant conditional access to pages based on user roles are part of this proposed paid plan. Read more about this feature on [authorisation](/toolpad/concepts/rbac/).

	? { id: 'azure-ad', label: 'Azure Active Directory authentication' }
	? { id: 'azure-ad', label: 'Azure Active Directory authentication provider' }

[core] [docs] Add warnings, docs and gating for paid features #3156

[core] [docs] Add warnings, docs and gating for paid features #3156

Conversation

bharatkashyap commented Feb 1, 2024 • edited Loading

Janpot Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janpot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apedroferreira left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apedroferreira Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apedroferreira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharatkashyap commented Feb 1, 2024 •

edited

Loading

Janpot Feb 2, 2024 •

edited

Loading

apedroferreira left a comment •

edited

Loading

apedroferreira Feb 2, 2024 •

edited

Loading