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

[Metrics v2] Add new /metric/:id and metric/:id/notebook pages #38554

Merged
merged 12 commits into from
Feb 14, 2024

Conversation

nemanjaglumac
Copy link
Member

@nemanjaglumac nemanjaglumac commented Feb 8, 2024

This PR introduces two new routes.

  1. /metric/:id that displays previously saved metric (which should open in the "chill mode")
  2. /metric/:id/notebook that provides the option to switch to a notebook mode

Things to look out for

  • UI should not offer to turn a metric into a model
  • UI should not offer to preview SQL or convert a metric into a SQL question
  • When one applies any change to the metric, its query should become "dirty", and the url should change to /question/query... indicating that we started a new ad-hoc question.

Notebook mode should not show metric's underlying query, but rather a new ad-hoc query started from the metric as a source:
image

Demo

https://www.loom.com/share/cbfe1f8fc3b045798cabf46ac0d812e4?sid=486b17fb-525d-4d37-b584-c8904490e980

Resolves #37368

@nemanjaglumac nemanjaglumac changed the base branch from metrics-v2 to 37353-new-metric-page February 12, 2024 13:07
Base automatically changed from 37353-new-metric-page to metrics-v2 February 12, 2024 13:51
Copy link

replay-io bot commented Feb 12, 2024

Status In Progress ↗︎ 51 / 52
Commit 289319e
Results
⚠️ 3 Flaky
2274 Passed

@nemanjaglumac nemanjaglumac linked an issue Feb 12, 2024 that may be closed by this pull request
2 tasks
@nemanjaglumac nemanjaglumac requested a review from a team February 12, 2024 14:19
@nemanjaglumac nemanjaglumac changed the title [Metrics v2] Add new metric page /metric/:id [WIP] [Metrics v2] Add new metric /metric/:id and metric/:id/notebook pages Feb 12, 2024
@nemanjaglumac nemanjaglumac changed the title [Metrics v2] Add new metric /metric/:id and metric/:id/notebook pages [Metrics v2] Add new /metric/:id and metric/:id/notebook pages Feb 12, 2024
@nemanjaglumac nemanjaglumac requested review from a team and kamilmielnik February 12, 2024 20:25
@nemanjaglumac
Copy link
Member Author

nemanjaglumac commented Feb 13, 2024

Self-review

There's one final thing at 5ab967c I can't currently solve.
When switching to a notebook view, the hash stays in the URL. This does not happen for models or questions so it must be a bug.

EDIT:
This should now be fixed by d017326.

Copy link
Contributor

@kamilmielnik kamilmielnik left a comment

Choose a reason for hiding this comment

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

URLs don't always work correctly for actions on /metric/:id page

@@ -497,18 +497,30 @@ class Question {
}
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Open up a metric
  2. Select "Move" from "..." menu
  3. Move it to another collection

❌ URL changes from /metric/:id-slug to /question/:id-slug

Copy link
Member Author

Choose a reason for hiding this comment

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

I spent hours trying to figure this one out.
In the end, I'm not entirely sure if this is a bug in the existing implementation, or does it result from us hacking the question.type() before the real backend implementation.

What I managed to determine is that, upon moving a metric, getQuestion selector initially returns the question.type() "question". This is why the composeDataset doesn't apply.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kamilmielnik I finally realized what's going on. The hack for CRUD endpoints does not work for the PUT request. The payload does not have a type unless you're literally converting the type itself.

For example, this is the payload when moving a metric from one collection to another:

{
  "id": 82,
  "collection_id": null
}

Unless you have a different idea, we might have to ignore this and call it a limitation until backend implements the card.type for real.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you have a different idea, we might have to ignore this and call it a limitation until backend implements the card.type for real.

All good, thanks for checking this ❤️

frontend/src/metabase-lib/Question.ts Show resolved Hide resolved
frontend/src/metabase-lib/Question.ts Show resolved Hide resolved
@kamilmielnik kamilmielnik requested a review from a team February 13, 2024 14:04
Copy link
Contributor

@kamilmielnik kamilmielnik left a comment

Choose a reason for hiding this comment

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

Works well 👍 🚢

@kamilmielnik kamilmielnik requested a review from a team February 14, 2024 05:41
@nemanjaglumac nemanjaglumac merged commit e4f1a32 into metrics-v2 Feb 14, 2024
106 checks passed
@nemanjaglumac nemanjaglumac deleted the metrics-v2-add-metric-id branch February 14, 2024 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FE - Metric page (/metric/:id + /metric/:id/notebook)
2 participants