From 3057e4270cd76232271072b74ad13e7301bd3a79 Mon Sep 17 00:00:00 2001 From: Daniel Vaz Gaspar Date: Tue, 4 Oct 2022 09:13:11 +0100 Subject: [PATCH] feat: deprecate created_slices API endpoint (#21664) Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> --- .../src/profile/components/CreatedContent.tsx | 41 +++++++++++++------ superset-frontend/src/types/bootstrapTypes.ts | 10 +++++ superset/charts/api.py | 4 +- superset/charts/filters.py | 16 ++++++++ superset/dashboards/filters.py | 2 +- superset/views/core.py | 5 +++ tests/integration_tests/charts/api_tests.py | 40 ++++++++++++++++++ .../integration_tests/dashboards/api_tests.py | 38 +++++++++++------ tests/integration_tests/model_tests.py | 4 +- 9 files changed, 132 insertions(+), 28 deletions(-) diff --git a/superset-frontend/src/profile/components/CreatedContent.tsx b/superset-frontend/src/profile/components/CreatedContent.tsx index 61dfb418f0..e32fde86b1 100644 --- a/superset-frontend/src/profile/components/CreatedContent.tsx +++ b/superset-frontend/src/profile/components/CreatedContent.tsx @@ -18,12 +18,14 @@ */ import rison from 'rison'; import React from 'react'; -import moment from 'moment'; import { t } from '@superset-ui/core'; -import TableLoader from '../../components/TableLoader'; -import { Slice } from '../types'; -import { User, DashboardResponse } from '../../types/bootstrapTypes'; +import TableLoader from 'src/components/TableLoader'; +import { + User, + DashboardResponse, + ChartResponse, +} from 'src/types/bootstrapTypes'; interface CreatedContentProps { user: User; @@ -31,17 +33,30 @@ interface CreatedContentProps { class CreatedContent extends React.PureComponent { renderSliceTable() { - const mutator = (data: Slice[]) => - data.map(slice => ({ - slice: {slice.title}, - created: moment.utc(slice.dttm).fromNow(), - _created: slice.dttm, + const search = [ + { col: 'created_by', opr: 'chart_created_by_me', value: 'me' }, + ]; + const query = rison.encode({ + keys: ['none'], + columns: ['created_on_delta_humanized', 'slice_name', 'url'], + filters: search, + order_column: 'changed_on_delta_humanized', + order_direction: 'desc', + page: 0, + page_size: 100, + }); + + const mutator = (data: ChartResponse) => + data.result.map(chart => ({ + chart: {chart.slice_name}, + created: chart.created_on_delta_humanized, + _created: chart.created_on_delta_humanized, })); return ( { } renderDashboardTable() { - const search = [{ col: 'created_by', opr: 'created_by_me', value: 'me' }]; + const search = [ + { col: 'created_by', opr: 'dashboard_created_by_me', value: 'me' }, + ]; const query = rison.encode({ keys: ['none'], columns: ['created_on_delta_humanized', 'dashboard_title', 'url'], diff --git a/superset-frontend/src/types/bootstrapTypes.ts b/superset-frontend/src/types/bootstrapTypes.ts index a6c1a32440..6d4605270a 100644 --- a/superset-frontend/src/types/bootstrapTypes.ts +++ b/superset-frontend/src/types/bootstrapTypes.ts @@ -64,6 +64,16 @@ export type DashboardResponse = { result: DashboardData[]; }; +export type ChartData = { + slice_name: string; + created_on_delta_humanized?: string; + url: string; +}; + +export type ChartResponse = { + result: ChartData[]; +}; + export interface CommonBootstrapData { flash_messages: string[][]; conf: JsonObject; diff --git a/superset/charts/api.py b/superset/charts/api.py index 30bf782730..5cb432f4f3 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -50,6 +50,7 @@ from superset.charts.dao import ChartDAO from superset.charts.filters import ( ChartAllTextFilter, ChartCertifiedFilter, + ChartCreatedByMeFilter, ChartFavoriteFilter, ChartFilter, ChartHasCreatedByFilter, @@ -150,6 +151,7 @@ class ChartRestApi(BaseSupersetModelRestApi): "created_by.first_name", "created_by.id", "created_by.last_name", + "created_on_delta_humanized", "datasource_id", "datasource_name_text", "datasource_type", @@ -211,7 +213,7 @@ class ChartRestApi(BaseSupersetModelRestApi): search_filters = { "id": [ChartFavoriteFilter, ChartCertifiedFilter], "slice_name": [ChartAllTextFilter], - "created_by": [ChartHasCreatedByFilter], + "created_by": [ChartHasCreatedByFilter, ChartCreatedByMeFilter], } # Will just affect _info endpoint diff --git a/superset/charts/filters.py b/superset/charts/filters.py index 625184df10..fd3fff7f6e 100644 --- a/superset/charts/filters.py +++ b/superset/charts/filters.py @@ -24,6 +24,7 @@ from superset import db, security_manager from superset.connectors.sqla import models from superset.connectors.sqla.models import SqlaTable from superset.models.slice import Slice +from superset.utils.core import get_user_id from superset.views.base import BaseFilter from superset.views.base_api import BaseFavoriteFilter @@ -109,3 +110,18 @@ class ChartHasCreatedByFilter(BaseFilter): # pylint: disable=too-few-public-met if value is False: return query.filter(and_(Slice.created_by_fk.is_(None))) return query + + +class ChartCreatedByMeFilter(BaseFilter): # pylint: disable=too-few-public-methods + name = _("Created by me") + arg_name = "chart_created_by_me" + + def apply(self, query: Query, value: Any) -> Query: + return query.filter( + or_( + Slice.created_by_fk # pylint: disable=comparison-with-callable + == get_user_id(), + Slice.changed_by_fk # pylint: disable=comparison-with-callable + == get_user_id(), + ) + ) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index eaac724ac9..e09609ff51 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -52,7 +52,7 @@ class DashboardTitleOrSlugFilter(BaseFilter): # pylint: disable=too-few-public- class DashboardCreatedByMeFilter(BaseFilter): # pylint: disable=too-few-public-methods name = _("Created by me") - arg_name = "created_by_me" + arg_name = "dashboard_created_by_me" def apply(self, query: Query, value: Any) -> Query: return query.filter( diff --git a/superset/views/core.py b/superset/views/core.py index 9b4b4dae50..1382fbd020 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -1708,6 +1708,11 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods @expose("/created_slices//", methods=["GET"]) def created_slices(self, user_id: Optional[int] = None) -> FlaskResponse: """List of slices created by this user""" + logger.warning( + "%s.created_slices " + "This API endpoint is deprecated and will be removed in version 3.0.0", + self.__class__.__name__, + ) if not user_id: user_id = cast(int, get_user_id()) error_obj = self.get_user_activity_access_error(user_id) diff --git a/tests/integration_tests/charts/api_tests.py b/tests/integration_tests/charts/api_tests.py index 48d0472636..63f530ffa9 100644 --- a/tests/integration_tests/charts/api_tests.py +++ b/tests/integration_tests/charts/api_tests.py @@ -101,6 +101,19 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin): db.session.delete(fav_chart) db.session.commit() + @pytest.fixture() + def create_charts_created_by_gamma(self): + with self.create_app().app_context(): + charts = [] + user = self.get_user("gamma") + for cx in range(CHARTS_FIXTURE_COUNT - 1): + charts.append(self.insert_chart(f"gamma{cx}", [user.id], 1)) + yield charts + # rollback changes + for chart in charts: + db.session.delete(chart) + db.session.commit() + @pytest.fixture() def create_certified_charts(self): with self.create_app().app_context(): @@ -1124,6 +1137,33 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin): assert rv.status_code == 200 assert len(expected_models) == data["count"] + @pytest.mark.usefixtures("create_charts_created_by_gamma") + def test_get_charts_created_by_me_filter(self): + """ + Chart API: Test get charts with created by me special filter + """ + gamma_user = self.get_user("gamma") + expected_models = ( + db.session.query(Slice).filter(Slice.created_by_fk == gamma_user.id).all() + ) + arguments = { + "filters": [ + {"col": "created_by", "opr": "chart_created_by_me", "value": "me"} + ], + "order_column": "slice_name", + "order_direction": "asc", + "keys": ["none"], + "columns": ["slice_name"], + } + self.login(username="gamma") + uri = f"api/v1/chart/?q={prison.dumps(arguments)}" + rv = self.client.get(uri) + data = json.loads(rv.data.decode("utf-8")) + assert rv.status_code == 200 + assert len(expected_models) == data["count"] + for i, expected_model in enumerate(expected_models): + assert expected_model.slice_name == data["result"][i]["slice_name"] + @pytest.mark.usefixtures("create_charts") def test_get_current_user_favorite_status(self): """ diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index 2c050538b0..363ba92320 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -161,16 +161,16 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi db.session.commit() @pytest.fixture() - def create_created_by_admin_dashboards(self): + def create_created_by_gamma_dashboards(self): with self.create_app().app_context(): dashboards = [] - admin = self.get_user("admin") + gamma = self.get_user("gamma") for cx in range(2): dashboard = self.insert_dashboard( f"create_title{cx}", f"create_slug{cx}", - [admin.id], - created_by=admin, + [gamma.id], + created_by=gamma, ) sleep(1) dashboards.append(dashboard) @@ -697,21 +697,23 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi data = json.loads(rv.data.decode("utf-8")) self.assertEqual(data["count"], 5) - @pytest.mark.usefixtures("create_created_by_admin_dashboards") + @pytest.mark.usefixtures("create_created_by_gamma_dashboards") def test_get_dashboards_created_by_me(self): """ Dashboard API: Test get dashboards created by current user """ query = { "columns": ["created_on_delta_humanized", "dashboard_title", "url"], - "filters": [{"col": "created_by", "opr": "created_by_me", "value": "me"}], + "filters": [ + {"col": "created_by", "opr": "dashboard_created_by_me", "value": "me"} + ], "order_column": "changed_on", "order_direction": "desc", "page": 0, "page_size": 100, } uri = f"api/v1/dashboard/?q={prison.dumps(query)}" - self.login(username="admin") + self.login(username="gamma") rv = self.client.get(uri) data = json.loads(rv.data.decode("utf-8")) assert rv.status_code == 200 @@ -1837,11 +1839,17 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi resp = self.get_assert_metric(uri, "get_embedded") self.assertEqual(resp.status_code, 404) - @pytest.mark.usefixtures("create_created_by_admin_dashboards") + @pytest.mark.usefixtures("create_created_by_gamma_dashboards") def test_gets_created_by_user_dashboards_filter(self): + expected_models = ( + db.session.query(Dashboard) + .filter(Dashboard.created_by_fk.isnot(None)) + .all() + ) + arguments = { "filters": [ - {"col": "id", "opr": "dashboard_has_created_by", "value": True} + {"col": "created_by", "opr": "dashboard_has_created_by", "value": True} ], "keys": ["none"], "columns": ["dashboard_title"], @@ -1852,23 +1860,27 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi rv = self.get_assert_metric(uri, "get_list") self.assertEqual(rv.status_code, 200) data = json.loads(rv.data.decode("utf-8")) - self.assertEqual(data["count"], 7) + self.assertEqual(data["count"], len(expected_models)) def test_gets_not_created_by_user_dashboards_filter(self): + dashboard = self.insert_dashboard(f"title", f"slug", []) + expected_models = ( + db.session.query(Dashboard).filter(Dashboard.created_by_fk.is_(None)).all() + ) + arguments = { "filters": [ - {"col": "id", "opr": "dashboard_has_created_by", "value": False} + {"col": "created_by", "opr": "dashboard_has_created_by", "value": False} ], "keys": ["none"], "columns": ["dashboard_title"], } - dashboard = self.insert_dashboard(f"title", f"slug", []) self.login(username="admin") uri = f"api/v1/dashboard/?q={prison.dumps(arguments)}" rv = self.get_assert_metric(uri, "get_list") self.assertEqual(rv.status_code, 200) data = json.loads(rv.data.decode("utf-8")) - self.assertEqual(data["count"], 6) + self.assertEqual(data["count"], len(expected_models)) db.session.delete(dashboard) db.session.commit() diff --git a/tests/integration_tests/model_tests.py b/tests/integration_tests/model_tests.py index 85014f34d8..c92de47f03 100644 --- a/tests/integration_tests/model_tests.py +++ b/tests/integration_tests/model_tests.py @@ -592,7 +592,9 @@ class TestSqlaTableModel(SupersetTestCase): assert len(data_for_slices["metrics"]) == 1 assert len(data_for_slices["columns"]) == 2 assert data_for_slices["metrics"][0]["metric_name"] == "sum__num" - assert data_for_slices["columns"][0]["column_name"] == "name" + column_names = [col["column_name"] for col in data_for_slices["columns"]] + assert "name" in column_names + assert "state" in column_names assert set(data_for_slices["verbose_map"].keys()) == { "__timestamp", "sum__num",