-
Notifications
You must be signed in to change notification settings - Fork 5k
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 isNavigationAllowed #38703
Fix isNavigationAllowed #38703
Conversation
@@ -208,7 +208,6 @@ export const getRoutes = store => { | |||
title={t`New Model`} | |||
component={NewModelOptions} | |||
/> | |||
<Route path="notebook" component={QueryBuilder} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/model/notebook
route is never used.
However /model/:id/notebook
is.
@@ -243,14 +244,6 @@ describe("isNavigationAllowed", () => { | |||
).toBe(true); | |||
}); | |||
|
|||
it("allows to run a model", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was invalid (copy-paste remnants) - it's for a model, but the describe
is for a question
`/model/${slug}`, | ||
`/model/${slug}/query`, | ||
`/model/${slug}/metadata`, | ||
`/model/${slug}/notebook`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this route
).toBe(true); | ||
}); | ||
|
||
it("allows to run the model and then edit it again", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was invalid (non-existing route).
).toBe(true); | ||
}); | ||
|
||
it("allows to run the model and then edit it again", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was invalid (non-existing route).
hash: `#${serializeCardForUrl(nativeModelCard)}`, | ||
}); | ||
|
||
const runModelEditNotebookLocation = createMockLocation({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-existing route.
const runModelEditNotebookLocation = createMockLocation({ | ||
pathname: "/model/notebook", | ||
const runNewModelLocation = createMockLocation({ | ||
pathname: "/model/query", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated pathname to correct one
|
@kamilmielnik Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
@metabase-bot backport release-x.49.x |
* Removed unused route * Remove invalid test * Remove invalid routes from isNavigationAllowed, update tests * Remove invalid route * Handle /model/:id/notebook route * Migrate Question.prototype.isDataset to Question.prototype.type * Consolidate code * Add missing attribute * Type cards and questions for extra safety * Fix running edited models * Improve diff
* Removed unused route * Remove invalid test * Remove invalid routes from isNavigationAllowed, update tests * Remove invalid route * Handle /model/:id/notebook route * Migrate Question.prototype.isDataset to Question.prototype.type * Consolidate code * Add missing attribute * Type cards and questions for extra safety * Fix running edited models * Improve diff Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
Prerequisite for #37357
Description
I got confused when working on #37357 as some of the existing things didn't make any sense. Blame me.
Luckily, nothing was really broken. More details below and in PR comments.
/model/notebook
routeQuestion.prototype.type
insteadQuestion.prototype.isDataset
/model/:id/notebook
is not preventedgetShouldShowUnsavedChangesWarning