You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/07/18 13:55:22 UTC
[GitHub] [superset] kgabryje opened a new pull request, #20743: feat: Pass dashboard context to explore through local storage
kgabryje opened a new pull request, #20743:
URL: https://github.com/apache/superset/pull/20743
### SUMMARY
Currently when user clicks "Edit chart" or chart's title in Dashboard page, we first send a POST request to retrieve form_data_key, then create an Explore URL with that form_data_key and redirect to Explore. We do that in order to pass dashboard context (such as native filters, cross filter, color scheme...) to Explore.
There are several problems with this approach:
- we redirect to another page, so from semantic HTML point of view this element should be a link
- user can't access standard link contextual menu when right clicking
- there's a noticeable delay between a click and redirection because of HTTP request being made in the middle
- we unnecessarily take up space in redis
In order to refactor "Edit chart" to be a link, we need to pass dashboard context in a different way.
This PR implements the following changes:
- save current dashboard state in local storage, where random id generated on Dashboard mount is a key and stringified state is a value
- update the value in local storage on each dashboard state change (such as applying new native filter or cross filter or changing color scheme)
- "Edit chart"/chart title is a React Router Link and it contains local storage key as a search param
- when mounting Explore, retrieve the dashboard state from local storage using the dashboard id search param and merge it with form data coming from initializing Explore request
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
https://user-images.githubusercontent.com/15073128/179526015-608cef82-85fd-432c-aa5b-57f563cd5e0b.mov
### TESTING INSTRUCTIONS
1. Open dashboard
2. Apply native filters, cross filters, filter box filters
3. Open Explore by clicking on chart title or clicking "Edit chart". Try opening in the same tab and opening in a new tab
4. Verify that filters from dashboard are present in Explore
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1194011577
Thank you for spotting that @jinghua-qa ! Fixed
https://user-images.githubusercontent.com/15073128/180782050-86a8a784-cce7-4fd6-a4e9-5a4eea43e81b.mov
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r925419236
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -306,13 +307,14 @@ class SliceHeaderControls extends React.PureComponent<
)}
{this.props.supersetCanExplore && (
- <Menu.Item
- key={MENU_KEYS.EXPLORE_CHART}
- onClick={({ domEvent }) => this.props.onExploreChart(domEvent)}
- >
- <Tooltip title={getSliceHeaderTooltip(this.props.slice.slice_name)}>
- {t('Edit chart')}
- </Tooltip>
+ <Menu.Item key={MENU_KEYS.EXPLORE_CHART}>
+ <Link to={this.props.exploreUrl ?? '#'}>
Review Comment:
Oops, forgot to remove that. Now exploreUrl is always defined so we don't need a null check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1188299114
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1190645302
> I noticed that the `form_data_key` is still being generated in Explore. Is there a reason for it? If it's not needed, can we remove it and mark the `explore/form_data` endpoint as deprecated? You can add a [deprecation warning](https://github.com/apache/superset/blob/81bd4968d0a916cb2a20e47b20e31a1434be4f46/superset/views/core.py#L750) to the API file.
The logic related to `form_data_key` is still used in a lot of places in Explore code. We should handle that separately, since this PR focuses on refactoring linking from Dashboard to Explore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r924826725
##########
superset-frontend/src/explore/controlUtils/getFormDataWithDashboardContext.ts:
##########
@@ -0,0 +1,191 @@
+/**
+ * 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.
+ */
+import isEqual from 'lodash/isEqual';
+import {
+ EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS,
+ EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS,
+ isDefined,
+ JsonObject,
+ ensureIsArray,
+ QueryObjectFilterClause,
+ SimpleAdhocFilter,
+ QueryFormData,
+} from '@superset-ui/core';
+import { NO_TIME_RANGE } from '../constants';
+
+const simpleFilterToAdhoc = (
+ filterClause: QueryObjectFilterClause,
+ clause = 'where',
+) => {
+ const result = {
+ clause: clause.toUpperCase(),
+ expressionType: 'SIMPLE',
+ operator: filterClause.op,
+ subject: filterClause.col,
+ comparator: 'val' in filterClause ? filterClause.val : undefined,
+ } as SimpleAdhocFilter;
+ if (filterClause.isExtra) {
+ Object.assign(result, {
+ isExtra: true,
+ filterOptionName: `filter_${Math.random()
+ .toString(36)
+ .substring(2, 15)}_${Math.random().toString(36).substring(2, 15)}`,
+ });
+ }
+ return result;
+};
+
+const removeAdhocFilterDuplicates = (filters: SimpleAdhocFilter[]) => {
+ const isDuplicate = (
+ adhocFilter: SimpleAdhocFilter,
+ existingFilters: SimpleAdhocFilter[],
+ ) =>
+ existingFilters.some(
+ (existingFilter: SimpleAdhocFilter) =>
+ existingFilter.operator === adhocFilter.operator &&
+ existingFilter.subject === adhocFilter.subject &&
+ ((!('comparator' in existingFilter) &&
+ !('comparator' in adhocFilter)) ||
+ ('comparator' in existingFilter &&
+ 'comparator' in adhocFilter &&
+ isEqual(existingFilter.comparator, adhocFilter.comparator))),
+ );
+
+ return filters.reduce((acc, filter) => {
+ if (!isDuplicate(filter, acc)) {
+ acc.push(filter);
+ }
+ return acc;
+ }, [] as SimpleAdhocFilter[]);
+};
+
+const mergeFilterBoxToFormData = (
+ exploreFormData: QueryFormData,
+ dashboardFormData: JsonObject,
+) => {
+ const dateColumns = {
+ __time_range: 'time_range',
+ __time_col: 'granularity_sqla',
+ __time_grain: 'time_grain_sqla',
+ __granularity: 'granularity',
+ };
+ const appliedTimeExtras = {};
+
+ const filterBoxData: JsonObject = {};
+ ensureIsArray(dashboardFormData.extra_filters).forEach(filter => {
+ if (dateColumns[filter.col]) {
+ if (filter.val !== NO_TIME_RANGE) {
+ filterBoxData[dateColumns[filter.col]] = filter.val;
+ appliedTimeExtras[filter.col] = filter.val;
+ }
+ } else {
+ const adhocFilter = simpleFilterToAdhoc({
+ ...(filter as QueryObjectFilterClause),
+ isExtra: true,
+ });
+ filterBoxData.adhoc_filters = [
+ ...ensureIsArray(filterBoxData.adhoc_filters),
+ adhocFilter,
+ ];
+ }
+ });
+ filterBoxData.applied_time_extras = appliedTimeExtras;
+ return filterBoxData;
+};
+
+const mergeNativeFiltersToFormData = (
Review Comment:
This logic is copied from `/superset/utils/core.py::merge_extra_form_data`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r925415104
##########
superset-frontend/src/explore/ExplorePage.tsx:
##########
@@ -57,6 +67,58 @@ const fetchExploreData = async (exploreUrlParams: URLSearchParams) => {
}
};
+const getDashboardContextFormData = () => {
+ const dashboardTabId = getUrlParam(URL_PARAMS.dashboardTabId);
+ const sliceId = getUrlParam(URL_PARAMS.sliceId) || 0;
+ let dashboardContextWithFilters = {};
+ if (dashboardTabId) {
+ const {
+ labelColors,
+ sharedLabelColors,
+ colorScheme,
+ chartConfiguration,
+ nativeFilters,
Review Comment:
It's `state.nativeFilters.filters` (so without filterSets). We use it in `getFormDataWithExtraFilters` called in line 88. However, now I noticed that we only need `chartsInScope` for each filter - the rest of metadata is redundant. Same goes for cross filters (chartConfiguration). I'll change that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r924826725
##########
superset-frontend/src/explore/controlUtils/getFormDataWithDashboardContext.ts:
##########
@@ -0,0 +1,191 @@
+/**
+ * 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.
+ */
+import isEqual from 'lodash/isEqual';
+import {
+ EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS,
+ EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS,
+ isDefined,
+ JsonObject,
+ ensureIsArray,
+ QueryObjectFilterClause,
+ SimpleAdhocFilter,
+ QueryFormData,
+} from '@superset-ui/core';
+import { NO_TIME_RANGE } from '../constants';
+
+const simpleFilterToAdhoc = (
+ filterClause: QueryObjectFilterClause,
+ clause = 'where',
+) => {
+ const result = {
+ clause: clause.toUpperCase(),
+ expressionType: 'SIMPLE',
+ operator: filterClause.op,
+ subject: filterClause.col,
+ comparator: 'val' in filterClause ? filterClause.val : undefined,
+ } as SimpleAdhocFilter;
+ if (filterClause.isExtra) {
+ Object.assign(result, {
+ isExtra: true,
+ filterOptionName: `filter_${Math.random()
+ .toString(36)
+ .substring(2, 15)}_${Math.random().toString(36).substring(2, 15)}`,
+ });
+ }
+ return result;
+};
+
+const removeAdhocFilterDuplicates = (filters: SimpleAdhocFilter[]) => {
+ const isDuplicate = (
+ adhocFilter: SimpleAdhocFilter,
+ existingFilters: SimpleAdhocFilter[],
+ ) =>
+ existingFilters.some(
+ (existingFilter: SimpleAdhocFilter) =>
+ existingFilter.operator === adhocFilter.operator &&
+ existingFilter.subject === adhocFilter.subject &&
+ ((!('comparator' in existingFilter) &&
+ !('comparator' in adhocFilter)) ||
+ ('comparator' in existingFilter &&
+ 'comparator' in adhocFilter &&
+ isEqual(existingFilter.comparator, adhocFilter.comparator))),
+ );
+
+ return filters.reduce((acc, filter) => {
+ if (!isDuplicate(filter, acc)) {
+ acc.push(filter);
+ }
+ return acc;
+ }, [] as SimpleAdhocFilter[]);
+};
+
+const mergeFilterBoxToFormData = (
+ exploreFormData: QueryFormData,
+ dashboardFormData: JsonObject,
+) => {
+ const dateColumns = {
+ __time_range: 'time_range',
+ __time_col: 'granularity_sqla',
+ __time_grain: 'time_grain_sqla',
+ __granularity: 'granularity',
+ };
+ const appliedTimeExtras = {};
+
+ const filterBoxData: JsonObject = {};
+ ensureIsArray(dashboardFormData.extra_filters).forEach(filter => {
+ if (dateColumns[filter.col]) {
+ if (filter.val !== NO_TIME_RANGE) {
+ filterBoxData[dateColumns[filter.col]] = filter.val;
+ appliedTimeExtras[filter.col] = filter.val;
+ }
+ } else {
+ const adhocFilter = simpleFilterToAdhoc({
+ ...(filter as QueryObjectFilterClause),
+ isExtra: true,
+ });
+ filterBoxData.adhoc_filters = [
+ ...ensureIsArray(filterBoxData.adhoc_filters),
+ adhocFilter,
+ ];
+ }
+ });
+ filterBoxData.applied_time_extras = appliedTimeExtras;
+ return filterBoxData;
+};
+
+const mergeNativeFiltersToFormData = (
Review Comment:
This logic is copied from `/superset/utils/core.py:merge_extra_form_data`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r924828738
##########
superset-frontend/src/explore/controlUtils/getFormDataWithDashboardContext.ts:
##########
@@ -0,0 +1,191 @@
+/**
+ * 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.
+ */
+import isEqual from 'lodash/isEqual';
+import {
+ EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS,
+ EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS,
+ isDefined,
+ JsonObject,
+ ensureIsArray,
+ QueryObjectFilterClause,
+ SimpleAdhocFilter,
+ QueryFormData,
+} from '@superset-ui/core';
+import { NO_TIME_RANGE } from '../constants';
+
+const simpleFilterToAdhoc = (
+ filterClause: QueryObjectFilterClause,
+ clause = 'where',
+) => {
+ const result = {
+ clause: clause.toUpperCase(),
+ expressionType: 'SIMPLE',
+ operator: filterClause.op,
+ subject: filterClause.col,
+ comparator: 'val' in filterClause ? filterClause.val : undefined,
+ } as SimpleAdhocFilter;
+ if (filterClause.isExtra) {
+ Object.assign(result, {
+ isExtra: true,
+ filterOptionName: `filter_${Math.random()
+ .toString(36)
+ .substring(2, 15)}_${Math.random().toString(36).substring(2, 15)}`,
+ });
+ }
+ return result;
+};
+
+const removeAdhocFilterDuplicates = (filters: SimpleAdhocFilter[]) => {
+ const isDuplicate = (
+ adhocFilter: SimpleAdhocFilter,
+ existingFilters: SimpleAdhocFilter[],
+ ) =>
+ existingFilters.some(
+ (existingFilter: SimpleAdhocFilter) =>
+ existingFilter.operator === adhocFilter.operator &&
+ existingFilter.subject === adhocFilter.subject &&
+ ((!('comparator' in existingFilter) &&
+ !('comparator' in adhocFilter)) ||
+ ('comparator' in existingFilter &&
+ 'comparator' in adhocFilter &&
+ isEqual(existingFilter.comparator, adhocFilter.comparator))),
+ );
+
+ return filters.reduce((acc, filter) => {
+ if (!isDuplicate(filter, acc)) {
+ acc.push(filter);
+ }
+ return acc;
+ }, [] as SimpleAdhocFilter[]);
+};
+
+const mergeFilterBoxToFormData = (
Review Comment:
This logic is copied from `/superset/utils/core.py:merge_extra_filters`
##########
superset-frontend/src/explore/controlUtils/getFormDataWithDashboardContext.ts:
##########
@@ -0,0 +1,191 @@
+/**
+ * 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.
+ */
+import isEqual from 'lodash/isEqual';
+import {
+ EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS,
+ EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS,
+ isDefined,
+ JsonObject,
+ ensureIsArray,
+ QueryObjectFilterClause,
+ SimpleAdhocFilter,
+ QueryFormData,
+} from '@superset-ui/core';
+import { NO_TIME_RANGE } from '../constants';
+
+const simpleFilterToAdhoc = (
+ filterClause: QueryObjectFilterClause,
+ clause = 'where',
+) => {
+ const result = {
+ clause: clause.toUpperCase(),
+ expressionType: 'SIMPLE',
+ operator: filterClause.op,
+ subject: filterClause.col,
+ comparator: 'val' in filterClause ? filterClause.val : undefined,
+ } as SimpleAdhocFilter;
+ if (filterClause.isExtra) {
+ Object.assign(result, {
+ isExtra: true,
+ filterOptionName: `filter_${Math.random()
+ .toString(36)
+ .substring(2, 15)}_${Math.random().toString(36).substring(2, 15)}`,
+ });
+ }
+ return result;
+};
+
+const removeAdhocFilterDuplicates = (filters: SimpleAdhocFilter[]) => {
+ const isDuplicate = (
+ adhocFilter: SimpleAdhocFilter,
+ existingFilters: SimpleAdhocFilter[],
+ ) =>
+ existingFilters.some(
+ (existingFilter: SimpleAdhocFilter) =>
+ existingFilter.operator === adhocFilter.operator &&
+ existingFilter.subject === adhocFilter.subject &&
+ ((!('comparator' in existingFilter) &&
+ !('comparator' in adhocFilter)) ||
+ ('comparator' in existingFilter &&
+ 'comparator' in adhocFilter &&
+ isEqual(existingFilter.comparator, adhocFilter.comparator))),
+ );
+
+ return filters.reduce((acc, filter) => {
+ if (!isDuplicate(filter, acc)) {
+ acc.push(filter);
+ }
+ return acc;
+ }, [] as SimpleAdhocFilter[]);
+};
+
+const mergeFilterBoxToFormData = (
Review Comment:
This logic is copied from `/superset/utils/core.py::merge_extra_filters`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1188804738
# [Codecov](https://codecov.io/gh/apache/superset/pull/20743?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#20743](https://codecov.io/gh/apache/superset/pull/20743?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7ea4fcc) into [master](https://codecov.io/gh/apache/superset/commit/a6abcd9ea8fac4a477b824adb367b4b5206a5d27?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a6abcd9) will **decrease** coverage by `0.12%`.
> The diff coverage is `12.80%`.
```diff
@@ Coverage Diff @@
## master #20743 +/- ##
==========================================
- Coverage 66.84% 66.71% -0.13%
==========================================
Files 1750 1754 +4
Lines 65896 66068 +172
Branches 7017 7067 +50
==========================================
+ Hits 44048 44079 +31
- Misses 20062 20190 +128
- Partials 1786 1799 +13
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.77% <12.80%> (-0.17%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/20743?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/dashboard/actions/hydrate.js](https://codecov.io/gh/apache/superset/pull/20743/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL2h5ZHJhdGUuanM=) | `2.02% <0.00%> (-0.05%)` | :arrow_down: |
| [...src/dashboard/components/FiltersBadge/selectors.ts](https://codecov.io/gh/apache/superset/pull/20743/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlcnNCYWRnZS9zZWxlY3RvcnMudHM=) | `70.37% <0.00%> (+1.90%)` | :arrow_up: |
| [...board/components/nativeFilters/FilterBar/index.tsx](https://codecov.io/gh/apache/superset/pull/20743/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL2luZGV4LnRzeA==) | `60.14% <0.00%> (-0.44%)` | :arrow_down: |
| [...nd/src/dashboard/components/nativeFilters/utils.ts](https://codecov.io/gh/apache/superset/pull/20743/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvdXRpbHMudHM=) | `46.66% <ø> (ø)` | |
| [...perset-frontend/src/dashboard/containers/Chart.jsx](https://codecov.io/gh/apache/superset/pull/20743/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL0NoYXJ0LmpzeA==) | `75.00% <ø> (ø)` | |
| [...set-frontend/src/dashboard/containers/Dashboard.ts](https://codecov.io/gh/apache/superset/pull/20743/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL0Rhc2hib2FyZC50cw==) | `0.00% <ø> (ø)` | |
| [...shboard/util/charts/getFormDataWithExtraFilters.ts](https://codecov.io/gh/apache/superset/pull/20743/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2NoYXJ0cy9nZXRGb3JtRGF0YVdpdGhFeHRyYUZpbHRlcnMudHM=) | `88.88% <ø> (-5.56%)` | :arrow_down: |
| [superset-frontend/src/explore/ExplorePage.tsx](https://codecov.io/gh/apache/superset/pull/20743/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvRXhwbG9yZVBhZ2UudHN4) | `0.00% <0.00%> (ø)` | |
| [...ponents/controls/VizTypeControl/VizTypeGallery.tsx](https://codecov.io/gh/apache/superset/pull/20743/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9WaXpUeXBlQ29udHJvbC9WaXpUeXBlR2FsbGVyeS50c3g=) | `88.17% <ø> (ø)` | |
| [...re/controlUtils/getFormDataWithDashboardContext.ts](https://codecov.io/gh/apache/superset/pull/20743/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFV0aWxzL2dldEZvcm1EYXRhV2l0aERhc2hib2FyZENvbnRleHQudHM=) | `0.00% <0.00%> (ø)` | |
| ... and [44 more](https://codecov.io/gh/apache/superset/pull/20743/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/20743?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/20743?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [a6abcd9...7ea4fcc](https://codecov.io/gh/apache/superset/pull/20743?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1189392578
@kgabryje Ephemeral environment spinning up at http://54.245.54.96:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1192429934
@jinghua-qa Ephemeral environment spinning up at http://52.24.5.129:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r925436975
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,12 +90,95 @@ type PageProps = {
idOrSlug: string;
};
+const getDashboardContextLocalStorage = () => {
+ const dashboardsContexts = getItem(
+ LocalStorageKeys.dashboard__explore_context,
+ {},
+ );
+ return Object.fromEntries(
+ Object.entries(dashboardsContexts).filter(
+ ([, value]) => !value.isRedundant,
+ ),
+ );
+};
+
+const updateDashboardTabLocalStorage = (
+ dashboardTabId: string,
+ payload?: Record<string, any>,
+) => {
+ const dashboardsContexts = getDashboardContextLocalStorage();
+ setItem(LocalStorageKeys.dashboard__explore_context, {
+ ...dashboardsContexts,
+ [dashboardTabId]: payload,
+ });
+};
+
+const useSyncDashboardStateWithLocalStorage = () => {
+ const labelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.label_colors || {},
+ );
+ const sharedLabelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.shared_label_colors || {},
+ );
+ const colorScheme = useSelector<RootState, string>(
+ state => state.dashboardState?.colorScheme,
+ );
+ const chartConfiguration = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo.metadata?.chart_configuration,
+ );
+ const nativeFilters = useSelector<RootState, Filters>(
+ state => state.nativeFilters.filters,
+ );
+ const dataMask = useSelector<RootState, DataMaskStateWithId>(
+ state => state.dataMask,
+ );
+ const dashboardId = useSelector<RootState, number>(
+ state => state.dashboardInfo.id,
+ );
+ const filterBoxFilters = useSelector<RootState, Record<string, any>>(() =>
+ getActiveFilters(),
+ );
+ const dashboardTabId = useMemo(() => shortid.generate(), []);
+
+ useEffect(() => {
+ const payload = {
+ labelColors,
+ sharedLabelColors,
+ colorScheme,
+ chartConfiguration,
+ nativeFilters,
+ dataMask,
+ dashboardId,
+ filterBoxFilters,
+ };
+ updateDashboardTabLocalStorage(dashboardTabId, payload);
+ return () => {
+ updateDashboardTabLocalStorage(dashboardTabId, {
+ ...payload,
+ isRedundant: true,
+ });
Review Comment:
That might lead to race conditions between removing local storage key and consuming it in Explore.
Case 1: You open Explore in a new tab and close the tab with dashboard before Explore mounts - key doesn't exist anymore in Explore.
Case 2: You open Explore in the same tab as dashboard - Dashboard component unmounts and removes the key before Explore mounts. That case could be handled by always removing the key in Explore instead of on Dashboard unmount. However, that leads us to Case 3: you open Explore in a new tab, key is removed on Explore mount, then you go back to the tab with dashboard and open a new tab with Explore - key doesn't exist anymore in this tab.
I think flagging key as redundant and removing it on next dashboard action is the safest approach here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r925441656
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -97,6 +98,7 @@ const SliceHeader: FC<SliceHeaderProps> = ({
}) => {
const dispatch = useDispatch();
const uiConfig = useUiConfig();
+ const dashboardTabId = useContext(DashboardTabIdContext);
Review Comment:
We only need to pass dashboardTabId deep into the component tree, with no intention of ever modifying it or interacting with other redux state variables. I think for such use cases Context is optimal
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r925456600
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -352,12 +354,10 @@ class SliceHeaderControls extends React.PureComponent<
/>
}
modalFooter={
- <Button
- buttonStyle="secondary"
- buttonSize="small"
- onClick={this.props.onExploreChart}
- >
- {t('Edit chart')}
+ <Button buttonStyle="secondary" buttonSize="small">
+ <Link to={this.props.exploreUrl ?? '#'}>
+ {t('Edit chart')}
+ </Link>
Review Comment:
Yeah it does. I'll change that to onClick with history.push
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r926582968
##########
superset-frontend/src/dashboard/actions/hydrate.js:
##########
@@ -256,6 +258,19 @@ export const hydrateDashboard =
layout[layoutId].meta.sliceName = slice.slice_name;
}
});
+
+ // make sure that parents tree is built
+ if (
+ Object.values(layout).some(
+ element => element.id !== DASHBOARD_ROOT_ID && !element.parents,
+ )
+ ) {
+ updateComponentParentsList({
Review Comment:
Can we fix the sample dashboards metadata instead of introducing this logic? We can do it in a follow-up if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1189377588
@jinghua-qa Can you help with testing please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r923605104
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,12 +90,95 @@ type PageProps = {
idOrSlug: string;
};
+const getDashboardContextLocalStorage = () => {
+ const dashboardsContexts = getItem(
+ LocalStorageKeys.dashboard__explore_context,
+ {},
+ );
+ return Object.fromEntries(
+ Object.entries(dashboardsContexts).filter(
+ ([, value]) => !value.isRedundant,
+ ),
+ );
+};
+
+const updateDashboardTabLocalStorage = (
+ dashboardTabId: string,
+ payload?: Record<string, any>,
+) => {
+ const dashboardsContexts = getDashboardContextLocalStorage();
+ setItem(LocalStorageKeys.dashboard__explore_context, {
+ ...dashboardsContexts,
+ [dashboardTabId]: payload,
+ });
+};
+
+const useSyncDashboardStateWithLocalStorage = () => {
+ const labelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.label_colors || {},
+ );
+ const sharedLabelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.shared_label_colors || {},
+ );
+ const colorScheme = useSelector<RootState, string>(
+ state => state.dashboardState?.colorScheme,
+ );
+ const chartConfiguration = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo.metadata?.chart_configuration,
+ );
+ const nativeFilters = useSelector<RootState, Filters>(
+ state => state.nativeFilters.filters,
+ );
+ const dataMask = useSelector<RootState, DataMaskStateWithId>(
+ state => state.dataMask,
+ );
+ const dashboardId = useSelector<RootState, number>(
+ state => state.dashboardInfo.id,
+ );
+ const filterBoxFilters = useSelector<RootState, Record<string, any>>(() =>
+ getActiveFilters(),
+ );
Review Comment:
Can we consolidate all these into one single state object, and maybe reuse [DashboardPermalinkState](https://github.com/apache/superset/blob/2a705406e1883834bb6696c1585ef65f787ce7b3/superset-frontend/src/dashboard/types.ts#L142-L147) (or at least extend from it and consolidate variable names) for it? We can rename it to something like `PersistableDashboardState` if necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r926350362
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,12 +90,95 @@ type PageProps = {
idOrSlug: string;
};
+const getDashboardContextLocalStorage = () => {
+ const dashboardsContexts = getItem(
+ LocalStorageKeys.dashboard__explore_context,
+ {},
+ );
+ return Object.fromEntries(
+ Object.entries(dashboardsContexts).filter(
+ ([, value]) => !value.isRedundant,
+ ),
+ );
+};
+
+const updateDashboardTabLocalStorage = (
+ dashboardTabId: string,
+ payload?: Record<string, any>,
+) => {
+ const dashboardsContexts = getDashboardContextLocalStorage();
+ setItem(LocalStorageKeys.dashboard__explore_context, {
+ ...dashboardsContexts,
+ [dashboardTabId]: payload,
+ });
+};
+
+const useSyncDashboardStateWithLocalStorage = () => {
+ const labelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.label_colors || {},
+ );
+ const sharedLabelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.shared_label_colors || {},
+ );
+ const colorScheme = useSelector<RootState, string>(
+ state => state.dashboardState?.colorScheme,
+ );
+ const chartConfiguration = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo.metadata?.chart_configuration,
+ );
+ const nativeFilters = useSelector<RootState, Filters>(
+ state => state.nativeFilters.filters,
+ );
+ const dataMask = useSelector<RootState, DataMaskStateWithId>(
+ state => state.dataMask,
+ );
+ const dashboardId = useSelector<RootState, number>(
+ state => state.dashboardInfo.id,
+ );
+ const filterBoxFilters = useSelector<RootState, Record<string, any>>(() =>
+ getActiveFilters(),
+ );
Review Comment:
I ended up creating a new state interface. The state that we save in local storage is trimmed specifically for the use of Explore in order to reduce the size of saved payload (to reduce the risk of overflowing the local storage), so extending interfaces used in Dashboard wasn't applicable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r926064984
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -97,6 +98,7 @@ const SliceHeader: FC<SliceHeaderProps> = ({
}) => {
const dispatch = useDispatch();
const uiConfig = useUiConfig();
+ const dashboardTabId = useContext(DashboardTabIdContext);
Review Comment:
This all good. I'm not a fan of Redux anyway... just thought creating a new provider has its own overhead---i.e., too many nested context provider always feels wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje merged pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje merged PR #20743:
URL: https://github.com/apache/superset/pull/20743
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r967228792
##########
superset-frontend/src/explore/ExplorePage.tsx:
##########
@@ -57,6 +61,43 @@ const fetchExploreData = async (exploreUrlParams: URLSearchParams) => {
}
};
+const getDashboardContextFormData = () => {
+ const dashboardPageId = getUrlParam(URL_PARAMS.dashboardPageId);
+ const sliceId = getUrlParam(URL_PARAMS.sliceId) || 0;
+ let dashboardContextWithFilters = {};
+ if (dashboardPageId) {
+ const {
+ labelColors,
+ sharedLabelColors,
+ colorScheme,
+ chartConfiguration,
+ nativeFilters,
+ filterBoxFilters,
+ dataMask,
+ dashboardId,
+ } =
Review Comment:
@kgabryje I believe this logic introduced a regression, i.e., `dataMask` could be `undefined` resulting in an error being thrown [here](https://github.com/apache/superset/blob/e3c6380258a46467ac8e09e64db60aad46bc4655/superset-frontend/src/dashboard/util/activeAllDashboardFilters.ts#L52). Would you be able to look into this issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jinghua-qa commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1192255776
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r925418196
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,12 +90,95 @@ type PageProps = {
idOrSlug: string;
};
+const getDashboardContextLocalStorage = () => {
+ const dashboardsContexts = getItem(
+ LocalStorageKeys.dashboard__explore_context,
+ {},
+ );
+ return Object.fromEntries(
+ Object.entries(dashboardsContexts).filter(
+ ([, value]) => !value.isRedundant,
+ ),
+ );
+};
+
+const updateDashboardTabLocalStorage = (
+ dashboardTabId: string,
+ payload?: Record<string, any>,
+) => {
+ const dashboardsContexts = getDashboardContextLocalStorage();
+ setItem(LocalStorageKeys.dashboard__explore_context, {
+ ...dashboardsContexts,
+ [dashboardTabId]: payload,
+ });
+};
+
+const useSyncDashboardStateWithLocalStorage = () => {
+ const labelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.label_colors || {},
+ );
+ const sharedLabelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.shared_label_colors || {},
+ );
+ const colorScheme = useSelector<RootState, string>(
+ state => state.dashboardState?.colorScheme,
+ );
+ const chartConfiguration = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo.metadata?.chart_configuration,
+ );
+ const nativeFilters = useSelector<RootState, Filters>(
+ state => state.nativeFilters.filters,
+ );
+ const dataMask = useSelector<RootState, DataMaskStateWithId>(
+ state => state.dataMask,
+ );
+ const dashboardId = useSelector<RootState, number>(
+ state => state.dashboardInfo.id,
+ );
+ const filterBoxFilters = useSelector<RootState, Record<string, any>>(() =>
+ getActiveFilters(),
+ );
+ const dashboardTabId = useMemo(() => shortid.generate(), []);
Review Comment:
Good point!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r925421187
##########
superset-frontend/src/explore/ExplorePage.tsx:
##########
@@ -57,6 +67,58 @@ const fetchExploreData = async (exploreUrlParams: URLSearchParams) => {
}
};
+const getDashboardContextFormData = () => {
+ const dashboardTabId = getUrlParam(URL_PARAMS.dashboardTabId);
+ const sliceId = getUrlParam(URL_PARAMS.sliceId) || 0;
+ let dashboardContextWithFilters = {};
+ if (dashboardTabId) {
+ const {
+ labelColors,
+ sharedLabelColors,
+ colorScheme,
+ chartConfiguration,
+ nativeFilters,
+ filterBoxFilters,
+ dataMask,
+ dashboardId,
+ } =
+ getItem(LocalStorageKeys.dashboard__explore_context, {})[
+ dashboardTabId
+ ] || {};
+ dashboardContextWithFilters = getFormDataWithExtraFilters({
+ chart: { id: sliceId },
+ filters: Object.fromEntries(
+ (
+ Object.entries(filterBoxFilters) as [
+ string,
+ {
+ scope: number[];
+ values: DataRecordValue[];
+ },
+ ][]
+ )
+ .filter(([, filter]) => filter.scope.includes(sliceId))
+ .map(([key, filter]) => [
+ getChartIdAndColumnFromFilterKey(key).column,
+ filter.values,
+ ]),
Review Comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jinghua-qa commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1191650490
> Code LGTM. Can we get @jinghua-qa's approval before merging the PR? @jinghua-qa It would be a good idea to run our complete test suite on this to make sure everything is working 😉
OK, i can create a test branch and deploy a workspace to run automation today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1196917425
> I'm afraid it breaks `DASHBOARD_EDIT_CHART_IN_NEW_TAB: True`. Is there a plan to remove this feature flag?
Yes you're right, that flag is no longer needed. Now you can click "Edit chart" like any other link to open in a new tab - either cmd + click, or right click + "open in a new tab", or middle mouse button.
I'll remove that feature flag as a follow up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r924810070
##########
superset-frontend/src/dashboard/actions/hydrate.js:
##########
@@ -333,9 +348,21 @@ export const hydrateDashboard =
rootPath: [DASHBOARD_ROOT_ID],
excluded: [chartId], // By default it doesn't affects itself
},
+ chartsInScope: Array.from(sliceIds),
},
};
}
+ if (
+ behaviors.includes(Behavior.INTERACTIVE_CHART) &&
+ !metadata.chart_configuration[chartId].crossFilters?.chartsInScope
+ ) {
+ metadata.chart_configuration[chartId].crossFilters.chartsInScope =
Review Comment:
Optimization for cross filters (works the same as the one implemented for native filters a few months ago) to speed up finding charts in scope. Also, thanks to that, we don't need to save dashboard layout in local storage
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r924808703
##########
superset-frontend/src/dashboard/actions/hydrate.js:
##########
@@ -256,6 +258,19 @@ export const hydrateDashboard =
layout[layoutId].meta.sliceName = slice.slice_name;
}
});
+
+ // make sure that parents tree is built
+ if (
+ Object.values(layout).some(
+ element => element.id !== DASHBOARD_ROOT_ID && !element.parents,
+ )
+ ) {
+ updateComponentParentsList({
Review Comment:
Some sample dashboards don't have `parents` in their layout metadata, which caused e2e tests to fail
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1190382088
Thanks for the PR @kgabryje. This is a nice improvement now that we have the permalink feature in place and form data keys are not shared anymore.
I noticed that the `form_data_key` is still being generated in Explore. Is there a reason for it? If it's not needed, can we remove it and mark the `explore/form_data` endpoint as deprecated? You can add a [deprecation warning](https://github.com/apache/superset/blob/81bd4968d0a916cb2a20e47b20e31a1434be4f46/superset/views/core.py#L750) to the API file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1188300654
@kgabryje Ephemeral environment spinning up at http://54.203.155.41:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r926064047
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,12 +90,95 @@ type PageProps = {
idOrSlug: string;
};
+const getDashboardContextLocalStorage = () => {
+ const dashboardsContexts = getItem(
+ LocalStorageKeys.dashboard__explore_context,
+ {},
+ );
+ return Object.fromEntries(
+ Object.entries(dashboardsContexts).filter(
+ ([, value]) => !value.isRedundant,
+ ),
+ );
+};
+
+const updateDashboardTabLocalStorage = (
+ dashboardTabId: string,
+ payload?: Record<string, any>,
+) => {
+ const dashboardsContexts = getDashboardContextLocalStorage();
+ setItem(LocalStorageKeys.dashboard__explore_context, {
+ ...dashboardsContexts,
+ [dashboardTabId]: payload,
+ });
+};
+
+const useSyncDashboardStateWithLocalStorage = () => {
+ const labelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.label_colors || {},
+ );
+ const sharedLabelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.shared_label_colors || {},
+ );
+ const colorScheme = useSelector<RootState, string>(
+ state => state.dashboardState?.colorScheme,
+ );
+ const chartConfiguration = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo.metadata?.chart_configuration,
+ );
+ const nativeFilters = useSelector<RootState, Filters>(
+ state => state.nativeFilters.filters,
+ );
+ const dataMask = useSelector<RootState, DataMaskStateWithId>(
+ state => state.dataMask,
+ );
+ const dashboardId = useSelector<RootState, number>(
+ state => state.dashboardInfo.id,
+ );
+ const filterBoxFilters = useSelector<RootState, Record<string, any>>(() =>
+ getActiveFilters(),
+ );
+ const dashboardTabId = useMemo(() => shortid.generate(), []);
+
+ useEffect(() => {
+ const payload = {
+ labelColors,
+ sharedLabelColors,
+ colorScheme,
+ chartConfiguration,
+ nativeFilters,
+ dataMask,
+ dashboardId,
+ filterBoxFilters,
+ };
+ updateDashboardTabLocalStorage(dashboardTabId, payload);
+ return () => {
+ updateDashboardTabLocalStorage(dashboardTabId, {
+ ...payload,
+ isRedundant: true,
+ });
Review Comment:
This makes sense. Thanks for the explanation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1193270452
@jinghua-qa Ephemeral environment spinning up at http://35.164.111.177:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] openaphid commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
openaphid commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1196896857
I'm afraid it breaks `DASHBOARD_EDIT_CHART_IN_NEW_TAB: True`. Is there a plan to remove this feature flag?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jinghua-qa commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1193269882
/testenv up FEATURE_DASHBOARD_CROSS_FILTERS=true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1194361729
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jinghua-qa commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1193609217
I found one issue that when open the chart from dashboard with color theme, and then save the chart as new chart, the color theme for the new chart still not able to change the color theme. Other than that i think the changes LGTM.
https://user-images.githubusercontent.com/81597121/180700795-d0df7584-0553-47f9-b682-8bdedd4e5571.mov
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r924813620
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,12 +90,95 @@ type PageProps = {
idOrSlug: string;
};
+const getDashboardContextLocalStorage = () => {
+ const dashboardsContexts = getItem(
+ LocalStorageKeys.dashboard__explore_context,
+ {},
+ );
+ return Object.fromEntries(
+ Object.entries(dashboardsContexts).filter(
+ ([, value]) => !value.isRedundant,
Review Comment:
A new dashboard tab id is generated on each dashboard page opening. We mark ids as redundant when user leaves the dashboard, because they won't be reused. Then we remove redundant dashboard contexts from local storage in order not to clutter it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r924810070
##########
superset-frontend/src/dashboard/actions/hydrate.js:
##########
@@ -333,9 +348,21 @@ export const hydrateDashboard =
rootPath: [DASHBOARD_ROOT_ID],
excluded: [chartId], // By default it doesn't affects itself
},
+ chartsInScope: Array.from(sliceIds),
},
};
}
+ if (
+ behaviors.includes(Behavior.INTERACTIVE_CHART) &&
+ !metadata.chart_configuration[chartId].crossFilters?.chartsInScope
+ ) {
+ metadata.chart_configuration[chartId].crossFilters.chartsInScope =
Review Comment:
Optimization for cross filters (works the same as the one implemented for native filters a few months ago) to speed up finding charts in scope
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #20743:
URL: https://github.com/apache/superset/pull/20743#issuecomment-1189388792
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r923611803
##########
superset-frontend/src/explore/ExplorePage.tsx:
##########
@@ -57,6 +67,58 @@ const fetchExploreData = async (exploreUrlParams: URLSearchParams) => {
}
};
+const getDashboardContextFormData = () => {
+ const dashboardTabId = getUrlParam(URL_PARAMS.dashboardTabId);
+ const sliceId = getUrlParam(URL_PARAMS.sliceId) || 0;
+ let dashboardContextWithFilters = {};
+ if (dashboardTabId) {
+ const {
+ labelColors,
+ sharedLabelColors,
+ colorScheme,
+ chartConfiguration,
+ nativeFilters,
Review Comment:
Is `nativeFilters` the full metadata of all native filters? Do we really need to carry all this information to the Explore page? I don't see it being used here.
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -306,13 +307,14 @@ class SliceHeaderControls extends React.PureComponent<
)}
{this.props.supersetCanExplore && (
- <Menu.Item
- key={MENU_KEYS.EXPLORE_CHART}
- onClick={({ domEvent }) => this.props.onExploreChart(domEvent)}
- >
- <Tooltip title={getSliceHeaderTooltip(this.props.slice.slice_name)}>
- {t('Edit chart')}
- </Tooltip>
+ <Menu.Item key={MENU_KEYS.EXPLORE_CHART}>
+ <Link to={this.props.exploreUrl ?? '#'}>
Review Comment:
Instead of `?? '#'` why not just remove the whole link in case url is not available?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,12 +90,95 @@ type PageProps = {
idOrSlug: string;
};
+const getDashboardContextLocalStorage = () => {
+ const dashboardsContexts = getItem(
+ LocalStorageKeys.dashboard__explore_context,
+ {},
+ );
+ return Object.fromEntries(
+ Object.entries(dashboardsContexts).filter(
+ ([, value]) => !value.isRedundant,
+ ),
+ );
+};
+
+const updateDashboardTabLocalStorage = (
+ dashboardTabId: string,
+ payload?: Record<string, any>,
+) => {
+ const dashboardsContexts = getDashboardContextLocalStorage();
+ setItem(LocalStorageKeys.dashboard__explore_context, {
+ ...dashboardsContexts,
+ [dashboardTabId]: payload,
+ });
+};
+
+const useSyncDashboardStateWithLocalStorage = () => {
+ const labelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.label_colors || {},
+ );
+ const sharedLabelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.shared_label_colors || {},
+ );
+ const colorScheme = useSelector<RootState, string>(
+ state => state.dashboardState?.colorScheme,
+ );
+ const chartConfiguration = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo.metadata?.chart_configuration,
+ );
+ const nativeFilters = useSelector<RootState, Filters>(
+ state => state.nativeFilters.filters,
+ );
+ const dataMask = useSelector<RootState, DataMaskStateWithId>(
+ state => state.dataMask,
+ );
+ const dashboardId = useSelector<RootState, number>(
+ state => state.dashboardInfo.id,
+ );
+ const filterBoxFilters = useSelector<RootState, Record<string, any>>(() =>
+ getActiveFilters(),
+ );
+ const dashboardTabId = useMemo(() => shortid.generate(), []);
Review Comment:
Can we rename this to `dashboardPageId` to avoid confusion with dashboard tabs?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,12 +90,95 @@ type PageProps = {
idOrSlug: string;
};
+const getDashboardContextLocalStorage = () => {
+ const dashboardsContexts = getItem(
+ LocalStorageKeys.dashboard__explore_context,
+ {},
+ );
+ return Object.fromEntries(
+ Object.entries(dashboardsContexts).filter(
+ ([, value]) => !value.isRedundant,
+ ),
+ );
+};
+
+const updateDashboardTabLocalStorage = (
+ dashboardTabId: string,
+ payload?: Record<string, any>,
+) => {
+ const dashboardsContexts = getDashboardContextLocalStorage();
+ setItem(LocalStorageKeys.dashboard__explore_context, {
+ ...dashboardsContexts,
+ [dashboardTabId]: payload,
+ });
+};
+
+const useSyncDashboardStateWithLocalStorage = () => {
+ const labelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.label_colors || {},
+ );
+ const sharedLabelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.shared_label_colors || {},
+ );
+ const colorScheme = useSelector<RootState, string>(
+ state => state.dashboardState?.colorScheme,
+ );
+ const chartConfiguration = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo.metadata?.chart_configuration,
+ );
+ const nativeFilters = useSelector<RootState, Filters>(
+ state => state.nativeFilters.filters,
+ );
+ const dataMask = useSelector<RootState, DataMaskStateWithId>(
+ state => state.dataMask,
+ );
+ const dashboardId = useSelector<RootState, number>(
+ state => state.dashboardInfo.id,
+ );
+ const filterBoxFilters = useSelector<RootState, Record<string, any>>(() =>
+ getActiveFilters(),
+ );
+ const dashboardTabId = useMemo(() => shortid.generate(), []);
+
+ useEffect(() => {
+ const payload = {
+ labelColors,
+ sharedLabelColors,
+ colorScheme,
+ chartConfiguration,
+ nativeFilters,
+ dataMask,
+ dashboardId,
+ filterBoxFilters,
+ };
+ updateDashboardTabLocalStorage(dashboardTabId, payload);
+ return () => {
+ updateDashboardTabLocalStorage(dashboardTabId, {
+ ...payload,
+ isRedundant: true,
+ });
Review Comment:
Instead of adding a `isRedundant` flag, can we just remove it?
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -97,6 +98,7 @@ const SliceHeader: FC<SliceHeaderProps> = ({
}) => {
const dispatch = useDispatch();
const uiConfig = useUiConfig();
+ const dashboardTabId = useContext(DashboardTabIdContext);
Review Comment:
Why do we use a new ReactContext for this? Can this be a Redux state?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,12 +90,95 @@ type PageProps = {
idOrSlug: string;
};
+const getDashboardContextLocalStorage = () => {
+ const dashboardsContexts = getItem(
+ LocalStorageKeys.dashboard__explore_context,
+ {},
+ );
+ return Object.fromEntries(
+ Object.entries(dashboardsContexts).filter(
+ ([, value]) => !value.isRedundant,
+ ),
+ );
+};
+
+const updateDashboardTabLocalStorage = (
+ dashboardTabId: string,
+ payload?: Record<string, any>,
+) => {
+ const dashboardsContexts = getDashboardContextLocalStorage();
+ setItem(LocalStorageKeys.dashboard__explore_context, {
+ ...dashboardsContexts,
+ [dashboardTabId]: payload,
+ });
+};
+
+const useSyncDashboardStateWithLocalStorage = () => {
+ const labelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.label_colors || {},
+ );
+ const sharedLabelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.shared_label_colors || {},
+ );
+ const colorScheme = useSelector<RootState, string>(
+ state => state.dashboardState?.colorScheme,
+ );
+ const chartConfiguration = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo.metadata?.chart_configuration,
+ );
+ const nativeFilters = useSelector<RootState, Filters>(
+ state => state.nativeFilters.filters,
+ );
+ const dataMask = useSelector<RootState, DataMaskStateWithId>(
+ state => state.dataMask,
+ );
+ const dashboardId = useSelector<RootState, number>(
+ state => state.dashboardInfo.id,
+ );
+ const filterBoxFilters = useSelector<RootState, Record<string, any>>(() =>
+ getActiveFilters(),
+ );
Review Comment:
Can we consolidate all these into one single state object, and maybe reuse [DashboardPermalinkState](https://github.com/apache/superset/blob/2a705406e1883834bb6696c1585ef65f787ce7b3/superset-frontend/src/dashboard/types.ts#L142-L147) (or at least extend from / consolidate variable names) for it? We can rename it to something like `PersistableDashboardState` if necessary.
##########
superset-frontend/src/explore/controlUtils/getFormDataWithDashboardContext.ts:
##########
@@ -0,0 +1,146 @@
+/**
+ * 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.
+ */
+import {
+ EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS,
+ EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS,
+ isDefined,
+ JsonObject,
+ ensureIsArray,
+ QueryObjectFilterClause,
+ SimpleAdhocFilter,
+} from '@superset-ui/core';
+import { NO_TIME_RANGE } from '../constants';
+
+const simpleFilterToAdhoc = (
+ filterClause: QueryObjectFilterClause,
+ clause = 'where',
+) => {
+ const result = {
+ clause: clause.toUpperCase(),
+ expressionType: 'SIMPLE',
+ operator: filterClause.op,
+ subject: filterClause.col,
+ comparator: 'val' in filterClause ? filterClause.val : undefined,
+ } as SimpleAdhocFilter;
+ if (filterClause.isExtra) {
+ Object.assign(result, {
+ isExtra: true,
+ filterOptionName: `filter_${Math.random()
+ .toString(36)
+ .substring(2, 15)}_${Math.random().toString(36).substring(2, 15)}`,
+ });
+ }
+
+ return result;
+};
+
+const mergeFilterBoxToFormData = (formData: Record<string, any>) => {
+ const dateColumns = {
+ __time_range: 'time_range',
+ __time_col: 'granularity_sqla',
+ __time_grain: 'time_grain_sqla',
+ __granularity: 'granularity',
+ };
+ const appliedTimeExtras = {};
+
+ const newFormData = { ...formData };
+ ensureIsArray(newFormData.extra_filters).forEach(filter => {
+ if (dateColumns[filter.col]) {
+ if (filter.val !== NO_TIME_RANGE) {
+ newFormData[dateColumns[filter.col]] = filter.val;
+ appliedTimeExtras[filter.col] = filter.val;
+ }
+ } else {
+ const adhocFilter = simpleFilterToAdhoc({
+ ...(filter as QueryObjectFilterClause),
+ isExtra: true,
+ });
+ if (!Array.isArray(newFormData.adhocFilters)) {
+ newFormData.adhoc_filters = [adhocFilter];
+ } else if (
+ newFormData.adhoc_filters.some(
+ (existingFilter: SimpleAdhocFilter) =>
+ existingFilter.operator === adhocFilter.operator &&
+ existingFilter.subject === adhocFilter.subject &&
+ ((!('comparator' in existingFilter) &&
+ !('comparator' in adhocFilter)) ||
+ ('comparator' in existingFilter &&
+ 'comparator' in adhocFilter &&
+ existingFilter.comparator === adhocFilter.comparator)),
+ )
+ ) {
+ newFormData.adhoc_filters.push(adhocFilter);
+ }
+ }
+ });
+ newFormData.applied_time_extras = appliedTimeExtras;
+ delete newFormData.extra_filters;
+ return newFormData;
+};
+
+export const getFormDataFromDashboardContext = (formData: JsonObject) => {
Review Comment:
I like this util function!
##########
superset-frontend/src/explore/ExplorePage.tsx:
##########
@@ -57,6 +67,58 @@ const fetchExploreData = async (exploreUrlParams: URLSearchParams) => {
}
};
+const getDashboardContextFormData = () => {
+ const dashboardTabId = getUrlParam(URL_PARAMS.dashboardTabId);
+ const sliceId = getUrlParam(URL_PARAMS.sliceId) || 0;
+ let dashboardContextWithFilters = {};
+ if (dashboardTabId) {
+ const {
+ labelColors,
+ sharedLabelColors,
+ colorScheme,
+ chartConfiguration,
+ nativeFilters,
+ filterBoxFilters,
+ dataMask,
+ dashboardId,
+ } =
+ getItem(LocalStorageKeys.dashboard__explore_context, {})[
+ dashboardTabId
+ ] || {};
+ dashboardContextWithFilters = getFormDataWithExtraFilters({
+ chart: { id: sliceId },
+ filters: Object.fromEntries(
+ (
+ Object.entries(filterBoxFilters) as [
+ string,
+ {
+ scope: number[];
+ values: DataRecordValue[];
+ },
+ ][]
+ )
+ .filter(([, filter]) => filter.scope.includes(sliceId))
+ .map(([key, filter]) => [
+ getChartIdAndColumnFromFilterKey(key).column,
+ filter.values,
+ ]),
Review Comment:
Can L90 to L104 be of its own utility function?
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -352,12 +354,10 @@ class SliceHeaderControls extends React.PureComponent<
/>
}
modalFooter={
- <Button
- buttonStyle="secondary"
- buttonSize="small"
- onClick={this.props.onExploreChart}
- >
- {t('Edit chart')}
+ <Button buttonStyle="secondary" buttonSize="small">
+ <Link to={this.props.exploreUrl ?? '#'}>
+ {t('Edit chart')}
+ </Link>
Review Comment:
Would `<Link >` actually add an `<a>` inside a button element?
Should we use react-router-dom's `useHistory` instead?
https://v5.reactrouter.com/web/api/Hooks/usehistory
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r923611803
##########
superset-frontend/src/explore/ExplorePage.tsx:
##########
@@ -57,6 +67,58 @@ const fetchExploreData = async (exploreUrlParams: URLSearchParams) => {
}
};
+const getDashboardContextFormData = () => {
+ const dashboardTabId = getUrlParam(URL_PARAMS.dashboardTabId);
+ const sliceId = getUrlParam(URL_PARAMS.sliceId) || 0;
+ let dashboardContextWithFilters = {};
+ if (dashboardTabId) {
+ const {
+ labelColors,
+ sharedLabelColors,
+ colorScheme,
+ chartConfiguration,
+ nativeFilters,
Review Comment:
Is `nativeFilters` the full metadata of all native filters? Do we really need to carry it over to the Explore page? I don't see it being used here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20743: feat: Pass dashboard context to explore through local storage
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r925444324
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,12 +90,95 @@ type PageProps = {
idOrSlug: string;
};
+const getDashboardContextLocalStorage = () => {
+ const dashboardsContexts = getItem(
+ LocalStorageKeys.dashboard__explore_context,
+ {},
+ );
+ return Object.fromEntries(
+ Object.entries(dashboardsContexts).filter(
+ ([, value]) => !value.isRedundant,
+ ),
+ );
+};
+
+const updateDashboardTabLocalStorage = (
+ dashboardTabId: string,
+ payload?: Record<string, any>,
+) => {
+ const dashboardsContexts = getDashboardContextLocalStorage();
+ setItem(LocalStorageKeys.dashboard__explore_context, {
+ ...dashboardsContexts,
+ [dashboardTabId]: payload,
+ });
+};
+
+const useSyncDashboardStateWithLocalStorage = () => {
+ const labelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.label_colors || {},
+ );
+ const sharedLabelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.shared_label_colors || {},
+ );
+ const colorScheme = useSelector<RootState, string>(
+ state => state.dashboardState?.colorScheme,
+ );
+ const chartConfiguration = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo.metadata?.chart_configuration,
+ );
+ const nativeFilters = useSelector<RootState, Filters>(
+ state => state.nativeFilters.filters,
+ );
+ const dataMask = useSelector<RootState, DataMaskStateWithId>(
+ state => state.dataMask,
+ );
+ const dashboardId = useSelector<RootState, number>(
+ state => state.dashboardInfo.id,
+ );
+ const filterBoxFilters = useSelector<RootState, Record<string, any>>(() =>
+ getActiveFilters(),
+ );
Review Comment:
Good idea!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org