Skip to content
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

Revert "Revert "Show correct state of sign-in button in dashboard"" #43782

Conversation

maureensturgeon
Copy link
Contributor

@maureensturgeon maureensturgeon commented Nov 23, 2021

Reverts #43780

Original PR: #43716

The issue: I was calling fetch without explicitly specifying credentials. For some ranges of browsers the default is omit (see https://github.com/github/fetch#sending-cookies), meaning that cookies are not sent along with the request and in my case this resulted in the server missing the current logged in user and sending back is_signed_in: false from the API. Sending credentials: same-origin means that cookies will be sent with the request when it's made to the same origin (in our case studio.code.org)

@maureensturgeon maureensturgeon marked this pull request as ready for review November 24, 2021 01:09
@maureensturgeon maureensturgeon requested a review from a team as a code owner November 24, 2021 01:09
@maureensturgeon maureensturgeon requested a review from a team November 24, 2021 01:09
Copy link
Contributor

@hannahbergam hannahbergam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job pushing through with this! I left a couple comments but they're non-blocking.

@@ -168,10 +170,44 @@ function setupReduxSubscribers(store) {
setupReduxSubscribers(getStore());

function setUpGlobalData(store) {
store.dispatch(asyncLoadUserData());
fetch('/api/v1/users/current', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we document this somewhere? I can't tell how widespread this application of fetch might be but if we're moving toward fetch over ajax and the default credentials are still different for older browsers I imagine we'll run into this again?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely agree. One way to handle this would be to not use fetch directly but write our own wrapper around it so that these kind of configurations will be set for it every time it's used. I'll start a conversation about this in apps-devs tomorrow!


const displayName = document.querySelector('#header_display_name');
displayName.textContent = shortName;
} else if (!isSignedIn && signinButton.style.display === 'none') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It strikes me that this would be the default- this is a NIT but it might be organizes as just a default 'let' for those two vars with a single 'if' instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand what you mean by default 'let' for those two vars. I may follow-up with you on this and get these changes in separately!

Copy link
Contributor

@jamescodeorg jamescodeorg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Way to stick with this and track this down!

Could you add a note to the PR comments about the additional change that you made on top of the original change and include a link to why we need the credentials: same-origin?

@maureensturgeon maureensturgeon merged commit 12cb459 into staging Nov 24, 2021
@maureensturgeon maureensturgeon deleted the revert-43780-revert-43716-maureen/caching-bug-bash-signin-state branch November 24, 2021 17:19
snickell pushed a commit that referenced this pull request Feb 3, 2024
…-maureen/caching-bug-bash-signin-state

Revert "Revert "Show correct state of sign-in button in dashboard""
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants