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-8857 - Fix unexpected filter MDX update during 4.3 to 5.0 migration #118

Merged
merged 6 commits into from
Oct 10, 2023

Conversation

Nouzbe
Copy link
Collaborator

@Nouzbe Nouzbe commented Oct 5, 2023

This is the same thing that I did for Rokos in https://github.com/activeviam/atoti-ui/pull/3944.

This change is bold: it means that for all data visualization widgets migrated from 4.3 to 5:

  • we let the filters in the MDX query, under whatever form they are written there
  • we also derive them and store them as we would under state.filters, so that the user sees them in the tools
  • when we run queries, we disregard the state.filters and just run the MDX query as it is
  • the next time the user uses a tool to edit filters, we ditch the filters from the MDX query and just store them under state.filters normally (that can cause their syntax to be modified)

We could consider introducing a new CLI argument in order to make this an opt-in instead of a default behavior.
Personally, I think it's good and should be the default.

@Nouzbe Nouzbe self-assigned this Oct 5, 2023
@Nouzbe Nouzbe marked this pull request as ready for review October 5, 2023 15:06
@mtadams007
Copy link
Contributor

This makes sense to me. On one hand, it's kicking the can down the road in case there are any issues, but on the other hand it solves the issues of when they want to look at dashboards without necessarily making too many changes. If there's an issue, one can fix it on a per dashboard basis (especially as it seems like this problem comes with custom mdx filters, so I'd assume the users are already used to custom MDX)

@Nouzbe Nouzbe assigned tibdex and unassigned Nouzbe Oct 5, 2023
Copy link
Member

@tibdex tibdex left a comment

Choose a reason for hiding this comment

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

I like the idea 👍

Should we mention somewhere that the next release of this tool will only work with Atoti UI >= 5.0.29 and >= 5.1.9 since areFiltersDrivenByMdx did not exist before that?

Copy link
Contributor

@mtadams007 mtadams007 left a comment

Choose a reason for hiding this comment

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

🚀

@antoinetissier
Copy link
Collaborator

antoinetissier commented Oct 5, 2023

This works well, but at least in the specific case of the ticket we could have fixed it on the side of the monorepo, which would be better to avoid having to rely on the areFiltersDrivenFromMdx when pasting in the query editor.
The problem is that our code wrongly considers that the hierarchy is already expressed on an axis, even though it's not: it's just used for ordering.

So we should be more precise here, and rather than just looking at the hierarchy compound identifier, see if it actually contributes to the axis dimensionality.

In general, many problems boil down to this: "does it contribute to the node dimensionality issue".
I have some generic ideas for that which are totally out of scope, but it would be nice to talk about it at some point.

@Nouzbe
Copy link
Collaborator Author

Nouzbe commented Oct 9, 2023

Should we mention somewhere that the next release of this tool will only work with Atoti UI >= 5.0.29 and >= 5.1.9 since areFiltersDrivenByMdx did not exist before that?

That's a good point. We could introduce an argument to decide whether this should be done or not, so that migrations to earlier patch versions on 5.0 and 5.1 still can be done. We should, imo.

@Nouzbe
Copy link
Collaborator Author

Nouzbe commented Oct 9, 2023

So we should be more precise here, and rather than just looking at the hierarchy compound identifier, see if it actually contributes to the axis dimensionality.

Indeed, we can improve setFilters in order to add more filters to the slicer and less to the subselect, which should prevent this specific bug. This is a good thing to do, independently from this PR. Indeed, it will prevent unexpected query updates when the user edits their filters. And I would bet that this is something that users will encounter in the future if we stop here.

I still want to move forward with this, as I believe it covers more cases than just this slicing vs subselect issue, by completely preventing updates to the filters part of MDX queries during the migration script run.

@antoinetissier
Copy link
Collaborator

Created https://activeviam.atlassian.net/browse/UI-8862 to keep track of it

@Nouzbe Nouzbe merged commit 71f31eb into main Oct 10, 2023
1 check passed
@Nouzbe Nouzbe deleted the UI-8857/no-updating-filters branch October 10, 2023 13:30
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.

4 participants