Remove Getting Started Guide related code from frontend #15209
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paulrosenzweig
approved these changes
Mar 22, 2021
kulyk
added a commit
to kulyk/metabase
that referenced
this pull request
Mar 23, 2021
For some reason, when running Cypress tests, the page appears in `cannot read property fields of undefined` However it works correctly when running Metabase locally Checking for undefined `fields` explicitly fixes the problem Anyway, most likely this component is going to be removed in metabase#15209
Co-authored-by: Paul Rosenzweig <paulrosenzweig@users.noreply.github.com>
Co-authored-by: Paul Rosenzweig <paulrosenzweig@users.noreply.github.com>
|
The failing test is failing correctly (that UI shouldn't appear on EE). The issue is that it's running at all. I think that's another quirk with our CI setup with external contributors. This should be good to merge. I'll ask a Github admin to merge it with the failing tests. Nice work! |
paulrosenzweig
added a commit
that referenced
this pull request
Mar 23, 2021
* Add cypress tests for reference/metrics/:id page * Add metrics/:metricId/edit route * Use router location for metric's isEditing state Data Reference keeps `isEditing` state flag in Redux store (it's shared across metrics, segments and DB references) Metrics page now uses router's path name to distinguish reading and editing states * Use non-Redux callbacks at MetricDetailContainer * Add missing prop type definition * Fix editing callbacks not passed to MetricDetail * Fix signInAsAdmin test utility usage * Reorder imports * Use cy.findByText instead of cy.contains * Fix Metrics data reference test For some reason, when running Cypress tests, the page appears in `cannot read property fields of undefined` However it works correctly when running Metabase locally Checking for undefined `fields` explicitly fixes the problem Anyway, most likely this component is going to be removed in #15209 * Fix metrics test Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com>
paulrosenzweig
added a commit
that referenced
this pull request
Mar 23, 2021
This was referenced Mar 23, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #15127
Fixes #15130
Problem
Looks like the frontend tries to interact with a removed feature on the Data Reference page (
/reference)Both issues this PR addresses are related to Metric's
important_fieldsproperty. As I see from the backend's source code, it was used for the Getting Started Guide feature. And it looks like it has been removed in Metabase 0.30.Also, at #15127 the issue happens because the frontend tries to access a property of the undefined
guideobject. It has to be retrieved from the backend viaGET /api/getting_startedrequest, but the backend route has been removed a few years ago.Solution
If my understanding of the problem is correct, we can just remove the frontend code related to Getting Started Guide and
important_fields. This PR does exactly that. Also, it removes a few fields from the Metric's reference form that were related toimportant_fields(like at #15130). The screenshots below demonstrate an updated page lookBy the way, I'm not sure Fields you can group this metric by section is relevant now. Please help me find out