dashboard unenroll dialog functionality #2303
Conversation
gumaerc
force-pushed
the
cg/dashboard-unenroll-dialog
branch
from
f2031ef to
0400775
Compare
There was a problem hiding this comment.
Thanks for all the help getting this integrated with mitxonline. It's looking good though I did notice a few things:
Dialog not closing, stale data
Noticed an issue with the modal not closing after deleting enrollments. See video—also left a comment about this.
unenroll_issue.mov
CSRFTokens
Currently the axios instance for Learn API endpoints and MITxOnline endpoints both use NEXT_PUBLIC_CSRF_COOKIE_NAME for the csrf cookie name.
In practice, the cookie names are different... Looks like in learn it's learn_rc_csrftoken.
I'm not 100% sure why the csrftoken name was customized; Asked about it in slack here https://mitodl.slack.com/archives/C03K5HYGPT9/p1749760933111239
Probably we need separate env vars for the two csrftoken names, though.
Or we can hardcode the mitxonline one... the csrftoken in mitxonline is hardcoded to csrftoken anyway.
There was a problem hiding this comment.
Should put the mutations here or remove the file.
| mutate() | ||
| if (isSuccess) { | ||
| queryClient.invalidateQueries({ | ||
| queryKey: enrollmentKeys.enrollmentsList(), | ||
| }) | ||
| modal.hide() | ||
| } |
There was a problem hiding this comment.
Request: Change destroyEnrollment to a custom hook useDestroyEnrollment and define the invalidation there.
I think I've mentioned in the past that queryOptions removes, IMO, the need for custom query hooks. But custom mutation hooks still seem very useful. (See TanStack/query#6096 (comment) for why there is no mutationOptions helper; TkDodo is one of the maintainers.)
Note: The code as written does not work, but
await mutateAsync()
queryClient.invalidateQueries({
queryKey: enrollmentKeys.enrollmentsList(),
})
modal.hide()probably would.
mutatedoesn't wait for the mutation to finish. It fires the mutation (here, an API request) and returns immediately. And sinceisSuccesswas defined above BEFORE the mutation fired, it isfalse(no mutation run yet) when the click handler runs. And the click handler isn't going to re-run when the api request finishes.- With
mutateAsync, you don't need theif (success) ...since it'll throw an error if there wasn't a success.
Suggestion: Maybe also worth rendering an alert if an error occurs.
|
@ChristopherChudzicki This is ready for another look, thanks. The only thing I left out for now is any kind of alerting, success or not. It looks like we aren't using a |
ManageListDialog does Something similar would probably be good here. 
Re toasts / snackbar: I'd want to check with @steven-hatch / @mbilalmughal re toasts...
IMO it's OK (and very desirable) to just add something simple like above w/o designs...It's using our smoot component and 99.9% of real users will never see it. (I do expect devs might see it more often, especially with something like this feature where it requires the mitxonline integration) |
gumaerc
force-pushed
the
cg/dashboard-unenroll-dialog
branch
from
64f892c to
d1153c6
Compare
There was a problem hiding this comment.
Going to finish looking over this on the bus in the morning, but left a few comments.
Additionally: Not really a change for this PR, but what we discussed in Slack: I think we need to change MITxOnline so that we can set CSRF_COOKIE_DOMAIN:
- Django won't automatically read
CSRF_COOKIE_DOMAINfrom the env. - In order for the unenrollment requests to work, I manually added
CSRF_COOKIE_DOMAIN = ".odl.local"to MITxOnline.
Without CSRF_COOKIE_DOMAIN = ".odl.local" (or the appropriate env-dependent setting), the frontend JS can't read the CSRF cookie. And, as ChatGPT reminded me:
CSRF Protection in Django (DRF): Cross-Origin
DELETERequests✅ What Django Requires for CSRF Protection
To satisfy Django’s CSRF protection on unsafe HTTP methods (like
DELETE,POST,PUT,PATCH), your frontend must provide both of the following:
- ✅ A
csrftokencookie sent with the request.- ✅ The CSRF token value in the
X-CSRFTokenrequest header.
❌ Common Misconception
“I only need to send
X-CSRFToken, not the cookie.”That’s not correct. Django requires the
csrftokencookie to be present because it validates that the header's value matches the cookie’s value.
env/frontend.env
Outdated
| @@ -6,6 +6,7 @@ SENTRY_ENV=dev # Re-enable sentry | |||
| NEXT_PUBLIC_ORIGIN=${MITOL_APP_BASE_URL} | |||
| NEXT_PUBLIC_MITOL_API_BASE_URL=${MITOL_API_BASE_URL} | |||
| NEXT_PUBLIC_CSRF_COOKIE_NAME=${CSRF_COOKIE_NAME} | |||
| NEXT_PUBLIC_MITXONLINE_CSRF_COOKIE_NAME=${MITXONLINE_CSRF_COOKIE_NAME} | |||
There was a problem hiding this comment.
A lot of the frontend settings are set up this way (NEXT_PUBLIC_XYZ=${XYZ}) because they are used on the backend, too, and we want them to be the same.
That's not the case for NEXT_PUBLIC_MITXONLINE_CSRF_COOKIE_NAME... I'd default it to csrftoken since that is what mitxonline uses. Then if someone wants to override it, they can set NEXT_PUBLIC_MITXONLINE_CSRF_COOKIE_NAME in frontend.local.env.
There was a problem hiding this comment.
I think you can just delete this file now that the hook exists...the hook does enrollmentsApi.enrollmentsDestroy directly
| setMockResponse.delete( | ||
| mitxonline.urls.enrollment.courseEnrollment(enrollment.id), | ||
| () => { | ||
| deleteCalls++ |
There was a problem hiding this comment.
Suggestion: instead of this, you could do const handleDelete = jest.fn(), and check expect(handleDelete).toHaveBeenCalledTimes(1).
Alternatively: I thought it was odd that we set up the mock response for every card, but only delete one of them.
I was curious what this would look like if we only set up the mock for a randomly chosen card. Here's what I came up with, which I think is also a good alternative:
index 228081f15..5db053086 100644
--- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardDialogs.test.tsx
+++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardDialogs.test.tsx
@@ -10,6 +10,9 @@ import { EnrollmentDisplay } from "./EnrollmentDisplay"
import * as mitxonline from "api/mitxonline-test-utils"
import { useFeatureFlagEnabled } from "posthog-js/react"
import { setupEnrollments } from "./test-utils"
+import { faker } from "@faker-js/faker/locale/en"
+import { mockAxiosInstance } from "api/test-utils"
+import invariant from "tiny-invariant"
jest.mock("posthog-js/react")
const mockedUseFeatureFlagEnabled = jest
@@ -30,27 +33,26 @@ describe("DashboardDialogs", () => {
return { enrollments, completed, expired, started, notStarted }
}
- test("Opening the unenroll dialog and confirming the unenroll fires the proper API call", async () => {
+ test.only("Opening the unenroll dialog and confirming the unenroll fires the proper API call", async () => {
const { enrollments } = setupApis()
- let deleteCalls = 0
- for (const enrollment of enrollments) {
- setMockResponse.delete(
- mitxonline.urls.enrollment.courseEnrollment(enrollment.id),
- () => {
- deleteCalls++
- },
- )
- }
+ const enrollment = faker.helpers.arrayElement(enrollments)
+
+ setMockResponse.delete(
+ mitxonline.urls.enrollment.courseEnrollment(enrollment.id),
+ null,
+ )
renderWithProviders(<EnrollmentDisplay />)
await screen.findByRole("heading", { name: "My Learning" })
const cards = await screen.findAllByTestId("enrollment-card-desktop")
expect(cards.length).toBe(enrollments.length)
-
- const contextMenuButton = await within(cards[0]).findByLabelText(
- "More options",
+ const card = cards.find(
+ (c) => !!within(c).queryByText(enrollment.run.title),
)
+ invariant(card)
+
+ const contextMenuButton = await within(card).findByLabelText("More options")
await user.click(contextMenuButton)
const unenrollButton = await screen.findByRole("menuitem", {
@@ -65,6 +67,11 @@ describe("DashboardDialogs", () => {
await user.click(confirmButton)
- expect(deleteCalls).toBe(1)
+ expect(mockAxiosInstance.request).toHaveBeenCalledWith(
+ expect.objectContaining({
+ method: "DELETE",
+ url: mitxonline.urls.enrollment.courseEnrollment(enrollment.id),
+ }),
+ )
})
})It's a bit awkward: Testing library doesn't have a great way to combine selectors like "Find an element with test ID XYZ that also satifises this other property (namely, has text...)". Playwright has a much more sophisticated (but natural-seeming) way to combine selectors like that. Oh well.
|
@ChristopherChudzicki Thanks, I implemented your suggestions. |
…t id, and add a new test for the unenroll dialog
gumaerc
force-pushed
the
cg/dashboard-unenroll-dialog
branch
from
ea5648d to
782dd65
Compare
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/7543
Description (What does it do?)
This PR wires up the unenroll button in the unenroll dialog to actually call the
mitxonlineAPI and perform unenrollments.How can this be tested?
In order to test this, you need a basic installation of
mitxonlineup and running with example data in it. You may be able to skip one or more steps if you have already done them:hostsredirects for the following domains, replacing the example IP with your local IP address (Google how to get this if unsure, mine is 192.168.1.50)mitxonline: https://github.com/mitodl/mitxonline.envfile with the following values (note that we are intentionally misconfiguring the cookie domain):mitxonlinewithdocker compose up --build -ddocker compose exec web ./manage.py promote_user --promote --superuser [email protected]docker compose exec web ./manage.py populate_course_datapants docs ::dist/sphinx/index.html, read the section on generating a B2B organization / contract and create one, adding some of the test courses to itProgramand add some courses to the program that are included in your B2B org, making sure to mark the program as "live"mit-learn, we need to set some env variables (note that we are intentionally misconfiguring the cookie domain):enrollment-dashboardandmitlearn-organization-dashboardfeature flags for all usersMIT Learn[email protected]test user