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

DAH-2876 feat: allow redirect after sign in #2435

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions app/assets/json/translations/react/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -1579,6 +1579,7 @@
"signIn.fillInFaster": "Fill in applications faster",
"signIn.forgotPassword": "Forgot password?",
"signIn.loggedOutDueToInactivity": "We care about your security. We logged you out due to inactivity. Please sign in to continue.",
"signIn.loginRequired": "You must be signed in to see this page. Sign in to continue.",
"signIn.newAccount.p1": "We sent a link to %{email}.",
"signIn.newAccount.p2": "Follow the link in your email to finish creating your account. It expires after 24 hours.",
"signIn.newAccount.sendEmailAgainButton": "Send email again",
Expand Down
10 changes: 10 additions & 0 deletions app/javascript/__tests__/pages/SignIn.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,16 @@ describe("<SignIn />", () => {
})
})

it("shows the sign in required banner", async () => {
window.sessionStorage.setItem("alert_message_secondary", t("signIn.loginRequired"))

render(<SiteAlert type="secondary" />)
window.sessionStorage.setItem("redirect", "account")

await renderAndLoadAsync(<SignIn assetPaths={{}} />)
expect(screen.getByText(t("signIn.loginRequired"))).not.toBeNull()
})

it("shows the timeout limit banner", async () => {
const mockGetItem = jest.fn()
const mockSetItem = jest.fn()
Expand Down
4 changes: 2 additions & 2 deletions app/javascript/authentication/SignInForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
import { Link, Heading } from "@bloom-housing/ui-seeds"
import { FieldValues, SubmitHandler, useForm } from "react-hook-form"

import { getForgotPasswordPath, getMyAccountPath } from "../util/routeUtil"
import { getSignInRedirectUrl, getForgotPasswordPath } from "../util/routeUtil"
import EmailFieldset from "../pages/account/components/EmailFieldset"
import PasswordFieldset from "../pages/account/components/PasswordFieldset"
import "../pages/account/styles/account.scss"
Expand Down Expand Up @@ -130,7 +130,7 @@ const SignInForm = () => {

signIn(email, password)
.then(() => {
window.location.href = getMyAccountPath()
window.location.href = getSignInRedirectUrl()
window.scrollTo(0, 0)
})
.catch((error: AxiosError<{ error: string; email: string }>) => {
Expand Down
4 changes: 3 additions & 1 deletion app/javascript/pages/account/account-settings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import { FormHeader, FormSection, getDobStringFromDobObject } from "../../util/a
import { AxiosError } from "axios"
import { ErrorSummaryBanner } from "./components/ErrorSummaryBanner"
import { ExpandedAccountAxiosError, getErrorMessage } from "./components/util"
import { setSiteAlertMessage } from "../../components/SiteAlert"

const Banner = ({
showBanner,
Expand Down Expand Up @@ -456,7 +457,8 @@ const AccountSettingsPage = () => {
const { profile, loading, initialStateLoaded } = React.useContext(UserContext)

if (!profile && !loading && initialStateLoaded) {
// TODO: Redirect to React sign in page and show a message that user needs to sign in
window.sessionStorage.setItem("redirect", "settings")
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought: We're using sessionStorage across various files, and it's not immediately obvious what arguments for setItem mean. We should look into consolidating all routing-related sessionStorage usage into a single file, probably in routeUtil.ts.

setSiteAlertMessage(t("signIn.loginRequired"), "secondary")
window.location.href = getSignInPath()
return null
}
Expand Down
4 changes: 3 additions & 1 deletion app/javascript/pages/account/my-account.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
getSignInPath,
} from "../../util/routeUtil"
import { renderInlineMarkup } from "../../util/languageUtil"
import { setSiteAlertMessage } from "../../components/SiteAlert"

interface MyAccountProps {
assetPaths: unknown
Expand Down Expand Up @@ -62,7 +63,8 @@ const MyAccount = (_props: MyAccountProps) => {
const { profile, loading, initialStateLoaded } = React.useContext(UserContext)

if (!profile && !loading && initialStateLoaded) {
// TODO: Redirect to React sign in page and show a message that user needs to sign in
window.sessionStorage.setItem("redirect", "account")
setSiteAlertMessage(t("signIn.loginRequired"), "secondary")
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought: We're well along this path now, but in the future we should look into using constants instead of strings for things like "redirect", "account", and "secondary".

window.location.href = getSignInPath()
return null
}
Expand Down
3 changes: 3 additions & 0 deletions app/javascript/pages/account/my-applications.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import "./styles/my-applications.scss"
import { DoubleSubmittedModal } from "./components/DoubleSubmittedModal"
import { AlreadySubmittedModal } from "./components/AlreadySubmittedModal"
import { extractModalParamsFromUrl } from "./components/util"
import { setSiteAlertMessage } from "../../components/SiteAlert"

export const noApplications = () => {
return (
Expand Down Expand Up @@ -198,6 +199,8 @@ const MyApplications = () => {
}, [authLoading, initialStateLoaded, profile])

if (!profile && !authLoading && initialStateLoaded) {
window.sessionStorage.setItem("redirect", "applications")
setSiteAlertMessage(t("signIn.loginRequired"), "secondary")
Copy link
Collaborator

@jimlin-sfgov jimlin-sfgov Dec 11, 2024

Choose a reason for hiding this comment

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

issue: I think something here needs to be updated to address the following issue: the "You must be signed in..." banner shows up when I click the sign out button
Screenshot 2024-12-11 at 10 35 31 AM

Copy link
Contributor

@tallulahkay tallulahkay Dec 12, 2024

Choose a reason for hiding this comment

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

(I think) This has been fixed in main and the fix just hasn't been pulled into this branch yet

window.location.href = getSignInPath()
return null
}
Expand Down
18 changes: 18 additions & 0 deletions app/javascript/util/routeUtil.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,3 +75,21 @@ export const getEligibilityEstimatorLink = localizedPathGetter("/eligibility-est
// Footer
export const getDisclaimerPath = localizedPathGetter("/disclaimer")
export const getPrivacyPolicyPath = localizedPathGetter("/privacy")

const SignInRedirects = {
account: getMyAccountPath(),
applications: getMyApplicationsPath(),
settings: getMyAccountSettingsPath(),
home: getHomepagePath(),
}

const getRedirectUrl = (key: string): string => {
return SignInRedirects[key] || SignInRedirects.home
}

export const getSignInRedirectUrl = () => {
const redirect = window.sessionStorage.getItem("redirect")
window.sessionStorage.removeItem("redirect")

return getRedirectUrl(redirect || "account")
}
Loading