From 5b8c7629d9b420d47105f5a8d3275f92e2705e34 Mon Sep 17 00:00:00 2001 From: mazuh Date: Tue, 24 Oct 2023 08:54:45 -0300 Subject: [PATCH] fix tests and reorganize code to separete loaduser from login --- src/App.tsx | 3 +- src/features/auth/AuthMain.test.tsx | 11 +++- src/features/auth/AuthMain.tsx | 11 ++-- src/services/firestore-auth.test.ts | 9 +-- src/services/firestore-auth.ts | 77 ++++++++++------------- src/state/user.ts | 2 +- src/testing-helpers/mock-firebase-auth.ts | 5 +- 7 files changed, 58 insertions(+), 60 deletions(-) diff --git a/src/App.tsx b/src/App.tsx index fd3fd08..3219ca1 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -4,7 +4,7 @@ import { ThemeProvider } from "@mui/material/styles"; import { Switch, Route, Router, Redirect } from "wouter"; import { darkTheme } from "./components/app/mui-styles"; import { useAppDispatch } from "./state"; -import { loadUser, login } from "./state/user"; +import { loadUser } from "./state/user"; import AuthMain from "./features/auth/AuthMain"; import P2PCallMain from "./features/p2p-call/P2PCallMain"; import CallCreationMain from "./features/call-selection/CallCreationMain"; @@ -15,7 +15,6 @@ export default function App() { const dispatch = useAppDispatch(); useEffect(() => { - dispatch(login()); dispatch(loadUser()); }, [dispatch]); diff --git a/src/features/auth/AuthMain.test.tsx b/src/features/auth/AuthMain.test.tsx index 7af4331..f1a66b5 100644 --- a/src/features/auth/AuthMain.test.tsx +++ b/src/features/auth/AuthMain.test.tsx @@ -9,7 +9,11 @@ describe("CurrentUserStateIndicator", () => { (firestoreAuth.login as jest.Mock).mockClear(); }); - it("can login", async () => { + it("login will set loading ui to prevent double trigger", async () => { + (firestoreAuth.login as jest.Mock).mockResolvedValue( + new Promise((resolve) => setTimeout(resolve, 500)) + ); + const { store } = fullRender(); const loginButtonElement = screen.getByRole("button", { @@ -17,10 +21,11 @@ describe("CurrentUserStateIndicator", () => { }); await act(() => fireEvent.click(loginButtonElement)); - await waitFor(() => expect(store.getState().user.uid).not.toBe("")); + await waitFor(() => expect(store.getState().user.uid).toBe("")); + await waitFor(() => expect(screen.getByText("Checking...")).toBeTruthy()); }); - it("login is integrated with firestore", async () => { + it("triggering login will rely on firestore module", async () => { fullRender(); const loginButtonElement = screen.getByRole("button", { diff --git a/src/features/auth/AuthMain.tsx b/src/features/auth/AuthMain.tsx index 10db6d8..74ab92a 100644 --- a/src/features/auth/AuthMain.tsx +++ b/src/features/auth/AuthMain.tsx @@ -3,13 +3,16 @@ import Button from "@mui/material/Button"; import Typography from "@mui/material/Typography"; import GoogleIcon from "@mui/icons-material/Google"; import HomeTemplate from "../../components/templates/HomeTemplate"; -import { useAppSelector } from "../../state"; -import { selectIsUserPendingAuthentication } from "../../state/user"; -import firestoreAuth from "../../services/firestore-auth"; +import { useAppDispatch, useAppSelector } from "../../state"; +import { login, selectIsUserPendingAuthentication } from "../../state/user"; export default function AuthMain() { - const handleLoginClick = () => firestoreAuth.redirectLogin(); + const dispatch = useAppDispatch(); + + const handleLoginClick = () => dispatch(login()); + const isPending = useAppSelector(selectIsUserPendingAuthentication); + return ( { it("should successfully log in and return user information", async () => { jest.spyOn(firebaseAuth, "getAuth"); - jest.spyOn(firebaseAuth, "signInWithPopup"); - const user = await firestoreAuth.login(); + jest.spyOn(firebaseAuth, "getRedirectResult"); + await firestoreAuth.login(); + const user = await firestoreAuth.loadUser(); expect(user).toEqual({ uid: "abc123def456", displayName: "Jane Doe", @@ -16,7 +17,7 @@ describe("firestoreAuth", () => { it("should block log in when user email is null", async () => { jest.spyOn(firebaseAuth, "getAuth"); - jest.spyOn(firebaseAuth, "signInWithPopup").mockResolvedValue({ + jest.spyOn(firebaseAuth, "getRedirectResult").mockResolvedValue({ operationType: "signIn", providerId: "google.com", user: { @@ -39,7 +40,7 @@ describe("firestoreAuth", () => { photoURL: "", }, }); - await expect(firestoreAuth.login()).rejects.toThrow( + await expect(firestoreAuth.loadUser()).rejects.toThrow( "Login blocked: unidentified user." ); }); diff --git a/src/services/firestore-auth.ts b/src/services/firestore-auth.ts index 83c3b3a..81added 100644 --- a/src/services/firestore-auth.ts +++ b/src/services/firestore-auth.ts @@ -1,5 +1,5 @@ import once from "lodash.once"; -import type { User as FirebaseUser, UserCredential } from "firebase/auth"; +import type { User as FirebaseUser } from "firebase/auth"; import { browserLocalPersistence, getAuth, @@ -14,67 +14,56 @@ import type { User } from "../webrtc"; const firestoreAuth = { loadUser, login, - redirectLogin, logout, }; export default firestoreAuth; +/** Loads user from redirection result, persistence, whatever. */ async function loadUser(): Promise { - const empty: User = { uid: "", displayName: "", email: "" }; - - return new Promise((resolve) => { - const done = (user: FirebaseUser | null) => { - try { - if (user) { - const { uid, displayName, email } = user; - resolve({ - uid, - displayName: displayName ?? email ?? "", - email: email ?? "?@?", - }); - } else { - resolve(empty); - } - } catch (error) { + return new Promise((resolve, reject) => { + const handleResult = (user: FirebaseUser | null) => { + if (!user) { + const empty: User = { uid: "", displayName: "", email: "" }; resolve(empty); + return; + } + + if (!user.uid || !user.email) { + reject(new Error("Login blocked: unidentified user.")); + return; } + + resolve({ + uid: user.uid, + displayName: user.displayName ?? user.email, + email: user.email, + }); }; - setTimeout(() => done(null), 3000); - getAuth().onAuthStateChanged(once(done)); + // prepare auth for one of the following events: + const auth = getAuth(); + const onceHandleResult = once(handleResult); + // case of timeout (this was specially important for popup method but now not so much) + setTimeout(() => onceHandleResult(null), 5 * 1000); + // case of loaded by something else (like persistent successfully loaded) + auth.onAuthStateChanged(onceHandleResult); + // case of got result from redirect flow + getRedirectResult(auth) + .then((credentials) => credentials?.user || null) + .then(onceHandleResult); }); } -async function redirectLogin(): Promise { +/** Trigger the login function, it redirects to an authentication page. */ +async function login(): Promise { const auth = getAuth(); await setPersistence(auth, browserLocalPersistence); - return signInWithRedirect(auth, googleAuthProvider); -} - -async function login(): Promise { - const empty: User = { uid: "", displayName: "", email: "" }; - const auth = getAuth(); - const result = await getRedirectResult(auth); - - if (result === null) { - Promise.resolve(empty); - } - const { - user: { uid, displayName, email }, - } = result as UserCredential; - if (!email) { - signOut(auth); - throw new Error("Login blocked: unidentified user."); - } - return { - uid, - displayName: displayName ?? email, - email, - }; + await signInWithRedirect(auth, googleAuthProvider); } +/** Clears the authentication data. */ async function logout() { signOut(getAuth()); } diff --git a/src/state/user.ts b/src/state/user.ts index 6693219..c734310 100644 --- a/src/state/user.ts +++ b/src/state/user.ts @@ -64,7 +64,7 @@ export const loadUser = createAsyncThunk( export const login = createAsyncThunk( "user/login", - () => firestoreAuth.login(), + () => firestoreAuth.login().then(firestoreAuth.loadUser), { condition: (_arg, thunkAPI) => (thunkAPI.getState() as RootState).user.status === "idle", diff --git a/src/testing-helpers/mock-firebase-auth.ts b/src/testing-helpers/mock-firebase-auth.ts index d20e792..0c13f19 100644 --- a/src/testing-helpers/mock-firebase-auth.ts +++ b/src/testing-helpers/mock-firebase-auth.ts @@ -1,7 +1,7 @@ import type { Auth, AuthProvider } from "firebase/auth"; jest.mock("firebase/auth", () => ({ - getAuth: jest.fn(), + getAuth: jest.fn().mockReturnValue({ onAuthStateChanged: jest.fn() }), GoogleAuthProvider: jest.fn(() => ({ Qc: ["client_id", "response_type", "scope", "redirect_uri", "state"], providerId: "google.com", @@ -13,8 +13,9 @@ jest.mock("firebase/auth", () => ({ })), setPersistence: jest.fn(), browserLocalPersistence: jest.fn(), + signInWithRedirect: jest.fn(), // eslint-disable-next-line @typescript-eslint/no-unused-vars - signInWithPopup: jest.fn((_: Auth, __: AuthProvider) => + getRedirectResult: jest.fn((_: Auth, __: AuthProvider) => Promise.resolve({ operationType: "signIn", providerId: "google.com",