Skip to content

Commit

Permalink
Update contentful client to use system id (#1088)
Browse files Browse the repository at this point in the history
  • Loading branch information
@lkostrowski
lkostrowski authored Nov 3, 2023
1 parent 458292a commit 958dbcd
Show file tree
Hide file tree
Showing 7 changed files with 167 additions and 70 deletions.
14 changes: 14 additions & 0 deletions .changeset/twenty-garlics-count.md
View file
Original file line number Diff line number Diff line change
@@ -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
6 changes: 4 additions & 2 deletions apps/cms-v2/src/modules/providers/contentful/contentful-bulk-sync-processor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export class ContentfulBulkSyncProcessor implements BulkSyncProcessor {

async uploadProducts(
products: BulkImportProductFragment[],
hooks: BulkSyncProcessorHooks
hooks: BulkSyncProcessorHooks,
): Promise<void> {
const contentful = new ContentfulClient({
accessToken: this.config.authToken,
Expand Down Expand Up @@ -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 });
}
Expand Down
51 changes: 35 additions & 16 deletions apps/cms-v2/src/modules/providers/contentful/contentful-client.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -60,10 +61,13 @@ const mockContentfulSdk: ContentfulApiClientChunk = {
items: [{}],
}),
getEntry: mockGetEnvEntry.mockReturnValue({}),
createEntryWithId: mockCreateEntryWithId.mockReturnValue({}),
getEntries: mockGetEntries.mockReturnValue({
items: [],
}),
createEntry: mockCreateEntry.mockReturnValue({}),
}),
getEnvironments: mockGetEnvironments.mockReturnValue({}),
})
}),
),
};

Expand All @@ -78,7 +82,7 @@ describe("ContentfulClient", () => {
accessToken: "test-token",
space: "test-space",
},
() => mockContentfulSdk
() => mockContentfulSdk,
);
});

Expand Down Expand Up @@ -106,6 +110,7 @@ describe("ContentfulClient", () => {
};

mockGetEnvEntry.mockReturnValue(mockEntry);
mockGetEntries.mockReturnValue({ items: [mockEntry] });

const mockConfig = getMockContenfulConfiguration();
const mockMapping = mockConfig.productVariantFieldsMapping;
Expand All @@ -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
Expand Down Expand Up @@ -156,6 +168,7 @@ describe("ContentfulClient", () => {
};

mockGetEnvEntry.mockReturnValue(mockEntry);
mockGetEntries.mockReturnValue({ items: [mockEntry] });

const mockConfig = getMockContenfulConfiguration();
const mockVariant = getMockWebhookProductVariant();
Expand All @@ -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;

Expand All @@ -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,
Expand Down Expand Up @@ -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({
Expand All @@ -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,
Expand All @@ -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,
Expand Down
114 changes: 79 additions & 35 deletions apps/cms-v2/src/modules/providers/contentful/contentful-client.ts
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
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";

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;
Expand Down Expand Up @@ -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");

Expand Down Expand Up @@ -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<WebhookProductVariantFragment, "id">;
}) {
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({
Expand All @@ -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;
}
}
}
Loading

0 comments on commit 958dbcd

Please sign in to comment.