Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Remove CROSS_REFERENCES feature flag #21815

Merged
merged 4 commits into from
Oct 21, 2022
Merged

chore: Remove CROSS_REFERENCES feature flag #21815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5871c0c
Remove feature flag
geido Oct 14, 2022
ad82a9d
Merge branch 'master' of https://github.com/apache/superset into chor…
geido Oct 18, 2022
881a027
Remove feature flag from resources
geido Oct 18, 2022
da3399b
Remove trim
geido Oct 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion RESOURCES/FEATURE_FLAGS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ These features are considered **unfinished** and should only be used on developm
[//]: # "PLEASE KEEP THE LIST SORTED ALPHABETICALLY"

- CLIENT_CACHE
- CROSS_REFERENCES
- DASHBOARD_CACHE
- DASHBOARD_NATIVE_FILTERS_SET
- DISABLE_DATASET_SOURCE_EDIT
Expand Down
1 change: 0 additions & 1 deletion superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ export enum FeatureFlag {
ALLOW_DASHBOARD_DOMAIN_SHARDING = 'ALLOW_DASHBOARD_DOMAIN_SHARDING',
ALLOW_FULL_CSV_EXPORT = 'ALLOW_FULL_CSV_EXPORT',
CLIENT_CACHE = 'CLIENT_CACHE',
CROSS_REFERENCES = 'CROSS_REFERENCES',
DASHBOARD_CROSS_FILTERS = 'DASHBOARD_CROSS_FILTERS',
DASHBOARD_EDIT_CHART_IN_NEW_TAB = 'DASHBOARD_EDIT_CHART_IN_NEW_TAB',
DASHBOARD_FILTERS_EXPERIMENTAL = 'DASHBOARD_FILTERS_EXPERIMENTAL',
Expand Down
5 changes: 1 addition & 4 deletions superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ import { Tooltip } from 'src/components/Tooltip';
import {
CategoricalColorNamespace,
css,
FeatureFlag,
isFeatureEnabled,
logging,
SupersetClient,
t,
Expand Down Expand Up @@ -166,8 +164,7 @@ export const ExploreChartHeader = ({
? t('Added to %s dashboard(s)', metadata.dashboards.length)
: t('Not added to any dashboard'),
description:
metadata.dashboards.length > 0 &&
isFeatureEnabled(FeatureFlag.CROSS_REFERENCES)
metadata.dashboards.length > 0
? t(
'You can preview the list of dashboards on the chart settings dropdown.',
)
Expand Down
2 changes: 1 addition & 1 deletion ...set-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ const DashboardsSubMenu = ({
margin: ${theme.gridUnit * 2}px ${theme.gridUnit * 3}px;
`}
value={dashboardSearch}
onChange={e => setDashboardSearch(e.currentTarget.value?.trim())}
onChange={e => setDashboardSearch(e.currentTarget.value)}
/>
)}
<div
Expand Down
20 changes: 9 additions & 11 deletions superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,17 +255,15 @@ export const useExploreAdditionalActionsMenu = (
{t('Edit chart properties')}
</Menu.Item>
)}
{isFeatureEnabled(FeatureFlag.CROSS_REFERENCES) && (
<Menu.SubMenu
title={t('Dashboards added to')}
key={MENU_KEYS.DASHBOARDS_ADDED_TO}
>
<DashboardsSubMenu
chartId={slice?.slice_id}
dashboards={dashboards}
/>
</Menu.SubMenu>
)}
<Menu.SubMenu
title={t('Dashboards added to')}
key={MENU_KEYS.DASHBOARDS_ADDED_TO}
>
<DashboardsSubMenu
chartId={slice?.slice_id}
dashboards={dashboards}
/>
</Menu.SubMenu>
<Menu.Divider />
</>
<Menu.SubMenu title={t('Download')} key={MENU_KEYS.DOWNLOAD_SUBMENU}>
Expand Down
69 changes: 29 additions & 40 deletions superset-frontend/src/views/CRUD/chart/ChartList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,6 @@ function ChartList(props: ChartListProps) {
const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }];
const enableBroadUserAccess =
bootstrapData?.common?.conf?.ENABLE_BROAD_ACTIVITY_ACCESS;
const crossRefEnabled = isFeatureEnabled(FeatureFlag.CROSS_REFERENCES);
const handleBulkChartExport = (chartsToExport: Chart[]) => {
const ids = chartsToExport.map(({ id }) => id);
handleResourceExport('chart', ids, () => {
Expand Down Expand Up @@ -306,30 +305,6 @@ function ChartList(props: ChartListProps) {
};
};

const dashboardsCol = useMemo(
() => ({
Cell: ({
row: {
original: { dashboards },
},
}: any) => (
<CrossLinks
crossLinks={ensureIsArray(dashboards).map(
(d: ChartLinkedDashboard) => ({
title: d.dashboard_title,
id: d.id,
}),
)}
/>
),
Header: t('Dashboards added to'),
accessor: 'dashboards',
disableSortBy: true,
size: 'xxl',
}),
[],
);

const columns = useMemo(
() => [
{
Expand Down Expand Up @@ -407,7 +382,26 @@ function ChartList(props: ChartListProps) {
disableSortBy: true,
size: 'xl',
},
...(crossRefEnabled ? [dashboardsCol] : []),
{
Cell: ({
row: {
original: { dashboards },
},
}: any) => (
<CrossLinks
crossLinks={ensureIsArray(dashboards).map(
(d: ChartLinkedDashboard) => ({
title: d.dashboard_title,
id: d.id,
}),
)}
/>
),
Header: t('Dashboards added to'),
accessor: 'dashboards',
disableSortBy: true,
size: 'xxl',
},
{
Cell: ({
row: {
Expand Down Expand Up @@ -574,19 +568,6 @@ function ChartList(props: ChartListProps) {
[],
);

const dashboardsFilter: Filter = useMemo(
() => ({
Header: t('Dashboards'),
id: 'dashboards',
input: 'select',
operator: FilterOperator.relationManyMany,
unfilteredLabel: t('All'),
fetchSelects: fetchDashboards,
paginate: true,
}),
[],
);

const filters: Filters = useMemo(
() => [
{
Expand Down Expand Up @@ -665,7 +646,15 @@ function ChartList(props: ChartListProps) {
fetchSelects: createFetchDatasets,
paginate: true,
},
...(crossRefEnabled ? [dashboardsFilter] : []),
{
Header: t('Dashboards'),
id: 'dashboards',
input: 'select',
operator: FilterOperator.relationManyMany,
unfilteredLabel: t('All'),
fetchSelects: fetchDashboards,
paginate: true,
},
...(userId ? [favoritesFilter] : []),
{
Header: t('Certified'),
Expand Down
1 change: 0 additions & 1 deletion superset/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,6 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
"EMBEDDABLE_CHARTS": True,
"DRILL_TO_DETAIL": False,
"DATAPANEL_CLOSED_BY_DEFAULT": False,
"CROSS_REFERENCES": False,
Copy link
Member

Choose a reason for hiding this comment

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

Is the preferred strategy to first enable the CROSS_REFERENCES feature by default and then deprecate the feature in version 3.0?

Also SIP-57 changing the default or removing a feature flag is deemed a breaking change and thus the appropriate actions need to be undertaken.

Copy link
Member

Choose a reason for hiding this comment

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

This one is a little funny, because the feature flag never went out in a version, and will not. So adding and removing it between published versions was perceived as a non-issue. We were hoping to implement a good means of launching darkly using this sort of ephemeral feature flag in lieu of a giant feature branch. Perhaps if we need to do this again, we can add a new category of "Pending release" or something to FEATURE_FLAGS.md to explain these?

I'll bring this up in Town Hall, and be seeking more opinions in general, but hopefully we can let this one through since the FF was never released or intended to be released.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the context @rusackas. This SGTM.

}

# Feature flags may also be set via 'SUPERSET_FEATURE_' prefixed environment vars.
Expand Down
1 change: 0 additions & 1 deletion tests/integration_tests/superset_test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@
"ALERT_REPORTS": True,
"DASHBOARD_NATIVE_FILTERS": True,
"DRILL_TO_DETAIL": True,
"CROSS_REFERENCES": True,
}

WEBDRIVER_BASEURL = "http://0.0.0.0:8081/"
Expand Down