add error message for enrollment code issues

[gumaerc]

What are the relevant tickets?

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

Description (What does it do?)

This PR adds functionality that will display an error message if there was a problem redeeming an enrollment code. This is done by comparing the initial load of the MITx Online user's b2b_organizations property versus after redeeming the enrollment code. If the orgs haven't changed, that's an indication that something went wrong. It is done this way because the API endpoint always returns a 200. This is done so that nefarious actors can't probe the endpoint in an attempt to find a code that works. If the orgs haven't changed, you are redirected to the dashboard home page with a query string arg that triggers the dismissable error at the top of the screen.

Screenshots (if appropriate):

How can this be tested?

• Ensure that you have an instance of MITx Online up and running and configured to use the same Keycloak / APISIX instances as your instance of Learn
• In your instance of MITx Online, make sure you have a B2B organization set up with a contract with some courses added to it
• Generate enrollment codes for said contract, and keep one of them handy
• Spin up MIT Learn, and make sure it is configured to integrate with your instance of MITx Online (more on that in the README)
• Log in as a user that is not part of any B2B orgs
• Visit the enrollment code redemption page with your code from earlier, at /enrollmentcode/<code here>
• You should see a brief message that the code is being redeemed, and then be redirected to the dashboard home and see a tab for the org you just enrolled in
• Verify that no error message is displayed
• Try and redeem the enrollment code again with the same user
• Verify that you are redirected to the dashboard again, but this time you should see the error message indicated in the screenshots

Additional Context

In the Figma design, you may see a different look for the alert banner than what you see during testing. This is because the design for the Alert component in smoot-design has pending style updates that have not been made yet.

[ChristopherChudzicki]

Request: I noticed the mitxonline slack discussion about why you're checking this (API basically never errors, but returns 200 even if the code is invalid). That's not obvious and probably worth mentioning here. If I saw this code, I'd want to change it to check that response status.

From the /attach PR:

The endpoint returns the user's current contract associations, even if the code is not valid, so the API doesn't expose extra information about the discount.

But really: I think we should change the MITxOnline behavior.

• I'm skeptical that this adds any security. As demonstrated here, you can can just check if the original list of contracts has changed. If we need to prevent spamming the API, seems like rate-limiting would be better.
• We currently cannot handle the following situation well: user clicks link to open.odl.local:8062/enrollmentcode/code1 and joins contract1; then user clicks the same link again
  • This PR shows an error to the user
  • The API is a no-op... they're already in the contract.
  • Of course, clicking the link again is unnecessary, but ... people click links.

[ChristopherChudzicki]

cc @jkachel

[gumaerc]

I think you're right to question this. I think even with username / password forms, they return a 3xx / 4xx code depending on the situation, but they just don't tell you whether it was the username or password that was wrong. Like you said and as evidenced here, you can tell if a code worked or not by your b2b_organizations value changing. Returning an error saying you've already redeemed a code on your account seems harmless. If a potential attacker can already determine if they successfully redeemed a code, then retuning a 4xx when an invalid code is submitted seems like it doesn't make things any worse. Furthermore, returning a separate message if you have already redeemed a specific code on your account doesn't seem like it changes that.

[jkachel]

I don't have a problem with changing this to return a 404 or something appropriate if the code is invalid for the user. With further consideration I don't think the current behavior really adds anything, and it complicates things like this error message code. I do think we'd want it to return a 200 for success/extant or 201 for success/created, though, so it's easier to figure out if the code is newly redeemed or not.

[ChristopherChudzicki]

I do think we'd want it to return a 200 for success/extant or 201 for success/created, though,

Good point 👍

[ChristopherChudzicki]

Left some comments about race condition and simplifying things.

I do think it's worth checking with @jkachel about changing the mitxonline api.

[ChristopherChudzicki]

Could we replace hasEnrolled with enrollment.isSuccess?

[ChristopherChudzicki]

There's currently a race condition where if mitxOnlineUser resolves after the mutation, users
erroneously get the error message. (It's easily reproduceable with a timeout on mitxOnlineUser, though I suspect the mutation would generally be slower).

My preference would be to change this to use status of mutation response as discussed below.

If we need to compare orgs (or contracts?) before/after, I'd suggest something more imperative with queryCllient... you'd get to avoid the ref, too:

  React.useEffect(() => {
    const tryToEnroll = async () => {
      if (user?.is_authenticated && !enrollment.isPending) {
        const mitxMe = await queryClient.fetchQuery(mitxUserQueries.me())
        const oldContracts = [
          /* compute from mitxMe.b2b_organizations*/
        ]
        const contracts = (await enrollment.mutateAsync()).data
        const newContracts = [
          /* compute from contracts */
        ]

        // handle redirect
      }

      tryToEnroll()
    }
  }, [queryClient, user?.is_authenticated, enrollment])

[ChristopherChudzicki]

I think a query param is a reasonable way to do this. (If we had a global state store, that would be another option.)

But this behavior:

1. Page redirects with error
2. User seees error
3. user hits reload (or user navigates then presses "back")

seems odd to me. I'm inclined to clear the error on page load. What do you think?

[ChristopherChudzicki]

I think I led you somewhat astray here. As implemented, the URL param gets cleared on load, but...so does the error.

One idea is

const enrollmentError = useRef(searchParams.get(ENROLLMENT_ERROR_QUERY_PARAM))
// then
  React.useEffect(() => {
    if (searchParams.has(ENROLLMENT_ERROR_QUERY_PARAM)) {
      // simpler & safe IMO to read window.location
      // effects only run on the client, so it i will exist, and we don't need to re-run the effect
      // when window.location changes, so safe to access it
      const url = new URL(window.location.href)
      url.searchParams.delete(ENROLLMENT_ERROR_QUERY_PARAM)
      router.replace(url.toString())
    }
  }, [searchParams, router])
// then
enrollmentError.current ? <Alert /> : null

[github-actions]

OpenAPI Changes

Show/hide No detectable change.

Unexpected changes? Ensure your branch is up-to-date with main (consider rebasing).

[gumaerc]

@ChristopherChudzicki This is ready for another look. I merged mitodl/mitxonline#3084 in MITx Online, which added response codes to the attach API endpoint. The enrollment code page now bases its actions on these response codes as described, and I added some functionality that clears the query param after it is seen like you suggested.

[ChristopherChudzicki]

The error display isn't quite working because my suggestion for clearing URL param wasn't quite right. Left another comment, too, about enrollmentcode page

much simpler w/ proper error codes, though

[ChristopherChudzicki]

👍

I still think https://github.com/mitodl/mit-learn/pull/2685/files#r2511242400 is worth changing, though.