fix(site): display correct user_limit on license ui #8118
Conversation
| @@ -30,6 +30,9 @@ export const LicenseCard = ({ | |||
| number | undefined | |||
| >(undefined) | |||
|
|
|||
| const currentUserLimit = | |||
| license.claims.features["user_limit"] || userLimitLimit | |||
There was a problem hiding this comment.
This is probably out of scope of this PR, but userLimitLimit sounds weird.
There was a problem hiding this comment.
I find it weird as well
| @@ -72,7 +75,7 @@ export const LicenseCard = ({ | |||
| <Stack direction="column" spacing={0} alignItems="center"> | |||
| <span className={styles.secondaryMaincolor}>Users</span> | |||
| <span className={styles.userLimit}> | |||
| {userLimitActual} {` / ${userLimitLimit || "Unlimited"}`} | |||
| {userLimitActual} {` / ${currentUserLimit || "Unlimited"}`} | |||
There was a problem hiding this comment.
Should we move the condition || "Unlimited" to currentUserLimit?
There was a problem hiding this comment.
I personally would rather not doing it since the user limit is a number and here we are dealing with "presentational" logic
So, when the license UI was created, we used to have a bug on the backend that would return each license's user_limit as the same as the maximum number of users allowed on all licenses. For that reason we only displayed that max value. The bug was fixed, but we never updated the UI to display the correct value of each license. This PR fixes that. |
| @@ -1006,7 +1006,7 @@ export const getWorkspaceBuildParameters = async ( | |||
| return response.data | |||
| } | |||
| type Claims = { | |||
| license_expires?: number | |||
| license_expires: number | |||
There was a problem hiding this comment.
Is this type not returned from the typesGenerated.ts? Usually, the API endpoints and resources have generated types that are more trustable.
There was a problem hiding this comment.
@BrunoQuaresma No, the type returned from the typesGenerated is not that strict, check the claims type:
| await screen.findByText("#1") | ||
| await screen.findByText("1 / 10") | ||
| await screen.findByText("Enterprise") |
There was a problem hiding this comment.
It doesn't make sense to look at the entire screen for #1 as I'm sure that you will find something else. Same for Enterprise. Could we limit it only to the rendered element?
There was a problem hiding this comment.
I think this is ok and a common practice for JS tests when using react testing library
There was a problem hiding this comment.
The practice is fine, but we should limit/scope it to the element under test.
There was a problem hiding this comment.
I mean it is safe because if the matcher finds it in multiple elements, it will raise an error.
There was a problem hiding this comment.
What if it finds a static label placed somewhere else on the page, and our license card didn't render at all?
There was a problem hiding this comment.
The storybook should get that. IMO, integration tests should be used to catch network and "actionable" things. Check if things are rendered correctly should be on Storybook but I get your point.
There was a problem hiding this comment.
it is usually a recommendation to use screen: https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#not-using-screen
And also in this case we are only rendering the component, there isn't anything else in the screen that could be matched. I have printed the result of a screen.debug() and this is the result:
<body>
<div>
<div
class="MuiPaper-root MuiPaper-elevation MuiPaper-rounded MuiPaper-elevation2 makeStyles-licenseCard-27 css-19zg6r2-MuiPaper-root"
>
<div
class="makeStyles-stack-39 makeStyles-stack-40 makeStyles-cardContent-28"
>
<span
class="makeStyles-licenseId-29"
>
#
1
</span>
<span
class="makeStyles-accountType-30"
>
Enterprise
</span>
<div
class="makeStyles-stack-39 makeStyles-stack-41"
style="flex: 1;"
>
<div
class="makeStyles-stack-39 makeStyles-stack-42"
>
<span
class="makeStyles-secondaryMaincolor-33"
>
Users
</span>
<span
class="makeStyles-userLimit-26"
>
1
/ Unlimited
</span>
</div>
<div
class="makeStyles-stack-39 makeStyles-stack-43"
width="134px"
>
<span
class="makeStyles-secondaryMaincolor-33"
>
Valid Until
</span>
<span
class="makeStyles-licenseExpires-31"
>
May 20, 2078
</span>
</div>
<div
class="makeStyles-stack-39 makeStyles-stack-44"
>
<button
class="MuiButtonBase-root MuiButton-root MuiButton-text MuiButton-textNeutral MuiButton-sizeSmall MuiButton-textSizeSmall MuiButton-root MuiButton-text MuiButton-textNeutral MuiButton-sizeSmall MuiButton-textSizeSmall makeStyles-removeButton-34 css-1sr08q4-MuiButtonBase-root-MuiButton-root"
tabindex="0"
type="button"
>
Remove
</button>
</div>
</div>
</div>
</div>
</div>
</body>
as you can see, just the component is being rendered.
Fixes #7671
So, when the license UI was created, we used to have a bug on the backend that would return each license's user_limit as the same as the maximum number of users allowed on all licenses. For that reason we only displayed that max value. The bug was fixed, but we never updated the UI to display the correct value of each license. This PR fixes that.