aminghadersohi commented on code in PR #45504:
URL: https://github.com/apache/superset/pull/45504#discussion_r4331914413
##########
superset/mcp_service/app.py:
##########
@@ -329,7 +329,7 @@ def get_default_instructions(
- list_dashboards(request={{"created_by_me": true, "edited_by_me": true}})
- list_datasets(request={{"created_by_me": true, "edited_by_me": true}})
-Use created_by_me for authorship, edited_by_me for edit access, or both
+For dashboards, edited_by_me means direct USER editor (owner), excluding
role/group access. Use created_by_me for authorship, edited_by_me for direct
editor membership, or both
Review Comment:
Additionally fixed in 535aed49164938269cac002a5f7ef6ba10475237: my earlier
documentation incorrectly excluded list_datasets. The trash-listing guide
includes charts, dashboards and datasets, and distinguishes unsupported
parameters on other list tools from the strict rejection in
list_reports/list_databases. The visibility explanation includes
dataset_deleted_state as well.
Regression test:
tests/unit_tests/mcp_service/test_mcp_tool_registration.py::test_deleted_state_documentation_matches_list_schemas
compares the documented support set with every registered list tool's
published schema. It failed against cb9e08b488455b0a5642f8421466493073c71acb
because list_datasets was missing, and passes after the correction (targeted
run: 2 passed). Command: `pytest -q -m 'not manual and not llm_compliance'
tests/unit_tests/mcp_service/test_mcp_tool_registration.py::test_deleted_state_documentation_matches_list_schemas
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py::test_duplicate_description_explains_isolated_default`.
Siblings checked:
- docs/admin_docs/configuration/mcp-server.mdx — fixed in
535aed49164938269cac002a5f7ef6ba10475237
- superset/mcp_service/chart/schemas.py — not affected: declares
deleted_state.
- superset/mcp_service/dashboard/schemas.py — not affected: declares
deleted_state.
- superset/mcp_service/dataset/schemas.py — not affected: declares
deleted_state; no request behavior change is needed.
- superset/mcp_service/dataset/tool/list_datasets.py — not affected:
forwards deleted_state to ModelListCore.
- superset/mcp_service/report/schemas.py — not affected: does not declare
deleted_state and forbids unknown fields.
- superset/mcp_service/database/schemas.py — not affected: does not declare
deleted_state and forbids unknown fields.
- superset/mcp_service/annotation_layer/schemas.py — not affected: neither
list request declares deleted_state.
- superset/mcp_service/query/schemas.py — not affected: list request does
not declare deleted_state.
- superset/mcp_service/rls/schemas.py — not affected: list request does not
declare deleted_state.
- superset/mcp_service/role/schemas.py — not affected: list request does not
declare deleted_state.
- superset/mcp_service/saved_query/schemas.py — not affected: list request
does not declare deleted_state.
- superset/mcp_service/semantic_layer/schemas.py — not affected:
list_metrics request does not declare deleted_state.
- superset/mcp_service/tag/schemas.py — not affected: list request does not
declare deleted_state.
- superset/mcp_service/task/schemas.py — not affected: list request does not
declare deleted_state.
- superset/mcp_service/theme/schemas.py — not affected: list request does
not declare deleted_state.
- superset/mcp_service/user/schemas.py — not affected: list request does not
declare deleted_state.
- superset/mcp_service/common/pagination_schemas.py — not affected: shared
list fields do not add deleted_state.
- superset/mcp_service/app.py — not affected: list_datasets instructions
already advertise trash listing.
- superset/datasets/filters.py — not affected: DatasetDeletedStateFilter
uses the shared restore-audience filter.
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -292,8 +292,9 @@ async def duplicate_dashboard(
"""
Duplicate an existing dashboard under a new title.
- By default the copy references the same charts as the source.
- Set duplicate_slices=true to also deep-copy every chart into new
+ By default the copy deep-copies charts so subsequent edits are isolated.
+ Set duplicate_slices=false only to deliberately share source charts.
+ With duplicate_slices=true, every chart is copied into new
chart objects owned by you, so edits to the copies never affect
the originals.
Review Comment:
Additionally fixed in 377a9082300e23a02daf89e95da20b38daa8f5be: the server
instructions say that dashboard copies deep-copy charts by default for
isolation, and frame duplicate_slices=false as deliberate sharing. I also found
and corrected the same opt-in wording in the tool module's overview.
Regression test:
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py::test_duplicate_description_explains_isolated_default
covers the server instructions and module overview as well as the registered
description and schema default. It failed against
cb9e08b488455b0a5642f8421466493073c71acb and passes after the correction
(targeted run: 2 passed). A separate before run with only the module overview
corrected still fails on the stale server instruction. Command: `pytest -q -m
'not manual and not llm_compliance'
tests/unit_tests/mcp_service/test_mcp_tool_registration.py::test_deleted_state_documentation_matches_list_schemas
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py::test_duplicate_description_explains_isolated_default`.
Siblings checked:
- superset/mcp_service/app.py — fixed in
377a9082300e23a02daf89e95da20b38daa8f5be
- superset/mcp_service/dashboard/tool/duplicate_dashboard.py — fixed in
377a9082300e23a02daf89e95da20b38daa8f5be
- superset/mcp_service/dashboard/schemas.py — not affected: default=true and
explicit false sharing already agree with the guidance.
- docs/admin_docs/configuration/mcp-server.mdx — not affected: copying guide
already describes independent charts as the default.
- UPDATING.md — not affected: migration note already explains the true
default and explicit false sharing.
- superset/mcp_service/dashboard/prompts/__init__.py — not affected: no
duplicate_dashboard copy-mode guidance.
- superset/mcp_service/system/prompts/quickstart.py — not affected: no
duplicate_dashboard copy-mode guidance.
- superset/mcp_service/chart/prompts/create_chart_guided.py — not affected:
chart-generation guidance does not describe dashboard copying.
- superset/dashboards/schemas.py — not affected: separate REST copy default
is unchanged.
- superset/daos/dashboard.py — not affected: both explicit copy modes are
retained; no execution semantics changed by these wording corrections.
--
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]
