From bb504d95aac71afb2072696adb33d2ca0707f2bd Mon Sep 17 00:00:00 2001 From: Krzysztof Wolski Date: Thu, 10 Aug 2023 17:37:59 +0200 Subject: [PATCH] Product feed: Fix title template form (#882) * Improve logging * Fix error on parsing metadata created in the previous version * Fix title template form * Add changesets * Use info level instead of debug --- .changeset/nervous-tables-agree.md | 5 + .changeset/poor-mugs-fix.md | 5 + .changeset/tasty-icons-drum.md | 5 + .../app-configuration/app-config.test.ts | 101 ++++++++++-------- .../modules/app-configuration/app-config.ts | 30 ++++-- .../app-configuration.router.ts | 21 ++-- .../channels/channels.router.ts | 10 +- .../title-formatting-form.tsx | 19 ++-- .../src/modules/trpc/middlewares.ts | 32 ++++++ .../trpc/protected-client-procedure.ts | 63 +++++------ 10 files changed, 177 insertions(+), 114 deletions(-) create mode 100644 .changeset/nervous-tables-agree.md create mode 100644 .changeset/poor-mugs-fix.md create mode 100644 .changeset/tasty-icons-drum.md create mode 100644 apps/products-feed/src/modules/trpc/middlewares.ts diff --git a/.changeset/nervous-tables-agree.md b/.changeset/nervous-tables-agree.md new file mode 100644 index 0000000..8129784 --- /dev/null +++ b/.changeset/nervous-tables-agree.md @@ -0,0 +1,5 @@ +--- +"saleor-app-products-feed": patch +--- + +Fixed error on loading configuration created in the previous version of the app. diff --git a/.changeset/poor-mugs-fix.md b/.changeset/poor-mugs-fix.md new file mode 100644 index 0000000..1beee97 --- /dev/null +++ b/.changeset/poor-mugs-fix.md @@ -0,0 +1,5 @@ +--- +"saleor-app-products-feed": patch +--- + +Fixed issue with saving changes in the title template form. diff --git a/.changeset/tasty-icons-drum.md b/.changeset/tasty-icons-drum.md new file mode 100644 index 0000000..a4a9c43 --- /dev/null +++ b/.changeset/tasty-icons-drum.md @@ -0,0 +1,5 @@ +--- +"saleor-app-products-feed": patch +--- + +Improved error logging in the tRPC API. diff --git a/apps/products-feed/src/modules/app-configuration/app-config.test.ts b/apps/products-feed/src/modules/app-configuration/app-config.test.ts index 5e2f33b..37f5af9 100644 --- a/apps/products-feed/src/modules/app-configuration/app-config.test.ts +++ b/apps/products-feed/src/modules/app-configuration/app-config.test.ts @@ -1,9 +1,43 @@ import { describe, expect, it } from "vitest"; -import { AppConfig } from "./app-config"; +import { AppConfig, RootConfig } from "./app-config"; + +const exampleChannelConfig: RootConfig["channelConfig"] = { + test: { + storefrontUrls: { + productStorefrontUrl: "https://example.com", + storefrontUrl: "https://example.com/p/{{ variant.product.slug }}", + }, + }, +}; + +const exampleS3Config: RootConfig["s3"] = { + accessKeyId: "example-access-key", + bucketName: "example-bucket-name", + region: "eu-west-1", + secretAccessKey: "example-secret-key", +}; + +const exampleAttributeMappingConfig: RootConfig["attributeMapping"] = { + brandAttributeIds: ["brand-attribute-1"], + colorAttributeIds: [], + patternAttributeIds: [], + materialAttributeIds: [], + sizeAttributeIds: [], +}; + +const exampleTitleTemplate: RootConfig["titleTemplate"] = + "Example {{ variant.product.name }} - {{ variant.name }}"; + +const exampleConfiguration: RootConfig = { + channelConfig: exampleChannelConfig, + s3: exampleS3Config, + attributeMapping: exampleAttributeMappingConfig, + titleTemplate: exampleTitleTemplate, +}; describe("AppConfig", function () { describe("Construction", () => { - it("Constructs empty state", () => { + it("Constructs configuration with default values, when empty object is passed as initial data", () => { const instance = new AppConfig(); expect(instance.getRootConfig()).toEqual({ @@ -20,47 +54,24 @@ describe("AppConfig", function () { }); }); - it("Constructs from initial state", () => { - const instance = new AppConfig({ - s3: { - region: "region", - bucketName: "bucket", - accessKeyId: "access", - secretAccessKey: "secret", - }, - channelConfig: { - test: { - storefrontUrls: { - productStorefrontUrl: "https://example.com", - storefrontUrl: "https://example.com/p/{{ variant.product.slug }}", - }, - }, - }, - attributeMapping: { - brandAttributeIds: [], - colorAttributeIds: [], - patternAttributeIds: [], - materialAttributeIds: [], - sizeAttributeIds: [], - }, - titleTemplate: "{{ variant.name }}", - }); + it("Constructs configuration, when valid initial state is passed", () => { + const instance = new AppConfig(exampleConfiguration); + + expect(instance.getRootConfig()).toEqual(exampleConfiguration); + }); + + it("Fill attribute mapping and title template with default values, when initial data are lacking those fields", () => { + const configurationWithoutMapping = structuredClone(exampleConfiguration); + + // @ts-expect-error: Simulating data before the migration + delete configurationWithoutMapping.attributeMapping; + // @ts-expect-error + delete configurationWithoutMapping.titleTemplate; + + const instance = new AppConfig(configurationWithoutMapping as any); // Casting used to prevent TS from reporting an error expect(instance.getRootConfig()).toEqual({ - s3: { - bucketName: "bucket", - secretAccessKey: "secret", - accessKeyId: "access", - region: "region", - }, - channelConfig: { - test: { - storefrontUrls: { - productStorefrontUrl: "https://example.com", - storefrontUrl: "https://example.com/p/{{ variant.product.slug }}", - }, - }, - }, + ...exampleConfiguration, attributeMapping: { brandAttributeIds: [], colorAttributeIds: [], @@ -68,7 +79,7 @@ describe("AppConfig", function () { materialAttributeIds: [], sizeAttributeIds: [], }, - titleTemplate: "{{ variant.name }}", + titleTemplate: "{{variant.product.name}} - {{variant.name}}", }); }); @@ -78,7 +89,7 @@ describe("AppConfig", function () { new AppConfig({ // @ts-expect-error foo: "bar", - }) + }), ).toThrow(); }); @@ -226,7 +237,7 @@ describe("AppConfig", function () { }); // @ts-expect-error - expect(() => instance.setS3({ foo: "bar" })).toThrow(); + expect(() => instance.setS3({ foo: "bar" })).toThrowError(); }); it("setChannelUrls sets valid config to channelConfig[channelSlug] and rejects invalid config", () => { @@ -243,7 +254,7 @@ describe("AppConfig", function () { }); // @ts-expect-error - expect(() => instance.setChannelUrls("channel", "foo")).toThrow(); + expect(() => instance.setChannelUrls("channel", "foo")).toThrowError(); }); }); diff --git a/apps/products-feed/src/modules/app-configuration/app-config.ts b/apps/products-feed/src/modules/app-configuration/app-config.ts index 91c8625..956fe25 100644 --- a/apps/products-feed/src/modules/app-configuration/app-config.ts +++ b/apps/products-feed/src/modules/app-configuration/app-config.ts @@ -1,3 +1,4 @@ +import { createLogger } from "@saleor/apps-shared"; import { z } from "zod"; const titleTemplateFieldSchema = z.string().default("{{variant.product.name}} - {{variant.name}}"); @@ -30,8 +31,13 @@ const urlConfigurationSchema = z.object({ const rootAppConfigSchema = z.object({ s3: s3ConfigSchema.nullable(), - titleTemplate: titleTemplateFieldSchema, - attributeMapping: attributeMappingSchema.nullable(), + titleTemplate: titleTemplateFieldSchema + .optional() + .default(titleTemplateFieldSchema.parse(undefined)), + attributeMapping: attributeMappingSchema + .nullable() + .optional() + .default(attributeMappingSchema.parse({})), channelConfig: z.record(z.object({ storefrontUrls: urlConfigurationSchema })), }); @@ -46,6 +52,8 @@ export type RootConfig = z.infer; export type ChannelUrlsConfig = z.infer; +const logger = createLogger({ name: "AppConfig" }); + export class AppConfig { private rootData: RootConfig = { channelConfig: {}, @@ -56,7 +64,12 @@ export class AppConfig { constructor(initialData?: RootConfig) { if (initialData) { - this.rootData = rootAppConfigSchema.parse(initialData); + try { + this.rootData = rootAppConfigSchema.parse(initialData); + } catch (e) { + logger.error(e, "Could not parse initial data"); + throw new Error("Can't load the configuration"); + } } } @@ -78,8 +91,7 @@ export class AppConfig { return this; } catch (e) { - console.error(e); - + logger.info(e, "Invalid S3 config provided"); throw new Error("Invalid S3 config provided"); } } @@ -90,8 +102,7 @@ export class AppConfig { return this; } catch (e) { - console.error(e); - + logger.info(e, "Invalid mapping config provided"); throw new Error("Invalid mapping config provided"); } } @@ -106,9 +117,8 @@ export class AppConfig { return this; } catch (e) { - console.error(e); - - throw new Error("Invalid payload"); + logger.info(e, "Invalid channels config provided"); + throw new Error("Invalid channels config provided"); } } diff --git a/apps/products-feed/src/modules/app-configuration/app-configuration.router.ts b/apps/products-feed/src/modules/app-configuration/app-configuration.router.ts index 7d1f30e..7c0bb4f 100644 --- a/apps/products-feed/src/modules/app-configuration/app-configuration.router.ts +++ b/apps/products-feed/src/modules/app-configuration/app-configuration.router.ts @@ -17,11 +17,19 @@ export const appConfigurationRouter = router({ * Prefer fetching all to avoid unnecessary calls. Routes are cached by react-query */ fetch: protectedClientProcedure.query(async ({ ctx: { logger, getConfig } }) => { - return getConfig().then((c) => { - logger.debug("Fetched config"); + logger.debug("Fetching configuration"); - return c.getRootConfig(); - }); + try { + const configuration = await getConfig(); + + logger.debug("Configuration fetched"); + return configuration.getRootConfig(); + } catch (e) { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "Can't fetch the configuration", + }); + } }), testS3BucketConfiguration: protectedClientProcedure .meta({ requiredClientPermissions: ["MANAGE_APPS"] }) @@ -38,6 +46,7 @@ export const appConfigurationRouter = router({ bucketName: input.bucketName, s3Client, }); + logger.debug("Verification succeeded"); } catch { logger.debug("Validation failed"); throw new TRPCError({ @@ -88,7 +97,7 @@ export const appConfigurationRouter = router({ z.object({ channelSlug: z.string(), urls: AppConfigSchema.channelUrls, - }) + }), ) .mutation( async ({ @@ -117,7 +126,7 @@ export const appConfigurationRouter = router({ logger.debug("Saved config"); return null; - } + }, ), setAttributeMapping: protectedClientProcedure .meta({ requiredClientPermissions: ["MANAGE_APPS"] }) diff --git a/apps/products-feed/src/modules/app-configuration/channels/channels.router.ts b/apps/products-feed/src/modules/app-configuration/channels/channels.router.ts index 7c65ddb..84c07ef 100644 --- a/apps/products-feed/src/modules/app-configuration/channels/channels.router.ts +++ b/apps/products-feed/src/modules/app-configuration/channels/channels.router.ts @@ -5,12 +5,14 @@ import { ChannelFragment } from "../../../../generated/graphql"; export const channelsRouter = router({ fetch: protectedClientProcedure.query( - async ({ ctx: { logger, apiClient }, input }): Promise => { + async ({ ctx: { logger, apiClient } }): Promise => { const fetcher = new ChannelsFetcher(apiClient); - logger.debug("Will fetch channels"); + logger.debug("Fetching channels"); + const channels = fetcher.fetchChannels().then((channels) => channels ?? []); - return fetcher.fetchChannels().then((channels) => channels ?? []); - } + logger.debug("Channels fetched successfully"); + return channels; + }, ), }); diff --git a/apps/products-feed/src/modules/app-configuration/title-formatting-form.tsx b/apps/products-feed/src/modules/app-configuration/title-formatting-form.tsx index c625ddd..32c3d7b 100644 --- a/apps/products-feed/src/modules/app-configuration/title-formatting-form.tsx +++ b/apps/products-feed/src/modules/app-configuration/title-formatting-form.tsx @@ -1,4 +1,4 @@ -import { AppConfigSchema, TitleTemplateInput, titleTemplateInputSchema } from "./app-config"; +import { TitleTemplateInput, titleTemplateInputSchema } from "./app-config"; import { useForm } from "react-hook-form"; import { Box, Button, Text } from "@saleor/macaw-ui/next"; @@ -19,7 +19,7 @@ type Props = { export const TitleFormattingConfigurationForm = (props: Props) => { const { handleSubmit, control, getValues } = useForm({ defaultValues: props.initialData, - resolver: zodResolver(AppConfigSchema.attributeMapping), + resolver: zodResolver(titleTemplateInputSchema), }); return ( @@ -55,12 +55,7 @@ export const ConnectedTitleFormattingForm = () => { const { notifyError, notifySuccess } = useDashboardNotification(); const [preview, setPreview] = useState(); - const { data: attributes, isLoading: isAttributesLoading } = - trpcClient.appConfiguration.getAttributes.useQuery(); - - const { data, isLoading: isConfigurationLoading } = trpcClient.appConfiguration.fetch.useQuery(); - - const isLoading = isAttributesLoading || isConfigurationLoading; + const { data, isLoading } = trpcClient.appConfiguration.fetch.useQuery(); const { mutate } = trpcClient.appConfiguration.setTitleTemplate.useMutation({ onSuccess() { @@ -84,14 +79,14 @@ export const ConnectedTitleFormattingForm = () => { async (data: TitleTemplateInput) => { mutate(data); }, - [mutate] + [mutate], ); const handlePreview = useCallback( async (data: TitleTemplateInput) => { previewMutate(data); }, - [previewMutate] + [previewMutate], ); const formData: TitleTemplateInput = useMemo(() => { @@ -108,11 +103,9 @@ export const ConnectedTitleFormattingForm = () => { return Loading...; } - const showForm = !isLoading && attributes?.length; - return ( <> - {showForm ? ( + {!isLoading ? ( { + const loggerName = `tRPC ${type} ${path.replace(/\./g, "/")}`; + + const logger = createLogger({ + name: loggerName, + requestType: type, + path, + saleorApiUrl: ctx.saleorApiUrl, + }); + + const start = Date.now(); + + logger.debug(`Requested received`); + + const result = await next({ + ctx: { + logger, + }, + }); + const durationMs = Date.now() - start; + + if (result.ok) { + logger.debug({ durationMs }, `Response successful`); + } else { + logger.debug({ durationMs }, `Response with error`); + } + + return result; +}); diff --git a/apps/products-feed/src/modules/trpc/protected-client-procedure.ts b/apps/products-feed/src/modules/trpc/protected-client-procedure.ts index a0a244c..b52d7ec 100644 --- a/apps/products-feed/src/modules/trpc/protected-client-procedure.ts +++ b/apps/products-feed/src/modules/trpc/protected-client-procedure.ts @@ -3,14 +3,15 @@ import { middleware, procedure } from "./trpc-server"; import { TRPCError } from "@trpc/server"; import { ProtectedHandlerError } from "@saleor/app-sdk/handlers/next"; import { saleorApp } from "../../saleor-app"; -import { createLogger, logger } from "@saleor/apps-shared"; +import { createLogger } from "@saleor/apps-shared"; import { GraphqlClientFactory } from "../../lib/create-graphq-client"; import { AppConfigMetadataManager } from "../app-configuration/app-config-metadata-manager"; import { createSettingsManager } from "../../lib/metadata-manager"; import { AppConfig } from "../app-configuration/app-config"; +import { attachLogger } from "./middlewares"; const attachAppToken = middleware(async ({ ctx, next }) => { - logger.debug("attachAppToken middleware"); + const logger = createLogger({ name: "attachAppToken" }); if (!ctx.saleorApiUrl) { logger.debug("ctx.saleorApiUrl not found, throwing"); @@ -21,6 +22,7 @@ const attachAppToken = middleware(async ({ ctx, next }) => { }); } + logger.debug("Getting auth data"); const authData = await saleorApp.apl.get(ctx.saleorApiUrl); if (!authData) { @@ -31,6 +33,7 @@ const attachAppToken = middleware(async ({ ctx, next }) => { message: "Missing auth data", }); } + logger.debug("Auth data found, attaching it to the context"); return next({ ctx: { @@ -42,12 +45,7 @@ const attachAppToken = middleware(async ({ ctx, next }) => { }); const validateClientToken = middleware(async ({ ctx, next, meta }) => { - logger.debug( - { - permissions: meta?.requiredClientPermissions, - }, - "Calling validateClientToken middleware with permissions required" - ); + const logger = createLogger({ name: "validateClientToken" }); if (!ctx.token) { throw new TRPCError({ @@ -71,29 +69,27 @@ const validateClientToken = middleware(async ({ ctx, next, meta }) => { }); } - if (!ctx.ssr) { - try { - logger.debug("trying to verify JWT token from frontend"); - logger.debug({ token: ctx.token ? `${ctx.token[0]}...` : undefined }); + logger.debug( + { + permissions: meta?.requiredClientPermissions, + }, + "Calling validateClientToken middleware with permissions required", + ); - await verifyJWT({ - appId: ctx.appId, - token: ctx.token, - saleorApiUrl: ctx.saleorApiUrl, - requiredPermissions: meta?.requiredClientPermissions ?? [], - }); - } catch (e) { - logger.debug("JWT verification failed, throwing"); - throw new ProtectedHandlerError("JWT verification failed: ", "JWT_VERIFICATION_FAILED"); - } + try { + await verifyJWT({ + appId: ctx.appId, + token: ctx.token, + saleorApiUrl: ctx.saleorApiUrl, + requiredPermissions: meta?.requiredClientPermissions ?? [], + }); + } catch (e) { + logger.debug("JWT verification failed, throwing"); + throw new ProtectedHandlerError("JWT verification failed: ", "JWT_VERIFICATION_FAILED"); } - return next({ - ctx: { - ...ctx, - saleorApiUrl: ctx.saleorApiUrl, - }, - }); + logger.debug("Token verified"); + return next(); }); /** @@ -103,11 +99,12 @@ const validateClientToken = middleware(async ({ ctx, next, meta }) => { * otherwise jwks validation will fail (if createCaller used) */ export const protectedClientProcedure = procedure + .use(attachLogger) .use(attachAppToken) .use(validateClientToken) - .use(async ({ ctx, next, path, type }) => { + .use(async ({ ctx, next }) => { const client = GraphqlClientFactory.fromAuthData({ - token: ctx.appToken!, + token: ctx.appToken, saleorApiUrl: ctx.saleorApiUrl, }); @@ -125,12 +122,6 @@ export const protectedClientProcedure = procedure return metadata ? AppConfig.parse(metadata) : new AppConfig(); }, - logger: createLogger({ - appId: ctx.appId, - apiUrl: ctx.saleorApiUrl, - type, - path, - }), }, }); });