Skip to content
This repository has been archived by the owner on Apr 12, 2022. It is now read-only.

Add append, create and rename enrich, additive to upsert, update destination sync options #2998

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

edmundito
Copy link
Contributor

@edmundito edmundito commented Feb 23, 2022

Change description

This PR adds the ability to support append orcreate (create: true, update: false, delete: false) in destination apps. It also migrates the enrich and additive syncMode names to the more commonly known terms update and upsert.

The db will migrate the syncModes to the new values. In config files, enrich and additive are still supported but a warning is shown, and will be updated to the new value if the user updates the config via the UI.

All plugins have been updated to use the new terms.

Checklists

Development

  • Application changes have been tested appropriately

Impact

  • Code follows company security practices and guidelines
  • Security impact of change has been considered
  • Performance impact of change has been considered
  • Possible migration needs considered (model migrations, config migrations, etc.)

Please explain any security, performance, migration, or other impacts if relevant:

Migrations considered:

  1. Enrich and Additive are still supported on config files. When the file is loaded, a warning is shown to the user (also visible in grouparoo validate) and will continue to support the legacy values. However, if the user saves the config via the UI, the values are replaced with the new values.
  2. Added db migration to update the syncMode

Code review

  • Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached where applicable.
  • Relevant tags have been added to the PR (bug, enhancement, internal, etc.)

@edmundito edmundito added the enhancement New feature or request label Feb 23, 2022
@edmundito edmundito marked this pull request as ready for review February 23, 2022 21:57
displayName: "Additive",
upsert: {
key: "upsert",
displayName: "Upsert",
description: "Sync all records, but do not delete (create, update)",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking we should also update these descriptions away from the word "sync". Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Export perhaps?

@@ -860,6 +868,28 @@ describe("models/destination - with custom exportRecord plugin", () => {
await record.destroy();
});

test("Append syncMode only allows creating records", async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

await record.destroy();
});

test("Create syncMode only allows creating records", async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

append: {
key: "append",
displayName: "Append",
description: "Create new records (create)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description: "Create new records (create)",
description: "Always create new records (create)",

Comment on lines 24 to 27
log(
`[ config ] Found syncMode "${syncMode}" in Destination config "${configId}". "${syncMode}" is still supported but value should be updated to new name: "${newSyncMode}".`,
"warning"
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a DeprecationHelper to use for this kind of thing!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder about the Deprecation utility

create: true,
update: false,
delete: false,
description: "Create new records, but only when they don't exist (create)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these descriptions be retrieved from core? The boolean flags seem to be the same as well...

Co-authored-by: Pedro S. Lopez <pedroslopez@me.com>
Copy link
Member

@evantahler evantahler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the Deprecation utility in the config loader, and this is good-to-go!

description: values.description,
});

export const BatchSyncModeData: Record<BatchSyncMode, BatchSyncModeDataValues> =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much nicer!

Comment on lines 24 to 27
log(
`[ config ] Found syncMode "${syncMode}" in Destination config "${configId}". "${syncMode}" is still supported but value should be updated to new name: "${newSyncMode}".`,
"warning"
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder about the Deprecation utility

@evantahler
Copy link
Member

(re-ran a flaky test)

@pedroslopez pedroslopez removed their request for review March 10, 2022 16:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants