Skip to content
Snippets Groups Projects
Unverified Commit babbffbc authored by Jeff Evans's avatar Jeff Evans Committed by GitHub
Browse files

Improve driver deprecation UX (#17281) 

* Improve driver deprecation UX

Reworking the DriverWarning component to be able to change the engine itself by firing the event (so that the "go to older/newer version" links actually work)

Some styling fixes to match Figma mockup

Remove all grid-styled usage

Remove all styling (except class names) from DriverWarning jsx file, move to the styled-component

Implement db-ids-with-deprecated-drivers API endpoint for fetching DB IDs using deprecated drivers

Add some plugin test init code to properly handle the test legacy/new drivers

Adding test for new API

Adding new entry in `MetabaseApi` frontend component for new API method
parent 60146bf4
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 220 additions and 113 deletions
frontend/src/metabase/admin/databases/containers/DatabaseEditApp.jsx
+ 82
− 53
View file @ babbffbc
......@@ -172,60 +172,89 @@ export default class DatabaseEditApp extends Component {
/>
</div>
)}
<Flex>
<Box width={620}>
<LoadingAndErrorWrapper
loading={!database}
error={initializeError}
<LoadingAndErrorWrapper
loading={!database}
error={initializeError}
>
{() => (
<Databases.Form
database={database}
form={Databases.forms[currentTab]}
formName={DATABASE_FORM_NAME}
onSubmit={
addingNewDatabase && currentTab === "connection"
? this.props.proceedWithDbCreation
: this.props.saveDatabase
}
submitTitle={addingNewDatabase ? t`Save` : t`Save changes`}
renderSubmit={
// override use of ActionButton for the `Next` button, for adding a new database in which
// scheduling is being overridden
addingNewDatabase &&
currentTab === "connection" &&
letUserControlSchedulingForm &&
(({ handleSubmit, canSubmit }) => (
<Button
primary={canSubmit}
disabled={!canSubmit}
onClick={handleSubmit}
>
{t`Next`}
</Button>
))
}
submitButtonComponent={Button}
>
{() => (
<Databases.Form
database={database}
form={Databases.forms[currentTab]}
formName={DATABASE_FORM_NAME}
onSubmit={
addingNewDatabase && currentTab === "connection"
? this.props.proceedWithDbCreation
: this.props.saveDatabase
}
submitTitle={
addingNewDatabase ? t`Save` : t`Save changes`
}
renderSubmit={
// override use of ActionButton for the `Next` button
addingNewDatabase &&
currentTab === "connection" &&
letUserControlSchedulingForm &&
(({ handleSubmit, canSubmit }) => (
<Button
primary={canSubmit}
disabled={!canSubmit}
onClick={handleSubmit}
>
{t`Next`}
</Button>
))
}
submitButtonComponent={Button}
/>
)}
</LoadingAndErrorWrapper>
</Box>
<Box>
<DriverWarning
engine={selectedEngine}
ml={26}
data-testid="database-setup-driver-warning"
/>
{addingNewDatabase && (
<AddDatabaseHelpCard
engine={selectedEngine}
ml={26}
data-testid="database-setup-help-card"
/>
)}
</Box>
</Flex>
{({
Form,
FormField,
FormMessage,
FormSubmit,
formFields,
onChangeField,
submitTitle,
}) => {
return (
<Flex>
<Box width={620}>
<Form>
{formFields.map(formField => (
<FormField
key={formField.name}
name={formField.name}
/>
))}
<FormMessage />
<div className="Form-actions text-centered">
<FormSubmit className="block mb2">
{submitTitle}
</FormSubmit>
</div>
</Form>
</Box>
<Box>
{addingNewDatabase && (
<AddDatabaseHelpCard
engine={selectedEngine}
ml={26}
data-testid="database-setup-help-card"
/>
)}
<DriverWarning
engine={selectedEngine}
ml={26}
onChangeEngine={engine => {
onChangeField("engine", engine);
}}
data-testid="database-setup-driver-warning"
/>
</Box>
</Flex>
);
}}
</Databases.Form>
)}
</LoadingAndErrorWrapper>
</div>
</Box>
......
frontend/src/metabase/components/DriverWarning/DriverWarning.jsx
+ 43
− 35
View file @ babbffbc
......@@ -8,62 +8,76 @@ import {
engineSupersedesMap,
} from "metabase/entities/databases/forms";
import Warnings from "metabase/query_builder/components/Warnings";
import {
CardContent,
DriverWarningContainer,
IconContainer,
Link,
WarningIcon,
WarningParagraph,
} from "./DriverWarning.styled";
import ExternalLink from "metabase/components/ExternalLink";
import Icon from "metabase/components/Icon";
import MetabaseSettings from "metabase/lib/settings";
const propTypes = {
engine: PropTypes.string.isRequired,
hasCircle: PropTypes.bool,
onChangeEngine: PropTypes.func.isRequired,
};
const driverUpgradeHelpLink = MetabaseSettings.docsUrl(
"administration-guide/01-managing-databases",
);
function getSupersedesWarningContent(newDriver, supersedesDriver) {
function getSupersedesWarningContent(
newDriver,
supersedesDriver,
onChangeEngine,
) {
return (
<div>
<p className="text-medium m0">
<WarningParagraph>
{t`This is our new ${
allEngines[newDriver]["driver-name"]
} driver, which is faster and more reliable.`}
</p>
<p>{t`The old driver has been deprecated and will be removed in the next release. If you really
need to use it, you can select ${
allEngines[supersedesDriver]["driver-name"]
} now.`}</p>
</WarningParagraph>
<WarningParagraph hasMargin>
{t`The old driver has been deprecated and will be removed in the next release. If you really
need to use it, you can `}
&nbsp;
<Link
onClick={() => onChangeEngine(supersedesDriver)}
>{t`find it here`}</Link>
.
</WarningParagraph>
</div>
);
}
function getSupersededByWarningContent(engine) {
function getSupersededByWarningContent(engine, onChangeEngine) {
return (
<div>
<p className="text-medium m0">
<WarningParagraph>
{t`This driver has been deprecated and will be removed in the next release.`}
</p>
<p className="text-medium m0">
{t`We recommend that you upgrade to the new ${
</WarningParagraph>
<WarningParagraph hasMargin>
{t`We recommend that you upgrade to the`}
&nbsp;
<Link onClick={() => onChangeEngine(engine)}>{t`new ${
allEngines[engine]["driver-name"]
} driver, which is faster and more reliable.`}
</p>
<ExternalLink
href={driverUpgradeHelpLink}
className="text-brand text-bold"
>
} driver`}</Link>
{t`, which is faster and more reliable.`}
</WarningParagraph>
<Link href={driverUpgradeHelpLink} target={"_blank"}>
{t`How to upgrade a driver`}
</ExternalLink>
</Link>
</div>
);
}
function DriverWarning({ engine, ...props }) {
function DriverWarning({ engine, hasCircle = true, onChangeEngine, ...props }) {
const supersededBy = engineSupersedesMap["superseded_by"][engine];
const supersedes = engineSupersedesMap["supersedes"][engine];
......@@ -71,23 +85,17 @@ function DriverWarning({ engine, ...props }) {
return null;
}
const tooltipWarning = supersedes ? t`New driver` : t`Driver deprecated`;
const warningContent = supersedes
? getSupersedesWarningContent(engine, supersedes)
: getSupersededByWarningContent(supersededBy);
? getSupersedesWarningContent(engine, supersedes, onChangeEngine)
: getSupersededByWarningContent(supersededBy, onChangeEngine);
return (
<DriverWarningContainer p={2} {...props}>
<IconContainer>
<Warnings
className="mx2 text-gold"
warnings={[tooltipWarning]}
size={20}
/>
<IconContainer hasCircle={hasCircle}>
{(supersededBy && <WarningIcon size={20} name="warning" />) ||
(supersedes && <Icon size={20} name="info" />)}
</IconContainer>
<CardContent flexDirection="column" justify="center" className="ml2">
{warningContent}
</CardContent>
<CardContent className="ml2">{warningContent}</CardContent>
</DriverWarningContainer>
);
}
......
frontend/src/metabase/components/DriverWarning/DriverWarning.styled.jsx
+ 36
− 5
View file @ babbffbc
import styled from "styled-components";
import { Flex } from "grid-styled";
export const CardContent = styled(Flex)``;
import { color } from "metabase/lib/colors";
import { space } from "metabase/styled-components/theme";
export const IconContainer = styled.div`
import Icon from "metabase/components/Icon";
export const CardContent = styled.div`
display: flex;
align-items: center;
justify-content: center;
`;
export const IconContainer = styled.div`
border-radius: 99px !important;
flex-shrink: 0;
display: flex;
align-items: center;
justify-content: center;
width: ${props => (props.hasCircle ? "52px" : "32px")};
height: ${props => (props.hasCircle ? "52px" : "32px")};
background-color: ${props => (props.hasCircle ? "#EEF2F5" : "transparent")};
`;
export const DriverWarningContainer = styled(Flex)`
export const DriverWarningContainer = styled.div`
background-color: #f9fbfb;
border-radius: 10px;
width: 300px;
width: 320px;
display: flex;
margin-top: 8px;
margin-bottom: 8px;
margin-left: 26px;
padding: 16px;
`;
export const WarningIcon = styled(Icon)`
color: ${color("accent5")};
`;
export const WarningParagraph = styled.p`
margin: ${props => (props.hasMargin ? `${space(1)} 0;` : 0)};
color: ${color("text-medium")};
`;
export const Link = styled.a`
color: ${color("brand")};
font-weight: 700;
`;
frontend/src/metabase/services.js
+ 1
− 0
View file @ babbffbc
......@@ -231,6 +231,7 @@ export const MetabaseApi = {
db_sync_schema: POST("/api/database/:dbId/sync_schema"),
db_rescan_values: POST("/api/database/:dbId/rescan_values"),
db_discard_values: POST("/api/database/:dbId/discard_values"),
db_get_db_ids_with_deprecated_drivers: GET("/db-ids-with-deprecated-drivers"),
table_list: GET("/api/table"),
// table_get: GET("/api/table/:tableId"),
table_update: PUT("/api/table/:id"),
......
frontend/src/metabase/setup/components/Setup.jsx
+ 5
− 5
View file @ babbffbc
......@@ -204,11 +204,6 @@ export default class Setup extends Component {
</div>
<AddDatabaseHelpCardHolder isVisible={isDatabaseHelpCardVisible}>
<DriverWarning
engine={selectedDatabaseEngine}
ml={26}
data-testid="database-setup-driver-warning"
/>
<AddDatabaseHelpCard
engine={selectedDatabaseEngine}
hasCircle={false}
......@@ -218,6 +213,11 @@ export default class Setup extends Component {
backgroundColor: color("white"),
}}
/>
<DriverWarning
engine={selectedDatabaseEngine}
ml={26}
data-testid="database-setup-driver-warning"
/>
</AddDatabaseHelpCardHolder>
</div>
);
......
src/metabase/api/database.clj
+ 11
− 0
View file @ babbffbc
......@@ -742,5 +742,16 @@
[:in :collection_id (api/check-404 (seq (db/select-ids Collection :name schema)))])])
(map table-api/card->virtual-table))))
(api/defendpoint GET "/db-ids-with-deprecated-drivers"
"Return a list of database IDs using currently deprecated drivers."
[]
(map
u/the-id
(filter
(fn [database]
(let [info (driver.u/available-drivers-info)
d (driver.u/database->driver database)]
(some? (:superseded-by (d info)))))
(db/select-ids Database))))
(api/define-routes)
test/metabase/api/database_test.clj
+ 12
− 1
View file @ babbffbc
......@@ -25,7 +25,7 @@
[toucan.db :as db]
[toucan.hydrate :as hydrate]))
(use-fixtures :once (fixtures/initialize :db :plugins))
(use-fixtures :once (fixtures/initialize :db :plugins :test-drivers))
;; HELPER FNS
......@@ -1092,3 +1092,14 @@
:tunnel-private-key-passphrase protected-password
:access-token protected-password
:refresh-token protected-password})))))
(deftest db-ids-with-deprecated-drivers-test
(mt/with-driver :driver-deprecation-test-legacy
(testing "GET /api/database/db-ids-with-deprecated-drivers"
(mt/with-temp Database [{db-id :id} {:engine :driver-deprecation-test-legacy}]
(is (not-empty (filter #(= % db-id) (mt/user-http-request
:crowberto
:get
200
"database/db-ids-with-deprecated-drivers"))))))))
test/metabase/plugins/driver_deprecation_test.clj
+ 2
− 2
View file @ babbffbc
......@@ -4,9 +4,9 @@
[metabase.test :as mt]
[metabase.test.fixtures :as fixtures]))
(use-fixtures :once (fixtures/initialize :plugins))
(use-fixtures :once (fixtures/initialize :plugins :test-drivers))
(deftest driver-deprecation-test
(mt/test-driver :driver-deprecation-test-legacy
(mt/with-driver :driver-deprecation-test-legacy
(is (= :driver-deprecation-test-new
(get-in (setting/properties :public) [:engines :driver-deprecation-test-legacy :superseded-by])))))
test/metabase/query_processor_test/explicit_joins_test.clj
+ 12
− 12
View file @ babbffbc
......@@ -173,24 +173,24 @@
(mt/test-drivers (mt/normal-drivers-with-feature :left-join)
(testing "Can we include no Fields (with `:none`)"
(is (= {:columns (mapv mt/format-name ["id" "name" "flock_id"])
:rows [[2 "Big Red" 5 ]
[7 "Callie Crow" 4 ]
:rows [[2 "Big Red" 5]
[7 "Callie Crow" 4]
[3 "Camellia Crow" nil]
[16 "Carson Crow" 4 ]
[12 "Chicken Little" 5 ]
[16 "Carson Crow" 4]
[12 "Chicken Little" 5]
[5 "Geoff Goose" nil]
[9 "Gerald Goose" 1 ]
[6 "Greg Goose" 1 ]
[14 "McNugget" 5 ]
[9 "Gerald Goose" 1]
[6 "Greg Goose" 1]
[14 "McNugget" 5]
[17 "Olita Owl" nil]
[18 "Oliver Owl" 3 ]
[15 "Orville Owl" 3 ]
[18 "Oliver Owl" 3]
[15 "Orville Owl" 3]
[11 "Oswald Owl" nil]
[10 "Pamela Pelican" nil]
[8 "Patricia Pelican" nil]
[13 "Paul Pelican" 2 ]
[4 "Peter Pelican" 2 ]
[1 "Russell Crow" 4 ]]}
[13 "Paul Pelican" 2]
[4 "Peter Pelican" 2]
[1 "Russell Crow" 4]]}
(mt/format-rows-by [#(some-> % int) str #(some-> % int)]
(mt/rows+column-names
(mt/dataset bird-flocks
......
test/metabase/test/initialize.clj
+ 9
− 0
View file @ babbffbc
......@@ -83,6 +83,15 @@
(classloader/require 'metabase.test.initialize.plugins)
((resolve 'metabase.test.initialize.plugins/init!)))
;; initialize test drivers that are not shipped as part of the product
;; this is needed because if DRIVERS=all in the environment, then only the directories within modules are searched to
;; determine the set of available drivers, so the "test only" drivers that live under test_modules will never be
;; registered
(define-initialization :test-drivers
(classloader/require 'metabase.test.initialize.plugins)
((resolve 'metabase.test.initialize.plugins/init-test-drivers!)
[:driver-deprecation-test-legacy :driver-deprecation-test-new]))
;; initializing the DB also does setup needed so the scheduler will work correctly. (Remember that the scheduler uses
;; a JDBC backend!)
(define-initialization :db
......
test/metabase/test/initialize/plugins.clj
+ 7
− 0
View file @ babbffbc
......@@ -52,3 +52,10 @@
(defn init! []
(plugins/load-plugins!)
(load-plugin-manifests!))
(defn init-test-drivers!
"Explicitly initialize the given test `drivers` via plugin manifests. These manifests can live in test_modules (having
the same directory structure as modules), but test_modules will not be built or shipped as part of the core product."
{:added "0.41.0"}
[drivers]
(load-plugin-manifests! drivers))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment