Clever takeover bugfixes #18460
Clever takeover bugfixes #18460
Conversation
| @@ -93,7 +93,7 @@ export default class LinkCleverAccountModal extends React.Component { | |||
| fixedWidth={600} | |||
| fixedHeight={310} | |||
| isOpen={this.props.isOpen} | |||
| handleClose={this.cancel} | |||
| uncloseable | |||
There was a problem hiding this comment.
👍 I can confirm that uncloseable doesn't just remove the 'x' button, it also removes the ability to close by clicking outside the dialog or pressing the Esc key.
| @@ -39,12 +40,13 @@ def health_check | |||
| # Signed out: redirect to /courses | |||
| def index | |||
| if current_user | |||
| if current_user.student? && current_user.primary_script | |||
| if current_user.student? && current_user.primary_script && !session['clever_link_flag'] | |||
There was a problem hiding this comment.
Bug 1 (skip redirect if coming in for the first time via Clever, in which case the link flag should be set)
| redirect_to script_path(current_user.primary_script) | ||
| else | ||
| redirect_to '/home' | ||
| end | ||
| else | ||
| clear_clever_session_variables |
| @@ -195,6 +195,7 @@ window.CleverTakeoverManager = function (options) { | |||
|
|
|||
| function onCancelModal() { | |||
| $("#user_user_type").val("student"); | |||
| $.get("/users/clever_modal_dismissed"); | |||
There was a problem hiding this comment.
Question: Why GET instead of POST here? I can see arguments either way, and I'm curious what you had in mind.
My first instinct is POST because we're changing state, and we don't necessarily want this route to be cached, bookmarked, or navigated to manually. But when I went looking for guidelines, GET also seems reasonable here because the operation is idempotent and therefore "safe," and apparently GET is preferred for AJAX because it's a faster operation (news to me).
Second question: should we ensure this request is successful, or are we cool with fire-and-forget here? What's the negative user impact if we return a 500 here?
There was a problem hiding this comment.
I'm not really sure re: GET vs POST, since it is changing state it actually may make more sense for it to be a POST. I think because no data goes along with it I picked GET (also I wanted to test the method manually in the browser), but I may actually change this because you're right, POST makes more sense re: caching and whatnot.
The user impact if this fails is pretty minimal, it just means that the dialog will show up again if they navigate back to the home page during the same session. That's a minor annoyance, so I opted to ignore errors rather than surfacing or retrying.
Show interstitial on course and script pages as well (not just homepage)
Turns out this is not doable, because some of those pages are cached. Solved by redirecting to home page instead if the interstitial should be shown.
Remove “x” from dialog (light dismiss should also not work, but I think you get that for free when you remove the “x”)
Interrupting takeover flow leaves cookies in place, and shows dialog next time
Session variables are now cleared if the current_user is not set when hitting the homepage, which mostly resolves this problem.
Shouldn’t show the dialog after the user has made a choice to create a new account (Right now, if you choose to create a new account, you’ll keep getting the dialog when you reload the homepage until you sign out and sign back in again)
This is also solved by clearing the session variables, this time when the dialog is dismissed (via an XHR request to a SessionsController#clever_modal_dismissed)