Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

chore!: remove ENABLE_REACT_CRUD_VIEWS feature flag (permanently enable) #19231

Merged
merged 9 commits into from
Mar 18, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .github/workflows/superset-e2e.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ jobs:
browser: ["chrome"]
env:
FLASK_ENV: development
ENABLE_REACT_CRUD_VIEWS: true
SUPERSET_CONFIG: tests.integration_tests.superset_test_config
SUPERSET__SQLALCHEMY_DATABASE_URI: postgresql+psycopg2://superset:[email protected]:15432/superset
PYTHONPATH: ${{ github.workspace }}
Expand Down
1 change: 0 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -804,7 +804,6 @@ We use [Cypress](https://www.cypress.io/) for integration tests. Tests can be ru
```bash
export SUPERSET_CONFIG=tests.integration_tests.superset_test_config
export SUPERSET_TESTENV=true
export ENABLE_REACT_CRUD_VIEWS=true
export CYPRESS_BASE_URL="http://localhost:8081"
superset db upgrade
superset load_test_users
Expand Down
1 change: 0 additions & 1 deletion RESOURCES/FEATURE_FLAGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,4 +64,3 @@ These features flags currently default to True and **will be removed in a future

- ALLOW_DASHBOARD_DOMAIN_SHARDING
- DISPLAY_MARKDOWN_HTML
- ENABLE_REACT_CRUD_VIEWS
1 change: 1 addition & 0 deletions UPDATING.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ assists people when migrating to a new version.

### Breaking Changes

- [19231](https://github.com/apache/superset/pull/19231): The `ENABLE_REACT_CRUD_VIEWS` feature flag has been removed (permanently enabled). Any deployments which had set this flag to false will need to verify that the React views support their use case.
- [19113](https://github.com/apache/superset/pull/19113): The `ENABLE_JAVASCRIPT_CONTROLS` setting has moved from app config to a feature flag. Any deployments who overrode this setting will now need to override the feature flag from here onward.
- [18976](https://github.com/apache/superset/pull/18976): When running the app in debug mode, the app will default to use `SimpleCache` for `FILTER_STATE_CACHE_CONFIG` and `EXPLORE_FORM_DATA_CACHE_CONFIG`. When running in non-debug mode, a cache backend will need to be defined, otherwise the application will fail to start. For installations using Redis or other caching backends, it is recommended to use the same backend for both cache configs.
- [17881](https://github.com/apache/superset/pull/17881): Previously simple adhoc filter values on string columns were stripped of enclosing single and double quotes. To fully support literal quotes in filters, both single and double quotes will no longer be removed from filter values.
Expand Down
1 change: 0 additions & 1 deletion docker/docker-bootstrap.sh
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ REQUIREMENTS_LOCAL="/app/docker/requirements-local.txt"
if [ "$CYPRESS_CONFIG" == "true" ]; then
export SUPERSET_CONFIG=tests.integration_tests.superset_test_config
export SUPERSET_TESTENV=true
export ENABLE_REACT_CRUD_VIEWS=true
export SUPERSET__SQLALCHEMY_DATABASE_URI=postgresql+psycopg2://superset:superset@db:5432/superset
fi
#
Expand Down
1 change: 0 additions & 1 deletion docker/docker-init.sh
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ if [ "$CYPRESS_CONFIG" == "true" ]; then
ADMIN_PASSWORD="general"
export SUPERSET_CONFIG=tests.superset_test_config
export SUPERSET_TESTENV=true
export ENABLE_REACT_CRUD_VIEWS=true
export SUPERSET__SQLALCHEMY_DATABASE_URI=postgresql+psycopg2://superset:superset@db:5432/superset
fi
# Initialize the database
Expand Down
1 change: 0 additions & 1 deletion docs/docs/contributing/testing-locally.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ We use [Cypress](https://www.cypress.io/) for integration tests. Tests can be ru
```bash
export SUPERSET_CONFIG=tests.integration_tests.superset_test_config
export SUPERSET_TESTENV=true
export ENABLE_REACT_CRUD_VIEWS=true
export CYPRESS_BASE_URL="http://localhost:8081"
superset db upgrade
superset load_test_users
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ export enum FeatureFlag {
THUMBNAILS = 'THUMBNAILS',
LISTVIEWS_DEFAULT_CARD_VIEW = 'LISTVIEWS_DEFAULT_CARD_VIEW',
DISABLE_LEGACY_DATASOURCE_EDITOR = 'DISABLE_LEGACY_DATASOURCE_EDITOR',
ENABLE_REACT_CRUD_VIEWS = 'ENABLE_REACT_CRUD_VIEWS',
DISABLE_DATASET_SOURCE_EDIT = 'DISABLE_DATASET_SOURCE_EDIT',
DISPLAY_MARKDOWN_HTML = 'DISPLAY_MARKDOWN_HTML',
ESCAPE_MARKDOWN_HTML = 'ESCAPE_MARKDOWN_HTML',
Expand Down
23 changes: 3 additions & 20 deletions superset-frontend/src/SqlLab/components/App/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import PropTypes from 'prop-types';
import { bindActionCreators } from 'redux';
import { connect } from 'react-redux';
import { t, supersetTheme, ThemeProvider } from '@superset-ui/core';
import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags';
import throttle from 'lodash/throttle';
import ToastContainer from 'src/components/MessageToasts/ToastContainer';
import {
Expand All @@ -32,7 +31,6 @@ import {
import * as Actions from 'src/SqlLab/actions/sqlLab';
import TabbedSqlEditors from '../TabbedSqlEditors';
import QueryAutoRefresh from '../QueryAutoRefresh';
import QuerySearch from '../QuerySearch';

class App extends React.PureComponent {
constructor(props) {
Expand Down Expand Up @@ -96,29 +94,14 @@ class App extends React.PureComponent {
}

render() {
let content;
if (this.state.hash && this.state.hash === '#search') {
if (isFeatureEnabled(FeatureFlag.ENABLE_REACT_CRUD_VIEWS)) {
Copy link
Member Author

@suddjian suddjian Mar 17, 2022

Choose a reason for hiding this comment

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

I kept this logic consistent with permanently enabling the flag, but the logic doesn't look quite right to me. Shouldn't we be rendering with react instead of location.replaceing when react crud is enabled?

return window.location.replace('/superset/sqllab/history/');
}
content = (
<QuerySearch
actions={this.props.actions}
displayLimit={this.props.common.conf.DISPLAY_MAX_ROW}
/>
);
} else {
content = (
<>
<QueryAutoRefresh />
<TabbedSqlEditors />
</>
);
return window.location.replace('/superset/sqllab/history/');
}
return (
<ThemeProvider theme={supersetTheme}>
<div className="App SqlLab">
{content}
<QueryAutoRefresh />
<TabbedSqlEditors />
<ToastContainer />
</div>
</ThemeProvider>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { Provider } from 'react-redux';
import fetchMock from 'fetch-mock';
import thunk from 'redux-thunk';
import sinon from 'sinon';
import { supersetTheme, ThemeProvider, FeatureFlag } from '@superset-ui/core';
import { supersetTheme, ThemeProvider } from '@superset-ui/core';

import waitForComponentToPaint from 'spec/helpers/waitForComponentToPaint';
import Modal from 'src/components/Modal';
Expand Down Expand Up @@ -70,11 +70,7 @@ describe('DatasourceModal', () => {
let wrapper;
let isFeatureEnabledMock;
beforeEach(async () => {
isFeatureEnabledMock = jest
.spyOn(featureFlags, 'isFeatureEnabled')
.mockImplementation(
featureFlag => featureFlag === FeatureFlag.ENABLE_REACT_CRUD_VIEWS,
);
isFeatureEnabledMock = jest.spyOn(featureFlags, 'isFeatureEnabled');
fetchMock.reset();
wrapper = await mountAndWait();
});
Expand Down Expand Up @@ -125,28 +121,3 @@ describe('DatasourceModal', () => {
).toExist();
});
});

describe('DatasourceModal without legacy data btn', () => {
let wrapper;
let isFeatureEnabledMock;
beforeEach(async () => {
isFeatureEnabledMock = jest
.spyOn(featureFlags, 'isFeatureEnabled')
.mockReturnValue(false);
fetchMock.reset();
wrapper = await mountAndWait();
});

afterAll(() => {
isFeatureEnabledMock.restore();
});

it('hides legacy data source btn', () => {
isFeatureEnabledMock = jest
.spyOn(featureFlags, 'isFeatureEnabled')
.mockReturnValue(false);
expect(
wrapper.find('button[data-test="datasource-modal-legacy-edit"]'),
).not.toExist();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -183,9 +183,9 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
});
};

const showLegacyDatasourceEditor =
isFeatureEnabled(FeatureFlag.ENABLE_REACT_CRUD_VIEWS) &&
!isFeatureEnabled(FeatureFlag.DISABLE_LEGACY_DATASOURCE_EDITOR);
const showLegacyDatasourceEditor = !isFeatureEnabled(
FeatureFlag.DISABLE_LEGACY_DATASOURCE_EDITOR,
);

return (
<StyledDatasourceModal
Expand Down
2 changes: 0 additions & 2 deletions superset-frontend/src/views/routes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
* specific language governing permissions and limitations
* under the License.
*/
import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags';
import React, { lazy } from 'react';

// not lazy loaded since this is the home page.
Expand Down Expand Up @@ -182,7 +181,6 @@ const frontEndRoutes = routes
);

export function isFrontendRoute(path?: string) {
if (!isFeatureEnabled(FeatureFlag.ENABLE_REACT_CRUD_VIEWS)) return false;
if (path) {
const basePath = path.split(/[?#]/)[0]; // strip out query params and link bookmarks
return !!frontEndRoutes[basePath];
Expand Down
5 changes: 0 additions & 5 deletions superset/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -392,11 +392,6 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
"TAGGING_SYSTEM": False,
"SQLLAB_BACKEND_PERSISTENCE": False,
"LISTVIEWS_DEFAULT_CARD_VIEW": False,
# Enables the replacement React views for all the FAB views (list, edit, show) with
# designs introduced in https://github.com/apache/superset/issues/8976
# (SIP-34). This is a work in progress so not all features available in FAB have
# been implemented.
"ENABLE_REACT_CRUD_VIEWS": True,
# When True, this flag allows display of HTML tags in Markdown components
"DISPLAY_MARKDOWN_HTML": True,
# When True, this escapes HTML (rather than rendering it) in Markdown components
Expand Down
3 changes: 0 additions & 3 deletions superset/connectors/sqla/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,4 @@ class RefreshResults:
@expose("/list/")
@has_access
def list(self) -> FlaskResponse:
if not is_feature_enabled("ENABLE_REACT_CRUD_VIEWS"):
return super().list()

return super().render_app_template()
10 changes: 2 additions & 8 deletions superset/views/alerts.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,21 +92,15 @@ class BaseAlertReportView(BaseSupersetView):
@has_access
@permission_name("read")
def list(self) -> FlaskResponse:
if not (
is_feature_enabled("ENABLE_REACT_CRUD_VIEWS")
and is_feature_enabled("ALERT_REPORTS")
):
if not is_feature_enabled("ALERT_REPORTS"):
return abort(404)
return super().render_app_template()

@expose("/<pk>/log/", methods=["GET"])
@has_access
@permission_name("read")
def log(self, pk: int) -> FlaskResponse: # pylint: disable=unused-argument
if not (
is_feature_enabled("ENABLE_REACT_CRUD_VIEWS")
and is_feature_enabled("ALERT_REPORTS")
):
if not is_feature_enabled("ALERT_REPORTS"):
return abort(404)

return super().render_app_template()
Expand Down
7 changes: 0 additions & 7 deletions superset/views/annotations.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
from flask_babel import lazy_gettext as _
from wtforms.validators import StopValidation

from superset import is_feature_enabled
from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP, RouteMethod
from superset.models.annotations import Annotation, AnnotationLayer
from superset.typing import FlaskResponse
Expand Down Expand Up @@ -100,9 +99,6 @@ def pre_update(self, item: "AnnotationModelView") -> None:
@expose("/<pk>/annotation/", methods=["GET"])
@has_access
def annotation(self, pk: int) -> FlaskResponse: # pylint: disable=unused-argument
if not is_feature_enabled("ENABLE_REACT_CRUD_VIEWS"):
return super().list()

return super().render_app_template()


Expand All @@ -128,7 +124,4 @@ class AnnotationLayerModelView(SupersetModelView):
@expose("/list/")
@has_access
def list(self) -> FlaskResponse:
if not is_feature_enabled("ENABLE_REACT_CRUD_VIEWS"):
return super().list()

return super().render_app_template()
4 changes: 0 additions & 4 deletions superset/views/chart/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
from flask_appbuilder.models.sqla.interface import SQLAInterface
from flask_babel import lazy_gettext as _

from superset import is_feature_enabled
from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP, RouteMethod
from superset.models.slice import Slice
from superset.typing import FlaskResponse
Expand Down Expand Up @@ -73,9 +72,6 @@ def add(self) -> FlaskResponse:
@expose("/list/")
@has_access
def list(self) -> FlaskResponse:
if not is_feature_enabled("ENABLE_REACT_CRUD_VIEWS"):
return super().list()

return super().render_app_template()


Expand Down
3 changes: 0 additions & 3 deletions superset/views/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -2870,9 +2870,6 @@ def sqllab(self) -> FlaskResponse:
@expose("/sqllab/history/", methods=["GET"])
@event_logger.log_this
def sqllab_history(self) -> FlaskResponse:
if not is_feature_enabled("ENABLE_REACT_CRUD_VIEWS"):
return redirect("/superset/sqllab#search", code=307)

return super().render_app_template()

@api
Expand Down
4 changes: 0 additions & 4 deletions superset/views/css_templates.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
from flask_appbuilder.security.decorators import has_access
from flask_babel import lazy_gettext as _

from superset import is_feature_enabled
from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP, RouteMethod
from superset.models import core as models
from superset.typing import FlaskResponse
Expand All @@ -46,9 +45,6 @@ class CssTemplateModelView(SupersetModelView, DeleteMixin):
@expose("/list/")
@has_access
def list(self) -> FlaskResponse:
if not is_feature_enabled("ENABLE_REACT_CRUD_VIEWS"):
return super().list()

return super().render_app_template()


Expand Down
3 changes: 0 additions & 3 deletions superset/views/dashboard/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,6 @@ class DashboardModelView(
@has_access
@expose("/list/")
def list(self) -> FlaskResponse:
if not is_feature_enabled("ENABLE_REACT_CRUD_VIEWS"):
return super().list()

return super().render_app_template()

@action("mulexport", __("Export"), __("Export dashboards?"), "fa-database")
Expand Down
5 changes: 1 addition & 4 deletions superset/views/database/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
from wtforms.validators import ValidationError

import superset.models.core as models
from superset import app, db, is_feature_enabled
from superset import app, db
from superset.connectors.sqla.models import SqlaTable
from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP, RouteMethod
from superset.exceptions import CertificateException
Expand Down Expand Up @@ -106,9 +106,6 @@ def _delete(self, pk: int) -> None:
@expose("/list/")
@has_access
def list(self) -> FlaskResponse:
if not is_feature_enabled("ENABLE_REACT_CRUD_VIEWS"):
return super().list()

return super().render_app_template()


Expand Down
5 changes: 1 addition & 4 deletions superset/views/sql_lab.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
from flask_appbuilder.security.decorators import has_access, has_access_api
from flask_babel import lazy_gettext as _

from superset import db, is_feature_enabled
from superset import db
from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP, RouteMethod
from superset.models.sql_lab import Query, SavedQuery, TableSchema, TabState
from superset.typing import FlaskResponse
Expand Down Expand Up @@ -78,9 +78,6 @@ class SavedQueryView(SupersetModelView, DeleteMixin):
@expose("/list/")
@has_access
def list(self) -> FlaskResponse:
if not is_feature_enabled("ENABLE_REACT_CRUD_VIEWS"):
return super().list()

return super().render_app_template()

def pre_add(self, item: "SavedQueryView") -> None:
Expand Down
1 change: 1 addition & 0 deletions tests/integration_tests/core_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,7 @@ def test_slices(self):
self.assertEqual(resp.status_code, 200)

def test_tablemodelview_list(self):
pytest.skip("Depends on ENABLE_REACT_CRUD_VIEWS=False")
Copy link
Member Author

@suddjian suddjian Mar 17, 2022

Choose a reason for hiding this comment

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

I'm skipping these and requesting Preset QA to help with replacing the lost test coverage.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's any real loss of coverage here. ENABLE_REACT_CRUD_VIEWS=True has been the case for most large deployment of superset so far, so these test are just covering functionality that no one is using. The api tests and js tests provide decent coverage for the new crud view, imo.

self.login(username="admin")

url = "/tablemodelview/list/"
Expand Down
Loading