Skip to content
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

Refactor Sample Data Home section which also fixes flaky sample data functional test #21655

Merged
merged 14 commits into from
Aug 27, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,23 +33,32 @@ import {
uninstallSampleDataSet
} from '../sample_data_sets';

const INSTALLED_STATUS = 'installed';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess these are returned from the objects in listSampleDataSets? Our folder structure of core plugins makes it difficult to push common consts in an intuitive place (the server side code of this is far away). If we typescripted this and the files in ../sample_data_sets we could at least show that the object being returned has a status and is one of installed or not_installed. Suppose that's fine for this PR but I think it'd be really helpful to understand and follow this code if it was in typescript.

const UNINSTALLED_STATUS = 'not_installed';

export class SampleDataSetCard extends React.Component {

constructor(props) {
super(props);

this.state = {
isProcessingRequest: false,
status: this.props.status,
};
}

static getDerivedStateFromProps(nextProps) {
return {
status: nextProps.status,
};
}

startRequest = async () => {
install = async () => {
const {
getConfig,
setConfig,
id,
name,
onRequestComplete,
defaultIndex,
clearIndexPatternsCache,
} = this.props;
Expand All @@ -58,36 +67,53 @@ export class SampleDataSetCard extends React.Component {
isProcessingRequest: true,
});

if (this.isInstalled()) {
await uninstallSampleDataSet(id, name, defaultIndex, getConfig, setConfig, clearIndexPatternsCache);
} else {
await installSampleDataSet(id, name, defaultIndex, getConfig, setConfig, clearIndexPatternsCache);
}
const isSuccess = await installSampleDataSet(id, name, defaultIndex, getConfig, setConfig, clearIndexPatternsCache);

this.setState({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accessing "this" here and below after an async call, i think we need the isMounted checks.

isProcessingRequest: false,
status: isSuccess ? INSTALLED_STATUS : UNINSTALLED_STATUS
});
}

uninstall = async () => {
const {
getConfig,
setConfig,
id,
name,
defaultIndex,
clearIndexPatternsCache,
} = this.props;

this.setState({
isProcessingRequest: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just another status. Can we merge the props into one? Status = PROCESSING, INSTALLED, UNINSTALLED. and I guess maybe UNKOWN?

How do I simulate UNKNOWN? I can't seem to trigger it.

I think even if you handle the isMounted checks below you can get into a weird state by hitting 'Add' or 'Remove' the quickly switching tabs and going back. isProcessingREquest is state on this button so that gets lost but the request is still processing. Then I think you will end up with indicating the wrong status.

screen shot 2018-08-06 at 2 01 50 pm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UNKNOWN surfaces if there are any problems accessing the data, like permission problems

});

onRequestComplete();
const isSuccess = await uninstallSampleDataSet(id, name, defaultIndex, getConfig, setConfig, clearIndexPatternsCache);

this.setState({
isProcessingRequest: false,
status: isSuccess ? UNINSTALLED_STATUS : INSTALLED_STATUS
});
}

isInstalled = () => {
if (this.props.status === 'installed') {
if (this.state.status === 'installed') {
return true;
}

return false;
}

renderBtn = () => {
switch (this.props.status) {
case 'installed':
switch (this.state.status) {
case INSTALLED_STATUS:
return (
<EuiFlexGroup justifyContent="spaceBetween">
<EuiFlexItem grow={false}>
<EuiButtonEmpty
isLoading={this.state.isProcessingRequest}
onClick={this.startRequest}
onClick={this.uninstall}
color="danger"
data-test-subj={`removeSampleDataSet${this.props.id}`}
>
Expand All @@ -105,13 +131,13 @@ export class SampleDataSetCard extends React.Component {
</EuiFlexGroup>
);

case 'not_installed':
case UNINSTALLED_STATUS:
return (
<EuiFlexGroup justifyContent="flexEnd">
<EuiFlexItem grow={false}>
<EuiButton
isLoading={this.state.isProcessingRequest}
onClick={this.startRequest}
onClick={this.install}
data-test-subj={`addSampleDataSet${this.props.id}`}
>
{this.state.isProcessingRequest ? 'Adding' : 'Add'}
Expand Down Expand Up @@ -162,12 +188,11 @@ SampleDataSetCard.propTypes = {
name: PropTypes.string.isRequired,
launchUrl: PropTypes.string.isRequired,
status: PropTypes.oneOf([
'installed',
'not_installed',
INSTALLED_STATUS,
UNINSTALLED_STATUS,
'unknown',
]).isRequired,
statusMsg: PropTypes.string,
onRequestComplete: PropTypes.func.isRequired,
getConfig: PropTypes.func.isRequired,
setConfig: PropTypes.func.isRequired,
clearIndexPatternsCache: PropTypes.func.isRequired,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ export class TutorialDirectory extends React.Component {
launchUrl={this.props.addBasePath(`/app/kibana#/dashboard/${sampleDataSet.overviewDashboard}`)}
status={sampleDataSet.status}
statusMsg={sampleDataSet.statusMsg}
onRequestComplete={this.loadSampleDataSets}
getConfig={this.props.getConfig}
setConfig={this.props.setConfig}
clearIndexPatternsCache={this.props.clearIndexPatternsCache}
Expand Down
8 changes: 6 additions & 2 deletions src/core_plugins/kibana/public/home/sample_data_sets.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export async function installSampleDataSet(id, name, defaultIndex, getConfig, se
title: `Unable to install sample data set: ${name}`,
text: `${err.message}`,
});
return;
return false;
}

const existingDefaultIndex = await getConfig('defaultIndex');
Expand All @@ -80,6 +80,8 @@ export async function installSampleDataSet(id, name, defaultIndex, getConfig, se
title: `${name} installed`,
['data-test-subj']: 'sampleDataSetInstallToast'
});

return true;
}

export async function uninstallSampleDataSet(id, name, defaultIndex, getConfig, setConfig, clearIndexPatternsCache) {
Expand All @@ -98,7 +100,7 @@ export async function uninstallSampleDataSet(id, name, defaultIndex, getConfig,
title: `Unable to uninstall sample data set`,
text: `${err.message}`,
});
return;
return false;
}

const existingDefaultIndex = await getConfig('defaultIndex');
Expand All @@ -112,4 +114,6 @@ export async function uninstallSampleDataSet(id, name, defaultIndex, getConfig,
title: `${name} uninstalled`,
['data-test-subj']: 'sampleDataSetUninstallToast'
});

return true;
}
12 changes: 12 additions & 0 deletions test/functional/page_objects/home_page.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,22 @@ export function HomePageProvider({ getService }) {

async addSampleDataSet(id) {
await testSubjects.click(`addSampleDataSet${id}`);
await this._waitForSampleDataLoadingAction(id);
}

async removeSampleDataSet(id) {
await testSubjects.click(`removeSampleDataSet${id}`);
await this._waitForSampleDataLoadingAction(id);
}

// loading action is either uninstall and install
async _waitForSampleDataLoadingAction(id) {
const sampleDataCard = await testSubjects.find(`sampleDataSetCard${id}`);
await retry.try(async () => {
// waitForDeletedByClassName needs to be inside retry because it will timeout at least once
// before action is complete
await sampleDataCard.waitForDeletedByClassName('euiLoadingSpinner');
});
}

async launchSampleDataSet(id) {
Expand Down