sadpandajoe commented on code in PR #42404:
URL: https://github.com/apache/superset/pull/42404#discussion_r3906028590
##########
superset/themes/api.py:
##########
@@ -205,6 +222,8 @@ def delete(self, pk: int) -> Response:
return self.response_404()
except SystemThemeProtectedError:
return self.response_403()
+ except ThemeForbiddenError:
Review Comment:
Fixed in 468fd18f — `bulk_delete()` now catches `ThemeForbiddenError` and
returns 403, matching the single-delete route. Covered by
`test_non_editor_cannot_bulk_delete` in
`tests/integration_tests/themes/test_theme_editors.py`.
##########
superset/commands/theme/create.py:
##########
@@ -0,0 +1,62 @@
+# 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.
+import logging
+from functools import partial
+from typing import Any
+
+from marshmallow import ValidationError
+
+from superset.commands.base import BaseCommand, CreateMixin
+from superset.commands.theme.exceptions import (
+ ThemeCreateFailedError,
+ ThemeInvalidError,
+)
+from superset.commands.utils import populate_subjects
+from superset.daos.theme import ThemeDAO
+from superset.models.core import Theme
+from superset.utils.decorators import on_error, transaction
+
+logger = logging.getLogger(__name__)
+
+
+class CreateThemeCommand(CreateMixin, BaseCommand):
+ def __init__(self, data: dict[str, Any]):
+ self._properties = data.copy()
+
+ @transaction(on_error=partial(on_error, reraise=ThemeCreateFailedError))
+ def run(self) -> Theme:
+ self.validate()
+ # User-created themes are never system themes.
+ self._properties["is_system"] = False
+ return ThemeDAO.create(attributes=self._properties)
+
+ def _populate_subjects(self, exceptions: list[ValidationError]) -> None:
Review Comment:
Fixed in 468fd18f — the MCP `create_theme` tool now persists through
`CreateThemeCommand` (the same command the REST create path uses) instead of
calling `ThemeDAO.create` directly, so the creator is seeded as an editor.
Regression coverage in
`tests/unit_tests/mcp_service/theme/tool/test_create_theme.py::test_create_theme_seeds_editors_via_shared_command`
asserts the command's editor-seeding step (`populate_subjects`) is actually
invoked.
##########
superset-frontend/src/features/themes/ThemeModal.tsx:
##########
@@ -139,13 +160,41 @@ const ThemeModal: FunctionComponent<ThemeModalProps> = ({
const supersetTheme = useTheme();
const { setTemporaryTheme } = useThemeContext();
const [disableSave, setDisableSave] = useState<boolean>(true);
- const [currentTheme, setCurrentTheme] = useState<ThemeObject | null>(null);
- const [initialTheme, setInitialTheme] = useState<ThemeObject | null>(null);
+ const [currentTheme, setCurrentTheme] = useState<ThemeModalObject | null>(
+ null,
+ );
+ const [initialTheme, setInitialTheme] = useState<ThemeModalObject | null>(
+ null,
+ );
const [isHidden, setIsHidden] = useState<boolean>(true);
const [showConfirmAlert, setShowConfirmAlert] = useState<boolean>(false);
const isEditMode = theme !== null;
const isSystemTheme = currentTheme?.is_system === true;
- const isReadOnly = isSystemTheme;
+
+ const currentUser = useSelector<any, UserWithPermissionsAndRoles>(
+ state => state.user,
+ );
+ const currentUserSubjectId = getBootstrapData()?.common?.user_subject_id;
+
+ // theme fetch logic
+ const {
+ state: { loading, resource },
+ fetchResource,
+ createResource,
+ updateResource,
+ } = useSingleViewResource<ThemeObject, ThemeSavePayload>(
+ 'theme',
+ t('theme'),
+ addDangerToast,
+ );
+
+ // In edit mode a non-editor (and non-admin) may only view the theme. The
+ // editorship check runs against the persisted editors from the fetched
+ // resource, not the in-progress picker selection.
+ const canEditTheme =
Review Comment:
Fixed — the API now attaches `extra_editors` to theme GET responses
(468fd18f), and `ThemeModal`'s editorship check now factors it in alongside the
persisted `editors` list (f61fb270), matching the chart/dashboard read-only
checks. Covered by the new test in `ThemeModal.test.tsx`.
##########
superset-frontend/src/pages/ThemeList/index.tsx:
##########
@@ -466,13 +484,19 @@ function ThemesList({
const handleApply = () => handleThemeApply(original);
const handleExport = () => handleBulkThemeExport([original]);
+ // A user may edit a non-system theme only if they are an editor
+ // (or an admin). Everyone else gets a read-only view.
+ const allowEdit =
+ !original.is_system &&
+ isUserEditorOrAdmin(currentUser, original.editors);
Review Comment:
Good catch, thanks — fixed in a4575999. The list-row `allowEdit` check now
passes `original.extra_editors` to `isUserEditorOrAdmin` the same way
`ThemeModal` does, so resolver-granted editors see the edit action in the list
too. Added `ThemeList.test.tsx` coverage asserting the row action factors in
`extra_editors`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
