Merge pull request #43782 from code-dot-org/revert-43780-revert-43716…

…-maureen/caching-bug-bash-signin-state

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

apps/src/code-studio/header.js

@@ -12,14 +12,16 @@ import {
   refreshProjectName,
   setShowTryAgainDialog
 } from './headerRedux';
-
 import React from 'react';
 import ReactDOM from 'react-dom';
 
 import {Provider} from 'react-redux';
 import progress from './progress';
 import {getStore} from '../redux';
-import {asyncLoadUserData} from '@cdo/apps/templates/currentUserRedux';
+import {
+  setUserSignedIn,
+  setInitialData
+} from '@cdo/apps/templates/currentUserRedux';
 
 import {PUZZLE_PAGE_NONE} from '@cdo/apps/templates/progress/progressTypes';
 import HeaderMiddle from '@cdo/apps/code-studio/components/header/HeaderMiddle';
@@ -168,10 +170,44 @@ function setupReduxSubscribers(store) {
 setupReduxSubscribers(getStore());
 
 function setUpGlobalData(store) {
-  store.dispatch(asyncLoadUserData());
+  fetch('/api/v1/users/current', {
+    credentials: 'same-origin'
+  })
+    .then(response => response.json())
+    .then(data => {
+      store.dispatch(setUserSignedIn(data.is_signed_in));
+      if (data.is_signed_in) {
+        store.dispatch(setInitialData(data));
+        ensureHeaderSigninState(true, data.short_name);
+      } else {
+        ensureHeaderSigninState(false);
+      }
+    })
+    .catch(err => {
+      console.log(err);
+    });
 }
 setUpGlobalData(getStore());
 
+// Some of our cached pages can become cached by the browser with the
+// wrong sign-in state. This is a temporary patch to ensure that the header
+// displays the correct sign-in state for the user.
+function ensureHeaderSigninState(isSignedIn, shortName) {
+  const userMenu = document.querySelector('#header_user_menu');
+  const signinButton = document.querySelector('#signin_button');
+
+  if (isSignedIn && userMenu.style.display === 'none') {
+    userMenu.style.display = 'block';
+    signinButton.style.display = 'none';
+
+    const displayName = document.querySelector('#header_display_name');
+    displayName.textContent = shortName;
+  } else if (!isSignedIn && signinButton.style.display === 'none') {
+    userMenu.style.display = 'none';
+    signinButton.style.display = 'inline';
+  }
+}
+
 header.showMinimalProjectHeader = function() {
   getStore().dispatch(refreshProjectName());
   getStore().dispatch(showMinimalProjectHeader());

apps/src/templates/currentUserRedux.js

@@ -23,7 +23,10 @@ export const setUserSignedIn = isSignedIn => ({
   isSignedIn
 });
 export const setUserType = userType => ({type: SET_USER_TYPE, userType});
-const setInitialData = serverUser => ({type: SET_INITIAL_DATA, serverUser});
+export const setInitialData = serverUser => ({
+  type: SET_INITIAL_DATA,
+  serverUser
+});
 
 const initialState = {
   userId: null,
@@ -73,28 +76,3 @@ export default function currentUser(state = initialState, action) {
 
   return state;
 }
-
-export const asyncLoadUserData = () => dispatch => {
-  currentUserFromServer(dispatch);
-};
-
-const currentUserFromServer = dispatch => {
-  return fetch('/api/v1/users/current')
-    .then(response => response.json())
-    .then(data => {
-      dispatch(setUserSignedIn(data.is_signed_in));
-      if (data.is_signed_in) {
-        dispatch(setInitialData(data));
-      }
-    })
-    .catch(err => {
-      console.log(err);
-    });
-};
-
-// export private function(s) to expose to unit testing
-export const __testonly__ = IN_UNIT_TEST
-  ? {
-      currentUserFromServer
-    }
-  : {};

apps/test/unit/code-studio/currentUserReduxTest.js

@@ -1,12 +1,11 @@
 import {assert} from '../../util/reconfiguredChai';
-import sinon from 'sinon';
 import currentUser, {
   SignInState,
   setUserSignedIn,
   setUserType,
   setCurrentUserHasSeenStandardsReportInfo,
   setCurrentUserName,
-  __testonly__
+  setInitialData
 } from '@cdo/apps/templates/currentUserRedux';
 
 describe('currentUserRedux', () => {
@@ -52,40 +51,18 @@ describe('currentUserRedux', () => {
     });
   });
 
-  describe('asyncLoadUserData', () => {
-    const {currentUserFromServer} = __testonly__;
-
-    it('calls /users/current and sets user data to state', async () => {
-      const dispatchSpy = sinon.spy();
-      const serverUser = {
-        id: 1,
-        username: 'test_user',
-        user_type: 'teacher',
-        is_signed_in: true
-      };
-
-      function mockApiResponse() {
-        return new window.Response(JSON.stringify(serverUser), {
-          status: 200,
-          headers: {'Content-type': 'application/json'}
-        });
-      }
-
-      const fetchStub = sinon.stub(window, 'fetch');
-      fetchStub
-        .withArgs('/api/v1/users/current')
-        .returns(Promise.resolve(mockApiResponse()));
-
-      await currentUserFromServer(dispatchSpy);
-
-      const dispatchCalls = dispatchSpy.getCalls();
-      const action1 = dispatchCalls[0].args[0];
-      assert.equal('currentUser/SET_USER_SIGNED_IN', action1.type);
-      assert.equal(true, action1.isSignedIn);
-
-      const action2 = dispatchCalls[1].args[0];
-      assert.equal('currentUser/SET_INITIAL_DATA', action2.type);
-      assert.deepEqual(serverUser, action2.serverUser);
-    });
+  describe('setInitialData', () => {
+    const serverUser = {
+      id: 1,
+      username: 'test_user',
+      user_type: 'teacher',
+      is_signed_in: true
+    };
+    const action = setInitialData(serverUser);
+    const nextState = currentUser(initialState, action);
+
+    assert.equal(nextState.userId, 1);
+    assert.equal(nextState.userName, 'test_user');
+    assert.equal(nextState.userType, 'teacher');
   });
 });

dashboard/app/controllers/api/v1/users_controller.rb

@@ -21,7 +21,8 @@ def current
         id: current_user.id,
         username: current_user.username,
         user_type: current_user.user_type,
-        is_signed_in: true
+        is_signed_in: true,
+        short_name: current_user.short_name
       }
     else
       render json: {

dashboard/app/views/layouts/_header.html.haml

@@ -49,7 +49,7 @@
 
   sign_in_user = current_user || user_type && OpenStruct.new(
     id: nil,
-    short_name: I18n.t("nav.user.#{user_type}"),
+    short_name: I18n.t("nav.user.loading"),
     can_pair?: false
   )
   show_pairing_dialog = !!session.delete(:show_pairing_dialog)

dashboard/config/locales/en.yml

@@ -431,6 +431,7 @@ en:
       starwars: 'Star Wars'
       starwarsblocks: 'Star Wars (Blocks)'
       starwarsblocks_hour: 'Star Wars (Blocks)'
+      loading: 'Loading...'
       weblab: 'Web Lab'
       view_all: 'View all projects...'
 

dashboard/test/controllers/api/v1/users_controller_test.rb

@@ -184,6 +184,7 @@ class Api::V1::UsersControllerTest < ActionController::TestCase
     assert_equal teacher.id, response["id"]
     assert_equal teacher.username, response["username"]
     assert_equal "teacher", response["user_type"]
+    assert_equal teacher.short_name, response["short_name"]
   end
 
   test "a get request to get school_name returns school object" do

dashboard/test/ui/features/foundations/signing_in.feature

@@ -7,13 +7,13 @@ Scenario: Student sign in from code.org
   And I sign out
   Given I am on "http://code.org/"
   And I reload the page
-  Then I wait to see ".header_user"
-  Then I click ".header_user"
+  Then I wait to see "#header_user_signin"
+  Then I click "#header_user_signin"
   And I wait to see "#signin"
   And I fill in username and password for "Bob"
   And I click "#signin-button" to load a new page
   Then I wait until I am on "http://studio.code.org/home"
-  Then I wait to see ".user_menu"
+  Then I wait to see "#header_user_menu"
   And I wait until element ".display_name" is visible
   And element ".display_name" contains text "Bob"
 
@@ -22,13 +22,13 @@ Scenario: Student sign in from studio.code.org
   And I sign out
   Given I am on "http://studio.code.org/"
   And I reload the page
-  Then I wait to see ".header_user"
-  Then I click ".header_user"
+  Then I wait to see "#header_user_signin"
+  Then I click "#header_user_signin"
   And I wait to see "#signin"
   And I fill in username and password for "Alice"
   And I click "#signin-button" to load a new page
   Then I wait until I am on "http://studio.code.org/home"
-  Then I wait to see ".user_menu"
+  Then I wait to see "#header_user_menu"
   And I wait until element ".display_name" is visible
   And element ".display_name" contains text "Alice"
 
@@ -37,13 +37,13 @@ Scenario: Student sign in from studio.code.org in the eu
   And I sign out
   Given I am on "http://studio.code.org/"
   And I reload the page
-  Then I wait to see ".header_user"
-  Then I click ".header_user"
+  Then I wait to see "#header_user_signin"
+  Then I click "#header_user_signin"
   And I wait to see "#signin"
   And I fill in username and password for "Alice"
   And I click "#signin-button" to load a new page
   Then I wait until I am on "http://studio.code.org/home"
-  Then I wait to see ".user_menu"
+  Then I wait to see "#header_user_menu"
   And I wait until element ".display_name" is visible
   And element ".display_name" contains text "Alice"
 
@@ -52,13 +52,13 @@ Scenario: Teacher sign in from studio.code.org
   And I sign out
   Given I am on "http://studio.code.org/"
   And I reload the page
-  Then I wait to see ".header_user"
-  Then I click ".header_user"
+  Then I wait to see "#header_user_signin"
+  Then I click "#header_user_signin"
   And I wait to see "#signin"
   And I fill in username and password for "Casey"
   And I click "#signin-button" to load a new page
   Then I wait until I am on "http://studio.code.org/home"
-  Then I wait to see ".user_menu"
+  Then I wait to see "#header_user_menu"
   And I wait until element ".display_name" is visible
   And element ".display_name" contains text "Casey"
 

shared/haml/user_header.haml

@@ -34,40 +34,44 @@
       %a.project_link_box{href: CDO.studio_url('projects')}
         .project_link#view_all_projects
           = I18n.t("#{loc_prefix}view_all")
-- if current_user
-  .header_button.header_user.user_menu{tabindex: 0, role: "button"}
-    - if current_user.can_pair? && session_pairings.present?
-      %i.fa.fa-users.pairing_icon
-      %span.pairing_name= I18n.t("#{loc_prefix}team")
-    - else
-      - short_name = ERB::Util.h(current_user.short_name)
-      %span.display_name{'data-id': current_user.id, 'data-shortname': short_name}= short_name
-     
-    %i.user_menu_arrow_down{class: "fa fa-caret-down"}
-    %i.user_menu_arrow_up{class: "fa fa-caret-up", style: "display: none"}
-    .user_options{style: 'display: none', dir: language_dir_class}
-      %a.linktag#my-projects{href: CDO.studio_url('projects')}= I18n.t("#{loc_prefix}my_projects")
-      - if current_user.can_pair?
-        - if session_pairings.present?
-          = link_to '#', {id: 'pairing_link', style: 'display: none'} do
-            = I18n.t("#{loc_prefix}pair_programming")
-            .pairing_summary
-              #{I18n.t("#{loc_prefix}driver")}:
-              = h(current_user.short_name)
-              - session_pairings.map do |id|
-                %br
-                #{I18n.t("#{loc_prefix}navigator")}:
-                = h(User.find(id).short_name)
-        - else
-          = link_to '#', {id: 'pairing_link', style: 'display: none'} do
-            = I18n.t("#{loc_prefix}pair_programming")
-      %a.linktag#user-edit{href: CDO.studio_url('users/edit')}= I18n.t("#{loc_prefix}settings")
-      %a.linktag#user-signout{href: CDO.studio_url('users/sign_out')}= I18n.t("#{loc_prefix}logout")
-- else
-  %a.linktag{href: CDO.studio_url('users/sign_in'), class: 'button-signin', id: 'signin_button'}
-    .header_button.header_user
-      %span= I18n.t("#{loc_prefix}signin")
-      .signin_callout_wrapper
+-# The user menu button is hidden using css so that we can verify the login state of the user on the client
+-# and adjust as needed for cached pages (see header.js in apps)
+- user_dropdown_display = current_user.present? ? "block" : "none"
+.header_button.header_user.user_menu{id: "header_user_menu", tabindex: 0, role: "button", style: "display: #{user_dropdown_display}"}
+  - if current_user&.can_pair? && session_pairings.present?
+    %i.fa.fa-users.pairing_icon
+    %span.pairing_name= I18n.t("#{loc_prefix}team")
+  - else
+    - short_name = ERB::Util.h(current_user&.short_name)
+    %span.display_name{id: 'header_display_name', 'data-id': current_user&.id, 'data-shortname': short_name}= short_name
+   
+  %i.user_menu_arrow_down{class: "fa fa-caret-down"}
+  %i.user_menu_arrow_up{class: "fa fa-caret-up", style: "display: none"}
+  .user_options{style: 'display: none', dir: language_dir_class}
+    %a.linktag#my-projects{href: CDO.studio_url('projects')}= I18n.t("#{loc_prefix}my_projects")
+    - if current_user&.can_pair?
+      - if session_pairings.present?
+        = link_to '#', {id: 'pairing_link', style: 'display: none'} do
+          = I18n.t("#{loc_prefix}pair_programming")
+          .pairing_summary
+            #{I18n.t("#{loc_prefix}driver")}:
+            = h(current_user&.short_name)
+            - session_pairings.map do |id|
+              %br
+              #{I18n.t("#{loc_prefix}navigator")}:
+              = h(User.find(id).short_name)
+      - else
+        = link_to '#', {id: 'pairing_link', style: 'display: none'} do
+          = I18n.t("#{loc_prefix}pair_programming")
+    %a.linktag#user-edit{href: CDO.studio_url('users/edit')}= I18n.t("#{loc_prefix}settings")
+    %a.linktag#user-signout{href: CDO.studio_url('users/sign_out')}= I18n.t("#{loc_prefix}logout")
+-# The sign-in button is hidden using css so that we can verify the login state of the user on the client
+-# and adjust as needed for cached pages (see header.js in apps)
+- signin_display = current_user.present? ? "none" : "inline"
+%a.linktag{href: CDO.studio_url('users/sign_in'), class: 'button-signin', id: 'signin_button', style: "display: #{signin_display}"}
+  .header_button.header_user#header_user_signin
+    %span= I18n.t("#{loc_prefix}signin")
+    .signin_callout_wrapper
 
 :javascript
   window.cookieEnvSuffix = '#{rack_env?(:production) ? '' : "_#{rack_env}"}';
@@ -82,7 +86,7 @@
     // Provide current_user.short_name to cached pages via session cookie.
     // There is apps code that also depends on this query-selector, so if changes are made
     // here we should be sure to also update other locations.
-    var displayNameSpan = document.querySelector('.header_button.header_user.user_menu .display_name');
+    var displayNameSpan = document.querySelector('#header_display_name');
 
     function retrieveUserShortName(element) {
       if (element) {