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

@@ -0,0 +1,5 @@
+---
+"saleor-app-products-feed": patch
+---
+
+Fixed error on loading configuration created in the previous version of the app.

.changeset/poor-mugs-fix.md

@@ -0,0 +1,5 @@
+---
+"saleor-app-products-feed": patch
+---
+
+Fixed issue with saving changes in the title template form.

.changeset/tasty-icons-drum.md

@@ -0,0 +1,5 @@
+---
+"saleor-app-products-feed": patch
+---
+
+Improved error logging in the tRPC API.

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();
     });
   });
 

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<typeof rootAppConfigSchema>;
 
 export type ChannelUrlsConfig = z.infer<typeof AppConfigSchema.channelUrls>;
 
+const logger = createLogger({ name: "AppConfig" });
+
 export class AppConfig {
   private rootData: RootConfig = {
     channelConfig: {},
@@ -56,7 +64,12 @@ export class AppConfig {
 
   constructor(initialData?: RootConfig) {
     if (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");
     }
   }
 

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"] })

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<ChannelFragment[]> => {
+    async ({ ctx: { logger, apiClient } }): Promise<ChannelFragment[]> => {
       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;
+    },
   ),
 });

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<TitleTemplateInput>({
     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<string | undefined>();
 
-  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 <Text>Loading...</Text>;
   }
 
-  const showForm = !isLoading && attributes?.length;
-
   return (
     <>
-      {showForm ? (
+      {!isLoading ? (
         <TitleFormattingConfigurationForm
           onSubmit={handleSubmit}
           initialData={formData}

apps/products-feed/src/modules/trpc/middlewares.ts

@@ -0,0 +1,32 @@
+import { createLogger } from "@saleor/apps-shared";
+import { middleware } from "./trpc-server";
+
+export const attachLogger = middleware(async ({ ctx, next, type, path }) => {
+  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;
+});

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,11 +69,14 @@ 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",
+  );
 
+  try {
     await verifyJWT({
       appId: ctx.appId,
       token: ctx.token,
@@ -86,14 +87,9 @@ const validateClientToken = middleware(async ({ ctx, next, meta }) => {
     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,
-        }),
       },
     });
   });