Redirect to dashboard home after login, usually (#2600)

* redirect to dashboard on most logins

* encode pathname, too; remove default signup url

* update customloginview

* make tests less fragile to env settings

* fix ts error

* rename loginNext to next

* check user auth before enrolling

Author: ChristopherChudzicki

authentication/views.py

@@ -1,15 +1,13 @@
 """Authentication views"""
 
 import logging
-from urllib.parse import urljoin
 
+from django.conf import settings
 from django.contrib.auth import logout
 from django.shortcuts import redirect
 from django.utils.http import url_has_allowed_host_and_scheme, urlencode
-from django.utils.text import slugify
 from django.views import View
 
-from main import settings
 from main.middleware.apisix_user import ApisixUserMiddleware, decode_apisix_headers
 
 log = logging.getLogger(__name__)
@@ -80,6 +78,7 @@ def get(
         """
         redirect_url = get_redirect_url(request, ["next"])
         signup_redirect_url = get_redirect_url(request, ["signup_next", "next"])
+        should_skip_onboarding = request.GET.get("skip_onboarding", "0") != "0"
         if not request.user.is_anonymous:
             profile = request.user.profile
 
@@ -91,31 +90,16 @@ def get(
             )
 
             if user_organizations:
-                # First-time login for org user: redirect to org dashboard
-                if not profile.has_logged_in:
-                    first_org_name = next(iter(user_organizations.keys()))
-                    org_slug = slugify(first_org_name)
-
-                    log.info(
-                        "User %s belongs to organization: %s (slug: %s)",
-                        request.user.email,
-                        first_org_name,
-                        org_slug,
-                    )
-
-                    redirect_url = urljoin(
-                        settings.APP_BASE_URL, f"/dashboard/organization/{org_slug}"
-                    )
-            # first-time non-org users
-            elif not profile.has_logged_in:
-                if request.GET.get("skip_onboarding", "0") == "0":
+                should_skip_onboarding = True
+
+            if not profile.has_logged_in:
+                if should_skip_onboarding:
+                    redirect_url = signup_redirect_url
+                else:
                     params = urlencode({"next": signup_redirect_url})
                     redirect_url = f"{settings.MITOL_NEW_USER_LOGIN_URL}?{params}"
                     profile.save()
-                else:
-                    redirect_url = signup_redirect_url
 
-            if not profile.has_logged_in:
                 profile.has_logged_in = True
                 profile.save()
 

authentication/views_test.py

@@ -2,6 +2,7 @@
 
 import json
 from base64 import b64encode
+from typing import NamedTuple
 from unittest.mock import MagicMock
 from urllib.parse import urljoin
 
@@ -29,7 +30,7 @@
         (["allowed-2"], "https://good.com/url-2"),
     ],
 )
-def test_get_redirect_url(mocker, param_names, expected_redirect):
+def test_get_redirect_url(mocker, param_names, expected_redirect, settings):
     """Next url should be respected if host is allowed"""
     GET = {
         "exists-a": "/url-a",
@@ -38,10 +39,7 @@ def test_get_redirect_url(mocker, param_names, expected_redirect):
         "disallowed-a": "https://malicious.com/url-1",
         "allowed-2": "https://good.com/url-2",
     }
-    mocker.patch(
-        "authentication.views.settings.ALLOWED_REDIRECT_HOSTS",
-        ["good.com"],
-    )
+    settings.ALLOWED_REDIRECT_HOSTS = ["good.com"]
 
     mock_request = mocker.MagicMock(GET=GET)
     assert get_redirect_url(mock_request, param_names) == expected_redirect
@@ -50,6 +48,7 @@ def test_get_redirect_url(mocker, param_names, expected_redirect):
 @pytest.mark.parametrize(
     "test_params",
     [
+        # has_apisix_header, next_url
         (True, "/search"),
         (True, None),
         (False, "/search"),
@@ -129,8 +128,9 @@ def test_next_logout(mocker, client, user, test_params, settings):
 
 @pytest.mark.parametrize("is_authenticated", [True, False])
 @pytest.mark.parametrize("has_next", [True, False])
-def test_custom_logout_view(mocker, client, user, is_authenticated, has_next):
+def test_custom_logout_view(mocker, client, user, is_authenticated, has_next, settings):  # noqa: PLR0913
     """Test logout redirect"""
+    settings.ALLOWED_REDIRECT_HOSTS = ["ocw.mit.edu"]
     next_url = "https://ocw.mit.edu" if has_next else ""
     mock_request = mocker.MagicMock(user=user, META={})
     if is_authenticated:
@@ -245,23 +245,43 @@ def test_custom_login_view_first_time_login_sets_has_logged_in(mocker):
     mock_profile.save.assert_called_once()
 
 
+class LoginOrgUserRedirectParams(NamedTuple):
+    """Parameters for testing org user login redirect behavior"""
+
+    has_logged_in: bool
+    login_url: str
+    expected_redirect: str
+
+
 @pytest.mark.parametrize(
-    "test_case",
+    "params",
     [
-        (
-            False,
-            "/dashboard/organization/test-organization",
-        ),  # First-time login → org dashboard
-        (
-            True,
-            "/app",
-        ),  # Subsequent login → normal app
+        LoginOrgUserRedirectParams(
+            has_logged_in=False,
+            login_url="/login/?next=/dashboard",
+            expected_redirect="/dashboard",
+        ),
+        LoginOrgUserRedirectParams(
+            has_logged_in=False,
+            login_url="/login/?next=/dashboard&signup_next=/somewhere-else",
+            expected_redirect="/somewhere-else",
+        ),
+        LoginOrgUserRedirectParams(
+            has_logged_in=True,
+            login_url="/login/?next=/dashboard&signup_next=/somewhere-else",
+            expected_redirect="/dashboard",
+        ),
     ],
 )
-def test_login_org_user_redirect(mocker, client, user, test_case, settings):
+def test_login_org_user_redirect(
+    mocker,
+    client,
+    user,
+    params,
+    settings,
+):
     """Test organization user redirect behavior - org users skip onboarding regardless of login history"""
-    # Unpack test case
-    has_logged_in, expected_url = test_case
+    has_logged_in, login_url, expected_redirect = params
 
     # Set up user profile based on test scenario
     user.profile.has_logged_in = has_logged_in
@@ -284,15 +304,18 @@ def test_login_org_user_redirect(mocker, client, user, test_case, settings):
     )
     client.force_login(user)
     response = client.get(
-        "/login/",
+        login_url,
         follow=False,
         HTTP_X_USERINFO=header_str,
     )
     assert response.status_code == 302
     # Handle environment differences - in some envs it returns full URL, in others just path
-    expected_full_url = urljoin(settings.APP_BASE_URL, expected_url)
-    assert response.url in [expected_url, expected_full_url]
+    expected_full_redirect = urljoin(settings.APP_BASE_URL, expected_redirect)
+    assert response.url in [expected_redirect, expected_full_redirect]
 
     # Verify that org users are never sent to onboarding
     # (onboarding URL would contain settings.MITOL_NEW_USER_LOGIN_URL)
     assert settings.MITOL_NEW_USER_LOGIN_URL not in response.url
+
+    user.profile.refresh_from_db()
+    assert user.profile.has_logged_in is True

frontends/main/src/app-pages/EnrollmentCodePage/EnrollmentCodePage.test.tsx

@@ -1,15 +1,10 @@
 import React from "react"
 import { renderWithProviders, setMockResponse, waitFor } from "@/test-utils"
-import { urls } from "api/test-utils"
-import {
-  urls as b2bUrls,
-  factories as mitxOnlineFactories,
-  urls as mitxOnlineUrls,
-} from "api/mitxonline-test-utils"
+import { makeRequest, urls } from "api/test-utils"
+import { urls as b2bUrls } from "api/mitxonline-test-utils"
 import * as commonUrls from "@/common/urls"
 import { Permission } from "api/hooks/user"
 import EnrollmentCodePage from "./EnrollmentCodePage"
-import invariant from "tiny-invariant"
 
 // Mock next-nprogress-bar for App Router
 const mockPush = jest.fn<void, [string]>()
@@ -30,8 +25,9 @@ describe("EnrollmentCodePage", () => {
       [Permission.Authenticated]: false,
     })
 
-    setMockResponse.get(mitxOnlineUrls.userMe.get(), null)
-    setMockResponse.post(b2bUrls.b2bAttach.b2bAttachView("test-code"), [])
+    setMockResponse.post(b2bUrls.b2bAttach.b2bAttachView("test-code"), [], {
+      code: 403,
+    })
 
     renderWithProviders(<EnrollmentCodePage code="test-code" />, {
       url: commonUrls.B2B_ATTACH_VIEW,
@@ -42,26 +38,22 @@ describe("EnrollmentCodePage", () => {
     })
 
     const url = new URL(mockPush.mock.calls[0][0])
-    expect(url.searchParams.get("skip_onboarding")).toBe("1")
-    const nextUrl = url.searchParams.get("next")
-    const signupNextUrl = url.searchParams.get("signup_next")
-    invariant(nextUrl)
-    invariant(signupNextUrl)
-    const attachView = commonUrls.b2bAttachView("test-code")
-    expect(new URL(nextUrl).pathname).toBe(attachView)
-    expect(new URL(signupNextUrl).pathname).toBe(attachView)
+    url.searchParams.sort()
+    const expectedParams = new URLSearchParams({
+      skip_onboarding: "1",
+      next: `http://test.learn.odl.local:8062${commonUrls.b2bAttachView("test-code")}`,
+    })
+    expectedParams.sort()
+    expect([...url.searchParams.entries()]).toEqual([
+      ...expectedParams.entries(),
+    ])
   })
 
   test("Renders when logged in", async () => {
     setMockResponse.get(urls.userMe.get(), {
       [Permission.Authenticated]: true,
     })
 
-    setMockResponse.get(
-      mitxOnlineUrls.userMe.get(),
-      mitxOnlineFactories.user.user(),
-    )
-
     setMockResponse.post(b2bUrls.b2bAttach.b2bAttachView("test-code"), [])
 
     renderWithProviders(<EnrollmentCodePage code="test-code" />, {
@@ -70,36 +62,21 @@ describe("EnrollmentCodePage", () => {
   })
 
   test("Redirects to dashboard on successful attachment", async () => {
-    const orgSlug = "test-org"
-    const mitxOnlineUser = mitxOnlineFactories.user.user({
-      b2b_organizations: [
-        {
-          id: 1,
-          name: "Test Organization",
-          description: "A test organization",
-          logo: "https://example.com/logo.png",
-          slug: `org-${orgSlug}`,
-          contracts: [],
-        },
-      ],
-    })
-
     setMockResponse.get(urls.userMe.get(), {
       [Permission.Authenticated]: true,
     })
 
-    setMockResponse.get(mitxOnlineUrls.userMe.get(), mitxOnlineUser)
-
-    setMockResponse.post(b2bUrls.b2bAttach.b2bAttachView("test-code"), [])
+    const attachUrl = b2bUrls.b2bAttach.b2bAttachView("test-code")
+    setMockResponse.post(attachUrl, [])
 
     renderWithProviders(<EnrollmentCodePage code="test-code" />, {
       url: commonUrls.B2B_ATTACH_VIEW,
     })
 
     await waitFor(() => {
-      expect(mockPush).toHaveBeenCalledWith(
-        commonUrls.organizationView(orgSlug),
-      )
+      expect(makeRequest).toHaveBeenCalledWith("post", attachUrl, undefined)
     })
+
+    expect(mockPush).toHaveBeenCalledWith(commonUrls.DASHBOARD_HOME)
   })
 })

frontends/main/src/app-pages/EnrollmentCodePage/EnrollmentCodePage.tsx

@@ -3,7 +3,6 @@ import React from "react"
 import { styled, Breadcrumbs, Container, Typography } from "ol-components"
 import * as urls from "@/common/urls"
 import { useB2BAttachMutation } from "api/mitxonline-hooks/organizations"
-import { useMitxOnlineUserMe } from "api/mitxonline-hooks/user"
 import { userQueries } from "api/hooks/user"
 import { useQuery } from "@tanstack/react-query"
 import { useRouter } from "next-nprogress-bar"
@@ -18,11 +17,7 @@ const InterstitialMessage = styled(Typography)(({ theme }) => ({
 }))
 
 const EnrollmentCodePage: React.FC<EnrollmentCodePage> = ({ code }) => {
-  const {
-    mutate: attach,
-    isSuccess,
-    isPending,
-  } = useB2BAttachMutation({
+  const enrollment = useB2BAttachMutation({
     enrollment_code: code,
   })
   const router = useRouter()
@@ -31,24 +26,21 @@ const EnrollmentCodePage: React.FC<EnrollmentCodePage> = ({ code }) => {
     ...userQueries.me(),
     staleTime: 0,
   })
-  const { data: mitxOnlineUser } = useMitxOnlineUserMe()
 
+  const enrollAsync = enrollment.mutateAsync
   React.useEffect(() => {
-    attach?.()
-  }, [attach])
+    if (user?.is_authenticated) {
+      enrollAsync().then(() => router.push(urls.DASHBOARD_HOME))
+    }
+  }, [user?.is_authenticated, enrollAsync, router])
 
   React.useEffect(() => {
     if (userLoading) {
       return
     }
     if (!user?.is_authenticated) {
       const loginUrlString = urls.auth({
-        loginNext: {
-          pathname: urls.b2bAttachView(code),
-          searchParams: null,
-        },
-        // On signup, redirect to the attach page so attachment can occur.
-        signupNext: {
+        next: {
           pathname: urls.b2bAttachView(code),
           searchParams: null,
         },
@@ -57,13 +49,7 @@ const EnrollmentCodePage: React.FC<EnrollmentCodePage> = ({ code }) => {
       loginUrl.searchParams.set("skip_onboarding", "1")
       router.push(loginUrl.toString())
     }
-    if (isSuccess) {
-      const org = mitxOnlineUser?.b2b_organizations?.[0]
-      if (org) {
-        router.push(urls.organizationView(org.slug.replace("org-", "")))
-      }
-    }
-  }, [isSuccess, userLoading, user, mitxOnlineUser, code, router])
+  }, [userLoading, user, code, router])
 
   return (
     <Container>
@@ -72,7 +58,7 @@ const EnrollmentCodePage: React.FC<EnrollmentCodePage> = ({ code }) => {
         ancestors={[{ href: urls.HOME, label: "Home" }]}
         current="Use Enrollment Code"
       />
-      {isPending && (
+      {enrollment.isPending && (
         <InterstitialMessage>Validating code "{code}"...</InterstitialMessage>
       )}
     </Container>

frontends/main/src/app-pages/ErrorPage/ForbiddenPage.test.tsx

@@ -54,7 +54,7 @@ test("Fetches auth data afresh and redirects unauthenticated users to auth", asy
   await waitFor(() => {
     expect(mockedRedirect).toHaveBeenCalledWith(
       routes.auth({
-        loginNext: {
+        next: {
           pathname: "/foo",
           searchParams: new URLSearchParams({ cat: "meow" }),
         },

frontends/main/src/app-pages/HomePage/HomePage.test.tsx

@@ -296,7 +296,7 @@ describe("Home Page personalize section", () => {
     expect(link).toHaveAttribute(
       "href",
       routes.auth({
-        loginNext: {
+        next: {
           pathname: routes.DASHBOARD_HOME,
           searchParams: null,
         },

frontends/main/src/app-pages/HomePage/PersonalizeSection.tsx

@@ -77,7 +77,7 @@ const AUTH_TEXT_DATA = {
     linkProps: {
       children: "Sign Up for Free",
       href: urls.auth({
-        loginNext: { pathname: urls.DASHBOARD_HOME, searchParams: null },
+        next: { pathname: urls.DASHBOARD_HOME, searchParams: null },
       }),
     },
   },