From 698f60be32cdcfae93b8d3addaea1417efb683eb Mon Sep 17 00:00:00 2001 From: Clay Diffrient Date: Thu, 21 Feb 2019 14:47:03 -0700 Subject: [PATCH] Enhance performance for the security tab This makes it so we show spinners until things have loaded. It also makes it so that we begin to fetch the data sooner and rely on our cache a bit more after. Not only should this increase speed perception, but should also really enhance the speed and experience. closes CORE-2324 Test Plan: - Enable CSP feature flag - Go to the security tab on an account/subaccount - You should initially see a spinner, then the page should show fully loaded once it disappears. Change-Id: Ia0c4b556cfd6e1a364443fe8f800e8c9ac4e10c9 Reviewed-on: https://gerrit.instructure.com/182384 Tested-by: Jenkins QA-Review: Tucker Mcknight Product-Review: Clay Diffrient Reviewed-by: Brent Burgoyne --- app/controllers/accounts_controller.rb | 5 ++ .../account_settings/__tests__/index.test.js | 9 ++- app/jsx/account_settings/actions.js | 6 ++ .../components/SecurityPanel.js | 25 +++++--- .../account_settings/components/Whitelist.js | 2 +- .../components/__tests__/utils.js | 9 ++- app/jsx/account_settings/index.js | 7 ++- app/jsx/account_settings/reducers.js | 15 ++++- app/jsx/account_settings/store.js | 14 +---- app/jsx/bundles/account_settings.js | 15 +++++ public/javascripts/account_settings.js | 58 ++++++++++++++----- 11 files changed, 126 insertions(+), 39 deletions(-) diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index eaa7160ae40..5f94d856b4d 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -986,6 +986,11 @@ class AccountsController < ApplicationController MASKED_APP_CENTER_ACCESS_TOKEN: @account.settings[:app_center_access_token].try(:[], 0...5), PERMISSIONS: { :create_tool_manually => @account.grants_right?(@current_user, session, :create_tool_manually), + }, + CSP: { + :enabled => @account.csp_enabled?, + :inherited => @account.csp_inherited?, + :settings_locked => @account.csp_locked?, } }) js_env(edit_help_links_env, true) diff --git a/app/jsx/account_settings/__tests__/index.test.js b/app/jsx/account_settings/__tests__/index.test.js index 09b7eb6ea23..580221845dc 100644 --- a/app/jsx/account_settings/__tests__/index.test.js +++ b/app/jsx/account_settings/__tests__/index.test.js @@ -27,10 +27,17 @@ describe('start', () => { const fixtures = document.createElement('div') fixtures.setAttribute('id', 'fixtures') document.body.appendChild(fixtures) + + const fakeAxios = { + put: jest.fn(() => ({then() {}})), + get: jest.fn(() => ({then() {}})) + } + expect(() => { start(fixtures, { context: 'account', - contextId: '1' + contextId: '1', + api: fakeAxios }) }).not.toThrowError() }) diff --git a/app/jsx/account_settings/actions.js b/app/jsx/account_settings/actions.js index b5fdf78a862..51e639723d6 100644 --- a/app/jsx/account_settings/actions.js +++ b/app/jsx/account_settings/actions.js @@ -23,6 +23,11 @@ function pluralize(word) { return word } +export const WHITELISTS_LOADED = 'WHITELISTS_LOADED' +export function setWhitelistsLoaded(value) { + return {type: WHITELISTS_LOADED, payload: value} +} + export const SET_DIRTY = 'SET_DIRTY' export function setDirtyAction(value) { if (typeof value !== 'boolean') { @@ -211,6 +216,7 @@ export function getCurrentWhitelist(context, contextId) { tools: response.data.tools_whitelist || {} } dispatch(addDomainBulkAction(addDomainMap)) + dispatch(setWhitelistsLoaded(true)) }) } diff --git a/app/jsx/account_settings/components/SecurityPanel.js b/app/jsx/account_settings/components/SecurityPanel.js index 3bf06deda6c..d77b54aa73f 100644 --- a/app/jsx/account_settings/components/SecurityPanel.js +++ b/app/jsx/account_settings/components/SecurityPanel.js @@ -24,6 +24,7 @@ import Heading from '@instructure/ui-elements/lib/components/Heading' import Text from '@instructure/ui-elements/lib/components/Text' import View from '@instructure/ui-layout/lib/components/View' import Checkbox from '@instructure/ui-forms/lib/components/Checkbox' +import Spinner from '@instructure/ui-elements/lib/components/Spinner' import Grid, {GridCol, GridRow} from '@instructure/ui-layout/lib/components/Grid' import { getCspEnabled, @@ -47,7 +48,8 @@ export class SecurityPanel extends Component { getCspInherited: func.isRequired, setCspInherited: func.isRequired, getCurrentWhitelist: func.isRequired, - isSubAccount: bool + isSubAccount: bool, + whitelistsHaveLoaded: bool } static defaultProps = { @@ -114,12 +116,18 @@ export class SecurityPanel extends Component { - + {!this.props.whitelistsHaveLoaded ? ( + + + + ) : ( + + )} @@ -132,7 +140,8 @@ function mapStateToProps(state, ownProps) { return { ...ownProps, cspEnabled: state.cspEnabled, - cspInherited: state.cspInherited + cspInherited: state.cspInherited, + whitelistsHaveLoaded: state.whitelistsHaveLoaded } } diff --git a/app/jsx/account_settings/components/Whitelist.js b/app/jsx/account_settings/components/Whitelist.js index f8ceeb6abdd..76e808aa573 100644 --- a/app/jsx/account_settings/components/Whitelist.js +++ b/app/jsx/account_settings/components/Whitelist.js @@ -193,7 +193,7 @@ export class Whitelist extends Component { {whitelistToShow.length <= 0 ? ( } /> diff --git a/app/jsx/account_settings/components/__tests__/utils.js b/app/jsx/account_settings/components/__tests__/utils.js index 3e6bd07fd26..7f2548e0318 100644 --- a/app/jsx/account_settings/components/__tests__/utils.js +++ b/app/jsx/account_settings/components/__tests__/utils.js @@ -21,13 +21,20 @@ import {Provider} from 'react-redux' import {render} from 'react-testing-library' import {configStore} from '../../store' +const fakeAxios = { + delete: jest.fn(() => ({then() {}})), + get: jest.fn(() => ({then() {}})), + post: jest.fn(() => ({then() {}})), + put: jest.fn(() => ({then() {}})) +} + // This is modified from a version by Kent C. Dodds described here: // https://github.com/kentcdodds/react-testing-library/blob/master/examples/__tests__/react-redux.js export function renderWithRedux( ui, { initialState, - store = configStore(initialState, { + store = configStore(initialState, fakeAxios, { disableLogger: true }) } = {} diff --git a/app/jsx/account_settings/index.js b/app/jsx/account_settings/index.js index 26b3998d2f2..f0b999e383d 100644 --- a/app/jsx/account_settings/index.js +++ b/app/jsx/account_settings/index.js @@ -27,7 +27,12 @@ export const CONFIG = { } export function start(element, props = {}, state = defaultState) { - const store = configStore(state) + const initialState = {...state} + if (props.initialCspSettings) { + initialState.cspEnabled = props.initialCspSettings.enabled + initialState.cspInherited = props.initialCspSettings.inherited + } + const store = configStore(initialState, props.api) render( diff --git a/app/jsx/account_settings/reducers.js b/app/jsx/account_settings/reducers.js index 6745f674ec2..8296893b6ce 100644 --- a/app/jsx/account_settings/reducers.js +++ b/app/jsx/account_settings/reducers.js @@ -28,7 +28,8 @@ import { SET_CSP_INHERITED, SET_CSP_INHERITED_OPTIMISTIC, SET_DIRTY, - COPY_INHERITED_SUCCESS + COPY_INHERITED_SUCCESS, + WHITELISTS_LOADED } from './actions' export function cspEnabled(state = false, action) { @@ -60,6 +61,15 @@ export function isDirty(state = false, action) { } } +export function whitelistsHaveLoaded(state = false, action) { + switch (action.type) { + case WHITELISTS_LOADED: + return action.payload + default: + return state + } +} + function getInheritedList(toolsWhiteList, effectiveWhitelist) { const toolsKeys = Object.keys(toolsWhiteList) return effectiveWhitelist.filter(domain => !toolsKeys.includes(domain)) @@ -128,5 +138,6 @@ export default combineReducers({ cspEnabled, cspInherited, isDirty, - whitelistedDomains + whitelistedDomains, + whitelistsHaveLoaded }) diff --git a/app/jsx/account_settings/store.js b/app/jsx/account_settings/store.js index 9881bf4aff4..4e827a0c6c5 100644 --- a/app/jsx/account_settings/store.js +++ b/app/jsx/account_settings/store.js @@ -19,29 +19,21 @@ import {createStore, applyMiddleware} from 'redux' import ReduxThunk from 'redux-thunk' import rootReducer from './reducers' -import axios from 'axios' -import {setupCache} from 'axios-cache-adapter' export const defaultState = { cspEnabled: false, cspInherited: false, isDirty: false, + whitelistsHaveLoaded: false, whitelistedDomains: { account: [], effective: [], + inherited: [], tools: {} } } -const cache = setupCache({ - maxAge: 0.5 * 60 * 1000 // Hold onto the data for 30 seconds -}) - -const api = axios.create({ - adapter: cache.adapter -}) - -export function configStore(initialState, options = {}) { +export function configStore(initialState, api, options = {}) { const middleware = [ ReduxThunk.withExtraArgument({axios: api}), process.env.NODE_ENV !== 'production' && diff --git a/app/jsx/bundles/account_settings.js b/app/jsx/bundles/account_settings.js index 68e3a65fafa..215ac55159f 100644 --- a/app/jsx/bundles/account_settings.js +++ b/app/jsx/bundles/account_settings.js @@ -18,8 +18,11 @@ import React from 'react' import ReactDOM from 'react-dom' +import I18n from 'i18n!account_settings_jsx_bundle' import FeatureFlagAdminView from 'compiled/views/feature_flags/FeatureFlagAdminView' import CustomHelpLinkSettings from '../custom_help_link_settings/CustomHelpLinkSettings' +import Spinner from '@instructure/ui-elements/lib/components/Spinner' +import View from '@instructure/ui-layout/lib/components/View' import 'account_settings' import 'compiled/bundles/modules/account_quota_settings' @@ -40,3 +43,15 @@ if (document.getElementById('custom_help_link_settings')) { ) } +if (document.getElementById('tab-security')) { + ReactDOM.render( + + + , document.getElementById('tab-security')) +} + diff --git a/public/javascripts/account_settings.js b/public/javascripts/account_settings.js index 915bb2b9533..4b3dee98002 100644 --- a/public/javascripts/account_settings.js +++ b/public/javascripts/account_settings.js @@ -21,6 +21,8 @@ import I18n from 'i18n!account_settings' import $ from 'jquery' import htmlEscape from 'str/htmlEscape' import RichContentEditor from 'jsx/shared/rce/RichContentEditor' +import axios from 'axios' +import {setupCache} from 'axios-cache-adapter' import 'jqueryui/tabs' import globalAnnouncements from './global_announcements' import './jquery.ajaxJSON' @@ -117,25 +119,53 @@ import './vendor/jquery.scrollTo' globalAnnouncements.augmentView() globalAnnouncements.bindDomEvents() - $("#account_settings_tabs").tabs({ + $('#account_settings_tabs') + .tabs({ beforeActivate: (event, ui) => { if (ui.newTab.context.id === 'tab-security-link') { - import('jsx/account_settings') - .then(({start}) => { - const splitContext = window.ENV.context_asset_string.split('_') - start(document.getElementById('tab-security'), { - context: splitContext[0], - contextId: splitContext[1], - isSubAccount: !ENV.ACCOUNT.root_account - }); - }).catch(() => { - // We really should never get here... but if we do... do something. - const $message = $('
').text('Security Tab failed to load') - $('tab-security').append($message) + // Set up axios and send a prefetch request to get the data we need, + // this should make things appear to be much quicker once the bundle + // loads in. + const cache = setupCache({ + maxAge: 0.5 * 60 * 1000, // Hold onto the data for 30 seconds + debug: true }) + + const api = axios.create({ + adapter: cache.adapter + }) + + const splitContext = window.ENV.context_asset_string.split('_') + + api + .get(`/api/v1/${splitContext[0]}s/${splitContext[1]}/csp_settings`) + .then(() => { + // Bring in the actual bundle of files to use + import('jsx/account_settings') + .then(({ start }) => { + start(document.getElementById('tab-security'), { + context: splitContext[0], + contextId: splitContext[1], + isSubAccount: !ENV.ACCOUNT.root_account, + initialCspSettings: ENV.CSP, + api + }) + }) + .catch(() => { + // We really should never get here... but if we do... do something. + $('#tab-security').text(I18n.t('Security Tab failed to load')) + }) + }) + .catch(() => { + // We really should never get here... but if we do... do something. + $('#tab-security').text(I18n.t('Security Tab failed to load')) + }) } } - }).show(); + }) + .show() + + $(".add_ip_filter_link").click(function(event) { event.preventDefault(); var $filter = $(".ip_filter.blank:first").clone(true).removeClass('blank');