From bfdcafbc8fa5f0713a3a65b1b46391576f8dd4e6 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Mon, 19 Aug 2024 10:31:36 -0400 Subject: [PATCH] #8860: show activated mod metadata in mod metadata form (#9026) --- src/components/AsyncStateGate.tsx | 6 +- .../tabs/MatchRulesSection.test.tsx | 1 - .../modMetadata/ModMetadataEditor.test.tsx | 109 ++++++++++++++++++ .../tabs/modMetadata/ModMetadataEditor.tsx | 68 +++++------ src/tsconfig.strictNullChecks.json | 1 + src/types/modComponentTypes.ts | 5 +- 6 files changed, 144 insertions(+), 46 deletions(-) create mode 100644 src/pageEditor/tabs/modMetadata/ModMetadataEditor.test.tsx diff --git a/src/components/AsyncStateGate.tsx b/src/components/AsyncStateGate.tsx index c0a19795dd..d14630ea13 100644 --- a/src/components/AsyncStateGate.tsx +++ b/src/components/AsyncStateGate.tsx @@ -21,7 +21,6 @@ import { getErrorMessage } from "@/errors/errorHelpers"; import { type AsyncState, type FetchableAsyncState } from "@/types/sliceTypes"; import { Button } from "react-bootstrap"; import { isFetchableAsyncState } from "@/utils/asyncStateUtils"; -import { assertNotNullish } from "@/utils/nullishUtils"; /** * A standard error display for use with AsyncStateGate @@ -96,8 +95,9 @@ function AsyncStateGate( throw error; } - assertNotNullish(data, "data should be defined"); // Type-only check - return <>{children({ data })}; + // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion -- at this point, the data can only be null/undefined if Data is explicitly set to be nullable + const dataResult = data as Data; + return <>{children({ data: dataResult })}; } export default AsyncStateGate; diff --git a/src/pageEditor/tabs/MatchRulesSection.test.tsx b/src/pageEditor/tabs/MatchRulesSection.test.tsx index 939ee0d844..3d608286bb 100644 --- a/src/pageEditor/tabs/MatchRulesSection.test.tsx +++ b/src/pageEditor/tabs/MatchRulesSection.test.tsx @@ -50,7 +50,6 @@ describe("MatchRulesSection", () => { ], selectors: ["#foo"], }), - setupRedux(dispatch) {}, }).asFragment(), ).toMatchSnapshot(); }); diff --git a/src/pageEditor/tabs/modMetadata/ModMetadataEditor.test.tsx b/src/pageEditor/tabs/modMetadata/ModMetadataEditor.test.tsx new file mode 100644 index 0000000000..1792e6ebd6 --- /dev/null +++ b/src/pageEditor/tabs/modMetadata/ModMetadataEditor.test.tsx @@ -0,0 +1,109 @@ +/* + * Copyright (C) 2024 PixieBrix, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +import { render } from "@/pageEditor/testHelpers"; +import React from "react"; +import ModMetadataEditor from "@/pageEditor/tabs/modMetadata/ModMetadataEditor"; +import { formStateFactory } from "@/testUtils/factories/pageEditorFactories"; +import { actions as editorActions } from "@/pageEditor/store/editor/editorSlice"; +import modComponentSlice from "@/store/modComponents/modComponentSlice"; +import { + activatedModComponentFactory, + modMetadataFactory, +} from "@/testUtils/factories/modComponentFactories"; +import { type ModMetadata } from "@/types/modComponentTypes"; +import { screen } from "@testing-library/react"; +import { modDefinitionFactory } from "@/testUtils/factories/modDefinitionFactories"; +import { waitForEffect } from "@/testUtils/testHelpers"; +import { useOptionalModDefinition } from "@/modDefinitions/modDefinitionHooks"; +import { valueToAsyncState } from "@/utils/asyncStateUtils"; +import { type SemVerString } from "@/types/registryTypes"; + +// :shrug: ideally would mock the definitions returned from the server. But that's more complicated than it's worth, +// because you can't just mock the /api/registry/bricks/ endpoint due to the way our tests mock the background worker +jest.mock("@/modDefinitions/modDefinitionHooks"); + +const useOptionalModDefinitionMock = jest.mocked(useOptionalModDefinition); + +beforeEach(() => { + useOptionalModDefinitionMock.mockReset(); +}); + +describe("ModMetadataEditor", () => { + it("should render latest version", async () => { + const modMetadata: ModMetadata = modMetadataFactory(); + const formState = formStateFactory({ formStateConfig: { modMetadata } }); + + const modComponent = activatedModComponentFactory({ + _recipe: modMetadata, + }); + + useOptionalModDefinitionMock.mockReturnValue( + valueToAsyncState(modDefinitionFactory({ metadata: modMetadata })), + ); + + render(, { + setupRedux(dispatch) { + dispatch( + modComponentSlice.actions.UNSAFE_setModComponents([modComponent]), + ); + dispatch(editorActions.addModComponentFormState(formState)); + dispatch(editorActions.setActiveModId(formState.modMetadata!.id)); + }, + }); + + await waitForEffect(); + + expect(screen.getByLabelText("Name")).toHaveValue(modMetadata.name); + expect(screen.getByLabelText("Version")).toHaveValue(modMetadata.version); + }); + + it("should render if newer version", async () => { + const modMetadata: ModMetadata = modMetadataFactory(); + + const formState = formStateFactory({ formStateConfig: { modMetadata } }); + + const modComponent = activatedModComponentFactory({ + _recipe: modMetadata, + }); + + useOptionalModDefinitionMock.mockReturnValue( + valueToAsyncState( + modDefinitionFactory({ + metadata: { + ...modMetadata, + version: "999.999.999" as SemVerString, + }, + }), + ), + ); + + render(, { + setupRedux(dispatch) { + dispatch( + modComponentSlice.actions.UNSAFE_setModComponents([modComponent]), + ); + dispatch(editorActions.addModComponentFormState(formState)); + dispatch(editorActions.setActiveModId(formState.modMetadata!.id)); + }, + }); + + expect(screen.getByText("re-activate the mod")).toBeInTheDocument(); + // Should be the version of the activated mod component, not the latest version + expect(screen.getByLabelText("Version")).toHaveValue(modMetadata.version); + }); +}); diff --git a/src/pageEditor/tabs/modMetadata/ModMetadataEditor.tsx b/src/pageEditor/tabs/modMetadata/ModMetadataEditor.tsx index fbf21c8929..f177eaf4a5 100644 --- a/src/pageEditor/tabs/modMetadata/ModMetadataEditor.tsx +++ b/src/pageEditor/tabs/modMetadata/ModMetadataEditor.tsx @@ -22,8 +22,6 @@ import { selectDirtyMetadataForModId, } from "@/pageEditor/store/editor/editorSelectors"; import { Card, Container } from "react-bootstrap"; -import Loader from "@/components/Loader"; -import { getErrorMessage } from "@/errors/errorHelpers"; import { actions } from "@/pageEditor/store/editor/editorSlice"; import ErrorBoundary from "@/components/ErrorBoundary"; import Effect from "@/components/Effect"; @@ -44,6 +42,8 @@ import cx from "classnames"; import { assertNotNullish } from "@/utils/nullishUtils"; import { type RegistryId } from "@/types/registryTypes"; import { getActivateModHashRoute } from "@/extensionConsole/shared/routeHelpers"; +import { pick } from "lodash"; +import AsyncStateGate from "@/components/AsyncStateGate"; // TODO: This should be yup.SchemaOf but we can't set the `id` property to `RegistryId` // see: https://github.com/jquense/yup/issues/1183#issuecomment-749186432 @@ -99,11 +99,8 @@ const ModMetadataEditor: React.VoidFunctionComponent = () => { assertNotNullish(modId, "No active mod id"); - const { - data: modDefinition, - isFetching, - error, - } = useOptionalModDefinition(modId); + const modDefinitionQuery = useOptionalModDefinition(modId); + const modDefinition = modDefinitionQuery.data; // Select a single mod component for the mod to check the activated version. // We rely on the assumption that every component in the mod has the same version. @@ -117,15 +114,14 @@ const ModMetadataEditor: React.VoidFunctionComponent = () => { lt(activatedModVersion, latestModVersion); const dirtyMetadata = useSelector(selectDirtyMetadataForModId(modId)); - const savedMetadata = modDefinition?.metadata; - const metadata = dirtyMetadata ?? savedMetadata; + // Prefer the metadata from the activated mod component + const currentMetadata = + dirtyMetadata ?? modDefinitionComponent?._recipe ?? modDefinition?.metadata; - const initialFormState: Partial = { - id: metadata?.id, - name: metadata?.name, - version: metadata?.version, - description: metadata?.description, - }; + const initialFormState: Partial = pick( + currentMetadata, + ["id", "name", "version", "description"], + ); const dispatch = useDispatch(); const updateRedux = useCallback( @@ -135,18 +131,6 @@ const ModMetadataEditor: React.VoidFunctionComponent = () => { [dispatch], ); - if (isFetching || error) { - return ( - - {isFetching ? ( - - ) : ( -
{getErrorMessage(error)}
- )} - - ); - } - const renderBody: RenderBody = ({ values }) => ( @@ -190,19 +174,23 @@ const ModMetadataEditor: React.VoidFunctionComponent = () => { return ( - -
{ - console.error( - "The form's submit should not be called to save mod metadata. Use 'saveMod' from 'useSaveMod' instead.", - ); - }} - renderBody={renderBody} - renderSubmit={() => null} - /> - + + {() => ( + + { + console.error( + "The form's submit should not be called to save mod metadata. Use 'saveMod' from 'useSaveMod' instead.", + ); + }} + renderBody={renderBody} + renderSubmit={() => null} + /> + + )} + ); }; diff --git a/src/tsconfig.strictNullChecks.json b/src/tsconfig.strictNullChecks.json index 6e732e8cf3..32c3ebf6d8 100644 --- a/src/tsconfig.strictNullChecks.json +++ b/src/tsconfig.strictNullChecks.json @@ -1426,6 +1426,7 @@ "./pageEditor/tabs/logs/NavItemBadge.tsx", "./pageEditor/tabs/logs/useLogsBadgeState.ts", "./pageEditor/tabs/modMetadata/ModMetadataEditor.tsx", + "./pageEditor/tabs/modMetadata/ModMetadataEditor.test.tsx", "./pageEditor/tabs/modOptionsDefinitions/ModOptionsDefinitionEditor.test.tsx", "./pageEditor/tabs/modOptionsDefinitions/ModOptionsDefinitionEditor.tsx", "./pageEditor/tabs/modOptionsValues/ModOptionsValuesEditor.test.tsx", diff --git a/src/types/modComponentTypes.ts b/src/types/modComponentTypes.ts index 94774c21b1..a66b3001dc 100644 --- a/src/types/modComponentTypes.ts +++ b/src/types/modComponentTypes.ts @@ -44,9 +44,10 @@ import { isRegistryId, isUUID } from "@/types/helpers"; * * @see optionsSlice * @see ModComponentBase._recipe + * @see Metadata */ -// Don't export -- the use is clearer if it's always written as ModComponentBase[_recipe] property -// XXX: Usage may be clearer, but the ergonomics of (ModMetadata | undefined) are terrible to handle with strict null checks +// XXX: previously we didn't export because the usage was clearer as ModComponentBase[_recipe]. However, the ergonomics +// of (ModMetadata | undefined) were bad to handle with strict null checks export type ModMetadata = Metadata & { /** * `undefined` for mods that were activated prior to the field being added