Devrel 1068 import item and variants #15

JiriLojda · 2024-01-04T11:25:52Z

Motivation

Which issue does this fix? Fixes #issue number

If no issue exists, what is the fix or new feature? Were there any reasons to fix/implement things that are not obvious?

Checklist

Code follows coding conventions held in this repo
Automated tests have been added
Tests are passing
Docs have been updated (if applicable)
Temporary settings (e.g. variables used during development and testing) have been reverted to defaults

How to test

If manual testing is required, what are the steps?

IvanKiral · 2024-01-04T13:14:35Z

let's test the situation when having option of multiple choice element with name same as some other element in the same content_type.. external_ids might collide

IvanKiral

In general, there might be problem with external ids. Otherwise the logic looks good, however, I would do some refactoring, code is hard to follow, I would extract some variables of functions

IvanKiral · 2024-01-04T11:32:28Z

src/commands/importExportEntities/entities/contentTypes.ts

+                  if (el.type === "snippet") {
+                    const typedEl = el as unknown as ContentTypeElements.ISnippetElement;
+                    return [
+                      ...context.contentTypeSnippetContextWithElementsByOldIds.get(typedEl.snippet.id ?? "")


nitpick: contentTypeSnippetContextByOldIds

IvanKiral · 2024-01-04T11:37:29Z

src/commands/import.ts

-  contentTypeIdsWithElementsByOldIds: new Map(),
+  contentTypeSnippetContextWithElementsByOldIds: new Map(),
+  contentItemContextByOldIds: new Map(),
+  contentTypeContextWithElementsByOldIds: new Map(),


ContentTypeContextByOldIds

IvanKiral · 2024-01-04T11:42:36Z

src/commands/importExportEntities/entities/contentTypes.ts

@@ -18,13 +18,58 @@ export const contentTypesEntity: EntityDefinition<ReadonlyArray<ContentTypeContr

    return {
      ...context,
-      contentTypeIdsWithElementsByOldIds: new Map(
+      contentTypeContextWithElementsByOldIds: new Map(


this is getting kind of hard to read.. let's extract those maps into variables
The other option is to create functions for map lambdas to kind of getting easier for eyes

IvanKiral · 2024-01-04T11:49:32Z

src/commands/importExportEntities/entities/contentTypes.ts

@@ -92,7 +137,7 @@ const createUpdateTypeItemReferencesFetcher =

    return params.client
      .modifyContentType()
-      .byTypeId(params.context.contentTypeIdsWithElementsByOldIds.get(type.id)?.selfId ?? "")
+      .byTypeId(params.context.contentTypeContextWithElementsByOldIds.get(type.id)?.selfId ?? "")


you can use codename if i understand this correctly :)

IvanKiral · 2024-01-04T12:05:04Z

src/commands/importExportEntities/entities/contentTypesSnippets.ts

@@ -21,15 +21,42 @@ export const contentTypesSnippetsEntity: EntityDefinition<

    return {
      ...context,
-      contentTypeSnippetIdsWithElementsByOldIds: new Map(
+      contentTypeSnippetContextWithElementsByOldIds: new Map(


let's refactor this a bit... mayb extracting maps into variables?

IvanKiral · 2024-01-04T12:11:00Z

src/commands/importExportEntities/entities/languageVariants.ts

+      .withData(builder => ({
+        workflow: {
+          workflow_identifier: {
+            id: newWfContext.wfId,


nitpick: i would use workflowId

Yeah, but isn't wf an obvious abbreviation in this context?

IvanKiral · 2024-01-04T12:14:00Z

src/commands/importExportEntities/entities/languageVariants.ts

+    .byLanguageId(variant.language.id ?? "");
+
+  return scheduleTo
+    ? await sharedRequest


do you need await here, when you are awaiting when using the publishVariant function?

You are right, I do not. 👍

IvanKiral · 2024-01-04T12:15:45Z

src/commands/importExportEntities/entities/languageVariants.ts

+(
+  fileElement: ElementContracts.IContentItemElementContract,
+): LanguageVariantElements.ILanguageVariantElementBase | null => {
+  const elementType = params.elementTypeByOldId.get(fileElement.element.id ?? "");


I would use as string to indicate that fileElement.element must have an id

See the other comment I made.

IvanKiral · 2024-01-04T12:26:27Z

src/commands/importExportEntities/entities/languageVariants.ts

+  context.worfklowStepsIdsWithTransitionsByOldIds.get(stepId)?.selfId ?? "";
+
+const getProjectElementId = (params: TransformElementParams, fileId: string): string | null =>
+  Array.from(params.context.contentTypeSnippetContextWithElementsByOldIds.values())


why this use only contentTypeSnippets and no also contentTypes?

nwm I see it now, but it's kind of hard to follow

I managed to inline the snippets here as well. The snippet elements don't need an update later on so I don't need their ids anyway.

IvanKiral · 2024-01-04T12:43:29Z

src/commands/importExportEntities/entities/utils/typeElements.ts

@@ -272,7 +273,7 @@ export const createPatchItemAndTypeReferencesInTypeElement =
            op: "replace" as const,
            path: `/elements/id:${newElementId}/allowed_content_types`,
            value: typedElement.allowed_content_types.map(ref => ({
-              id: context.contentTypeIdsWithElementsByOldIds.get(ref.id ?? "")?.selfId,
+              id: context.contentTypeContextWithElementsByOldIds.get(ref.id ?? "")?.selfId,


why not use codenames?

The codenames are not in scope. I would have to add them into the context, but the ids are already there so I can as well use them.

JiriLojda · 2024-01-08T10:40:11Z

let's test the situation when having option of multiple choice element with name same as some other element in the same content_type.. external_ids might collide

It seems that the external id of a multi-choice element's option must be unique even across multiple types. I changed it to use external ids like so: {type/snippet codename}_{element codename}_{option codename}

JiriLojda · 2024-01-08T10:42:40Z

Since Ivan is on a long vacation, I will merge this for now. We can talk about the comments later.

Change fallback multi-choice option externalId so it is unique across multiple types

Add support for importing items

ba50e12

JiriLojda requested a review from a team as a code owner January 4, 2024 11:25

IvanKiral requested changes Jan 5, 2024

View reviewed changes

Add support for importing language variants

484fcb0

Change fallback multi-choice option externalId so it is unique across multiple types

JiriLojda force-pushed the DEVREL-1068_import_item_and_variants branch from de2a6f0 to 484fcb0 Compare January 8, 2024 10:44

JiriLojda merged commit a557c25 into main Jan 8, 2024
1 check passed

JiriLojda deleted the DEVREL-1068_import_item_and_variants branch January 8, 2024 10:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Devrel 1068 import item and variants #15

Devrel 1068 import item and variants #15

JiriLojda commented Jan 4, 2024

IvanKiral commented Jan 4, 2024

IvanKiral left a comment

IvanKiral Jan 4, 2024

IvanKiral Jan 4, 2024

IvanKiral Jan 4, 2024

IvanKiral Jan 4, 2024

IvanKiral Jan 4, 2024

IvanKiral Jan 4, 2024

JiriLojda Jan 5, 2024

IvanKiral Jan 4, 2024

JiriLojda Jan 5, 2024 •

edited

Loading

IvanKiral Jan 4, 2024

JiriLojda Jan 5, 2024

IvanKiral Jan 4, 2024

IvanKiral Jan 4, 2024

JiriLojda Jan 8, 2024

IvanKiral Jan 4, 2024

JiriLojda Jan 5, 2024

JiriLojda commented Jan 8, 2024

JiriLojda commented Jan 8, 2024

Devrel 1068 import item and variants #15

Devrel 1068 import item and variants #15

Conversation

JiriLojda commented Jan 4, 2024

Motivation

Checklist

How to test

IvanKiral commented Jan 4, 2024

IvanKiral left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiriLojda Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiriLojda commented Jan 8, 2024

JiriLojda commented Jan 8, 2024

JiriLojda Jan 5, 2024 •

edited

Loading