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-9249 - Migrate default selected drillthrough columns #123

Merged
merged 2 commits into from
Mar 15, 2024

Conversation

Nouzbe
Copy link
Collaborator

@Nouzbe Nouzbe commented Mar 14, 2024

The migration script used to disregard default selected drillthrough columns configured in UI 4.3 when it is used to migrate the content to a more recent version.

This PR takes care of it and makes sure that this configuration is not lost.

@Nouzbe Nouzbe added enhancement New feature or request priority labels Mar 14, 2024
@Nouzbe Nouzbe force-pushed the UI-9249/migrate-default-selected-drillthrough-columns branch from c563998 to a9598ac Compare March 14, 2024 17:05
Copy link
Contributor

@messaooudi messaooudi left a comment

Choose a reason for hiding this comment

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

Looks good, I have one question to make sur i understand the remove duplicate part.

Comment on lines +72 to +77
// Remove duplicates.
migratedSettingsMap["drillthrough.defaultSelectedColumns"] = _mapValues(
migratedDrillthroughColumns,
_uniq,
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This implies that two drillthrough columns with the same columnName but different functionName are considered duplicates. Right ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed. In fact there are even two ways to have duplicates:

  • because UI4 was typically storing the following for each column:
{
  functionName: "MemberValue",
  columnName: "myColumn",
}, {
  functionName: "MemberCaption",
  columnName: "myColumn"
}

So we don't want to save each column twice in the new Atoti UI 5 setting.

The other way is more rare. It is because UI4 was saving the columns per server and per cube whereas UI5 only stores them per cube. So you could have a duplicate because a column was save for a cube with the same name under two different servers.

The current implementation in this PR should get rid of any duplicate, whether they come from the first way or the second one I described here.

Comment on lines +38 to +45
{
functionName: "MemberValue",
columnName: "delta",
},
{
functionName: "Caption",
columnName: "delta",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This is considered duplicates, so we are testing that part of the implementation ?

Copy link
Collaborator Author

@Nouzbe Nouzbe Mar 15, 2024

Choose a reason for hiding this comment

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

This is considered duplicates indeed. It's the first way to have duplicate I described above.

I also tested the second way of having duplicates, as I added the column subTradeId in MarketRiskCube under two different servers.

@messaooudi messaooudi assigned Nouzbe and unassigned messaooudi Mar 15, 2024
@Nouzbe Nouzbe assigned messaooudi and unassigned Nouzbe Mar 15, 2024
Copy link
Contributor

@messaooudi messaooudi left a comment

Choose a reason for hiding this comment

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

Thanks for the explanations. LGTM

@messaooudi messaooudi assigned Nouzbe and unassigned messaooudi Mar 15, 2024
@Nouzbe Nouzbe merged commit 2c5a623 into main Mar 15, 2024
2 checks passed
@Nouzbe Nouzbe deleted the UI-9249/migrate-default-selected-drillthrough-columns branch March 15, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants