From 958dbcdbc16858e569a446aa0338988017076143 Mon Sep 17 00:00:00 2001 From: Lukasz Ostrowski Date: Fri, 3 Nov 2023 12:36:27 +0100 Subject: [PATCH] Update contentful client to use system id (#1088) --- .changeset/twenty-garlics-count.md | 14 +++ .../contentful-bulk-sync-processor.ts | 6 +- .../contentful/contentful-client.test.ts | 51 +++++--- .../providers/contentful/contentful-client.ts | 114 ++++++++++++------ .../contentful-webhooks-processor.ts | 11 +- .../providers/contentful/contentful.tsx | 12 +- .../webhooks-processors-delegator.ts | 29 +++-- 7 files changed, 167 insertions(+), 70 deletions(-) create mode 100644 .changeset/twenty-garlics-count.md diff --git a/.changeset/twenty-garlics-count.md b/.changeset/twenty-garlics-count.md new file mode 100644 index 000000000..eb2d8eec9 --- /dev/null +++ b/.changeset/twenty-garlics-count.md @@ -0,0 +1,14 @@ +--- +"saleor-app-cms-v2": minor +--- + +Changed how Contentful ID is created. + +Previously, Saleor's ID was used to create entry in Contentful. +However, this implementation was failing when special characters in ID occurred (like "=="). + +Now, entry in Contentful is created using it's own system ID, generated by Contentful itself. + +App will first query entries via variant ID field, so it still works without a breaking change. + +It's recommended to set "unique" parameter on variant ID field \ No newline at end of file diff --git a/apps/cms-v2/src/modules/providers/contentful/contentful-bulk-sync-processor.ts b/apps/cms-v2/src/modules/providers/contentful/contentful-bulk-sync-processor.ts index 4b3915ff0..bb9b74768 100644 --- a/apps/cms-v2/src/modules/providers/contentful/contentful-bulk-sync-processor.ts +++ b/apps/cms-v2/src/modules/providers/contentful/contentful-bulk-sync-processor.ts @@ -12,7 +12,7 @@ export class ContentfulBulkSyncProcessor implements BulkSyncProcessor { async uploadProducts( products: BulkImportProductFragment[], - hooks: BulkSyncProcessorHooks + hooks: BulkSyncProcessorHooks, ): Promise { const contentful = new ContentfulClient({ accessToken: this.config.authToken, @@ -41,7 +41,9 @@ export class ContentfulBulkSyncProcessor implements BulkSyncProcessor { }, }) .then((r) => { - if (r?.metadata) { + const resp = Array.isArray(r) ? r[0] : r; + + if (resp?.metadata) { if (hooks.onUploadSuccess) { hooks.onUploadSuccess({ variantId: variant.id }); } diff --git a/apps/cms-v2/src/modules/providers/contentful/contentful-client.test.ts b/apps/cms-v2/src/modules/providers/contentful/contentful-client.test.ts index 34db20750..97cae2ed8 100644 --- a/apps/cms-v2/src/modules/providers/contentful/contentful-client.test.ts +++ b/apps/cms-v2/src/modules/providers/contentful/contentful-client.test.ts @@ -50,7 +50,8 @@ const mockGetEnvironment = vi.fn(); const mockGetEnvironments = vi.fn(); const mockGetEnvEntry = vi.fn(); const mockGetContentTypes = vi.fn(); -const mockCreateEntryWithId = vi.fn(); +const mockCreateEntry = vi.fn(); +const mockGetEntries = vi.fn(); const mockContentfulSdk: ContentfulApiClientChunk = { getSpace: mockGetSpace.mockReturnValue( @@ -60,10 +61,13 @@ const mockContentfulSdk: ContentfulApiClientChunk = { items: [{}], }), getEntry: mockGetEnvEntry.mockReturnValue({}), - createEntryWithId: mockCreateEntryWithId.mockReturnValue({}), + getEntries: mockGetEntries.mockReturnValue({ + items: [], + }), + createEntry: mockCreateEntry.mockReturnValue({}), }), getEnvironments: mockGetEnvironments.mockReturnValue({}), - }) + }), ), }; @@ -78,7 +82,7 @@ describe("ContentfulClient", () => { accessToken: "test-token", space: "test-space", }, - () => mockContentfulSdk + () => mockContentfulSdk, ); }); @@ -106,6 +110,7 @@ describe("ContentfulClient", () => { }; mockGetEnvEntry.mockReturnValue(mockEntry); + mockGetEntries.mockReturnValue({ items: [mockEntry] }); const mockConfig = getMockContenfulConfiguration(); const mockMapping = mockConfig.productVariantFieldsMapping; @@ -117,7 +122,14 @@ describe("ContentfulClient", () => { variant: mockVariant, }); - expect(mockGetEnvEntry).toHaveBeenCalledWith(mockVariant.id); + /** + * Query Contentful first, to get the entry. content_type is configured by user. + * Then field path, matching variant id, also provided by user. + */ + expect(mockGetEntries).toHaveBeenCalledWith({ + content_type: mockConfig.contentId, + [`fields.${mockConfig.productVariantFieldsMapping.variantId}`]: mockVariant.id, + }); /** * Fields must reflect mapping config to variant real data @@ -156,6 +168,7 @@ describe("ContentfulClient", () => { }; mockGetEnvEntry.mockReturnValue(mockEntry); + mockGetEntries.mockReturnValue({ items: [mockEntry] }); const mockConfig = getMockContenfulConfiguration(); const mockVariant = getMockWebhookProductVariant(); @@ -165,13 +178,20 @@ describe("ContentfulClient", () => { variant: { id: mockVariant.id }, }); - expect(mockGetEnvEntry).toHaveBeenCalledWith(mockVariant.id); + /** + * Query Contentful first, to get the entry. content_type is configured by user. + * Then field path, matching variant id, also provided by user. + */ + expect(mockGetEntries).toHaveBeenCalledWith({ + content_type: mockConfig.contentId, + [`fields.${mockConfig.productVariantFieldsMapping.variantId}`]: mockVariant.id, + }); expect(mockEntry.delete).toHaveBeenCalled(); }); }); describe("uploadProductVariant", () => { - it("Calls contentful createEntryWithId method with correct mapped fields", async () => { + it("Calls contentful createEntry method with correct mapped fields", async () => { const mockConfig = getMockContenfulConfiguration(); const mockMapping = mockConfig.productVariantFieldsMapping; @@ -182,7 +202,7 @@ describe("ContentfulClient", () => { variant: mockVariant, }); - expect(mockCreateEntryWithId).toHaveBeenCalledWith(mockConfig.contentId, mockVariant.id, { + expect(mockCreateEntry).toHaveBeenCalledWith(mockConfig.contentId, { fields: { [mockMapping.productId]: { "en-US": mockVariant.product.id, @@ -212,6 +232,9 @@ describe("ContentfulClient", () => { const mockConfig = getMockContenfulConfiguration(); const mockMapping = mockConfig.productVariantFieldsMapping; + mockGetEnvEntry.mockReturnValue(undefined); + mockGetEntries.mockReturnValue({ items: [] }); + const mockVariant = getMockWebhookProductVariant(); await contentfulClient.upsertProductVariant({ @@ -220,7 +243,7 @@ describe("ContentfulClient", () => { }); expect(mockGetEnvEntry).not.toHaveBeenCalled(); - expect(mockCreateEntryWithId).toHaveBeenCalledWith(mockConfig.contentId, mockVariant.id, { + expect(mockCreateEntry).toHaveBeenCalledWith(mockConfig.contentId, { fields: { [mockMapping.productId]: { "en-US": mockVariant.product.id, @@ -244,30 +267,26 @@ describe("ContentfulClient", () => { }); }); - it("Calls update method if SDK returned 409 error", async () => { + it("Calls update method if entries exist", async () => { const mockConfig = getMockContenfulConfiguration(); const mockMapping = mockConfig.productVariantFieldsMapping; const mockVariant = getMockWebhookProductVariant(); - mockCreateEntryWithId.mockRejectedValue({ - message: JSON.stringify({ - status: 409, - }), - }); - const mockEntry = { fields: {}, update: vi.fn().mockReturnValue(Promise.resolve({})), }; mockGetEnvEntry.mockReturnValue(mockEntry); + mockGetEntries.mockReturnValue({ items: [mockEntry] }); await contentfulClient.upsertProductVariant({ configuration: mockConfig, variant: mockVariant, }); + // todo fix expect(mockEntry.fields).toEqual({ [mockMapping.productId]: { "en-US": mockVariant.product.id, diff --git a/apps/cms-v2/src/modules/providers/contentful/contentful-client.ts b/apps/cms-v2/src/modules/providers/contentful/contentful-client.ts index 7bead1255..b94b42684 100644 --- a/apps/cms-v2/src/modules/providers/contentful/contentful-client.ts +++ b/apps/cms-v2/src/modules/providers/contentful/contentful-client.ts @@ -1,4 +1,4 @@ -import { createClient, ClientAPI } from "contentful-management"; +import { createClient, ClientAPI, Environment } from "contentful-management"; import { WebhookProductVariantFragment } from "../../../../generated/graphql"; import { ContentfulProviderConfig } from "@/modules/configuration"; import { z } from "zod"; @@ -6,10 +6,6 @@ import { z } from "zod"; import * as Sentry from "@sentry/nextjs"; import { createLogger } from "@saleor/apps-shared"; -const ContentfulErrorMessageSchema = z.object({ - status: z.number(), -}); - type ConstructorOptions = { space: string; accessToken: string; @@ -82,6 +78,23 @@ export class ContentfulClient { }; }; + private getEntriesBySaleorId = + ({ + contentId, + env, + variantIdFieldName, + }: { + env: Environment; + contentId: string; + variantIdFieldName: string; + }) => + (saleorId: string) => { + return env.getEntries({ + content_type: contentId, + [`fields.${variantIdFieldName}`]: saleorId, + }); + }; + async getContentTypes(env: string) { this.logger.trace("Attempting to get content types"); @@ -116,28 +129,48 @@ export class ContentfulClient { const space = await this.client.getSpace(this.space); const env = await space.getEnvironment(configuration.environment); - const entry = await env.getEntry(variant.id); - - entry.fields = this.mapVariantToConfiguredFields( - variant, - configuration.productVariantFieldsMapping, + const contentEntries = await this.getEntriesBySaleorId({ + contentId: configuration.contentId, + env, + variantIdFieldName: configuration.productVariantFieldsMapping.variantId, + })(variant.id); + + return Promise.all( + contentEntries.items.map((item) => { + item.fields = this.mapVariantToConfiguredFields( + variant, + configuration.productVariantFieldsMapping, + ); + + return item.update(); + }), ); - - return entry.update(); } async deleteProductVariant(opts: { configuration: ContentfulProviderConfig.FullShape; variant: Pick; }) { - this.logger.debug("Attempting to delete product variant"); + this.logger.debug({ variantId: opts.variant.id }, "Attempting to delete product variant"); const space = await this.client.getSpace(this.space); const env = await space.getEnvironment(opts.configuration.environment); + const contentEntries = await this.getEntriesBySaleorId({ + contentId: opts.configuration.contentId, + env, + variantIdFieldName: opts.configuration.productVariantFieldsMapping.variantId, + })(opts.variant.id); - const entry = await env.getEntry(opts.variant.id); + this.logger.trace(contentEntries, "Found entries to delete"); - return await entry.delete(); + /** + * In general it should be only one item, but in case of duplication run through everything + */ + return Promise.all( + contentEntries.items.map(async (item) => { + return item.delete(); + }), + ); } async uploadProductVariant({ @@ -156,42 +189,53 @@ export class ContentfulClient { * TODO: add translations * TODO: - should it create published? is draft */ - return env.createEntryWithId(configuration.contentId, variant.id, { + return env.createEntry(configuration.contentId, { fields: this.mapVariantToConfiguredFields(variant, configuration.productVariantFieldsMapping), }); } - async upsertProductVariant(opts: { + async upsertProductVariant({ + configuration, + variant, + }: { configuration: ContentfulProviderConfig.FullShape; variant: WebhookProductVariantFragment; }) { this.logger.debug("Attempting to upsert product variant"); try { - this.logger.trace("Attempting to upload product variant first"); - - return await this.uploadProductVariant(opts); - } catch (e: unknown) { - this.logger.trace("Upload failed"); - - if (typeof e !== "object" || e === null) { - Sentry.captureMessage("Contentful error is not expected shape"); - Sentry.captureException(e); + const space = await this.client.getSpace(this.space); + const env = await space.getEnvironment(configuration.environment); - throw e; - } + const entries = await this.getEntriesBySaleorId({ + contentId: configuration.contentId, + env, + variantIdFieldName: configuration.productVariantFieldsMapping.variantId, + })(variant.id); - const parsedError = ContentfulErrorMessageSchema.parse(JSON.parse((e as Error).message)); + this.logger.trace(entries, "Found entries"); - if (parsedError.status === 409) { - this.logger.trace("Contentful returned 409 status, will try to update instead"); + if (entries.items.length > 0) { + this.logger.debug("Found existing entry, will update"); + Sentry.addBreadcrumb({ + message: "Found entry for variant", + level: "debug", + }); - return this.updateProductVariant(opts); + return this.updateProductVariant({ configuration, variant }); } else { - Sentry.captureMessage("Contentful error failed and is not handled"); - Sentry.captureException(e); - throw e; + this.logger.debug("No existing entry found, will create"); + Sentry.addBreadcrumb({ + message: "Did not found entry for variant", + level: "debug", + }); + + return this.uploadProductVariant({ configuration, variant }); } + } catch (err) { + Sentry.captureException(err); + + throw err; } } } diff --git a/apps/cms-v2/src/modules/providers/contentful/contentful-webhooks-processor.ts b/apps/cms-v2/src/modules/providers/contentful/contentful-webhooks-processor.ts index 3e9b1f1a7..454586c81 100644 --- a/apps/cms-v2/src/modules/providers/contentful/contentful-webhooks-processor.ts +++ b/apps/cms-v2/src/modules/providers/contentful/contentful-webhooks-processor.ts @@ -13,7 +13,7 @@ export type ContentfulClientStrip = Pick< >; export type ContentfulClientFactory = ( - config: ContentfulProviderConfig.FullShape + config: ContentfulProviderConfig.FullShape, ) => ContentfulClientStrip; export class ContentfulWebhooksProcessor implements ProductWebhooksProcessor { @@ -26,7 +26,7 @@ export class ContentfulWebhooksProcessor implements ProductWebhooksProcessor { new ContentfulClient({ accessToken: providerConfig.authToken, space: providerConfig.spaceId, - }) + }), ) { this.client = clientFactory(providerConfig); } @@ -56,6 +56,11 @@ export class ContentfulWebhooksProcessor implements ProductWebhooksProcessor { }); } + /** + * TODO Must check channels, otherwise variants that are not available, will be sent to CMS anyway. + * Probably happens in every provider type. + * Context of process must include channel-config mapping. + */ async onProductUpdated(product: WebhookProductFragment): Promise { this.logger.trace("onProductUpdated called"); @@ -73,7 +78,7 @@ export class ContentfulWebhooksProcessor implements ProductWebhooksProcessor { }, }, }); - }) + }), ); } } diff --git a/apps/cms-v2/src/modules/providers/contentful/contentful.tsx b/apps/cms-v2/src/modules/providers/contentful/contentful.tsx index a30a3f582..e0ade24a1 100644 --- a/apps/cms-v2/src/modules/providers/contentful/contentful.tsx +++ b/apps/cms-v2/src/modules/providers/contentful/contentful.tsx @@ -1,10 +1,18 @@ -import { Text } from "@saleor/macaw-ui/next"; +import { Box, Text } from "@saleor/macaw-ui/next"; import logo from "./contentful-logo.svg"; import { CMSProviderMeta } from "../cms-provider-meta"; export const Contentful = { - formSideInfo: App will save each variant with the same ID as variant ID., + formSideInfo: ( + + + App will use Saleor Product Variant as a unique identifier. It will be saved as one of the + fields. Please ensure you map Variant ID to field that is UNIQUE in Contentful. + + Otherwise, products may be duplicated + + ), type: "contentful" as const, logoUrl: logo.src as string, displayName: "Contentful", diff --git a/apps/cms-v2/src/modules/webhooks-operations/webhooks-processors-delegator.ts b/apps/cms-v2/src/modules/webhooks-operations/webhooks-processors-delegator.ts index 8e8e80804..3d3ee5f59 100644 --- a/apps/cms-v2/src/modules/webhooks-operations/webhooks-processors-delegator.ts +++ b/apps/cms-v2/src/modules/webhooks-operations/webhooks-processors-delegator.ts @@ -15,7 +15,7 @@ export class WebhooksProcessorsDelegator { private opts: { context: WebhookContext; injectProcessorFactory?: ProcessorFactory; - } + }, ) { if (opts.injectProcessorFactory) { this.processorFactory = opts.injectProcessorFactory; @@ -42,6 +42,11 @@ export class WebhooksProcessorsDelegator { }); } + /** + * Will work only if variant deleted. Variant will not be deleted if product is deleted. + * Must subscribe on new event, PRODUCT_DELETED + * https://github.com/saleor/saleor/issues/14579 + */ async delegateVariantCreatedOperations(productVariant: WebhookProductVariantFragment) { this.logger.trace("delegateVariantCreatedOperations called"); @@ -55,19 +60,19 @@ export class WebhooksProcessorsDelegator { } const connectionsToInclude = connections.filter((conn) => - relatedVariantChannels.includes(conn.channelSlug) + relatedVariantChannels.includes(conn.channelSlug), ); this.logger.trace( { connections: connectionsToInclude.length }, - "Resolved a number of connections to include" + "Resolved a number of connections to include", ); const processors = this.mapConnectionsToProcessors(connectionsToInclude); this.logger.trace( { processors: processors.length }, - "Resolved a number of processor to delegate to" + "Resolved a number of processor to delegate to", ); return Promise.all( @@ -75,7 +80,7 @@ export class WebhooksProcessorsDelegator { this.logger.trace("Calling processor.onProductVariantCreated"); return processor.onProductVariantCreated(productVariant); - }) + }), ); } @@ -90,12 +95,12 @@ export class WebhooksProcessorsDelegator { } const connectionsToInclude = connections.filter((conn) => - relatedVariantChannels.includes(conn.channelSlug) + relatedVariantChannels.includes(conn.channelSlug), ); this.logger.trace( { connections: connectionsToInclude.length }, - "Resolved a number of connections to include" + "Resolved a number of connections to include", ); const processors = this.mapConnectionsToProcessors(connectionsToInclude); @@ -103,7 +108,7 @@ export class WebhooksProcessorsDelegator { return Promise.all( processors.map((processor) => { return processor.onProductVariantUpdated(productVariant); - }) + }), ); } @@ -114,7 +119,7 @@ export class WebhooksProcessorsDelegator { this.logger.trace( { connections: connections.length }, - "Resolved a number of connections to include" + "Resolved a number of connections to include", ); const processors = this.mapConnectionsToProcessors(connections); @@ -122,7 +127,7 @@ export class WebhooksProcessorsDelegator { return Promise.all( processors.map((processor) => { return processor.onProductVariantDeleted(productVariant); - }) + }), ); } @@ -133,7 +138,7 @@ export class WebhooksProcessorsDelegator { this.logger.trace( { connections: connections.length }, - "Resolved a number of connections to include" + "Resolved a number of connections to include", ); const processors = this.mapConnectionsToProcessors(connections); @@ -141,7 +146,7 @@ export class WebhooksProcessorsDelegator { return Promise.all( processors.map((processor) => { return processor.onProductUpdated(product); - }) + }), ); } }