ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix broken "View App Configurations" link 

Also fully remove `alreadyRendered`, which was added in 309e5fd9 but saw
its usage removed in 0bf0399c.

Added PropType `baseUrl` to ensure that is sent; Gergich then required my
to add a PropType for `app`.

fixes INTEROP-7118
flag=none

The value of page.js's "pathname" is dependent on whether there is a
hash symbol (anchor) in the current URL. If there is no anchor, the
pathname contains (starts with) the base URL (e.g.
"/accounts/self/settings"). If there is an anchor, it does not contain
it. The problem is that sometimes, the clicking the Apps tab changed the
web browser's current URL to "/accounts/self/settings#tab-tools". As a
result, pathname was "", and the configurations URL, which is normally
"/accounts/self/settings/configurations", was then just
"/configurations". I have filed a bug report on page.js, but the project
doesn't seem terribly active.
https://github.com/visionmedia/page.js/issues/589

This bug apparently cropped up with d07531a5, before which we never
added a anchor onto the settings URL. This commit was never in prod, but
my guess is that the times we seen it in prod were when we first went to
beta to repro it, then simply got rid of the "beta." from the URL,
leaving canvas.instructure.com/accounts/self/setting#tab-tools, from
which we could repro the issue.

For the fix, "baseUrl" is already used successfully in AppDetails to
construct URLs.  I can't find any reason to rely on pathname, and
pathname is not used for any other purpose.

6e31dcf1 fixed a similar (same?) issue, but that was on an older version
of path.js which did not overwrite pathname if there was a hash symbol.
In the old version of path.js, "/accounts/self/settings#tab-tools" would
yield a pathname of "/accounts/self/settings#tab-tools" so we needed to
cut off the "#tab-tools". In the current version, it yields a pathname
of "".

Test plan:
- go to /accounts/self/settings
- click various tabs and try setting a couple options to make sure
  nothing is broken there.
- click the Apps tab. Note whether or not the browser's URL has changed
  to add "#tab-tools" or not. Within the Apps tab, click "View App
  Configurations", then back to "View App Center", then click
  a particular app (which adds "app/abcdef123-..." to the browser
  location) and click both "View App Configurations" and "View App
  Center" from there. Make sure all the links work.
- If your browser did add "#tab-tools" which clicking the tab, try
  disabling the "remember_settings_tab" feature flag and repeat the tests.
  If it didn't, check that that flag is on.
- go to "/accounts/self/settings#tab-tools". It should jump to the Apps
  tab. Check out that links again and make sure they all work. In
  particular make sure the "View App Configurations" button works when
  the browser URL has "#tab-tools"
- Try editing tools, clicking the info icon or disabling/enabling
  placements. Try to break stuff.
- The test plan from 6e31dcf1 says to use the keyboard, but that didn't
  do anything different for me. You can try it.
- Run all the above steps for the course settings page.
- Run all the above steps for the course details page
  (/courses/123/details).

Change-Id: I2d5cfdcbbb8f0cfd2d43b61b2b5eebf034c77062
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/276597
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Sean Scally <sean.scally@instructure.com>
QA-Review: Xander Moffatt <xmoffatt@instructure.com>
Product-Review: Evan Battaglia <ebattaglia@instructure.com>
This commit is contained in:
Evan Battaglia 2021-10-22 10:10:22 -06:00
parent 3aec488ad7
commit 4644fdecb6
6 changed files with 38 additions and 107 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
spec/javascripts/jsx/external_apps/routerSpec.js
View File

@ -1,53 +0,0 @@
/*
* Copyright (C) 2016 - present Instructure, Inc.
*
* This file is part of Canvas.
*
* Canvas is free software: you can redistribute it and/or modify it under
* the terms of the GNU Affero General Public License as published by the Free
* Software Foundation, version 3 of the License.
*
* Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
* WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
* A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
* details.
*
* You should have received a copy of the GNU Affero General Public License along
* with this program. If not, see <http://www.gnu.org/licenses/>.
*/
import regularizePathname from 'ui/features/external_apps/react/lib/regularizePathname.js'
QUnit.module('External Apps Client-side Router', {
before() {
window.ENV = window.ENV || {}
window.ENV.TESTING_PATH = '/settings/something'
}
})
test('regularizePathname removes trailing slash', () => {
const fakeCtx = {
pathname: '/app/something/else/'
}
// No op for next().
const fakeNext = () => {}
regularizePathname(fakeCtx, fakeNext)
equal(fakeCtx.pathname, '/app/something/else', 'trailing slash is gone')
})
test('regularizePathname removes url hash fragment', () => {
const fakeCtx = {
hash: 'blah-ha-ba',
pathname: '/app/something/else/#blah-ha-ba'
}
// No op for next().
const fakeNext = () => {}
regularizePathname(fakeCtx, fakeNext)
equal(fakeCtx.pathname, '/app/something/else', 'url hash fragment is gone')
})

9
ui/features/external_apps/react/components/AppList.js
View File

@ -18,6 +18,7 @@
import I18n from 'i18n!external_tools'
import React from 'react'
import PropTypes from 'prop-types'
import store from '../lib/AppCenterStore'
import extStore from '../lib/ExternalAppsStore'
import AppTile from './AppTile'
@ -27,6 +28,10 @@ import ManageAppListButton from './ManageAppListButton'
import splitAssetString from '@canvas/util/splitAssetString'
export default class AppList extends React.Component {
static propTypes = {
baseUrl: PropTypes.string.isRequired
}
state = store.getState()
onChange = () => {
@ -67,7 +72,7 @@ export default class AppList extends React.Component {
} else {
return store
.filteredApps()
.map(app => <AppTile key={app.app_id} app={app} pathname={this.props.pathname} />)
.map(app => <AppTile key={app.app_id} app={app} baseUrl={this.props.baseUrl} />)
}
}
@ -79,7 +84,7 @@ export default class AppList extends React.Component {
<Header>
{this.manageAppListButton()}
<a
href={`${this.props.pathname}/configurations`}
href={`${this.props.baseUrl}/configurations`}
className="btn view_tools_link lm pull-right"
>
{I18n.t('View App Configurations')}

8
ui/features/external_apps/react/components/AppTile.js
View File

@ -19,11 +19,17 @@
import I18n from 'i18n!external_tools'
import $ from 'jquery'
import React from 'react'
import PropTypes from 'prop-types'
import page from 'page'
export default class extends React.Component {
static displayName = 'AppTile'
static propTypes = {
app: PropTypes.object,
baseUrl: PropTypes.string.isRequired
}
state = {
isHidingDetails: true
}
@ -53,7 +59,7 @@ export default class extends React.Component {
handleClick = e => {
e.preventDefault()
page(`${this.props.pathname}/app/${this.props.app.short_name}`)
page(`${this.props.baseUrl}/app/${this.props.app.short_name}`)
}
installedRibbon = () => {

24
ui/features/external_apps/react/components/__tests__/AppList.test.js
View File

@ -127,35 +127,47 @@ describe('AppList', () => {
$.ajax = originalAJAX
})
function renderAppList() {
return render(<AppList baseUrl="/the/base/url" />)
}
it('renders loading indicator', () => {
store.setState({isLoading: true})
const {getByTestId} = render(<AppList />)
const {getByTestId} = renderAppList()
expect(getByTestId(/Spinner/i)).toBeVisible()
})
it('does not apply focus to filter text input on render', () => {
const {getByText} = render(<AppList />)
const {getByText} = renderAppList()
expect(getByText('Filter by name')).not.toHaveFocus()
})
it('renders manage app list button if context type is account', () => {
const {getByText} = render(<AppList />)
const stuff = renderAppList()
const {getByText} = stuff
expect(getByText('Manage App List')).toBeVisible()
})
it('does not render manage app list button if context type is not account', () => {
window.ENV = {context_asset_string: 'course_1'}
const {queryByText} = render(<AppList />)
const {queryByText} = renderAppList()
expect(queryByText('Manage App List')).not.toBeInTheDocument()
})
it('renders view app configurations link', () => {
const {getByText} = render(<AppList />)
const {getByText} = renderAppList()
expect(getByText('View App Configurations')).toBeVisible()
})
it('uses baseUrl for the app configurations link', () => {
const {getByText} = renderAppList()
expect(getByText('View App Configurations').closest('a').getAttribute('href')).toEqual(
'/the/base/url/configurations'
)
})
it('renders app filters', () => {
const {getByText} = render(<AppList />)
const {getByText} = renderAppList()
expect(getByText('All')).toBeVisible()
expect(getByText('Not Installed')).toBeVisible()
expect(getByText('Installed')).toBeVisible()

29
ui/features/external_apps/react/lib/regularizePathname.js
View File

@ -1,29 +0,0 @@
/*
* Copyright (C) 2016 - present Instructure, Inc.
*
* This file is part of Canvas.
*
* Canvas is free software: you can redistribute it and/or modify it under
* the terms of the GNU Affero General Public License as published by the Free
* Software Foundation, version 3 of the License.
*
* Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
* WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
* A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
* details.
*
* You should have received a copy of the GNU Affero General Public License along
* with this program. If not, see <http://www.gnu.org/licenses/>.
*/
/**
* This makes the pathname always leave off a potential trailing
* slash.
*/
const regularizePathname = (ctx, next) => {
ctx.originalPathname = ctx.pathname
ctx.pathname = ctx.pathname.replace('#' + ctx.hash, '').replace(/\/$/, '')
next()
}
export default regularizePathname

22
ui/features/external_apps/react/router.js
View File

@ -24,7 +24,6 @@ import AppList from './components/AppList'
import AppDetails from './components/AppDetails'
import Configurations from './components/Configurations'
import AppCenterStore from './lib/AppCenterStore'
import regularizePathname from './lib/regularizePathname'
const currentPath = window.location.pathname
const re = /(.*\/settings|.*\/details)/
@ -32,46 +31,39 @@ const matches = re.exec(currentPath)
const baseUrl = matches[0]
let targetNodeToRenderIn = null
let alreadyRendered = false
/**
* Route Handlers
*/
const renderAppList = ctx => {
const renderAppList = _ctx => {
if (!window.ENV.APP_CENTER.enabled) {
page.redirect('/configurations')
} else {
ReactDOM.render(
<Root>
<AppList pathname={ctx.pathname} alreadyRendered={alreadyRendered} />
<AppList baseUrl={baseUrl} />
</Root>,
targetNodeToRenderIn
)
alreadyRendered = true
}
}
const renderAppDetails = ctx => {
ReactDOM.render(
<Root>
<AppDetails
shortName={ctx.params.shortName}
pathname={ctx.pathname}
baseUrl={baseUrl}
store={AppCenterStore}
/>
<AppDetails shortName={ctx.params.shortName} baseUrl={baseUrl} store={AppCenterStore} />
</Root>,
targetNodeToRenderIn
)
}
const renderConfigurations = ctx => {
const renderConfigurations = _ctx => {
// router.start is only called when loading the Apps tab
// so we don't want to try anything here that hasn't happened.
if (targetNodeToRenderIn) {
ReactDOM.render(
<Root>
<Configurations pathname={ctx.pathname} env={window.ENV} />
<Configurations pathname={baseUrl} env={window.ENV} />
</Root>,
targetNodeToRenderIn
)
@ -82,7 +74,6 @@ const renderConfigurations = ctx => {
* Route Configuration
*/
page.base(baseUrl)
page('*', regularizePathname)
page('/', renderAppList)
page('/app/:shortName', renderAppDetails)
page('/configurations', renderConfigurations)
@ -95,6 +86,5 @@ export default {
stop() {
// we may not be the only thing using page on this page.
page.stop()
},
regularizePathname
}
}