SCC-4236: Username for My Account 2.0 #397

7emansell · 2024-11-20T20:28:54Z

Ticket:

Resolves JIRA ticket SCC-4236.

This PR does the following:

Adds UsernameForm to profile header
Adds api/account/username route to validate new patron usernames
Pulls out the settings status banner into a reusable StatusBanner

NOTE: the deletion process is still up for debate. As it is right now, the user can submit an empty string which works the same as using the trash icon to officially delete

**** Out of scope things that also happened in this PR: ****

Removes "discovery" from src/config (it's a duplicate of "platform")
Updates nyplApiClient to take an api version (defaults to v0.1)
- We need this so we can use this client to hit validations/username in v0.3

How has this been tested?

Accessibility concerns or updates

Checklist:

I updated the CHANGELOG with the appropriate information and JIRA ticket number (if applicable).
I have added relevant accessibility documentation for this pull request.
All new and existing tests passed.

vercel · 2024-11-20T20:28:59Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
research-catalog	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 26, 2024 3:53pm

7emansell · 2024-11-20T20:39:20Z

src/server/nyplApiClient/index.ts

+const nyplApiClient = async ({
+  apiName = "platform",
+  version = "v0.1",
+} = {}) => {


Should this be more specific/different ? this is my first time using this so I'm not super familiar with its use across the repo, this didn't seem to break anything though

What do you mean by specific? Also, can you explain what the = {} in the function signature does?
FWIW this looks fine by me!

@dgcohen maybe you can confirm if this looks kosher? the username validation endpoint requires v0.3, so i suggested emma make the version be a param. at the same time, i thought this was a good opportunity to trim away the duplicated discovery/platform urls in the config.

I don't know actually how it could be more specific, just generally looking for confirmation that this makes sense from people who wrote/used this before

I think it's clear enough when you call the function what version is being used since you pass it as an argument. All tests also continue to pass so it shouldn't break anything.

One possible option to make this clearer (maybe?) is to make this a higher-order function. So it can be something like

const baseNyplApiClient = ({ version = "v0.1", } = {}) => { return async ({ apiName = "platform", } = {}) => { if (CACHE.clients[apiName]) { return CACHE.clients[apiName] } const baseUrl = appConfig.apiEndpoints[apiName][appEnvironment] + "/" + version //... }) }) export const nyplApiClientv01 = baseNyplApiClient() export const nyplApiClientv03 = baseNyplApiClient({ version: "v0.3" })

And then call either function like they're being called now such as const platformClient = await nyplApiClientv03().

Caveat: the syntax might be off and I haven't tested this. If you like this approach, do it in another PR because of its use across the repo. But what do you think, @charmingduchess ?

I like the look of that!
I just noticed that with this updated client factory, we will need to update the cache to include the version in the cache key

charmingduchess · 2024-11-21T21:21:58Z

src/components/MyAccount/NewSettings/StatusBanner.tsx

+export const StatusBanner = ({ status, statusMessage }: StatusBannerProps) => {
+  const bannerContent = (
+    <div style={{ alignItems: "center" }}>
+      {status === "failure" ? (


Can you pull out this conditional logic so it's not in nested ternaries, and illustrates the possibilities more clearly? Something along the lines of...

let statusComponent if (status === 'success'){ statusComponent = ... } if (status === 'failure && statusMessage){ statusComponent = ... else { statusComponent = } etc

potentially also with some descriptive variable names that point to when a statusMessage might be there or not

Lmk if this is better– I do think, since the parent component is responsible for passing the statusMessage, and there's several potential parents and reasons for passing a specific message, any explanation or indication of what that message will be should also live on the parent component if that makes sense

charmingduchess · 2024-11-21T21:32:26Z

src/components/MyAccount/NewSettings/UsernameForm.tsx

+        </Text>
+      </Flex>
+
+      {isLoading ? (


Similar request here. Can you replace this nested ternary with a conditional outside of the return statement so it's a easier to scan which components get rendered when?

Again lmk if this is better/ more readable

Same note here - the extraction is good, but the nested ternary itself is hard to read. Can you replace it?

src/components/MyAccount/ProfileHeader.tsx

charmingduchess · 2024-11-21T21:44:25Z

src/server/nyplApiClient/index.ts

+const nyplApiClient = async ({
+  apiName = "platform",
+  version = "v0.1",
+} = {}) => {


What do you mean by specific? Also, can you explain what the = {} in the function signature does?
FWIW this looks fine by me!

charmingduchess · 2024-11-21T21:47:34Z

src/server/nyplApiClient/index.ts

+const nyplApiClient = async ({
+  apiName = "platform",
+  version = "v0.1",
+} = {}) => {


@dgcohen maybe you can confirm if this looks kosher? the username validation endpoint requires v0.3, so i suggested emma make the version be a param. at the same time, i thought this was a good opportunity to trim away the duplicated discovery/platform urls in the config.

src/components/MyAccount/NewSettings/UsernameForm.test.tsx

charmingduchess · 2024-11-21T21:53:25Z

src/components/MyAccount/NewSettings/NewAccountSettingsTab.tsx

I like this extraction 🌝

src/components/MyAccount/NewSettings/AddButton.tsx

charmingduchess · 2024-11-22T21:39:40Z

src/components/MyAccount/NewSettings/StatusBanner.tsx

-          Your changes were saved.
-        </Text>
-      )}
+      {status === "failure"


This is definitely better, but I'd really like to have this nested ternary replaced. Its ok if you end up with a conditional that seems redundant or repetitive, ie if failure && statusMessage, if failure && !statusMessage and if !failure. the conditional logic doesn't have to be inline of the return statement. in fact, it might have to come out.

Okay ternary gone but I'd like to register my dissent... I think this makes it less readable both here and in the username form

charmingduchess · 2024-11-22T22:05:03Z

src/components/MyAccount/NewSettings/UsernameForm.tsx

+        </Text>
+      </Flex>
+
+      {isLoading ? (


Same note here - the extraction is good, but the nested ternary itself is hard to read. Can you replace it?

EdwinGuzman

NOTE: the deletion process is still up for debate. As it is right now, the user can submit an empty string which works the same as using the trash icon to officially delete

What's the debate? That there are two approaches for the same outcome?

This looks really good. There's the alignment issue in the description property when the username edits and I also left other comments.

EdwinGuzman · 2024-11-22T22:39:35Z

pages/api/account/helpers.ts

+        await client.put(`patrons/${patronId}`, {
+          varFields: [{ fieldTag: "u", content: newUsername }],
+        })


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.

Okay good to note

EdwinGuzman · 2024-11-22T22:40:26Z

pages/api/account/helpers.ts

+        // Username taken but not an error, returns a message.
+        return { status: 200, message: "Username taken" }
+      } else {
+        return { status: 500, message: "Username update failed" }


Why not throw an error here and return the error object on line 67?

src/components/MyAccount/NewSettings/NewAccountSettingsTab.tsx

src/components/MyAccount/NewSettings/StatusBanner.tsx

EdwinGuzman · 2024-11-22T22:47:11Z

src/components/MyAccount/NewSettings/StatusBanner.tsx

+          color: "ui.link.primary !important",
+          textDecorationColor: "ui.link.primary !important",


Two things:

I'm not sure this works, or rather, this should not work. I have not successfully added a design token value with !important. So this should be updated to "var(--nypl-colors-ui-link-primary) !important".

The above point should be resolved if you use the default style. Links in the negative Banner are meant to be red:

Are they blue in the designs?

They are blue in the designs and this does work but i'll change it

src/server/api/bib.ts

EdwinGuzman · 2024-11-22T23:19:47Z

src/server/nyplApiClient/index.ts

+const nyplApiClient = async ({
+  apiName = "platform",
+  version = "v0.1",
+} = {}) => {


I think it's clear enough when you call the function what version is being used since you pass it as an argument. All tests also continue to pass so it shouldn't break anything.

One possible option to make this clearer (maybe?) is to make this a higher-order function. So it can be something like

const baseNyplApiClient = ({ version = "v0.1", } = {}) => { return async ({ apiName = "platform", } = {}) => { if (CACHE.clients[apiName]) { return CACHE.clients[apiName] } const baseUrl = appConfig.apiEndpoints[apiName][appEnvironment] + "/" + version //... }) }) export const nyplApiClientv01 = baseNyplApiClient() export const nyplApiClientv03 = baseNyplApiClient({ version: "v0.3" })

And then call either function like they're being called now such as const platformClient = await nyplApiClientv03().

Caveat: the syntax might be off and I haven't tested this. If you like this approach, do it in another PR because of its use across the repo. But what do you think, @charmingduchess ?

EdwinGuzman · 2024-11-22T23:28:40Z

src/components/MyAccount/ProfileHeader.tsx

-    </List>
+    <>
+      {usernameStatus !== "" && (
+        <div


I think we talked about this in a different PR, check with Clare if the banner message is read. In the Dynamic Banners we say that this wrapper div should always be rendered and have an aria-live attribute. If Clare signs off on this, let me know so we can update the docs.

EdwinGuzman · 2024-11-22T23:40:27Z

src/components/MyAccount/NewSettings/UsernameForm.tsx

+          }}
+          value={tempInput || ""}
+          id="username-input"
+          labelText="Must be 5-15 characters and use only letters (a-z) and numbers (0-9)"


The space between the input and label seems too small, right?

Yes, j added more styling

It's like a mix of the label and helper text on this component I'm not sure how design arrived at this

If there is a default that makes sense functionally (like the helper text) coming from reservoir that deviates from the designs in a small way, can you ask Apoorva if it's alright to use the default instead of overriding?

Okay, I see what's going on. I thought this was the helperErrorText because it was below the input field but you're reversing the order on line 117. I don't understand why this was done.

You can try target the helperErrorText for styling purposes. Can you try having the text value in the helper text prop and invalid text prop? Then you can update the color with the isInvalid prop.

My concern is that a screenreader would read the "Must be 5-15 characters and use only letters (a-z) and numbers (0-9)" text but that's not necessarily clear that it's for the username. So a hidden "username" label, I think, is better. Let's chat about this.

EdwinGuzman · 2024-11-22T23:41:19Z

src/components/MyAccount/NewSettings/UsernameForm.tsx

+            flexDirection: "column-reverse",
+            label: {
+              fontWeight: "400",
+              color: error ? "ui.error.primary" : "ui.black",


This should happen automatically when isInvalid is true. Or is this because the additional !validateUsername check below affects the text color?

This is because the label and the invalid text are the same copy, and render in the same place (below the field). Since that's not how this component expects to work, I'm manually making the label text become the invalid text when needed

Can you use helperText here instead of labelText? It looks like the helper text is automatically with the correct styling for this.

charmingduchess · 2024-11-25T17:35:47Z

src/components/MyAccount/NewSettings/UsernameForm.tsx

+   * Within this form, the user NOT having a username is represented by: username = null, so the
+   * empty string is an invalid username.
+   */
+  const [input, setInput] = useState(


Can you rename this variable to usernameInSierra? It seems like it's not actually being updated as part of the controlled text input.

EdwinGuzman · 2024-11-26T03:11:53Z

pages/api/account/helpers.ts

+    const client = await sierraClient()
+    if (newUsername === "") {
+      const client = await sierraClient()
+      await client.put(`patrons/${patronId}`, {


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

Yes just 200 if successful, no body

EdwinGuzman · 2024-11-26T03:15:56Z

src/components/MyAccount/NewSettings/StatusBanner.tsx

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


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

EdwinGuzman · 2024-11-26T03:16:54Z

src/components/MyAccount/NewSettings/StatusBanner.tsx

+          color: "ui.link.primary !important",
+          textDecorationColor: "var(--nypl-colors-ui-link-primary) !important",


If both works, great, but pick one for consistency, either the design token or the CSS var.

EdwinGuzman · 2024-11-26T03:34:19Z

src/components/MyAccount/NewSettings/UsernameForm.test.tsx

+
+    fireEvent.click(screen.getByRole("button", { name: /save changes/i }))
+
+    await waitFor(() => expect(fetch).toHaveBeenCalledTimes(2))


The second expect is getMostUpdatedSierraAccountData, correct?

EdwinGuzman · 2024-11-26T03:39:34Z

src/components/MyAccount/NewSettings/UsernameForm.tsx

+          }}
+          value={tempInput || ""}
+          id="username-input"
+          labelText="Must be 5-15 characters and use only letters (a-z) and numbers (0-9)"


Okay, I see what's going on. I thought this was the helperErrorText because it was below the input field but you're reversing the order on line 117. I don't understand why this was done.

You can try target the helperErrorText for styling purposes. Can you try having the text value in the helper text prop and invalid text prop? Then you can update the color with the isInvalid prop.

My concern is that a screenreader would read the "Must be 5-15 characters and use only letters (a-z) and numbers (0-9)" text but that's not necessarily clear that it's for the username. So a hidden "username" label, I think, is better. Let's chat about this.

EdwinGuzman · 2024-11-26T03:40:27Z

src/components/MyAccount/NewSettings/UsernameForm.tsx

+  )
+
+  const notEditingView = (
+    <Flex alignItems="center" marginTop={{ base: "unset", md: "-xs" }}>


Did not know or think that -xs would work.

EdwinGuzman · 2024-11-26T03:46:55Z

src/components/MyAccount/NewSettings/UsernameForm.tsx

+  const [isEditing, setIsEditing] = useState(false)
+  const [error, setError] = useState(false)
+  /**
+   * In Sierra, the user NOT having a username is represented by the empty string: username = "".


Thanks for adding this comment here.

I'd like to also see this elsewhere but I don't know where yet...

7emansell added 6 commits November 15, 2024 16:33

Start of username

766b558

Pulling out status banner

7e67c6e

Start of username validation api route

47219eb

Removing discovery links from config and updating nyplApiClient

cd54635

Username form and cleanup in other files

f116cf8

Tests (need more once deletion is confirmed)

a6f7684

7emansell requested review from charmingduchess, EdwinGuzman and dgcohen November 20, 2024 20:36

7emansell commented Nov 20, 2024

View reviewed changes

Clean up

e52407e

vercel bot deployed to Preview November 20, 2024 20:43 View deployment

One more test

673d296

vercel bot deployed to Preview November 20, 2024 20:46 View deployment

Typooo

bc995e2

vercel bot deployed to Preview November 21, 2024 20:54 View deployment

charmingduchess requested changes Nov 21, 2024

View reviewed changes

7emansell added 3 commits November 22, 2024 12:25

Moving UsernameForm into list

f1a87f8

Typo/import clean up

ee979fc

Simplifying ternaries

28e698d

vercel bot deployed to Preview November 22, 2024 17:44 View deployment

7emansell requested a review from charmingduchess November 22, 2024 17:44

charmingduchess reviewed Nov 22, 2024

View reviewed changes

EdwinGuzman reviewed Nov 22, 2024

View reviewed changes

7emansell added 2 commits November 25, 2024 10:03

Removing ternaries and fixing alignment

303cf48

Removing DISCOVERY_API_NAME

d240835

vercel bot deployed to Preview November 25, 2024 15:11 View deployment

Removing more ternary

8ec5985

vercel bot deployed to Preview November 25, 2024 15:25 View deployment

Styling updates

4a5e036

vercel bot deployed to Preview November 25, 2024 15:35 View deployment

Tweaking error handling on helper

d78b943

7emansell requested review from charmingduchess and EdwinGuzman November 25, 2024 15:42

vercel bot deployed to Preview November 25, 2024 15:44 View deployment

7emansell added 2 commits November 25, 2024 11:47

Changing deletion strategy

013d510

Adding deletion test and defining tempInput null meaning

b17b5a1

vercel bot deployed to Preview November 25, 2024 17:14 View deployment

Commenting

6570b9f

vercel bot deployed to Preview November 25, 2024 17:18 View deployment

More clean up

0bcf57d

vercel bot deployed to Preview November 25, 2024 17:19 View deployment

charmingduchess reviewed Nov 25, 2024

View reviewed changes

Variable name updates

bc4f37e

vercel bot deployed to Preview November 25, 2024 17:57 View deployment

7emansell added 2 commits November 25, 2024 13:31

More styling tweaks

63aa14e

styling

f1e65a6

vercel bot deployed to Preview November 25, 2024 18:35 View deployment

EdwinGuzman reviewed Nov 26, 2024

View reviewed changes

7emansell added 2 commits November 26, 2024 10:05

Starting to fix helper error text

2863e93

using ui.link.primary where possible

5e720be

vercel bot deployed to Preview November 26, 2024 15:38 View deployment

7emansell added 2 commits November 26, 2024 10:50

more changes

3b317c3

Tests corrected

e7bf689

vercel bot deployed to Preview November 26, 2024 15:53 View deployment

EdwinGuzman approved these changes Nov 26, 2024

View reviewed changes

7emansell merged commit 865b452 into my-account-2.0 Nov 26, 2024
3 checks passed

		color: "ui.link.primary !important",
		textDecorationColor: "ui.link.primary !important",

		color: "ui.link.primary !important",
		textDecorationColor: "var(--nypl-colors-ui-link-primary) !important",


		fireEvent.click(screen.getByRole("button", { name: /save changes/i }))

		await waitFor(() => expect(fetch).toHaveBeenCalledTimes(2))

SCC-4236: Username for My Account 2.0 #397

SCC-4236: Username for My Account 2.0 #397

Conversation

7emansell commented Nov 20, 2024 • edited Loading

Ticket:

This PR does the following:

How has this been tested?

Accessibility concerns or updates

Checklist:

vercel bot commented Nov 20, 2024 • edited Loading

7emansell Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charmingduchess Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdwinGuzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charmingduchess Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7emansell commented Nov 20, 2024 •

edited

Loading

vercel bot commented Nov 20, 2024 •

edited

Loading

7emansell Nov 20, 2024 •

edited

Loading

charmingduchess Nov 25, 2024 •

edited

Loading

charmingduchess Nov 25, 2024 •

edited

Loading