-
Notifications
You must be signed in to change notification settings - Fork 3
Redirect to dashboard home after login, usually #2600
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
Conversation
7e8d1cf
to
5c373f2
Compare
* To include search parameters in the next URL, we need to encode them. | ||
* If we pass `?next=/foo/bar?cat=meow` directly, Django receives two separate | ||
* parameters: `next` and `cat`. | ||
* | ||
* There's no need to encode the path parameter (it might contain slashes, | ||
* but those are allowed in search parameters) so let's keep it readable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to encode the path parameter (it might contain slashes, but those are allowed in search parameters) so let's keep it readable.
It might also contain pluses, which are problematic.
Try logging into RC via login button at https://rc.learn.mit.edu/courses/course-v1:MITxT+8.01.1x/ to see the issue. (Once this PR is merged, that button will go to the dashboard, but it's still good to fix this. We will have the signup popover on the product pages, too.)
from django.utils.text import slugify | ||
from django.views import View | ||
|
||
from main import settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Importing from main
instead of django.conf
breaks the pytest django fixtures.
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"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was broken for me locally because I guess my local env file overrides this value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
loginUrl.searchParams.set("skip_onboarding", "1") | ||
router.push(loginUrl.toString()) | ||
} | ||
}, [userLoading, user, code, router]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably no race condition if the attachisSuccess
will never be true for unauthenticated users, but should we wait for !userLoading
before calling attach to avoid the unnecessary call?
8900933
to
f1cbaa1
Compare
What are the relevant tickets?
Closes
Description (What does it do?)
On login, redirect users to dashboard home in most cases. Exception: if they were trying to access a specific page (Signup/login popover, or auth redirect).
How can this be tested?
Prerequisite for #5 only: MITxOnline integration with organizations set up.
Start as an anonymous user in each scenario below.
my-lists
)my-lists
)/enrollmentcode/{enrollmentcode}
page.http://api.learn.odl.local:8065/login?next=http://learn.odl.local:8062/enrollmentcode/ENROLLMENT_CODE_VALUE
prior to redirecting you to keycloak. If I visithttp://api.learn.odl.local:8065/login?next=http://learn.odl.local:8062/enrollmentcode/ENROLLMENT_CODE_VALUE
directly, I tend not to get the 500 error.if true:
Additional Context
With the new simplified behavior, the
signup_next
parameter is unused. I've mostly removed it from the frontend code, but left it on the backend in case we need it.