Skip to content

fix(site): display correct user_limit on license ui #8118

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 23, 2023
Merged

fix(site): display correct user_limit on license ui #8118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
364c473
fix(site): display correct user_limit on license ui
rodrimaia Jun 20, 2023
42565c8
fix type and add tests
rodrimaia Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion site/src/api/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1006,7 +1006,7 @@ export const getWorkspaceBuildParameters = async (
return response.data
}
type Claims = {
license_expires?: number
license_expires: number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this type not returned from the typesGenerated.ts? Usually, the API endpoints and resources have generated types that are more trustable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BrunoQuaresma No, the type returned from the typesGenerated is not that strict, check the claims type:

image

account_type?: string
account_id?: string
trial: boolean
Expand Down
70 changes: 70 additions & 0 deletions site/src/components/LicenseCard/LicenseCard.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import { screen } from "@testing-library/react"
import { render } from "../../testHelpers/renderHelpers"
import { LicenseCard } from "./LicenseCard"
import { MockLicenseResponse } from "testHelpers/entities"

describe("LicenseCard", () => {
it("renders (smoke test)", async () => {
// When
render(
<LicenseCard
license={MockLicenseResponse[0]}
userLimitActual={1}
userLimitLimit={10}
onRemove={() => null}
isRemoving={false}
/>,
)

// Then
await screen.findByText("#1")
await screen.findByText("1 / 10")
await screen.findByText("Enterprise")
Comment on lines +20 to +22
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is ok and a common practice for JS tests when using react testing library

Copy link
Member

@mtojek mtojek Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The practice is fine, but we should limit/scope it to the element under test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean it is safe because if the matcher finds it in multiple elements, it will raise an error.

Copy link
Member

Choose a reason for hiding this comment

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

What if it finds a static label placed somewhere else on the page, and our license card didn't render at all?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

})

it("renders userLimit as unlimited if there is not user limit", async () => {
// When
render(
<LicenseCard
license={MockLicenseResponse[0]}
userLimitActual={1}
userLimitLimit={undefined}
onRemove={() => null}
isRemoving={false}
/>,
)

// Then
await screen.findByText("#1")
await screen.findByText("1 / Unlimited")
await screen.findByText("Enterprise")
})

it("renders license's user_limit when it is available instead of using the default", async () => {
const licenseUserLimit = 3
const license = {
...MockLicenseResponse[0],
claims: {
...MockLicenseResponse[0].claims,
features: {
...MockLicenseResponse[0].claims.features,
user_limit: licenseUserLimit,
},
},
}

// When
render(
<LicenseCard
license={license}
userLimitActual={1}
userLimitLimit={100} // This should not be used
onRemove={() => null}
isRemoving={false}
/>,
)

// Then
await screen.findByText("1 / 3")
})
})
9 changes: 6 additions & 3 deletions site/src/components/LicenseCard/LicenseCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
import Button from "@mui/material/Button"
import Paper from "@mui/material/Paper"
import { makeStyles } from "@mui/styles"
import { License } from "api/typesGenerated"
import { ConfirmDialog } from "components/Dialogs/ConfirmDialog/ConfirmDialog"
import { Stack } from "components/Stack/Stack"
import dayjs from "dayjs"
import { useState } from "react"
import { Pill } from "components/Pill/Pill"
import { compareAsc } from "date-fns"
import { GetLicensesResponse } from "api/api"

type LicenseCardProps = {
license: License
license: GetLicensesResponse
userLimitActual?: number
userLimitLimit?: number
onRemove: (licenseId: number) => void
Expand All @@ -30,6 +30,9 @@ export const LicenseCard = ({
number | undefined
>(undefined)

const currentUserLimit =
license.claims.features["user_limit"] || userLimitLimit
Copy link
Member

Choose a reason for hiding this comment

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

This is probably out of scope of this PR, but userLimitLimit sounds weird.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I find it weird as well


return (
<Paper key={license.id} elevation={2} className={styles.licenseCard}>
<ConfirmDialog
Expand Down Expand Up @@ -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"}`}
Copy link
Member

Choose a reason for hiding this comment

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

Should we move the condition || "Unlimited" to currentUserLimit?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally would rather not doing it since the user limit is a number and here we are dealing with "presentational" logic

Copy link
Member

Choose a reason for hiding this comment

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

Killer argument 👍

</span>
</Stack>
<Stack
Expand Down
4 changes: 2 additions & 2 deletions site/src/pages/DeploySettingsPage/LicensesSettingsPage/LicensesSettingsPageView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ const LicensesSettingsPageView: FC<Props> = ({
{licenses
?.sort(
(a, b) =>
new Date(b.claims.license_expires as number).valueOf() -
new Date(a.claims.license_expires as number).valueOf(),
new Date(b.claims.license_expires).valueOf() -
new Date(a.claims.license_expires).valueOf(),
)
.map((license) => (
<LicenseCard
Expand Down