Skip to content

Conversation

@shanbady
Copy link
Contributor

@shanbady shanbady commented Jul 29, 2024

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/4891

Description (What does it do?)

Adds a "Preferences" page to the dashboard that contains a list of channels a user has subscribed to (from which they can unsubscribe)

Screenshots (if appropriate):

Screenshot 2024-07-29 at 1 59 09 PM Screenshot 2024-07-29 at 1 59 43 PM

How can this be tested?

  1. Checkout this branch
  2. Subscribe to a few channels of different types (unit, department, topic)
  3. go to the settings page by navigating to the dashboard and clicking "settings"
Screenshot 2024-07-29 at 1 59 09 PM
  1. Unsubscribe from topics and see that it works as expected.

Additional context

Functionally the unsubscribe buttons on this preferences page are the same as if you unsubscribed directly from the relevant channel's page. We are just consolidating all the subscriptions in one location here.

@shanbady shanbady marked this pull request as ready for review July 30, 2024 14:23
@shanbady shanbady added Needs Review An open Pull Request that is ready for review and removed Work in Progress labels Jul 30, 2024
@ChristopherChudzicki ChristopherChudzicki self-assigned this Jul 30, 2024
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

This generally works well thought I noticed a couple issues.

Styling: Doesn't quite match figma in terms of spacing and, in a couple places, font sizes. Apologies we don't have a more immediately useable ListItem for you.

Functionality: I noticed some issues around url encoding. Note that we started url encoding search filters for channels in #1326 because new topic names have ampersands. I noticed two things:

  1. On the new subscripts page, channels subscriptions to topics with ampersands aren't compared correctly to the original search. I believe the immediate solution is to NOT unquote_plus the search filter.
  2. Topics with commas in their name can't be subscriped to.
    • this bug exists on main, but imo makes sense to fix here. I believe getSearchParamMap should NOT split on commas.

It seems to me that (1) and (2) above might both be easier if we stored search_filter—both for channels and for subscription queries—as objects rather than encoded strings, but that seems longer term.

@ChristopherChudzicki ChristopherChudzicki added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Jul 30, 2024
shanbady and others added 4 commits July 31, 2024 09:02
Co-authored-by: Chris Chudzicki <christopher.chudzicki@gmail.com>
Co-authored-by: Chris Chudzicki <christopher.chudzicki@gmail.com>
@shanbady
Copy link
Contributor Author

@ChristopherChudzicki this is ready to be looked at again

@shanbady shanbady added Needs Review An open Pull Request that is ready for review and removed Waiting on author labels Jul 31, 2024
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

Works and looks great. Noticed a small issue with borders.

I also think it would be worth adding a simple frontend test, along the lines of

// SettingsPage.test.tsx
import React from "react"
import { SettingsPage } from "./SettingsPage"
import { renderWithProviders } from "@/test-utils"

describe("SettingsPage", () => {
  it("Renders user subscriptions in a list", () => {

  })

  test("Clicking 'Unfollow' removes the subscription", () => {

  })
})

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

👍 LGTM

renderWithProviders(<SettingsPage />)

const followList = await screen.findByTestId("follow-list")
const unsubscribeButton = within(followList).getAllByText("Unfollow")[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

I try to use getByRole* for most things, since it effectively adds an assertion about the role in the accessibility treat. E.g., if we know it's an element with role link, it's almost certainly tab-navigable[^1]

Suggested change
const unsubscribeButton = within(followList).getAllByText("Unfollow")[0]
const unsubscribeButton = within(followList).getAllByRole("link", { name: "Unfollow" })[0]

[^1] I say "almost certainly" because it's probably an anchor tag. If someone did <div role="link" /> (don't!), that would show up as a link in the browser's accessibility treat, but wouldn't be tab navigable.

@shanbady shanbady merged commit e6ef6e8 into main Aug 2, 2024
@shanbady shanbady deleted the subscription-management-page branch August 2, 2024 19:44
@odlbot odlbot mentioned this pull request Aug 5, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review An open Pull Request that is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants