Fix broken "View App Configurations" link

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

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.
visionmedia/page.js#589

This bug apparently cropped up with d07531a, 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.

6e31dcf 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 6e31dcf 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 <[email protected]>
Reviewed-by: Sean Scally <[email protected]>
QA-Review: Xander Moffatt <[email protected]>
Product-Review: Evan Battaglia <[email protected]>

ui/features/external_apps/react/components/AppList.js

@@ -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')}

ui/features/external_apps/react/components/AppTile.js

@@ -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 = () => {

ui/features/external_apps/react/components/__tests__/AppList.test.js

@@ -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()

ui/features/external_apps/react/router.js

@@ -24,54 +24,46 @@ 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)/
 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
+  }
 }