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.

Sign up for GitHub

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

Jump to bottom

More tidying prior to Models in Browse Data #38126

Merged
merged 10 commits into from
Jan 26, 2024
Merged

More tidying prior to Models in Browse Data #38126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1fd7280
Make embedding-full-app.cy.spec.js and associated components more sem…
rafpaf Jan 25, 2024
72a2733
Make signin.cy.spec.js and associated components more semantic
rafpaf Jan 25, 2024
493a772
Fix comment
rafpaf Jan 25, 2024
586336c
minor tweak
rafpaf Jan 25, 2024
86ea8af
Remove aria-label=breadcrumbs
rafpaf Jan 25, 2024
cafd5b0
Fix variable name
rafpaf Jan 25, 2024
f6d1e82
Fix e2e tests
rafpaf Jan 25, 2024
507caec
Remove cy.heading
rafpaf Jan 25, 2024
d67e988
Restore test name
rafpaf Jan 25, 2024
5d75fe7
Merge branch 'master' into models-in-browse-data--more-initial-cleanup
rafpaf Jan 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 19 additions & 24 deletions e2e/test/scenarios/embedding/embedding-full-app.cy.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ describeEE("scenarios > embedding > full app", () => {

appBar().within(() => {
cy.findByTestId("main-logo").should("be.visible");
cy.findByText("Our analytics").should("not.exist");
cy.findByRole("treeitem", { name: "Our analytics" }).should(
"not.exist",
);
});
});

Expand Down Expand Up @@ -151,10 +153,9 @@ describeEE("scenarios > embedding > full app", () => {
url: "/browse",
qs: { side_nav: false, logo: false },
});
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("Our data").should("be.visible");
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("Our analytics").should("not.exist");
cy.findByRole("heading", { name: /Our data/ }).should("be.visible");
cy.findByRole("treeitem", { name: /Our data/ }).should("not.exist");
cy.findByRole("treeitem", { name: "Our analytics" }).should("not.exist");
appBar().should("not.exist");
Copy link
Contributor Author

@rafpaf rafpaf Jan 25, 2024 •
edited

Choose a reason for hiding this comment

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

I think this test is actually ensuring that the side nav, not the top nav, is not rendered

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, @WiNloSt originally wrote these tests.
I think the test is doing what it says on the tin - if we hide the side navigation and hide the logo, we want to make sure the top bar is not rendered.

There is a test below that explicitly makes sure nav bar is hidden via param.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for helping me understand. I've reverted that change

Copy link
Member

Choose a reason for hiding this comment

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

@nemanjaglumac got it exactly right. That test was added during the implementation of hiding Metabase logo.

For context if you're interested:

});
});
Expand All @@ -165,14 +166,12 @@ describeEE("scenarios > embedding > full app", () => {

cy.findByTestId("qb-header").should("be.visible");
cy.findByTestId("qb-header-left-side").realHover();
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText(/Edited/).should("be.visible");
cy.button(/Edited/).should("be.visible");

cy.icon("refresh").should("be.visible");
cy.icon("notebook").should("be.visible");
cy.button("Summarize").should("be.visible");
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("Filter").should("be.visible");
cy.button("Filter").should("be.visible");
});

it("should hide the question header by a param", () => {
Expand Down Expand Up @@ -222,8 +221,7 @@ describeEE("scenarios > embedding > full app", () => {
qs: { top_nav: true, new_button: true, side_nav: false },
});

// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("New").click();
cy.button("New").click();
popover().findByText("Question").click();
popover().findByText("Raw Data").click();
popover().findByText("Orders").click();
Expand All @@ -246,8 +244,9 @@ describeEE("scenarios > embedding > full app", () => {
qs: { side_nav: false },
});

// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("Sample Database").should("be.visible");
cy.findByTestId("native-query-editor-container")
.findByText(/Sample Database/)
.should("be.visible");
});
});

Expand Down Expand Up @@ -290,20 +289,18 @@ describeEE("scenarios > embedding > full app", () => {
it("should show the dashboard header by default", () => {
visitDashboardUrl({ url: `/dashboard/${ORDERS_DASHBOARD_ID}` });

// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("Orders in a dashboard").should("be.visible");
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText(/Edited/).should("be.visible");
cy.findByTestId("dashboard-name-heading").should("be.visible");
cy.button(/Edited.*by/).should("be.visible");
});

it("should hide the dashboard header by a param", () => {
visitDashboardUrl({
url: `/dashboard/${ORDERS_DASHBOARD_ID}`,
qs: { header: false },
});

// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("Orders in a dashboard").should("not.exist");
cy.findByRole("heading", { name: "Orders in a dashboard" }).should(
"not.exist",
);
});

it("should hide the dashboard's additional info by a param", () => {
Expand Down Expand Up @@ -406,8 +403,7 @@ describeEE("scenarios > embedding > full app", () => {
it("should show the dashboard header by default", () => {
visitXrayDashboardUrl({ url: "/auto/dashboard/table/1" });

// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("More X-rays").should("be.visible");
cy.findByRole("heading", { name: "More X-rays" }).should("be.visible");
cy.button("Save this").should("be.visible");
});

Expand All @@ -417,8 +413,7 @@ describeEE("scenarios > embedding > full app", () => {
qs: { header: false },
});

// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("More X-rays").should("be.visible");
cy.findByRole("heading", { name: "More X-rays" }).should("be.visible");
cy.button("Save this").should("not.exist");
});
});
Expand Down
58 changes: 23 additions & 35 deletions e2e/test/scenarios/onboarding/auth/signin.cy.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,40 +30,36 @@ describe("scenarios > auth > signin", () => {
cy.visit("/");
cy.findByLabelText("Email address").type(admin.email);
cy.findByLabelText("Password").type("INVALID" + admin.password);
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("Sign in").click();
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.contains("did not match stored password");
cy.button("Sign in").click();
cy.findByRole("alert")
.filter(':contains("did not match stored password")')
.should("be.visible");
Copy link
Contributor Author

@rafpaf rafpaf Jan 25, 2024 •
edited

Choose a reason for hiding this comment

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

cy.findByRole("alert", {name: "did not match stored password"}) does not work; this longer formula is needed.

});

it("should display same error for unknown users (to avoid leaking the existence of accounts)", () => {
cy.visit("/");
cy.findByLabelText("Email address").type("INVALID" + admin.email);
cy.findByLabelText("Password").type(admin.password);
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("Sign in").click();
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.contains("did not match stored password");
cy.button("Sign in").click();
cy.findByRole("alert")
.filter(':contains("did not match stored password")')
.should("be.visible");
});

it("should greet users after successful login", () => {
cy.visit("/auth/login");
cy.findByLabelText("Email address").should("be.focused").type(admin.email);
cy.findByLabelText("Password").type(admin.password);
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("Sign in").click();
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.contains(/[a-z ]+, Bob/i);
cy.button("Sign in").click();
cy.findByTestId("greeting-message").should("contain.text", "Bobby");
});

it("should allow login regardless of login email case", () => {
cy.visit("/auth/login");
cy.findByLabelText("Email address").type(admin.email.toUpperCase());
cy.findByLabelText("Password").type(admin.password);
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("Sign in").click();
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.contains(/[a-z ]+, Bob/i);
cy.button("Sign in").click();
cy.findByTestId("greeting-message").should("contain.text", "Bobby");
});

it("should allow toggling of Remember Me", () => {
Expand All @@ -76,34 +72,26 @@ describe("scenarios > auth > signin", () => {
cy.findByRole("checkbox").should("not.be.checked");
});

it("should redirect to a unsaved question after login", () => {
it("should redirect to an unsaved question after login", () => {
cy.signInAsAdmin();
cy.visit("/");
// Browse data moved to an icon
browse().click();
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.contains("Sample Database").click();
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.contains("Orders").click();
cy.findByRole("heading", { name: "Sample Database" }).click();
cy.findByRole("heading", { name: "Orders" }).click();
cy.wait("@dataset");
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.contains("37.65");
cy.findAllByRole("gridcell", { name: "37.65" });

// signout and reload page with question hash in url
cy.signOut();
cy.reload();

// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.contains("Sign in to Metabase");
cy.findByRole("heading", { name: "Sign in to Metabase" });
cy.findByLabelText("Email address").type(admin.email);
cy.findByLabelText("Password").type(admin.password);
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("Sign in").click();
cy.button("Sign in").click();

// order table should load after login
cy.wait("@dataset");
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.contains("37.65");
cy.findAllByRole("gridcell", { name: "37.65" });
});

sizes.forEach(size => {
Expand All @@ -116,11 +104,11 @@ describe("scenarios > auth > signin", () => {

cy.visit("/");
cy.url().should("contain", "auth/login");
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("I seem to have forgotten my password").click();
cy.findByRole("link", {
name: "I seem to have forgotten my password",
}).click();
cy.url().should("contain", "auth/forgot_password");
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("Back to sign in").click();
cy.findByRole("link", { name: "Back to sign in" }).click();
cy.url().should("contain", "auth/login");
});
});
Expand Down
9 changes: 8 additions & 1 deletion frontend/src/metabase/auth/components/Login/Login.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,14 @@ export const Login = ({ params, location }: LoginProps): JSX.Element => {
const applicationName = useSelector(getApplicationName);
return (
<AuthLayout>
<Box c="text-dark" fz="1.25rem" fw="bold" lh="1.5rem" ta="center">
<Box
role="heading"
c="text-dark"
fz="1.25rem"
fw="bold"
lh="1.5rem"
ta="center"
>
{t`Sign in to ${applicationName}`}
</Box>
{selection && selection.Panel && (
Expand Down
4 changes: 3 additions & 1 deletion frontend/src/metabase/core/components/FormField/FormField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,9 @@ const FormField = forwardRef(function FormField(
{hasTitle && (
<FieldLabel hasError={hasError} htmlFor={htmlFor}>
{title}
{hasError && <FieldLabelError>: {error}</FieldLabelError>}
{hasError && (
<FieldLabelError role="alert">: {error}</FieldLabelError>
)}
</FieldLabel>
)}
{!!optional && !hasError && (
Expand Down
6 changes: 5 additions & 1 deletion frontend/src/metabase/dashboard/components/DashboardHeader/DashboardHeaderView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,11 @@ export function DashboardHeaderComponent({
data-testid="dashboard-header"
ref={header}
>
<HeaderContent hasSubHeader showSubHeader={showSubHeader}>
<HeaderContent
role="heading"
hasSubHeader
showSubHeader={showSubHeader}
>
<HeaderCaptionContainer>
<HeaderCaption
key={dashboard.name}
Expand Down
4 changes: 3 additions & 1 deletion frontend/src/metabase/home/components/HomeGreeting/HomeGreeting.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ export const HomeGreeting = (): JSX.Element => {
<GreetingLogo />
</Tooltip>
)}
<GreetingMessage showLogo={showLogo}>{message}</GreetingMessage>
<GreetingMessage data-testid="greeting-message" showLogo={showLogo}>
{message}
</GreetingMessage>
</GreetingRoot>
);
};
Expand Down