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

Fix - diagnose-expression throws when Offset is nested #42497

Merged
merged 7 commits into from
May 10, 2024

Conversation

kamilmielnik
Copy link
Contributor

@kamilmielnik kamilmielnik commented May 10, 2024

Fixes #42377

Description

Switches the order of appliance of adjustOffset and adjustCase.

How to verify

New e2e test passes.

@kamilmielnik kamilmielnik added the no-backport Do not backport this PR to any branch label May 10, 2024
@kamilmielnik kamilmielnik marked this pull request as ready for review May 10, 2024 09:11
@kamilmielnik kamilmielnik requested a review from a team May 10, 2024 09:15
Copy link

replay-io bot commented May 10, 2024

Status In Progress ↗︎ 57 / 58
Commit a921213
Results
⚠️ 7 Flaky
2477 Passed

enterCustomColumnDetails({ formula, name });

cy.on("uncaught:exception", error => {
expect(error.message.includes("Error normalizing")).not.to.be.true;
Copy link
Contributor

Choose a reason for hiding this comment

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

what kind of error do we expect? is there any other way to verify we don't have normalization error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what kind of error do we expect?

We expect no error.

cy.on will however catch all the other errors from console, e.g. warnings about missing props coming from React, etc.

is there any other way to verify we don't have normalization error?

We could wrap Lib.diagnoseExpression call in diagnostics.ts in a try/catch block and then the error would be shown to the user. I'm not sure why we haven't done that originally.

@uladzimirdev
Copy link
Contributor

stress https://github.com/metabase/metabase/actions/runs/9031045381/job/24816487130

@kamilmielnik kamilmielnik requested review from uladzimirdev and a team May 10, 2024 10:58
@kamilmielnik kamilmielnik merged commit 3e9f4ac into master May 10, 2024
188 of 199 checks passed
@kamilmielnik kamilmielnik deleted the 42377-nested-offset branch May 10, 2024 13:39
Copy link

@kamilmielnik Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

uladzimirdev pushed a commit that referenced this pull request May 11, 2024
* Fix offset not working in case

* Make offset function return any

* Add a repro for #42377

* Fix order of adjustments

* Revert unrelated changes

* Remove commented code

* Revert unrelated changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/QueryingComponents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

diagnose-expression throws when Offset is nested
3 participants