rusackas commented on PR #40648: URL: https://github.com/apache/superset/pull/40648#issuecomment-4666044193
@richardfogaca Thanks for the thorough pass — addressed the actionable items in d796fc3: - **Integration test now earns its keep** (`commands_tests.py:483`): patched `ChartDAO.find_by_id` to return the chart directly, bypassing the `ChartFilter` base filter, and tightened the assertion to `ChartForbiddenError` only. It now deterministically exercises the new `raise_for_access` branch and fails on master. - **Unit assertion pinned** (`update_test.py:76`): `raise_for_access.assert_called_once_with(chart=...)` so a refactor cannot silently switch the authorized resource. - **Nits**: added `-> None`, renamed the mock params to match their decorators (`mock_core_g` / `mock_update_g` / `mock_sm_g`), switched the order-dependent `.all()[0]` to `filter_by(slice_name="Energy Sankey").one()`, and added an `_access_exc()` helper so the access-denied path uses a `CHART_SECURITY_ACCESS_ERROR` instead of mislabeling it as an ownership error. - Also added the positive test Copilot asked for: `test_update_chart_query_context_non_owner_with_access_allowed`, pinning that a non-owner whose access check passes can complete the backfill. On the `query_context.datasource` validation gap at `update.py:144` — agreed that lets a non-owner with datasource access overwrite a sibling chart's stored `query_context`, and that it predates this PR. I'll open a follow-up issue to validate the payload's datasource against the chart's own `datasource_id` rather than widen this PR's scope. Thanks for the static trace on the report path too — matches what I found. -- 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]
