Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[x-license] Add support for plan version #13459

Merged
merged 25 commits into from
Jun 20, 2024

Conversation

michelengelen
Copy link
Member

@michelengelen michelengelen commented Jun 12, 2024

@mui-bot
Copy link

mui-bot commented Jun 12, 2024

Deploy preview: https://deploy-preview-13459--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against a19da9d

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic seems good 👌
Thanks for taking care of it

and changed the licenseError for the product scope
@michelengelen
Copy link
Member Author

@flaviendelangle

describeConformance(<RichTreeViewPro items={[]} />, () => ({
seems to be the only commercial component where we do the conformance test. Should we skip that? Or should we adjust it to use the license package?

@flaviendelangle
Copy link
Member

@michelengelen I think we have also conformance test on range pickers.
We should not drop that

@michelengelen michelengelen marked this pull request as ready for review June 20, 2024 12:23
@michelengelen michelengelen enabled auto-merge (squash) June 20, 2024 12:24
@michelengelen michelengelen merged commit c84a2ee into mui:master Jun 20, 2024
15 checks passed
@michelengelen michelengelen deleted the license/2024-license branch June 20, 2024 13:16
@flaviendelangle
Copy link
Member

I would have liked to decide clarify the status here before merging: #13459 (comment)

@michelengelen
Copy link
Member Author

I would have liked to decide clarify the status here before merging: #13459 (comment)

Oh, I thought with @joserodolfofreitas giving his 👍🏼 it was resolved... sry for that.

I did however open a new PR #13568 where we can adjust this.

@flaviendelangle
Copy link
Member

I'm talking about the name of the status we use mostly, the error message is nice I think
It's related with #13568 so we can discuss it there 👍

@zannager zannager added the MUI X label Jun 20, 2024
@oliviertassinari oliviertassinari added the package: x-license Specific to @mui/x-license. label Jun 26, 2024
@oliviertassinari oliviertassinari changed the title [x-license] license update proposal [x-license] Add support for plan version Jun 26, 2024
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes make sense to me at a high level, great to have them 👍

Comment on lines 14 to +17
case LICENSE_STATUS.OutOfScope:
return 'MUI X License key plan mismatch';
case LICENSE_STATUS.ProductNotCovered:
return 'MUI X Product not covered by plan';
Copy link
Member

@oliviertassinari oliviertassinari Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like these two errors OutOfScope + ProductNotCovered are the same error. Should we merge them? As for the need to show different error messages, I think this should be done through the meta data.

If we want to keep them distinct, I would make their name match, e.g.

OutOfScope -> PlanScopeNotCovered
ProductNotCovered -> ProductScopeNotCovered

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I advocated to change ProductNotCovered into a more specific NotAvailableInInitialProPlan in #13568, so that we can display a more precise error in the doc.
We could indeed rename OutOfScope which is super broad right now.

Copy link
Member

@oliviertassinari oliviertassinari Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotAvailableInInitialProPlan so that we can display a more precise error in the doc.

There will be more cases like this in the future of having a paid for one of the MUI X plans, and trying to use a different plan. I don't see creating a new error type each time scaling.

So It feels like this should be one error type and use the metadata to have a precise error in the console.

For the docs, I agree, I can see why we need to separate the cases: (a.) buying pro and using pro without working is a different confusion source than (b.) buying pro and trying to use premium.

But anyway, no strong push on my end, more of a feeling that in the code, it feels strange.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see creating a new error type each time scaling.

The status is only used to do some sort of switch and display a warning message.
So I don't see how metadata would be more or less scalable than a different status message.
Even if we end up with 20 different statuses, the big part is the switch of conditions.

orderNumber: 'MUI-123',
scope: 'pro',
licensingModel: 'subscription',
planVersion: 'Q3-2024',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm used to:

Suggested change
planVersion: 'Q3-2024',
planVersion: '2024-Q3',

I wonder if we should change this. But I guess it's up to the license key generator logic.

@@ -1,3 +1,20 @@
export const LICENSE_SCOPES = ['pro', 'premium'] as const;
export const PRODUCT_SCOPES = ['data-grid', 'date-pickers', 'charts', 'tree-view'] as const;
export const PLAN_VERSIONS = ['initial', 'Q3-2024'] as const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced about "initial". To me, what we have today is

Suggested change
export const PLAN_VERSIONS = ['initial', 'Q3-2024'] as const;
export const PLAN_VERSIONS = ['Q3-2022', 'Q3-2024'] as const;

the last time we changed the plan.

@@ -14,9 +15,9 @@ const oneDayInMS = 1000 * 60 * 60 * 24;
const releaseDate = new Date(3000, 0, 0, 0, 0, 0, 0);
const RELEASE_INFO = generateReleaseInfo(releaseDate);

function TestComponent() {
const licesenStatus = useLicenseVerifier('x-date-pickers-pro', RELEASE_INFO);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I wrote licesen 🤪

export type ProductScope = (typeof PRODUCT_SCOPES)[number];
export type PlanVersion = (typeof PLAN_VERSIONS)[number];

export const extractProductScope = (packageName: string): ProductScope => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small detail, default function convention we use is function for top level scope and arrow function for lower scope; applied here would be:

Suggested change
export const extractProductScope = (packageName: string): ProductScope => {
export function extractProductScope(packageName: string): ProductScope {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: x-license Specific to @mui/x-license. plan: Premium Impact at least one Premium user plan: Pro Impact at least one Pro user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants