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

SCC-4236: Username for My Account 2.0 #397

Merged
merged 28 commits into from
Nov 26, 2024
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
766b558
Start of username
7emansell Nov 15, 2024
7e67c6e
Pulling out status banner
7emansell Nov 19, 2024
47219eb
Start of username validation api route
7emansell Nov 19, 2024
cd54635
Removing discovery links from config and updating nyplApiClient
7emansell Nov 19, 2024
f116cf8
Username form and cleanup in other files
7emansell Nov 19, 2024
a6f7684
Tests (need more once deletion is confirmed)
7emansell Nov 20, 2024
e52407e
Clean up
7emansell Nov 20, 2024
673d296
One more test
7emansell Nov 20, 2024
bc995e2
Typooo
7emansell Nov 21, 2024
f1a87f8
Moving UsernameForm into list
7emansell Nov 22, 2024
ee979fc
Typo/import clean up
7emansell Nov 22, 2024
28e698d
Simplifying ternaries
7emansell Nov 22, 2024
303cf48
Removing ternaries and fixing alignment
7emansell Nov 25, 2024
d240835
Removing DISCOVERY_API_NAME
7emansell Nov 25, 2024
8ec5985
Removing more ternary
7emansell Nov 25, 2024
4a5e036
Styling updates
7emansell Nov 25, 2024
d78b943
Tweaking error handling on helper
7emansell Nov 25, 2024
013d510
Changing deletion strategy
7emansell Nov 25, 2024
b17b5a1
Adding deletion test and defining tempInput null meaning
7emansell Nov 25, 2024
6570b9f
Commenting
7emansell Nov 25, 2024
0bcf57d
More clean up
7emansell Nov 25, 2024
bc4f37e
Variable name updates
7emansell Nov 25, 2024
63aa14e
More styling tweaks
7emansell Nov 25, 2024
f1e65a6
styling
7emansell Nov 25, 2024
2863e93
Starting to fix helper error text
7emansell Nov 26, 2024
5e720be
using ui.link.primary where possible
7emansell Nov 26, 2024
3b317c3
more changes
7emansell Nov 26, 2024
e7bf689
Tests corrected
7emansell Nov 26, 2024
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
16 changes: 0 additions & 16 deletions __test__/pages/account/account.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -231,22 +231,6 @@ describe("MyAccount page", () => {
const result = await getServerSideProps({ req: req, res: mockRes })
expect(result.props.tabsPath).toBe("overdues")
})
it("can handle no username", () => {
render(
<MyAccount
isAuthenticated={true}
accountData={{
checkouts: processedCheckouts,
holds: processedHolds,
patron: { ...processedPatron, username: undefined },
fines: processedFines,
pickupLocations: filteredPickupLocations,
}}
/>
)
const username = screen.queryByText("Username")
expect(username).toBeNull()
})
it("renders notification banner if user has fines", () => {
render(
<MyAccount
Expand Down
47 changes: 47 additions & 0 deletions pages/api/account/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import sierraClient from "../../../src/server/sierraClient"
import type { HTTPResponse } from "../../../src/types/appTypes"
import nyplApiClient from "../../../src/server/nyplApiClient"

/**
* PUT request to Sierra to update patron PIN, first validating with previous PIN.
Expand Down Expand Up @@ -27,6 +28,52 @@ export async function updatePin(
}
}

/**
* PUT request to Sierra to update patron username, first validating that it's available.
* Returns status and message about request.
*/
export async function updateUsername(
patronId: string,
newUsername: string
): Promise<HTTPResponse> {
try {
// If the new username is an empty string, skips validation and directly updates in Sierra.
const client = await sierraClient()
if (newUsername === "") {
const client = await sierraClient()
await client.put(`patrons/${patronId}`, {
Copy link
Member

Choose a reason for hiding this comment

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

Does this return a response to verify it was successful? Or is it a quiet "there was no error so it's good" validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes just 200 if successful, no body

varFields: [{ fieldTag: "u", content: newUsername }],
})
return { status: 200, message: "Username removed successfully" }
} else {
const platformClient = await nyplApiClient({ version: "v0.3" })
const response = await platformClient.post("/validations/username", {
username: newUsername,
})

if (response?.type === "available-username") {
await client.put(`patrons/${patronId}`, {
varFields: [{ fieldTag: "u", content: newUsername }],
})
Comment on lines +55 to +57
Copy link
Member

Choose a reason for hiding this comment

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

This is a general problem but I don't think it's a problem we'll encounter. This risks having race conditions if the username is available and then a separate request is made to update the patron with that username. If another patron is in the process of creating an account or updating their own username to the same one, then we'll have this race condition and error. This will be caught regardless on line 66 and also, there's really not much we can do anyway.
What you have here is fine, but wanted to point it out as possible error as an FYI in case it ever does come up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay good to note

return { status: 200, message: `Username updated to ${newUsername}` }
} else if (response?.type === "unavailable-username") {
// Username taken but not an error, returns a message.
return { status: 200, message: "Username taken" }
} else {
throw new Error("Username update failed")
}
}
} catch (error) {
return {
status: error?.status || 500,
message:
error?.message ||
error.response?.data?.description ||
"An error occurred",
}
}
}

/**
* PUT request to Sierra to update patron settings. Returns status and message about request.
*/
Expand Down
40 changes: 40 additions & 0 deletions pages/api/account/username/[id].ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import type { NextApiResponse, NextApiRequest } from "next"
import initializePatronTokenAuth from "../../../../src/server/auth"
import { updateUsername } from "../helpers"

/**
* API route handler for /api/account/username/{patronId}
*/
export default async function handler(
req: NextApiRequest,
res: NextApiResponse
) {
let responseMessage = "Request error"
let responseStatus = 400
const patronTokenResponse = await initializePatronTokenAuth(req.cookies)
const cookiePatronId = patronTokenResponse.decodedPatron?.sub
if (!cookiePatronId) {
responseStatus = 403
responseMessage = "No authenticated patron"
return res.status(responseStatus).json(responseMessage)
}
if (req.method == "GET") {
responseMessage = "Please make a PUT request to this endpoint."
}
if (req.method == "PUT") {
/** We get the patron id from the request: */
const patronId = req.query.id as string
const { username } = req.body
/** We check that the patron cookie matches the patron id in the request,
* i.e.,the logged in user is updating their own username. */
if (patronId == cookiePatronId) {
const response = await updateUsername(patronId, username)
responseStatus = response.status
responseMessage = response.message
} else {
responseStatus = 403
responseMessage = "Authenticated patron does not match request"
}
}
res.status(responseStatus).json(responseMessage)
}
2 changes: 1 addition & 1 deletion src/components/MyAccount/IconListElement.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export interface IconListElementPropType {

// This component is designed to centralize common styling patterns for a
// description type List with icons
const IconListElement = ({
export const IconListElement = ({
icon,
term,
description,
Expand Down
6 changes: 3 additions & 3 deletions src/components/MyAccount/NewSettings/AddButton.tsx
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
import { Button } from "@nypl/design-system-react-components"

type AddButtonProps = {
inputType: string
inputType?: string
label: string
onClick: () => void
}

const AddButton = ({ inputType, label, onClick }: AddButtonProps) => {
return (
<Button
id={`add-${inputType}-button`}
id={inputType ? `add-${inputType}-button` : "add-button"}
7emansell marked this conversation as resolved.
Show resolved Hide resolved
buttonType="text"
onClick={onClick}
size="large"
sx={{
justifyContent: "flex-start",
width: { base: "87%", md: "300px" },
width: { base: "100%", md: "300px" },
paddingLeft: { base: "m", md: "unset" },
paddingTop: "xs",
paddingBottom: "xs",
Expand Down
41 changes: 3 additions & 38 deletions src/components/MyAccount/NewSettings/NewAccountSettingsTab.tsx
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this extraction 🌝

Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { Flex, Banner, Link, Text } from "@nypl/design-system-react-components"
import { Flex } from "@nypl/design-system-react-components"
import { useContext, useEffect, useRef, useState } from "react"
import { PatronDataContext } from "../../../context/PatronDataContext"
import SettingsInputForm from "./SettingsInputForm"
import SettingsSelectForm from "./SettingsSelectForm"
import PasswordForm from "./PasswordForm"
import { StatusBanner } from "./StatusBanner"

type StatusType = "" | "failure" | "success"

Expand Down Expand Up @@ -33,47 +34,11 @@ const NewAccountSettingsTab = () => {
}
}, [status])

const bannerContent = (
<div style={{ alignItems: "center" }}>
{status === "failure" ? (
statusMessage !== "" ? (
<Text marginBottom={0} color={"ui.black !important"}>
{statusMessage} Please try again or{" "}
<Link
sx={{
color: "ui.link.primary !important",
textDecorationColor: "ui.link.primary !important",
textDecoration: "underline",
}}
href="https://www.nypl.org/get-help/contact-us"
>
contact us
</Link>{" "}
for assistance.
</Text>
) : (
<Text marginBottom={0} color={"ui.black !important"}>
Your changes were not saved.
</Text>
)
) : (
<Text marginBottom={0} color={"ui.black !important"}>
Your changes were saved.
</Text>
)}
</div>
)

return (
<>
{status !== "" && (
<div ref={bannerRef} tabIndex={-1}>
<Banner
sx={{ marginTop: "m" }}
isDismissible
content={bannerContent}
type={status === "failure" ? "negative" : "positive"}
/>
<StatusBanner status={status} statusMessage={statusMessage} />
7emansell marked this conversation as resolved.
Show resolved Hide resolved
</div>
)}
<Flex flexDir="column" sx={{ marginTop: "xl", gap: "s" }}>
Expand Down
56 changes: 56 additions & 0 deletions src/components/MyAccount/NewSettings/StatusBanner.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { Banner, Text, Link } from "@nypl/design-system-react-components"

export type StatusType = "" | "failure" | "usernameFailure" | "success"

type StatusBannerProps = {
status: StatusType
statusMessage: string
}

const successContent = (
<Text marginBottom={0} color="ui.black !important">
Copy link
Member

Choose a reason for hiding this comment

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

Just to verify, the design token with the !important flag does work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Your changes were saved.
</Text>
)

const generalFailureContent = (
<Text marginBottom={0} color="ui.black !important">
Your changes were not saved.
</Text>
)

const specificFailureContent = (statusMessage: string) => {
return (
<Text marginBottom={0} color={"ui.black !important"}>
{statusMessage} Please try again or{" "}
<Link href="https://www.nypl.org/get-help/contact-us">contact us</Link>{" "}
for assistance.
</Text>
)
}

const statusContent = (status, statusMessage) => {
if (status === "success") {
return successContent
}
if (status === "failure" && statusMessage !== "") {
return specificFailureContent(statusMessage)
} else {
return generalFailureContent
}
}

export const StatusBanner = ({ status, statusMessage }: StatusBannerProps) => {
return (
<Banner
sx={{ marginTop: "m" }}
isDismissible
content={
<div style={{ alignItems: "center" }}>
{statusContent(status, statusMessage)}
</div>
}
type={status === "failure" ? "negative" : "positive"}
/>
)
}
Loading