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

Upgrade react-router to v6 #3261

Merged
merged 28 commits into from
Feb 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
3c977e2
Upgrade react-router to v6
nickclyde Jan 21, 2022
1f24239
Merge branch 'main' into nick/upgrade-react-router
nickclyde Jan 21, 2022
0cda207
Fix tests
nickclyde Jan 21, 2022
5007567
Merge branch 'main' into nick/upgrade-react-router
nickclyde Jan 21, 2022
86e9268
Merge branch 'main' into nick/upgrade-react-router
nickclyde Jan 24, 2022
6bb30ac
Code smells
nickclyde Jan 24, 2022
7997852
Fix various TypeScript errors
nickclyde Jan 24, 2022
b57c3c4
Add missing useEffect dependency
nickclyde Jan 24, 2022
3daf66c
Fix QueueItem storybook story
nickclyde Jan 24, 2022
4fe9c77
Undo stupid change where I confused path params with URL search params
nickclyde Jan 24, 2022
713319f
Forgot a few wildcards
nickclyde Jan 24, 2022
b60ef2d
Add test for Prompt
nickclyde Jan 24, 2022
45db92a
Fix <Navigate> to props in AccountCreationApp
nickclyde Jan 25, 2022
3a5e30c
Merge branch 'main' into nick/upgrade-react-router
nickclyde Jan 25, 2022
414ab98
Make all navigation relative
nickclyde Jan 25, 2022
93a2988
Fix tests
nickclyde Jan 25, 2022
d0ad467
Use absolute paths in places where it makes more sense
nickclyde Jan 25, 2022
ce65cdc
A few navigation bugfixes
nickclyde Jan 25, 2022
66a434d
Code smells
nickclyde Jan 25, 2022
a2e0772
Add /reload-app path for reloading whoami query for TenantDataAccess
nickclyde Jan 25, 2022
33c7d5f
A little more test coverage for <Prompt>
nickclyde Jan 26, 2022
20256f8
Merge branch 'main' into nick/upgrade-react-router
nickclyde Jan 26, 2022
6a3a4ab
Merge branch 'main' into nick/upgrade-react-router
nickclyde Jan 27, 2022
fda9951
DRY up App component
nickclyde Jan 27, 2022
294d217
Apply style to active-nav-item class instead of using style prop
nickclyde Jan 27, 2022
594b6b6
Revert App changes
nickclyde Jan 27, 2022
7d43416
Merge branch 'main' into nick/upgrade-react-router
nickclyde Jan 28, 2022
008ed0e
Merge branch 'main' into nick/upgrade-react-router
nickclyde Feb 1, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions frontend/cypress/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ services:
REACT_APP_BACKEND_URL: http://localhost.simplereport.gov/api
REACT_APP_OKTA_ENABLED: "true"
REACT_APP_OKTA_URL: http://cypress:8088
REACT_APP_DISABLE_MAINTENANCE_BANNER: "true"
volumes:
- ../:/frontend/
- ../../backend/src/main/resources/graphql:/backend/src/main/resources/graphql
Expand Down
5 changes: 2 additions & 3 deletions frontend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"@trussworks/react-uswds": "^2.6.0",
"@types/google-libphonenumber": "^7.4.23",
"@types/history": "^4.7.10",
"@types/react-router-dom": "^5.1.6",
"@types/react-router-dom": "^5.3.3",
"@types/react-transition-group": "^4.4.4",
"@types/testing-library__jest-dom": "^5.14.2",
"apollo-upload-client": "^14.1.3",
Expand All @@ -37,8 +37,7 @@
"react-i18next": "^11.8.15",
"react-modal": "^3.13.1",
"react-redux": "^7.2.6",
"react-router": "^5.2.0",
"react-router-dom": "^5.2.0",
"react-router-dom": "^6.2.1",
"react-scripts": "^5.0.0",
"react-toastify": "^6.1.0",
"react-transition-group": "^4.4.2",
Expand Down
11 changes: 9 additions & 2 deletions frontend/src/app/App.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { BrowserRouter as Router, MemoryRouter, Route } from "react-router-dom";
import {
BrowserRouter as Router,
MemoryRouter,
Route,
Routes,
} from "react-router-dom";
import { Provider } from "react-redux";
import createMockStore, { MockStoreEnhanced } from "redux-mock-store";
import { MockedProvider, MockedResponse } from "@apollo/client/testing";
Expand Down Expand Up @@ -199,7 +204,9 @@ const renderApp = (
<Provider store={newStore}>
<MockedProvider mocks={queryMocks} addTypename={false}>
<Router>
<Route path="/" component={App} />
<Routes>
<Route path="/*" element={<App />} />
</Routes>
</Router>
</MockedProvider>
</Provider>
Expand Down
164 changes: 95 additions & 69 deletions frontend/src/app/App.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React, { useEffect } from "react";
import { gql, useQuery } from "@apollo/client";
import { useDispatch, connect } from "react-redux";
import { Redirect, Route, Switch } from "react-router-dom";
import { Navigate, Route, Routes, useLocation } from "react-router-dom";
import { ApplicationInsights } from "@microsoft/applicationinsights-web";

import ProtectedRoute from "./commonComponents/ProtectedRoute";
Expand Down Expand Up @@ -50,13 +50,14 @@ export const WHOAMI_QUERY = gql`
const App = () => {
const appInsights = getAppInsights();
const dispatch = useDispatch();
const location = useLocation();

// Check if the user is logged in, if not redirect to Okta
if (process.env.REACT_APP_OKTA_ENABLED === "true") {
const accessToken = localStorage.getItem("access_token");
if (!accessToken) {
// If Okta login has been attempted and returned to SR with an error, don't redirect back to Okta
const params = new URLSearchParams(window.location.hash.slice(1));
const params = new URLSearchParams(location.hash.slice(1));
if (params.get("error")) {
throw new Error(
params.get("error_description") || "Unknown Okta error"
Expand Down Expand Up @@ -118,100 +119,125 @@ const App = () => {
let homepagePath: string;

if (isSupportAdmin) {
homepagePath = "/admin";
homepagePath = "admin";
} else if (isOrgAdmin) {
homepagePath = "/dashboard";
homepagePath = "dashboard";
} else {
homepagePath = "/queue";
homepagePath = "queue";
}

const canViewResults = appPermissions.results.canView;
const canViewPeople = appPermissions.people.canView;
const canEditPeople = appPermissions.people.canEdit;
const canViewSettings = appPermissions.settings.canView;

return (
<>
<VersionEnforcer />
<MaintenanceBanner />
{process.env.REACT_APP_DISABLE_MAINTENANCE_BANNER === "true" ? null : (
<MaintenanceBanner />
)}
{process.env.REACT_APP_IS_TRAINING_SITE === "true" && (
<TrainingNotification />
)}
<WithFacility>
<Page>
<Header />
<Switch>
<Route
path="/queue"
render={() => {
return <TestQueueContainer />;
}}
/>
<Routes>
<Route
path="/"
exact
render={({ location }) => (
<Redirect
to={{
...location,
pathname: homepagePath,
}}
element={
<Navigate
to={{ pathname: homepagePath, search: location.search }}
/>
)}
}
/>
<ProtectedRoute
path="/results/:page"
render={({ match }: any) => {
return <TestResultsList pageNumber={match.params.page} />;
}}
requiredPermissions={appPermissions.results.canView}
userPermissions={data.whoami.permissions}
<Route path="queue" element={<TestQueueContainer />} />
<Route
path="results/:pageNumber"
element={
<ProtectedRoute
requiredPermissions={canViewResults}
userPermissions={data.whoami.permissions}
element={<TestResultsList />}
/>
}
/>
<ProtectedRoute
path="/results/"
render={() => <CleanTestResultsList />}
requiredPermissions={appPermissions.results.canView}
userPermissions={data.whoami.permissions}
<Route
path="results"
element={
<ProtectedRoute
requiredPermissions={canViewResults}
userPermissions={data.whoami.permissions}
element={<CleanTestResultsList />}
/>
}
/>
<ProtectedRoute
path={`/patients/:page?`}
render={({ match }: any) => {
return <ManagePatientsContainer page={match.params.page} />;
}}
requiredPermissions={appPermissions.people.canView}
userPermissions={data.whoami.permissions}
<Route
path="patients/:pageNumber"
element={
<ProtectedRoute
requiredPermissions={canViewPeople}
userPermissions={data.whoami.permissions}
element={<ManagePatientsContainer />}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's worth defining this protected route as a variable, since it's repeated under the regular patients route too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah not a bad idea! The reason I had to duplicate this route is because v5 supported optional URL params like /patients/:pageNumber? which would match both /patients and /patients/1, but v6 no longer supports that. Hence the need for two routes. But I will definitely DRY it up

Copy link
Member Author

Choose a reason for hiding this comment

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

Soooo I tried to do this, and for some reason it broke the patients page. It kept throwing "The operation is insecure", and I couldn't figure out why 🤔 so I reverted for now.

/>
}
/>
<ProtectedRoute
path={`/patient/:patientId`}
render={({ match }: any) => (
<EditPatientContainer patientId={match.params.patientId} />
)}
requiredPermissions={appPermissions.people.canEdit}
userPermissions={data.whoami.permissions}
<Route
path="patients"
element={
<ProtectedRoute
requiredPermissions={canViewPeople}
userPermissions={data.whoami.permissions}
element={<ManagePatientsContainer />}
/>
}
/>
<ProtectedRoute
path={`/add-patient/`}
render={() => <AddPatient />}
requiredPermissions={appPermissions.people.canEdit}
userPermissions={data.whoami.permissions}
<Route
path="patient/:patientId"
element={
<ProtectedRoute
requiredPermissions={canEditPeople}
userPermissions={data.whoami.permissions}
element={<EditPatientContainer />}
/>
}
/>
<ProtectedRoute
path="/settings"
component={Settings}
requiredPermissions={appPermissions.settings.canView}
userPermissions={data.whoami.permissions}
<Route
path="add-patient"
element={
<ProtectedRoute
requiredPermissions={canEditPeople}
userPermissions={data.whoami.permissions}
element={<AddPatient />}
/>
}
/>
<ProtectedRoute
path="/dashboard"
component={Analytics}
requiredPermissions={appPermissions.settings.canView}
userPermissions={data.whoami.permissions}
<Route
path="settings/*"
element={
<ProtectedRoute
requiredPermissions={canViewSettings}
userPermissions={data.whoami.permissions}
element={<Settings />}
/>
}
/>
<Route
path={"/admin"}
render={({ match }) => (
<SupportAdminRoutes
match={match}
isAdmin={data.whoami.isAdmin}
path="dashboard"
element={
<ProtectedRoute
requiredPermissions={canViewSettings}
userPermissions={data.whoami.permissions}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we may want to extract this data.whoami.permissions out to a variable

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above - moving to a shorter named const seemed to cause issues, so I reverted

element={<Analytics />}
/>
)}
}
/>
<Route
path="admin/*"
element={<SupportAdminRoutes isAdmin={isSupportAdmin} />}
/>
</Switch>
</Routes>
</Page>
</WithFacility>
</>
Expand Down
15 changes: 7 additions & 8 deletions frontend/src/app/HealthChecks.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
import React from "react";
import { Route } from "react-router-dom";
import { Route, Routes } from "react-router-dom";

const HealthChecks: React.FC<{}> = ({ match }: any) => (
<>
<Route path={match.url + "/ping"} render={() => <div>pong</div>} />
const HealthChecks = () => (
<Routes>
<Route path="ping" element={<div>pong</div>} />
<Route
path={match.url + "/commit"}
render={() => <div>{process.env.REACT_APP_CURRENT_COMMIT}</div>}
path="commit"
element={<div>{process.env.REACT_APP_CURRENT_COMMIT}</div>}
/>
</>
</Routes>
);

export default HealthChecks;
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React from "react";
import { render, screen, within } from "@testing-library/react";
import { MemoryRouter } from "react-router";
import { MemoryRouter } from "react-router-dom";

import OrderingProviderList from "./OrderingProviderList";

Expand Down
2 changes: 1 addition & 1 deletion frontend/src/app/Settings/Facility/FacilityForm.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React, { useCallback, useState } from "react";
import { Prompt } from "react-router-dom";

import iconSprite from "../../../../node_modules/uswds/dist/img/sprite.svg";
import Button from "../../commonComponents/Button/Button";
Expand All @@ -19,6 +18,7 @@ import {
AddressConfirmationModal,
AddressSuggestionConfig,
} from "../../commonComponents/AddressConfirmationModal";
import Prompt from "../../utils/Prompt";

import ManageDevices from "./Components/ManageDevices";
import OrderingProviderSettings from "./Components/OrderingProvider";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
import userEvent from "@testing-library/user-event";
import { MockedProvider } from "@apollo/client/testing";
import { Provider } from "react-redux";
import { MemoryRouter } from "react-router";
import { MemoryRouter } from "react-router-dom";
import configureStore from "redux-mock-store";

import { getAppInsights } from "../../TelemetryService";
Expand Down Expand Up @@ -166,10 +166,13 @@ jest.mock("./FacilityForm", () => {
);
};
});
jest.mock("react-router-dom", () => ({
Redirect: () => <p>Redirected</p>,
}));

jest.mock("react-router-dom", () => {
const original = jest.requireActual("react-router-dom");
return {
...original,
Navigate: () => <p>Redirected</p>,
};
});
jest.mock("../../TelemetryService", () => ({
getAppInsights: jest.fn(),
}));
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/app/Settings/Facility/FacilityFormContainer.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useState } from "react";
import { gql, useQuery, useMutation } from "@apollo/client";
import { Redirect } from "react-router-dom";
import { Navigate } from "react-router-dom";
import { useDispatch } from "react-redux";

import { updateFacility } from "../../store";
Expand Down Expand Up @@ -191,7 +191,7 @@ const FacilityFormContainer: any = (props: Props) => {
if (props.newOrg) {
window.location.pathname = process.env.PUBLIC_URL || "";
}
return <Redirect push to={{ pathname: "/settings/facilities" }} />;
return <Navigate to="/settings/facilities" />;
}

const saveFacility = async (facility: Facility) => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { render, screen, act } from "@testing-library/react";
import { MockedProvider } from "@apollo/client/testing";
import { Provider } from "react-redux";
import { MemoryRouter } from "react-router";
import { MemoryRouter } from "react-router-dom";
import configureStore from "redux-mock-store";

import { GetManagedFacilitiesDocument } from "../../../generated/graphql";
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/app/Settings/FacilityForm.test.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { render, screen, waitFor } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import { MemoryRouter } from "react-router";
import { MemoryRouter } from "react-router-dom";
import { ToastContainer } from "react-toastify";

import * as clia from "../utils/clia";
Expand Down
Loading