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

feat: add ECharts BoxPlot chart #11199

Merged
merged 21 commits into from
Nov 12, 2020
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -18,6 +18,7 @@
*/
import { WORLD_HEALTH_DASHBOARD } from './dashboard.helper';
import readResponseBlob from '../../utils/readResponseBlob';
import { isLegacyChart } from '../../utils/vizPlugins';

describe('Dashboard top-level controls', () => {
const sliceRequests = [];
Expand All @@ -38,20 +39,23 @@ describe('Dashboard top-level controls', () => {
).slice_id;

dashboard.slices.forEach(slice => {
const sliceRequest = `getJson_${slice.slice_id}`;
sliceRequests.push(`@${sliceRequest}`);
const formData = `{"slice_id":${slice.slice_id}}`;
cy.route(
'POST',
`/superset/explore_json/?form_data=${formData}&dashboard_id=${dashboardId}`,
).as(sliceRequest);
// TODO(villebro): enable V1 charts
if (isLegacyChart(slice.form_data.viz_type)) {
const sliceRequest = `getJson_${slice.slice_id}`;
sliceRequests.push(`@${sliceRequest}`);
const formData = `{"slice_id":${slice.slice_id}}`;
cy.route(
'POST',
`/superset/explore_json/?form_data=${formData}&dashboard_id=${dashboardId}`,
).as(sliceRequest);

const forceRefresh = `postJson_${slice.slice_id}_force`;
forceRefreshRequests.push(`@${forceRefresh}`);
cy.route(
'POST',
`/superset/explore_json/?form_data={"slice_id":${slice.slice_id}}&force=true&dashboard_id=${dashboardId}`,
).as(forceRefresh);
const forceRefresh = `postJson_${slice.slice_id}_force`;
forceRefreshRequests.push(`@${forceRefresh}`);
cy.route(
'POST',
`/superset/explore_json/?form_data={"slice_id":${slice.slice_id}}&force=true&dashboard_id=${dashboardId}`,
).as(forceRefresh);
}
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/
import { WORLD_HEALTH_DASHBOARD } from './dashboard.helper';
import { isLegacyChart } from '../../utils/vizPlugins';

interface Slice {
slice_id: number;
Expand Down Expand Up @@ -47,17 +48,21 @@ describe('Dashboard filter', () => {
cy.get('#app').then(app => {
const bootstrapData = app.data('bootstrap');
const dashboard = bootstrapData.dashboard_data as DashboardData;
const sliceIds = dashboard.slices.map(slice => slice.slice_id);
const { slices } = dashboard;
filterId =
dashboard.slices.find(
slice => slice.form_data.viz_type === 'filter_box',
)?.slice_id || 0;
aliases = sliceIds.map(id => {
const alias = getAlias(id);
const url = `/superset/explore_json/?*{"slice_id":${id}}*`;
cy.route('POST', url).as(alias.slice(1));
return alias;
});
aliases = slices
// TODO(villebro): enable V1 charts
.filter(slice => isLegacyChart(slice.form_data.viz_type))
.map(slice => {
const id = slice.slice_id;
const alias = getAlias(id);
const url = `/superset/explore_json/?*{"slice_id":${id}}*`;
cy.route('POST', url).as(alias.slice(1));
return alias;
});

// wait the initial page load requests
cy.wait(aliases);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@
* under the License.
*/
import readResponseBlob from '../../utils/readResponseBlob';
import { isLegacyChart } from '../../utils/vizPlugins';
import { WORLD_HEALTH_DASHBOARD } from './dashboard.helper';

describe('Dashboard load', () => {
const aliases = [];
let dashboard;

beforeEach(() => {
cy.server();
Expand All @@ -30,18 +32,27 @@ describe('Dashboard load', () => {

cy.get('#app').then(data => {
const bootstrapData = JSON.parse(data[0].dataset.bootstrap);
const { slices } = bootstrapData.dashboard_data;
// then define routes and create alias for each requests
slices.forEach(slice => {
dashboard = bootstrapData.dashboard_data;
});
});

it('should load dashboard', () => {
const { slices } = dashboard;

// then define routes and create alias for each requests
slices.forEach(slice => {
const vizType = slice.form_data.viz_type;
const isLegacy = isLegacyChart(vizType);
// TODO(villebro): enable V1 charts
if (isLegacy) {
const alias = `getJson_${slice.slice_id}`;
const formData = `{"slice_id":${slice.slice_id}}`;
cy.route('POST', `/superset/explore_json/?*${formData}*`).as(alias);
const route = `/superset/explore_json/?*${formData}*`;
cy.route('POST', `${route}`).as(alias);
aliases.push(`@${alias}`);
});
}
});
});

it('should load dashboard', () => {
// wait and verify one-by-one
cy.wait(aliases).then(requests => {
return Promise.all(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@
* under the License.
*/
import { WORLD_HEALTH_DASHBOARD } from './dashboard.helper';
import { isLegacyChart } from '../../utils/vizPlugins';

describe('Dashboard form data', () => {
const urlParams = { param1: '123', param2: 'abc' };
let sliceIds = [];
let dashboardId;
let dashboard;

beforeEach(() => {
cy.server();
Expand All @@ -31,21 +31,22 @@ describe('Dashboard form data', () => {

cy.get('#app').then(data => {
const bootstrapData = JSON.parse(data[0].dataset.bootstrap);
const dashboard = bootstrapData.dashboard_data;
dashboardId = dashboard.id;
sliceIds = dashboard.slices.map(slice => slice.slice_id);
dashboard = bootstrapData.dashboard_data;
});
});

it('should apply url params and queryFields to slice requests', () => {
const aliases = [];
sliceIds.forEach(id => {
dashboard.slices.forEach(slice => {
const { slice_id: id } = slice;
const isLegacy = isLegacyChart(slice.form_data.viz_type);
const route = `/superset/explore_json/?form_data={"slice_id":${id}}&dashboard_id=${dashboard.id}`;
const alias = `getJson_${id}`;
aliases.push(`@${alias}`);
cy.route(
'POST',
`/superset/explore_json/?form_data={"slice_id":${id}}&dashboard_id=${dashboardId}`,
).as(alias);
// TODO(villebro): enable V1 charts
if (isLegacy) {
aliases.push(`@${alias}`);
cy.route('POST', route).as(alias);
}
});

cy.wait(aliases).then(requests => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@ describe('AdhocFilters', () => {
cy.route('GET', '/superset/explore_json/**').as('getJson');
cy.route('POST', '/superset/explore_json/**').as('postJson');
cy.route('GET', '/superset/filter/table/*/name').as('filterValues');
});

it('Should not load mathjs when not needed', () => {
cy.visitChartByName('Boys'); // a table chart
cy.verifySliceSuccess({ waitAlias: '@postJson' });
});

xit('Should not load mathjs when not needed', () => {
Copy link
Member Author

@villebro villebro Nov 10, 2020

Choose a reason for hiding this comment

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

@ktmud (pinging as I noticed you had introduced this test recently) I had to disable this test, as apparently the function annotation layer that was added to the ECharts Timeseries viz loads mathjs despite not being used here. The relevant code is here (is called by transformProps.ts): https://github.com/apache-superset/superset-ui/blob/master/plugins/plugin-chart-echarts/src/utils/annotation.ts . Is there something that can be done differently to avoid having mathjs load here?

Copy link
Member

@ktmud ktmud Nov 10, 2020

Choose a reason for hiding this comment

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

You chart plugin seems to be using a different version of mathjs, which is probably incompatible with what's used in AnnotationLayer control. Do you mind changing them to the same version and see if the test still fails?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, good to know, I'll do that 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

@ktmud I've now upgraded all mathjs deps to 8.0.1, and still failing the test. I propose disabling it for now so we can merge this.

Copy link
Member

@ktmud ktmud Nov 12, 2020

Choose a reason for hiding this comment

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

The test is failing because of this import chain: @superset-ui/plugin-chart-echarts -> Timeseries -> transformProps -> utils/annotation -> mathjs. Unlike loadChart, transformProps is not loaded asynchronously, so Webpack could not defer the loading of mathjs, which means it will be loaded for all first-visits on almost all pages, undoing part of the optimization we did in #10837 .

The test is there to make sure this optimization still works. It didn't break because it was flaky. It caught the breaking changes it is supposed to catch.

It's OK to merge this PR as is for now, but I'd recommend finding ways to move the mathjs logic to the chart renderer so that we could continue benefiting from code split and asynchronous loading.

In general I think transformProps should be as lightweight as possible and preferably only return scalars. It should only be about applying defaults and consolidating parameter names/shapes. Most complex logics should be dealt with either by the chart renderer itself or at another data-processing layer on top of the chart. This way it's easier to reuse your chart at other places outside of Superset charting plugins.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the context. I'll move the mathjs (and any other similar) rendering logic out of transformProps and re-enable the test in a subsequent bump.

cy.get('script[src*="mathjs"]').should('have.length', 0);
});

Expand Down Expand Up @@ -55,7 +55,7 @@ describe('AdhocFilters', () => {
});
});

it('Set simple adhoc filter', () => {
xit('Set simple adhoc filter', () => {
cy.get('[data-test=adhoc-filter-simple-value] .Select__control').click();
cy.get('[data-test=adhoc-filter-simple-value] input[type=text]')
.focus()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,46 +23,28 @@ describe('Visualization > Box Plot', () => {
slice_id: 49,
granularity_sqla: 'year',
time_grain_sqla: 'P1D',
time_range: '1960-01-01+:+now',
time_range: '1960-01-01 : now',
metrics: ['sum__SP_POP_TOTL'],
adhoc_filters: [],
groupby: ['region'],
limit: '25',
color_scheme: 'bnbColors',
whisker_options: 'Min/max+(no+outliers)',
whisker_options: 'Min/max (no outliers)',
};

function verify(formData) {
cy.visitChartByParams(JSON.stringify(formData));
cy.verifySliceSuccess({ waitAlias: '@getJson', chartSelector: 'svg' });
cy.verifySliceSuccess({ waitAlias: '@getJson' });
}

beforeEach(() => {
cy.server();
cy.login();
cy.route('POST', '/superset/explore_json/**').as('getJson');
cy.route('POST', '/api/v1/chart/data').as('getJson');
});

it('should work', () => {
verify(BOX_PLOT_FORM_DATA);
cy.get('.chart-container svg rect.vx-boxplot-box').should('have.length', 7);
});

it('should work with filter', () => {
verify({
...BOX_PLOT_FORM_DATA,
adhoc_filters: [
{
expressionType: 'SIMPLE',
subject: 'region',
operator: '==',
comparator: 'South Asia',
clause: 'WHERE',
sqlExpression: null,
filterOptionName: 'filter_8aqxcf5co1a_x7lm2d1fq0l',
},
],
});
cy.get('.chart-container svg rect.vx-boxplot-box').should('have.length', 1);
cy.get('.chart-container .box_plot canvas').should('have.length', 1);
});
});
23 changes: 23 additions & 0 deletions superset-frontend/cypress-base/cypress/utils/vizPlugins.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
const V1_PLUGINS = ['box_plot', 'echarts_timeseries', 'word_cloud', 'pie'];

export function isLegacyChart(vizType: string): boolean {
return !V1_PLUGINS.includes(vizType);
}
Loading