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

O3-1242 App crashes on logout #423

Merged
merged 6 commits into from
May 6, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
54 changes: 0 additions & 54 deletions packages/apps/esm-login-app/src/CurrentUserContext.tsx

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ import {
navigate,
useConfig,
setSessionLocation,
useSession,
} from "@openmrs/esm-framework";
import { useLoginLocations } from "./choose-location.resource";
import { useCurrentUser } from "../CurrentUserContext";
import type { StaticContext } from "react-router";

export interface LoginReferrer {
Expand All @@ -27,7 +27,7 @@ export const ChooseLocation: React.FC<ChooseLocationProps> = ({
const returnToUrl = new URLSearchParams(location?.search).get("returnToUrl");
const referrer = location?.state?.referrer;
const config = useConfig();
const user = useCurrentUser();
const { user } = useSession();
const { locationData, isLoading } = useLoginLocations(
config.chooseLocation.useLoginLocationTag
);
Expand All @@ -39,8 +39,10 @@ export const ChooseLocation: React.FC<ChooseLocationProps> = ({
: Promise.resolve();

sessionDefined.then(() => {
// console.log("referrer: ", referrer);
if (referrer && referrer !== "/") {
if (
referrer &&
!["/", "/login", "/login/location"].includes(referrer)
) {
navigate({ to: "${openmrsSpaBase}" + referrer });
return;
}
Expand All @@ -55,6 +57,8 @@ export const ChooseLocation: React.FC<ChooseLocationProps> = ({
[referrer, config.links.loginSuccess, returnToUrl]
);

// Handle cases where the location picker is disabled, there is only one
// location, or there are no locations.
useEffect(() => {
if (!isLoading) {
if (!config.chooseLocation.enabled || locationData?.length === 1) {
Expand All @@ -72,6 +76,11 @@ export const ChooseLocation: React.FC<ChooseLocationProps> = ({
isLoading,
]);

if (!isLoading && !user) {
navigate({ to: "${openmrsSpaBase}/login" });
return null;
}

if (!isLoading || !isLoginEnabled) {
return (
<LocationPicker
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {
openmrsFetch,
fhirBaseUrl,
FetchResponse,
showNotification,
reportError,
} from "@openmrs/esm-framework";
import useSwrInfinite from "swr/infinite";
import { LocationEntry, LocationResponse } from "../types";
Expand Down Expand Up @@ -61,11 +61,8 @@ export function useLoginLocations(
>(getUrl, openmrsFetch);

if (error) {
showNotification({
title: error.name,
description: error.message,
kind: "error",
});
console.error(error);
reportError(error);
}

const memoizedLocationData = useMemo(() => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import { waitFor } from "@testing-library/react";
import renderWithRouter from "../test-helpers/render-with-router";
import { navigate, openmrsFetch, useConfig } from "@openmrs/esm-framework";
import {
navigate,
openmrsFetch,
useConfig,
useSession,
} from "@openmrs/esm-framework";
import {
mockSetSessionLocation,
mockSoleLoginLocation,
Expand All @@ -12,13 +17,13 @@ const mockedNavigate = navigate as jest.Mock;
const mockedOpenmrsFetch = openmrsFetch as jest.Mock;
const mockedUseConfig = useConfig as jest.Mock;

jest.mock("../CurrentUserContext", () => ({
useCurrentUser() {
return {
display: "Testy McTesterface",
};
const mockUseSession = useSession as jest.Mock;

mockUseSession.mockReturnValue({
user: {
display: "Testy McTesterface",
},
}));
});

describe("ChooseLocation: ", () => {
beforeEach(() => {
Expand Down
5 changes: 2 additions & 3 deletions packages/apps/esm-login-app/src/login/login.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@ import ArrowRight24 from "@carbon/icons-react/es/arrow--right/24";
import { Button, InlineNotification, TextInput } from "carbon-components-react";
import { RouteComponentProps } from "react-router-dom";
import { useTranslation } from "react-i18next";
import { useConfig, interpolateUrl } from "@openmrs/esm-framework";
import { useConfig, interpolateUrl, useSession } from "@openmrs/esm-framework";
import { performLogin } from "./login.resource";
import { useCurrentUser } from "../CurrentUserContext";
import type { StaticContext } from "react-router";

const hidden: React.CSSProperties = {
Expand All @@ -27,7 +26,7 @@ export interface LoginProps

const Login: React.FC<LoginProps> = ({ history, location, isLoginEnabled }) => {
const config = useConfig();
const user = useCurrentUser();
const { user } = useSession();
const [username, setUsername] = useState("");
const [password, setPassword] = useState("");
const [errorMessage, setErrorMessage] = useState("");
Expand Down
20 changes: 10 additions & 10 deletions packages/apps/esm-login-app/src/login/login.test.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { useState } from "react";
import { waitFor, screen } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import { setSessionLocation, useConfig } from "@openmrs/esm-framework";
import {
setSessionLocation,
useConfig,
useSession,
} from "@openmrs/esm-framework";
import { performLogin } from "./login.resource";
import { useCurrentUser } from "../CurrentUserContext";
import { mockConfig } from "../../__mocks__/config.mock";
import renderWithRouter from "../test-helpers/render-with-router";
import Login from "./login.component";
Expand All @@ -15,12 +18,8 @@ jest.mock("./login.resource", () => ({
}));

const mockedSetSessionLocation = setSessionLocation as jest.Mock;
const mockedUseCurrentUser = useCurrentUser as jest.Mock;
const mockedUseConfig = useConfig as jest.Mock;

jest.mock("../CurrentUserContext", () => ({
useCurrentUser: jest.fn(),
}));
const mockedUseSession = useSession as jest.Mock;

const loginLocations = [
{ uuid: "111", display: "Earth" },
Expand All @@ -31,7 +30,8 @@ describe(`<Login />`, () => {
beforeEach(() => {
mockedLogin.mockReset();
mockedSetSessionLocation.mockReset();
mockedUseCurrentUser.mockReset();
mockedUseSession.mockReset();
mockedUseSession.mockReturnValue({ authenticated: false });
mockedUseConfig.mockReturnValue(mockConfig);
});

Expand Down Expand Up @@ -106,10 +106,10 @@ describe(`<Login />`, () => {
});
return Promise.resolve({ data: { authenticated: true } });
});
mockedUseCurrentUser.mockImplementation(() => {
mockedUseSession.mockImplementation(() => {
const [user, setUser] = useState();
refreshUser = setUser;
return user;
return { user, authenticated: !!user };
});

const wrapper = renderWithRouter(
Expand Down
14 changes: 10 additions & 4 deletions packages/apps/esm-login-app/src/redirect-logout/logout.resource.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import { openmrsFetch, refetchCurrentUser } from "@openmrs/esm-framework";
import {
clearCurrentUser,
openmrsFetch,
refetchCurrentUser,
} from "@openmrs/esm-framework";

export function performLogout() {
return openmrsFetch("/ws/rest/v1/session", {
export async function performLogout() {
await openmrsFetch("/ws/rest/v1/session", {
method: "DELETE",
}).then(refetchCurrentUser);
});
clearCurrentUser();
await refetchCurrentUser();
}
Original file line number Diff line number Diff line change
@@ -1,26 +1,25 @@
import React, { useEffect } from "react";
import { RouteComponentProps } from "react-router-dom";
import { navigate, useConfig } from "@openmrs/esm-framework";
import { navigate, useConfig, useSession } from "@openmrs/esm-framework";
import { performLogout } from "./logout.resource";
import { useCurrentUser } from "../CurrentUserContext";

export interface RedirectLogoutProps extends RouteComponentProps<{}> {
isLoginEnabled: boolean;
}

const RedirectLogout: React.FC<RedirectLogoutProps> = ({ isLoginEnabled }) => {
const config = useConfig();
const user = useCurrentUser();
const user = useSession();
brandones marked this conversation as resolved.
Show resolved Hide resolved

useEffect(() => {
if (!user || !isLoginEnabled) {
if (!user.authenticated || !isLoginEnabled) {
navigate({ to: "${openmrsSpaBase}/login" });
} else {
performLogout().then(() => {
if (config.provider.type === "oauth2") {
location.href = config.provider.logoutUrl;
} else {
location.href = window.spaBase;
navigate({ to: "${openmrsSpaBase}/login" });
}
});
}
Expand Down
47 changes: 21 additions & 26 deletions packages/apps/esm-login-app/src/root.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,39 +3,34 @@ import Login from "./login/login.component";
import ChooseLocation from "./choose-location/choose-location.component";
import RedirectLogout from "./redirect-logout/redirect-logout.component";
import { BrowserRouter, Route } from "react-router-dom";
import { CurrentUserContext } from "./CurrentUserContext";

export interface RootProps {
isLoginEnabled: boolean;
}

const Root: React.FC<RootProps> = ({ isLoginEnabled }) => {
return (
<CurrentUserContext>
<BrowserRouter basename={window.spaBase}>
<Route
exact
path="/login(/confirm)?"
render={(props) => (
<Login {...props} isLoginEnabled={isLoginEnabled} />
)}
/>
<Route
exact
path="/login/location"
render={(props) => (
<ChooseLocation {...props} isLoginEnabled={isLoginEnabled} />
)}
/>
<Route
exact
path="/logout"
render={(props) => (
<RedirectLogout {...props} isLoginEnabled={isLoginEnabled} />
)}
/>
</BrowserRouter>
</CurrentUserContext>
<BrowserRouter basename={window.spaBase}>
<Route
exact
path="/login(/confirm)?"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking, I haven't seen this syntax from react-router-dom before... doesn't look like plain text and doesn't look like regex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't write this, but it looks like regex to me. Parentheses for grouping and a question mark for "optional;" i.e. this will match /login or /login/confirm.

render={(props) => <Login {...props} isLoginEnabled={isLoginEnabled} />}
/>
<Route
exact
path="/login/location"
render={(props) => (
<ChooseLocation {...props} isLoginEnabled={isLoginEnabled} />
)}
/>
<Route
exact
path="/logout"
render={(props) => (
<RedirectLogout {...props} isLoginEnabled={isLoginEnabled} />
)}
/>
</BrowserRouter>
);
};

Expand Down
22 changes: 11 additions & 11 deletions packages/apps/esm-primary-navigation-app/src/root.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,19 @@ const Root: React.FC<RootProps> = () => {
const openmrsSpaBase = window["getOpenmrsSpaBase"]();

useEffect(() => {
const currentUserSub = getSynchronizedCurrentUser({
includeAuthStatus: true,
}).subscribe((response) => {
setAllowedLocales(response.allowedLocales);
const currentUserSub = getSynchronizedCurrentUser().subscribe(
(response) => {
setAllowedLocales(response.allowedLocales);

if (response.authenticated) {
setUser(response.user);
} else {
setUser(false);
}
if (response.authenticated) {
setUser(response.user);
} else {
setUser(false);
}

createErrorHandler();
});
createErrorHandler();
}
);

const currentSessionSub = getCurrentSession().subscribe(({ data }) =>
setUserSession(data)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import {
CurrentUserWithResponseOption,
getCurrentUser,
getSynchronizationItemsFor,
openmrsObservableFetch,
Expand All @@ -15,10 +14,8 @@ export function getCurrentSession() {
* Returns an observable producing the current user, but also applies any unsynchronized user property
* changes to that user.
*/
export function getSynchronizedCurrentUser(
opts: CurrentUserWithResponseOption
) {
return getCurrentUser(opts).pipe(
export function getSynchronizedCurrentUser() {
return getCurrentUser({ includeAuthStatus: true }).pipe(
mergeMap(async (result) => {
const { user } = result;

Expand Down
Loading