Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI-8056 - Introduce getMigrateFilters function #60

Merged
merged 40 commits into from
Dec 19, 2022

Conversation

eliasinho
Copy link
Contributor

Description

Introduce a getMigrateFilters function, supposed to be generic, that:

  • returns a migration function that takes a callback argument. This callback function handles the actual migration of the filters, the logic in it depending on which migration is done
  • mutates contentServer and counters.
  • handles error cases by mutating errorReport

In bin.ts, a curried migrateFilters function will be created thanks to getMigrateFilters and passed to migrate_fromVersion_to_toVersion. The global variables meant to be mutated are also defined at the level of bin.ts and are passed to migrate_fromVersion_to_toVersion.

In migrate_fromVersion_to_toVersion, the filters migration is handled in the callback passed to migrateFilters, passed in the arguments. migrate_fromVersion_to_toVersion returns a void.

eliasinho and others added 30 commits November 18, 2022 17:26
Co-authored-by: Antoine Tissier <ati@activeviam.com>
Co-authored-by: Antoine Tissier <ati@activeviam.com>
@eliasinho eliasinho self-assigned this Dec 14, 2022
Base automatically changed from UI-7945/refactor-error-reports to main December 16, 2022 16:14
Copy link
Collaborator

@antoinetissier antoinetissier left a comment

Choose a reason for hiding this comment

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

More arguments need to be passed to the callback (and therefore to the getMigrateXyz function in the first place): dataModels for all content types, and keysOfWidgetPluginsToRemove for widgets.
Let's add dataModels to getMigrateFilters already.

@@ -56,6 +54,7 @@ export const getMigrateDashboards =
// The dashboard could not be migrated at all.
counters.dashboards.failed++;

const filesAncestry = _getFilesAncestry(dashboardsStructure);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change ? It's more efficient to do the work only once outside the loop, and I don't see the potential mutations being a problem, since we're only mutating the content and the structure (and so not the files ancestry).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good point, it's more efficient outside the loop, my bad!

@eliasinho
Copy link
Contributor Author

More arguments need to be passed to the callback (and therefore to the getMigrateXyz function in the first place): dataModels for all content types, and keysOfWidgetPluginsToRemove for widgets.

Okay, I'll add them. Can you explain why we need them?

@antoinetissier
Copy link
Collaborator

More arguments need to be passed to the callback (and therefore to the getMigrateXyz function in the first place): dataModels for all content types, and keysOfWidgetPluginsToRemove for widgets.

Okay, I'll add them. Can you explain why we need them?

We need the dataModels for any operation including a cube lookup, which in particular includes almost any MDX manipulation.
We need the keysOfWidgetPluginsToRemove to allow callers to ignore some widgets such as custom ones.

You can see example in the migration from 4.3 to 5.0.

Comment on lines 28 to 29
// `dataModels` is not used yet, but needs to be in the function's signature.
// eslint-disable-next-line
Copy link
Collaborator

Choose a reason for hiding this comment

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

The point is to pass it to the callback: it wouldn't be useful if we just put it in the signature.

But if you want let's just merge without the dataModels for now.
I have it ready on a subsequent branch, so we can add it from there.

@antoinetissier
Copy link
Collaborator

I pushed a commit to leave adding the dataModels out of the PR for now.
Looks good to me like this !

@eliasinho eliasinho merged commit f70bf83 into main Dec 19, 2022
@eliasinho eliasinho deleted the UI-8056/create-getMigrateFilters branch December 19, 2022 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants