From 5519f19af48183a417acc6a37ebce69b4373887e Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Fri, 1 Mar 2024 11:44:01 -0300 Subject: [PATCH 1/5] fix(API): Updating assets via the API should preserve ownership configuration --- superset/commands/base.py | 16 +++- superset/commands/chart/update.py | 5 +- superset/commands/dashboard/update.py | 9 +- superset/commands/dataset/update.py | 5 +- superset/commands/report/update.py | 7 +- superset/commands/utils.py | 18 ++++ tests/integration_tests/charts/api_tests.py | 53 ++++++++++- .../integration_tests/dashboards/api_tests.py | 37 ++++++++ tests/integration_tests/datasets/api_tests.py | 55 ++++++++++++ tests/integration_tests/reports/api_tests.py | 87 +++++++++++++++++++ 10 files changed, 280 insertions(+), 12 deletions(-) diff --git a/superset/commands/base.py b/superset/commands/base.py index caca50755de9b..7275f281cde4a 100644 --- a/superset/commands/base.py +++ b/superset/commands/base.py @@ -19,7 +19,7 @@ from flask_appbuilder.security.sqla.models import User -from superset.commands.utils import populate_owners +from superset.commands.utils import populate_owners, update_owner_list class BaseCommand(ABC): @@ -70,3 +70,17 @@ def populate_owners(owner_ids: Optional[list[int]] = None) -> list[User]: :returns: Final list of owners """ return populate_owners(owner_ids, default_to_user=False) + + @staticmethod + def update_owner_list( + current_owners: list[User], + new_owners: Optional[list[int]], + ) -> list[User]: + """ + Handle list of owners for update events. + + :param current_owners: list of current owners + :param new_owners: list of new owners specified in the update payload + :returns: Final list of owners + """ + return update_owner_list(current_owners, new_owners) diff --git a/superset/commands/chart/update.py b/superset/commands/chart/update.py index 40b36ebcc521e..d3b680792d35e 100644 --- a/superset/commands/chart/update.py +++ b/superset/commands/chart/update.py @@ -90,7 +90,10 @@ def validate(self) -> None: if not is_query_context_update(self._properties): try: security_manager.raise_for_ownership(self._model) - owners = self.populate_owners(owner_ids) + owners = self.update_owner_list( + self._model.owners, + owner_ids, + ) self._properties["owners"] = owners except SupersetSecurityException as ex: raise ChartForbiddenError() from ex diff --git a/superset/commands/dashboard/update.py b/superset/commands/dashboard/update.py index 22dcad4b2c86b..effc6ab7868fd 100644 --- a/superset/commands/dashboard/update.py +++ b/superset/commands/dashboard/update.py @@ -66,7 +66,7 @@ def run(self) -> Model: def validate(self) -> None: exceptions: list[ValidationError] = [] - owners_ids: Optional[list[int]] = self._properties.get("owners") + owner_ids: Optional[list[int]] = self._properties.get("owners") roles_ids: Optional[list[int]] = self._properties.get("roles") slug: Optional[str] = self._properties.get("slug") @@ -85,10 +85,11 @@ def validate(self) -> None: exceptions.append(DashboardSlugExistsValidationError()) # Validate/Populate owner - if owners_ids is None: - owners_ids = [owner.id for owner in self._model.owners] try: - owners = self.populate_owners(owners_ids) + owners = self.update_owner_list( + self._model.owners, + owner_ids, + ) self._properties["owners"] = owners except ValidationError as ex: exceptions.append(ex) diff --git a/superset/commands/dataset/update.py b/superset/commands/dataset/update.py index 8a72c24fd5f28..be8bbe6d29d3c 100644 --- a/superset/commands/dataset/update.py +++ b/superset/commands/dataset/update.py @@ -100,7 +100,10 @@ def validate(self) -> None: exceptions.append(DatabaseChangeValidationError()) # Validate/Populate owner try: - owners = self.populate_owners(owner_ids) + owners = self.update_owner_list( + self._model.owners, + owner_ids, + ) self._properties["owners"] = owners except ValidationError as ex: exceptions.append(ex) diff --git a/superset/commands/report/update.py b/superset/commands/report/update.py index a33ba6b59a726..4d02b49834387 100644 --- a/superset/commands/report/update.py +++ b/superset/commands/report/update.py @@ -118,10 +118,11 @@ def validate(self) -> None: raise ReportScheduleForbiddenError() from ex # Validate/Populate owner - if owner_ids is None: - owner_ids = [owner.id for owner in self._model.owners] try: - owners = self.populate_owners(owner_ids) + owners = self.update_owner_list( + self._model.owners, + owner_ids, + ) self._properties["owners"] = owners except ValidationError as ex: exceptions.append(ex) diff --git a/superset/commands/utils.py b/superset/commands/utils.py index b7121ec89f0e7..505691a412a79 100644 --- a/superset/commands/utils.py +++ b/superset/commands/utils.py @@ -62,6 +62,24 @@ def populate_owners( return owners +def update_owner_list( + current_owners: list[User], + new_owners: list[int] | None, +) -> list[User]: + """ + Helper function for update commands, to properly handle the owners list. + Preserve the previous configuration unless included in the update payload. + + :param current_owners: list of current owners + :param new_owners: list of new owners specified in the update payload + :returns: Final list of owners + """ + owners_ids = ( + [owner.id for owner in current_owners] if new_owners is None else new_owners + ) + return populate_owners(owners_ids, default_to_user=False) + + def populate_roles(role_ids: list[int] | None = None) -> list[Role]: """ Helper function for commands, will fetch all roles from roles id's diff --git a/tests/integration_tests/charts/api_tests.py b/tests/integration_tests/charts/api_tests.py index d0985124e228f..829c7ba51852e 100644 --- a/tests/integration_tests/charts/api_tests.py +++ b/tests/integration_tests/charts/api_tests.py @@ -735,10 +735,59 @@ def test_update_chart_new_owner_admin(self): db.session.delete(model) db.session.commit() + @pytest.mark.usefixtures("add_dashboard_to_chart") + def test_update_chart_preserve_ownership(self): + """ + Chart API: Test update chart preserves owner list (if un-changed) + """ + chart_data = { + "slice_name": "title1_changed", + } + admin = self.get_user("admin") + self.login(username="admin") + uri = f"api/v1/chart/{self.chart.id}" + rv = self.put_assert_metric(uri, chart_data, "put") + self.assertEqual(rv.status_code, 200) + self.assertEqual([admin], self.chart.owners) + + @pytest.mark.usefixtures("add_dashboard_to_chart") + def test_update_chart_clear_owner_list(self): + """ + Chart API: Test update chart admin can clear owner list + """ + chart_data = {"slice_name": "title1_changed", "owners": []} + admin = self.get_user("admin") + self.login(username="admin") + uri = f"api/v1/chart/{self.chart.id}" + rv = self.put_assert_metric(uri, chart_data, "put") + self.assertEqual(rv.status_code, 200) + self.assertEqual([], self.chart.owners) + + def test_update_chart_populate_owner(self): + """ + Chart API: Test update admin can update chart with + no owners to a different owner + """ + gamma = self.get_user("gamma") + admin = self.get_user("admin") + chart_id = self.insert_chart("title", [], 1).id + model = db.session.query(Slice).get(chart_id) + self.assertEqual(model.owners, []) + chart_data = {"owners": [gamma.id]} + self.login(username="admin") + uri = f"api/v1/chart/{chart_id}" + rv = self.put_assert_metric(uri, chart_data, "put") + self.assertEqual(rv.status_code, 200) + model_updated = db.session.query(Slice).get(chart_id) + self.assertNotIn(admin, model_updated.owners) + self.assertIn(gamma, model_updated.owners) + db.session.delete(model_updated) + db.session.commit() + @pytest.mark.usefixtures("add_dashboard_to_chart") def test_update_chart_new_dashboards(self): """ - Chart API: Test update set new owner to current user + Chart API: Test update chart associating it with new dashboard """ chart_data = { "slice_name": "title1_changed", @@ -754,7 +803,7 @@ def test_update_chart_new_dashboards(self): @pytest.mark.usefixtures("add_dashboard_to_chart") def test_not_update_chart_none_dashboards(self): """ - Chart API: Test update set new owner to current user + Chart API: Test update chart without changing dashboards configuration """ chart_data = {"slice_name": "title1_changed_again"} self.login(username="admin") diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index 623572c713945..fcc1031c2f4b2 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -1550,6 +1550,43 @@ def test_update_dashboard_new_owner_admin(self): db.session.delete(model) db.session.commit() + def test_update_dashboard_clear_owner_list(self): + """ + Dashboard API: Test update admin can clear up owners list + """ + admin = self.get_user("admin") + dashboard_id = self.insert_dashboard("title1", "slug1", [admin.id]).id + self.login(username="admin") + uri = f"api/v1/dashboard/{dashboard_id}" + dashboard_data = {"owners": []} + rv = self.client.put(uri, json=dashboard_data) + self.assertEqual(rv.status_code, 200) + model = db.session.query(Dashboard).get(dashboard_id) + self.assertEqual([], model.owners) + db.session.delete(model) + db.session.commit() + + def test_update_dashboard_populate_owner(self): + """ + Dashboard API: Test update admin can update dashboard with + no owners to a different owner + """ + self.login(username="admin") + gamma = self.get_user("gamma") + dashboard = self.insert_dashboard( + "title1", + "slug1", + [], + ) + uri = f"api/v1/dashboard/{dashboard.id}" + dashboard_data = {"owners": [gamma.id]} + rv = self.client.put(uri, json=dashboard_data) + self.assertEqual(rv.status_code, 200) + model = db.session.query(Dashboard).get(dashboard.id) + self.assertEqual([gamma], model.owners) + db.session.delete(model) + db.session.commit() + def test_update_dashboard_slug_formatting(self): """ Dashboard API: Test update slug formatting diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index 1ebe5bd1f7eb4..59d02a07d636a 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -833,12 +833,66 @@ def test_create_dataset_sqlalchemy_error(self, mock_dao_create): assert rv.status_code == 422 assert data == {"message": "Dataset could not be created."} + def test_update_dataset_preserve_ownership(self): + """ + Dataset API: Test update dataset preserves owner list (if un-changed) + """ + + dataset = self.insert_default_dataset() + current_owners = dataset.owners + self.login(username="admin") + dataset_data = {"description": "new description"} + uri = f"api/v1/dataset/{dataset.id}" + rv = self.put_assert_metric(uri, dataset_data, "put") + assert rv.status_code == 200 + model = db.session.query(SqlaTable).get(dataset.id) + assert model.owners == current_owners + + db.session.delete(dataset) + db.session.commit() + + def test_update_dataset_clear_owner_list(self): + """ + Dataset API: Test update dataset admin can clear ownership config + """ + + dataset = self.insert_default_dataset() + self.login(username="admin") + dataset_data = {"owners": []} + uri = f"api/v1/dataset/{dataset.id}" + rv = self.put_assert_metric(uri, dataset_data, "put") + assert rv.status_code == 200 + model = db.session.query(SqlaTable).get(dataset.id) + assert model.owners == [] + + db.session.delete(dataset) + db.session.commit() + + def test_update_dataset_populate_owner(self): + """ + Dataset API: Test update admin can update dataset with + no owners to a different owner + """ + self.login(username="admin") + gamma = self.get_user("gamma") + dataset = self.insert_dataset("ab_permission", [], get_main_database()) + dataset_data = {"owners": [gamma.id]} + uri = f"api/v1/dataset/{dataset.id}" + rv = self.put_assert_metric(uri, dataset_data, "put") + assert rv.status_code == 200 + model = db.session.query(SqlaTable).get(dataset.id) + assert model.owners == [gamma] + + db.session.delete(dataset) + db.session.commit() + def test_update_dataset_item(self): """ Dataset API: Test update dataset item """ dataset = self.insert_default_dataset() + current_owners = dataset.owners self.login(username="admin") dataset_data = {"description": "changed_description"} uri = f"api/v1/dataset/{dataset.id}" @@ -846,6 +900,7 @@ def test_update_dataset_item(self): assert rv.status_code == 200 model = db.session.query(SqlaTable).get(dataset.id) assert model.description == dataset_data["description"] + assert model.owners == current_owners db.session.delete(dataset) db.session.commit() diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index 12d00d59f3a5a..1b74d11f5a0a5 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -1458,6 +1458,93 @@ def test_update_report_not_owned(self): rv = self.put_assert_metric(uri, report_schedule_data, "put") self.assertEqual(rv.status_code, 403) + @pytest.mark.usefixtures("create_report_schedules") + def test_update_report_preserve_ownership(self): + """ + ReportSchedule API: Test update report preserves owner list (if un-changed) + """ + self.login(username="admin") + existing_report = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name1") + .one_or_none() + ) + current_owners = existing_report.owners + report_schedule_data = { + "description": "Updated description", + } + uri = f"api/v1/report/{existing_report.id}" + rv = self.put_assert_metric(uri, report_schedule_data, "put") + updated_report = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name1") + .one_or_none() + ) + assert updated_report.owners == current_owners + + @pytest.mark.usefixtures("create_report_schedules") + def test_update_report_clear_owner_list(self): + """ + ReportSchedule API: Test update report admin can clear ownership config + """ + self.login(username="admin") + existing_report = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name1") + .one_or_none() + ) + report_schedule_data = { + "owners": [], + } + uri = f"api/v1/report/{existing_report.id}" + rv = self.put_assert_metric(uri, report_schedule_data, "put") + updated_report = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name1") + .one_or_none() + ) + assert updated_report.owners == [] + + @pytest.mark.usefixtures("create_report_schedules") + def test_update_report_populate_owner(self): + """ + ReportSchedule API: Test update admin can update report with + no owners to a different owner + """ + gamma = self.get_user("gamma") + self.login(username="admin") + + # Modify an existing report to make remove all owners + existing_report = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name1") + .one_or_none() + ) + report_update_data = { + "owners": [], + } + uri = f"api/v1/report/{existing_report.id}" + rv = self.put_assert_metric(uri, report_update_data, "put") + updated_report = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name1") + .one_or_none() + ) + assert updated_report.owners == [] + + # Populate the field + report_update_data = { + "owners": [gamma.id], + } + uri = f"api/v1/report/{updated_report.id}" + rv = self.put_assert_metric(uri, report_update_data, "put") + updated_report = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name1") + .one_or_none() + ) + assert updated_report.owners == [gamma] + @pytest.mark.usefixtures("create_report_schedules") def test_delete_report_schedule(self): """ From 187aea0f299e98c6f53968036450720338127e34 Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Sat, 2 Mar 2024 01:36:32 -0300 Subject: [PATCH 2/5] Implementing unit tests --- superset/commands/base.py | 2 +- superset/commands/utils.py | 3 +- tests/unit_tests/commands/test_utils.py | 118 ++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 2 deletions(-) create mode 100644 tests/unit_tests/commands/test_utils.py diff --git a/superset/commands/base.py b/superset/commands/base.py index 7275f281cde4a..68ce87be0e9ac 100644 --- a/superset/commands/base.py +++ b/superset/commands/base.py @@ -73,7 +73,7 @@ def populate_owners(owner_ids: Optional[list[int]] = None) -> list[User]: @staticmethod def update_owner_list( - current_owners: list[User], + current_owners: Optional[list[User]], new_owners: Optional[list[int]], ) -> list[User]: """ diff --git a/superset/commands/utils.py b/superset/commands/utils.py index 505691a412a79..9a80a52bde2fd 100644 --- a/superset/commands/utils.py +++ b/superset/commands/utils.py @@ -63,7 +63,7 @@ def populate_owners( def update_owner_list( - current_owners: list[User], + current_owners: list[User] | None, new_owners: list[int] | None, ) -> list[User]: """ @@ -74,6 +74,7 @@ def update_owner_list( :param new_owners: list of new owners specified in the update payload :returns: Final list of owners """ + current_owners = current_owners or [] owners_ids = ( [owner.id for owner in current_owners] if new_owners is None else new_owners ) diff --git a/tests/unit_tests/commands/test_utils.py b/tests/unit_tests/commands/test_utils.py new file mode 100644 index 0000000000000..af72ebdd8b020 --- /dev/null +++ b/tests/unit_tests/commands/test_utils.py @@ -0,0 +1,118 @@ +# 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. + +from unittest.mock import MagicMock, patch + +import pytest + +from superset.commands.utils import populate_owners, update_owner_list, User + + +@patch("superset.commands.utils.g") +def test_populate_owners_default_to_user(mock_user): + owner_list = populate_owners([], True) + assert owner_list == [mock_user.user] + + +@patch("superset.commands.utils.g") +def test_populate_owners_default_to_user_handle_none(mock_user): + owner_list = populate_owners(None, True) + assert owner_list == [mock_user.user] + + +@patch("superset.commands.utils.g") +@patch("superset.commands.utils.security_manager") +@patch("superset.commands.utils.get_user_id") +def test_populate_owners_admin(mock_user_id, mock_sm, mock_g): + test_user = User(id=1, first_name="First", last_name="Last") + mock_g.user = User(id=4, first_name="Admin", last_name="User") + mock_user_id.return_value = 4 + mock_sm.is_admin = MagicMock(return_value=True) + mock_sm.get_user_by_id = MagicMock(return_value=test_user) + + owner_list = populate_owners([1], False) + assert owner_list == [test_user] + + +@patch("superset.commands.utils.g") +@patch("superset.commands.utils.security_manager") +@patch("superset.commands.utils.get_user_id") +def test_populate_owners_admin_empty_list(mock_user_id, mock_sm, mock_g): + mock_g.user = User(id=4, first_name="Admin", last_name="User") + mock_user_id.return_value = 4 + mock_sm.is_admin = MagicMock(return_value=True) + owner_list = populate_owners([], False) + assert owner_list == [] + + +@patch("superset.commands.utils.g") +@patch("superset.commands.utils.security_manager") +@patch("superset.commands.utils.get_user_id") +def test_populate_owners_non_admin(mock_user_id, mock_sm, mock_g): + test_user = User(id=1, first_name="First", last_name="Last") + mock_g.user = User(id=4, first_name="Non", last_name="admin") + mock_user_id.return_value = 4 + mock_sm.is_admin = MagicMock(return_value=False) + mock_sm.get_user_by_id = MagicMock(return_value=test_user) + + owner_list = populate_owners([1], False) + assert owner_list == [mock_g.user, test_user] + + +@patch("superset.commands.utils.populate_owners") +def test_update_owner_list_new_owners(mock_populate_owners): + current_owners = [User(id=1), User(id=2), User(id=3)] + new_owners = [4, 5, 6] + + update_owner_list(current_owners, new_owners) + mock_populate_owners.assert_called_once_with(new_owners, default_to_user=False) + + +@patch("superset.commands.utils.populate_owners") +def test_update_owner_list_no_new_owners(mock_populate_owners): + current_owners = [User(id=1), User(id=2), User(id=3)] + new_owners = None + + update_owner_list(current_owners, new_owners) + mock_populate_owners.assert_called_once_with([1, 2, 3], default_to_user=False) + + +@patch("superset.commands.utils.populate_owners") +def test_update_owner_list_new_owner_empty_list(mock_populate_owners): + current_owners = [User(id=1), User(id=2), User(id=3)] + new_owners = [] + + update_owner_list(current_owners, new_owners) + mock_populate_owners.assert_called_once_with(new_owners, default_to_user=False) + + +@patch("superset.commands.utils.populate_owners") +def test_update_owner_list_no_owners(mock_populate_owners): + current_owners = [] + new_owners = [4, 5, 6] + + update_owner_list(current_owners, new_owners) + mock_populate_owners.assert_called_once_with(new_owners, default_to_user=False) + + +@patch("superset.commands.utils.populate_owners") +def test_update_owner_list_no_owners_handle_none(mock_populate_owners): + current_owners = None + new_owners = [4, 5, 6] + + update_owner_list(current_owners, new_owners) + mock_populate_owners.assert_called_once_with(new_owners, default_to_user=False) From 85d5ef969fdd11d8d99cd1b01d149c0dfa700c13 Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Mon, 4 Mar 2024 10:30:28 -0300 Subject: [PATCH 3/5] Addressing PR feedback --- superset/commands/base.py | 8 ++-- superset/commands/chart/update.py | 2 +- superset/commands/dashboard/update.py | 2 +- superset/commands/dataset/update.py | 2 +- superset/commands/report/update.py | 2 +- superset/commands/utils.py | 4 +- superset/views/datasource/views.py | 4 +- tests/unit_tests/commands/test_utils.py | 52 ++++++++++++------------- 8 files changed, 38 insertions(+), 38 deletions(-) diff --git a/superset/commands/base.py b/superset/commands/base.py index 68ce87be0e9ac..496fa935de31b 100644 --- a/superset/commands/base.py +++ b/superset/commands/base.py @@ -19,7 +19,7 @@ from flask_appbuilder.security.sqla.models import User -from superset.commands.utils import populate_owners, update_owner_list +from superset.commands.utils import populate_owner_list, update_owner_list class BaseCommand(ABC): @@ -55,7 +55,7 @@ def populate_owners(owner_ids: Optional[list[int]] = None) -> list[User]: :raises OwnersNotFoundValidationError: if at least one owner can't be resolved :returns: Final list of owners """ - return populate_owners(owner_ids, default_to_user=True) + return populate_owner_list(owner_ids, default_to_user=True) class UpdateMixin: # pylint: disable=too-few-public-methods @@ -69,10 +69,10 @@ def populate_owners(owner_ids: Optional[list[int]] = None) -> list[User]: :raises OwnersNotFoundValidationError: if at least one owner can't be resolved :returns: Final list of owners """ - return populate_owners(owner_ids, default_to_user=False) + return populate_owner_list(owner_ids, default_to_user=False) @staticmethod - def update_owner_list( + def update_owners( current_owners: Optional[list[User]], new_owners: Optional[list[int]], ) -> list[User]: diff --git a/superset/commands/chart/update.py b/superset/commands/chart/update.py index d3b680792d35e..4080cee19bdbf 100644 --- a/superset/commands/chart/update.py +++ b/superset/commands/chart/update.py @@ -90,7 +90,7 @@ def validate(self) -> None: if not is_query_context_update(self._properties): try: security_manager.raise_for_ownership(self._model) - owners = self.update_owner_list( + owners = self.update_owners( self._model.owners, owner_ids, ) diff --git a/superset/commands/dashboard/update.py b/superset/commands/dashboard/update.py index effc6ab7868fd..cb22ebde777f7 100644 --- a/superset/commands/dashboard/update.py +++ b/superset/commands/dashboard/update.py @@ -86,7 +86,7 @@ def validate(self) -> None: # Validate/Populate owner try: - owners = self.update_owner_list( + owners = self.update_owners( self._model.owners, owner_ids, ) diff --git a/superset/commands/dataset/update.py b/superset/commands/dataset/update.py index be8bbe6d29d3c..d1582f0bffef3 100644 --- a/superset/commands/dataset/update.py +++ b/superset/commands/dataset/update.py @@ -100,7 +100,7 @@ def validate(self) -> None: exceptions.append(DatabaseChangeValidationError()) # Validate/Populate owner try: - owners = self.update_owner_list( + owners = self.update_owners( self._model.owners, owner_ids, ) diff --git a/superset/commands/report/update.py b/superset/commands/report/update.py index 4d02b49834387..3c63438762e8c 100644 --- a/superset/commands/report/update.py +++ b/superset/commands/report/update.py @@ -119,7 +119,7 @@ def validate(self) -> None: # Validate/Populate owner try: - owners = self.update_owner_list( + owners = self.update_owners( self._model.owners, owner_ids, ) diff --git a/superset/commands/utils.py b/superset/commands/utils.py index 9a80a52bde2fd..ab143056c8a48 100644 --- a/superset/commands/utils.py +++ b/superset/commands/utils.py @@ -35,7 +35,7 @@ from superset.connectors.sqla.models import BaseDatasource -def populate_owners( +def populate_owner_list( owner_ids: list[int] | None, default_to_user: bool, ) -> list[User]: @@ -78,7 +78,7 @@ def update_owner_list( owners_ids = ( [owner.id for owner in current_owners] if new_owners is None else new_owners ) - return populate_owners(owners_ids, default_to_user=False) + return populate_owner_list(owners_ids, default_to_user=False) def populate_roles(role_ids: list[int] | None = None) -> list[Role]: diff --git a/superset/views/datasource/views.py b/superset/views/datasource/views.py index 2e46faf0af9dc..eba3acf36edd4 100644 --- a/superset/views/datasource/views.py +++ b/superset/views/datasource/views.py @@ -31,7 +31,7 @@ DatasetForbiddenError, DatasetNotFoundError, ) -from superset.commands.utils import populate_owners +from superset.commands.utils import populate_owner_list from superset.connectors.sqla.models import SqlaTable from superset.connectors.sqla.utils import get_physical_table_metadata from superset.daos.datasource import DatasourceDAO @@ -94,7 +94,7 @@ def save(self) -> FlaskResponse: except SupersetSecurityException as ex: raise DatasetForbiddenError() from ex - datasource_dict["owners"] = populate_owners( + datasource_dict["owners"] = populate_owner_list( datasource_dict["owners"], default_to_user=False ) diff --git a/tests/unit_tests/commands/test_utils.py b/tests/unit_tests/commands/test_utils.py index af72ebdd8b020..2c7609934f815 100644 --- a/tests/unit_tests/commands/test_utils.py +++ b/tests/unit_tests/commands/test_utils.py @@ -19,100 +19,100 @@ import pytest -from superset.commands.utils import populate_owners, update_owner_list, User +from superset.commands.utils import populate_owner_list, update_owner_list, User @patch("superset.commands.utils.g") -def test_populate_owners_default_to_user(mock_user): - owner_list = populate_owners([], True) +def test_populate_owner_list_default_to_user(mock_user): + owner_list = populate_owner_list([], True) assert owner_list == [mock_user.user] @patch("superset.commands.utils.g") -def test_populate_owners_default_to_user_handle_none(mock_user): - owner_list = populate_owners(None, True) +def test_populate_owner_list_default_to_user_handle_none(mock_user): + owner_list = populate_owner_list(None, True) assert owner_list == [mock_user.user] @patch("superset.commands.utils.g") @patch("superset.commands.utils.security_manager") @patch("superset.commands.utils.get_user_id") -def test_populate_owners_admin(mock_user_id, mock_sm, mock_g): +def test_populate_owner_list_admin_user(mock_user_id, mock_sm, mock_g): test_user = User(id=1, first_name="First", last_name="Last") mock_g.user = User(id=4, first_name="Admin", last_name="User") mock_user_id.return_value = 4 mock_sm.is_admin = MagicMock(return_value=True) mock_sm.get_user_by_id = MagicMock(return_value=test_user) - owner_list = populate_owners([1], False) + owner_list = populate_owner_list([1], False) assert owner_list == [test_user] @patch("superset.commands.utils.g") @patch("superset.commands.utils.security_manager") @patch("superset.commands.utils.get_user_id") -def test_populate_owners_admin_empty_list(mock_user_id, mock_sm, mock_g): +def test_populate_owner_list_admin_user_empty_list(mock_user_id, mock_sm, mock_g): mock_g.user = User(id=4, first_name="Admin", last_name="User") mock_user_id.return_value = 4 mock_sm.is_admin = MagicMock(return_value=True) - owner_list = populate_owners([], False) + owner_list = populate_owner_list([], False) assert owner_list == [] @patch("superset.commands.utils.g") @patch("superset.commands.utils.security_manager") @patch("superset.commands.utils.get_user_id") -def test_populate_owners_non_admin(mock_user_id, mock_sm, mock_g): +def test_populate_owner_list_non_admin(mock_user_id, mock_sm, mock_g): test_user = User(id=1, first_name="First", last_name="Last") mock_g.user = User(id=4, first_name="Non", last_name="admin") mock_user_id.return_value = 4 mock_sm.is_admin = MagicMock(return_value=False) mock_sm.get_user_by_id = MagicMock(return_value=test_user) - owner_list = populate_owners([1], False) + owner_list = populate_owner_list([1], False) assert owner_list == [mock_g.user, test_user] -@patch("superset.commands.utils.populate_owners") -def test_update_owner_list_new_owners(mock_populate_owners): +@patch("superset.commands.utils.populate_owner_list") +def test_update_owner_list_new_owners(mock_populate_owner_list): current_owners = [User(id=1), User(id=2), User(id=3)] new_owners = [4, 5, 6] update_owner_list(current_owners, new_owners) - mock_populate_owners.assert_called_once_with(new_owners, default_to_user=False) + mock_populate_owner_list.assert_called_once_with(new_owners, default_to_user=False) -@patch("superset.commands.utils.populate_owners") -def test_update_owner_list_no_new_owners(mock_populate_owners): +@patch("superset.commands.utils.populate_owner_list") +def test_update_owner_list_no_new_owners(mock_populate_owner_list): current_owners = [User(id=1), User(id=2), User(id=3)] new_owners = None update_owner_list(current_owners, new_owners) - mock_populate_owners.assert_called_once_with([1, 2, 3], default_to_user=False) + mock_populate_owner_list.assert_called_once_with([1, 2, 3], default_to_user=False) -@patch("superset.commands.utils.populate_owners") -def test_update_owner_list_new_owner_empty_list(mock_populate_owners): +@patch("superset.commands.utils.populate_owner_list") +def test_update_owner_list_new_owner_empty_list(mock_populate_owner_list): current_owners = [User(id=1), User(id=2), User(id=3)] new_owners = [] update_owner_list(current_owners, new_owners) - mock_populate_owners.assert_called_once_with(new_owners, default_to_user=False) + mock_populate_owner_list.assert_called_once_with(new_owners, default_to_user=False) -@patch("superset.commands.utils.populate_owners") -def test_update_owner_list_no_owners(mock_populate_owners): +@patch("superset.commands.utils.populate_owner_list") +def test_update_owner_list_no_owners(mock_populate_owner_list): current_owners = [] new_owners = [4, 5, 6] update_owner_list(current_owners, new_owners) - mock_populate_owners.assert_called_once_with(new_owners, default_to_user=False) + mock_populate_owner_list.assert_called_once_with(new_owners, default_to_user=False) -@patch("superset.commands.utils.populate_owners") -def test_update_owner_list_no_owners_handle_none(mock_populate_owners): +@patch("superset.commands.utils.populate_owner_list") +def test_update_owner_list_no_owners_handle_none(mock_populate_owner_list): current_owners = None new_owners = [4, 5, 6] update_owner_list(current_owners, new_owners) - mock_populate_owners.assert_called_once_with(new_owners, default_to_user=False) + mock_populate_owner_list.assert_called_once_with(new_owners, default_to_user=False) From 29164874cbe9bf827726d63c5f87a7e05b8ce4fb Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Mon, 4 Mar 2024 15:32:54 -0300 Subject: [PATCH 4/5] Addressing other PR feedback --- superset/commands/base.py | 6 +++--- superset/commands/chart/update.py | 2 +- superset/commands/dashboard/update.py | 2 +- superset/commands/dataset/update.py | 2 +- superset/commands/report/update.py | 2 +- superset/commands/utils.py | 2 +- tests/unit_tests/commands/test_utils.py | 22 +++++++++++----------- 7 files changed, 19 insertions(+), 19 deletions(-) diff --git a/superset/commands/base.py b/superset/commands/base.py index 496fa935de31b..8b330d0669c0b 100644 --- a/superset/commands/base.py +++ b/superset/commands/base.py @@ -19,7 +19,7 @@ from flask_appbuilder.security.sqla.models import User -from superset.commands.utils import populate_owner_list, update_owner_list +from superset.commands.utils import compute_owner_list, populate_owner_list class BaseCommand(ABC): @@ -72,7 +72,7 @@ def populate_owners(owner_ids: Optional[list[int]] = None) -> list[User]: return populate_owner_list(owner_ids, default_to_user=False) @staticmethod - def update_owners( + def compute_owners( current_owners: Optional[list[User]], new_owners: Optional[list[int]], ) -> list[User]: @@ -83,4 +83,4 @@ def update_owners( :param new_owners: list of new owners specified in the update payload :returns: Final list of owners """ - return update_owner_list(current_owners, new_owners) + return compute_owner_list(current_owners, new_owners) diff --git a/superset/commands/chart/update.py b/superset/commands/chart/update.py index 4080cee19bdbf..178344634e186 100644 --- a/superset/commands/chart/update.py +++ b/superset/commands/chart/update.py @@ -90,7 +90,7 @@ def validate(self) -> None: if not is_query_context_update(self._properties): try: security_manager.raise_for_ownership(self._model) - owners = self.update_owners( + owners = self.compute_owners( self._model.owners, owner_ids, ) diff --git a/superset/commands/dashboard/update.py b/superset/commands/dashboard/update.py index cb22ebde777f7..b2b11e5f6e4e8 100644 --- a/superset/commands/dashboard/update.py +++ b/superset/commands/dashboard/update.py @@ -86,7 +86,7 @@ def validate(self) -> None: # Validate/Populate owner try: - owners = self.update_owners( + owners = self.compute_owners( self._model.owners, owner_ids, ) diff --git a/superset/commands/dataset/update.py b/superset/commands/dataset/update.py index d1582f0bffef3..5c0d87b230eff 100644 --- a/superset/commands/dataset/update.py +++ b/superset/commands/dataset/update.py @@ -100,7 +100,7 @@ def validate(self) -> None: exceptions.append(DatabaseChangeValidationError()) # Validate/Populate owner try: - owners = self.update_owners( + owners = self.compute_owners( self._model.owners, owner_ids, ) diff --git a/superset/commands/report/update.py b/superset/commands/report/update.py index 3c63438762e8c..cb63ec5011daf 100644 --- a/superset/commands/report/update.py +++ b/superset/commands/report/update.py @@ -119,7 +119,7 @@ def validate(self) -> None: # Validate/Populate owner try: - owners = self.update_owners( + owners = self.compute_owners( self._model.owners, owner_ids, ) diff --git a/superset/commands/utils.py b/superset/commands/utils.py index ab143056c8a48..f01c96ba28223 100644 --- a/superset/commands/utils.py +++ b/superset/commands/utils.py @@ -62,7 +62,7 @@ def populate_owner_list( return owners -def update_owner_list( +def compute_owner_list( current_owners: list[User] | None, new_owners: list[int] | None, ) -> list[User]: diff --git a/tests/unit_tests/commands/test_utils.py b/tests/unit_tests/commands/test_utils.py index 2c7609934f815..cb99ac37bfa72 100644 --- a/tests/unit_tests/commands/test_utils.py +++ b/tests/unit_tests/commands/test_utils.py @@ -19,7 +19,7 @@ import pytest -from superset.commands.utils import populate_owner_list, update_owner_list, User +from superset.commands.utils import compute_owner_list, populate_owner_list, User @patch("superset.commands.utils.g") @@ -74,45 +74,45 @@ def test_populate_owner_list_non_admin(mock_user_id, mock_sm, mock_g): @patch("superset.commands.utils.populate_owner_list") -def test_update_owner_list_new_owners(mock_populate_owner_list): +def test_compute_owner_list_new_owners(mock_populate_owner_list): current_owners = [User(id=1), User(id=2), User(id=3)] new_owners = [4, 5, 6] - update_owner_list(current_owners, new_owners) + compute_owner_list(current_owners, new_owners) mock_populate_owner_list.assert_called_once_with(new_owners, default_to_user=False) @patch("superset.commands.utils.populate_owner_list") -def test_update_owner_list_no_new_owners(mock_populate_owner_list): +def test_compute_owner_list_no_new_owners(mock_populate_owner_list): current_owners = [User(id=1), User(id=2), User(id=3)] new_owners = None - update_owner_list(current_owners, new_owners) + compute_owner_list(current_owners, new_owners) mock_populate_owner_list.assert_called_once_with([1, 2, 3], default_to_user=False) @patch("superset.commands.utils.populate_owner_list") -def test_update_owner_list_new_owner_empty_list(mock_populate_owner_list): +def test_compute_owner_list_new_owner_empty_list(mock_populate_owner_list): current_owners = [User(id=1), User(id=2), User(id=3)] new_owners = [] - update_owner_list(current_owners, new_owners) + compute_owner_list(current_owners, new_owners) mock_populate_owner_list.assert_called_once_with(new_owners, default_to_user=False) @patch("superset.commands.utils.populate_owner_list") -def test_update_owner_list_no_owners(mock_populate_owner_list): +def test_compute_owner_list_no_owners(mock_populate_owner_list): current_owners = [] new_owners = [4, 5, 6] - update_owner_list(current_owners, new_owners) + compute_owner_list(current_owners, new_owners) mock_populate_owner_list.assert_called_once_with(new_owners, default_to_user=False) @patch("superset.commands.utils.populate_owner_list") -def test_update_owner_list_no_owners_handle_none(mock_populate_owner_list): +def test_compute_owner_list_no_owners_handle_none(mock_populate_owner_list): current_owners = None new_owners = [4, 5, 6] - update_owner_list(current_owners, new_owners) + compute_owner_list(current_owners, new_owners) mock_populate_owner_list.assert_called_once_with(new_owners, default_to_user=False) From 22c38ff47f17c86ebda69cc4da5d18577c789dc2 Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Mon, 4 Mar 2024 17:56:52 -0300 Subject: [PATCH 5/5] Retriggering CI