-
Notifications
You must be signed in to change notification settings - Fork 361
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
fix: [M3-7249] - Linode Landing flickering #9836
Changes from 15 commits
ee56a25
a87259a
abaf5ae
e8c1be5
473c710
ac73415
eaecf6a
3966d0b
ec5f134
91e98b1
67adbf4
bd1967d
2832482
eb7ae37
d8af7a1
ab8bcfe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@linode/manager": Fixed | ||
--- | ||
|
||
Linodes Landing flickering ([#9836](https://github.com/linode/manager/pull/9836)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import { apiMatcher } from 'support/util/intercepts'; | ||
|
||
describe('account activation', () => { | ||
/** | ||
* The API will return 403 with the body below for most endpoint except `/v4/profile`. | ||
* | ||
* { "errors": [ { "reason": "Your account must be activated before you can use this endpoint" } ] } | ||
*/ | ||
it('should render an activation landing page if the customer is not activated', () => { | ||
cy.intercept('GET', apiMatcher('*'), { | ||
statusCode: 403, | ||
body: { | ||
errors: [ | ||
{ | ||
reason: | ||
'Your account must be activated before you can use this endpoint', | ||
}, | ||
], | ||
}, | ||
}); | ||
|
||
cy.visitWithLogin('/'); | ||
|
||
cy.findByText('Your account is currently being reviewed.'); | ||
cy.findByText('open a support ticket', { exact: false }); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,7 @@ | ||
import { render, waitFor } from '@testing-library/react'; | ||
import * as React from 'react'; | ||
|
||
import { rest, server } from 'src/mocks/testServer'; | ||
import { wrapWithTheme } from 'src/utilities/testHelpers'; | ||
|
||
import MainContent, { | ||
import { | ||
checkFlagsForMainContentBanner, | ||
checkPreferencesForBannerDismissal, | ||
} from './MainContent'; | ||
import { queryClientFactory } from './queries/base'; | ||
|
||
const queryClient = queryClientFactory(); | ||
|
||
const mainContentBanner = { | ||
key: 'Test Text Key', | ||
|
@@ -52,31 +43,3 @@ describe('checkPreferencesForBannerDismissal', () => { | |
expect(checkPreferencesForBannerDismissal({}, 'key1')).toBe(false); | ||
}); | ||
}); | ||
|
||
describe('Databases menu item for a restricted user', () => { | ||
it('should not render the menu item', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The intentions of this test are not clear. We have tests for this case in the |
||
server.use( | ||
rest.get('*/account', (req, res, ctx) => { | ||
return res(ctx.json({})); | ||
}) | ||
); | ||
const { getByText } = render( | ||
wrapWithTheme( | ||
<MainContent appIsLoading={false} isLoggedInAsCustomer={true} />, | ||
{ | ||
flags: { databases: false }, | ||
queryClient, | ||
} | ||
) | ||
); | ||
|
||
await waitFor(() => { | ||
let el; | ||
try { | ||
el = getByText('Databases'); | ||
} catch (e) { | ||
expect(el).not.toBeDefined(); | ||
} | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ import { handleInitTokens } from 'src/store/authentication/authentication.action | |
import { handleLoadingDone } from 'src/store/initialLoad/initialLoad.actions'; | ||
import { State as PendingUploadState } from 'src/store/pendingUpload'; | ||
import { MapState } from 'src/store/types'; | ||
import SplashScreen from '../SplashScreen'; | ||
|
||
interface Props { | ||
children: React.ReactNode; | ||
|
@@ -34,11 +35,6 @@ type CombinedProps = Props & | |
WithApplicationStoreProps; | ||
|
||
export class AuthenticationWrapper extends React.Component<CombinedProps> { | ||
state = { | ||
hasEnsuredAllTypes: false, | ||
showChildren: false, | ||
}; | ||
|
||
componentDidMount() { | ||
const { initSession } = this.props; | ||
/** | ||
|
@@ -54,8 +50,6 @@ export class AuthenticationWrapper extends React.Component<CombinedProps> { | |
* to show the children onMount | ||
*/ | ||
if (this.props.isAuthenticated) { | ||
this.setState({ showChildren: true }); | ||
|
||
this.makeInitialRequests(); | ||
} | ||
} | ||
|
@@ -72,8 +66,6 @@ export class AuthenticationWrapper extends React.Component<CombinedProps> { | |
!this.state.showChildren | ||
) { | ||
this.makeInitialRequests(); | ||
|
||
return this.setState({ showChildren: true }); | ||
} | ||
|
||
/** basically handles for the case where our token is expired or we got a 401 error */ | ||
|
@@ -90,8 +82,12 @@ export class AuthenticationWrapper extends React.Component<CombinedProps> { | |
render() { | ||
const { children } = this.props; | ||
const { showChildren } = this.state; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any thoughts on renaming There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was considering something more like |
||
// eslint-disable-next-line | ||
return <React.Fragment>{showChildren ? children : null}</React.Fragment>; | ||
|
||
if (showChildren) { | ||
return children; | ||
} | ||
|
||
return <SplashScreen />; | ||
} | ||
|
||
static defaultProps = { | ||
|
@@ -109,6 +105,7 @@ export class AuthenticationWrapper extends React.Component<CombinedProps> { | |
makeInitialRequests = async () => { | ||
// When loading Lish we avoid all this extra data loading | ||
if (window.location?.pathname?.match(/linodes\/[0-9]+\/lish/)) { | ||
this.setState({ showChildren: true }); | ||
return; | ||
} | ||
|
||
|
@@ -142,8 +139,13 @@ export class AuthenticationWrapper extends React.Component<CombinedProps> { | |
/** We choose to do nothing, relying on the Redux error state. */ | ||
} finally { | ||
this.props.markAppAsDoneLoading(); | ||
this.setState({ showChildren: true }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixes e2e failure @jdamore-linode mentioned.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's my first time visiting this code in a long time. I understand we to load content and render tree in the case of a fetching error, but what does the redux error do exactly? The user does not get any feedback something fails (ex: try blocking any API request). It's probably beyond the scope of this PR but wondering what your thoughts are about it. |
||
} | ||
}; | ||
|
||
state = { | ||
showChildren: false, | ||
}; | ||
} | ||
|
||
interface StateProps { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!!!
Edit: Whoops, meant for this to be for the entire file lol