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

My Account 2.0 #407

Merged
merged 92 commits into from
Nov 27, 2024
Merged

My Account 2.0 #407

merged 92 commits into from
Nov 27, 2024

Conversation

7emansell
Copy link
Contributor

Ticket:

  • Updated phone, email, notification preference and home library to be individually editable in Account Settings (SCC-4337, SCC-4254, SCC-4253)
  • Updated username to be editable in My Account header (SCC-4236)

This PR does the following:

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.

Copy link

vercel bot commented Nov 26, 2024

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 27, 2024 1:00pm

Copy link
Contributor

@charmingduchess charmingduchess left a comment

Choose a reason for hiding this comment

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

A couple small things that are not super crucial, so if you can include those that would be great. But the API client needs to be updated before this can go out.

pages/api/account/username/[id].ts Show resolved Hide resolved
src/components/MyAccount/Settings/PhoneForm.test.tsx Outdated Show resolved Hide resolved
src/components/MyAccount/Settings/PasswordForm.test.tsx Outdated Show resolved Hide resolved
)
})

test("successfully sets patron data if every field is valid", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the correct title for this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What might make more sense as the title for this test?

src/components/MyAccount/Settings/HomeLibraryForm.test.tsx Outdated Show resolved Hide resolved
src/server/nyplApiClient/index.ts Outdated Show resolved Hide resolved
expect(screen.getByRole("button", { name: /edit/i })).toBeInTheDocument()
})

it("renders correctly with when user has no username", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a few more tests here? there are a bunch of different combinations of isEditing/hasUsername/etc that are represented by these ternaries here.
This doesn't have to block QA deployment, but these expectations for these different cases should be tested. It will help with making updates from the QA process.

Copy link
Member

Choose a reason for hiding this comment

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

I agree there should be more tests but they can also be written while the team starts the QA cycle.

Copy link
Member

@EdwinGuzman EdwinGuzman left a comment

Choose a reason for hiding this comment

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

There are updates needed but approving to start the QA process.

expect(screen.getByRole("button", { name: /edit/i })).toBeInTheDocument()
})

it("renders correctly with when user has no username", () => {
Copy link
Member

Choose a reason for hiding this comment

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

I agree there should be more tests but they can also be written while the team starts the QA cycle.

@7emansell 7emansell merged commit 8c4bf84 into main Nov 27, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants