Addressing PR feedback

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]:

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,
                 )

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,
             )

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,
             )

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,
             )

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]:

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
         )
 

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)