rusackas commented on code in PR #38584:
URL: https://github.com/apache/superset/pull/38584#discussion_r3662396491
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -148,7 +148,10 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
if (dashboardId) {
try {
const result = (await this.loadDashboard(dashboardId)) as Dashboard;
- if (canUserEditDashboard(result, this.props.user)) {
+ if (
+ canUserEditDashboard(result, this.props.user) &&
+ !result.is_managed_externally
+ ) {
Review Comment:
Both are covered now, `does not preselect an externally managed dashboard on
mount` and `loadDashboards includes is_managed_externally filter`.
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -240,6 +240,87 @@ test('renders a message when saving as with new
dashboard', () => {
);
});
+test('does not preselect an externally managed dashboard on mount', async ()
=> {
+ const dashboardId = 1;
+ fetchMock.removeRoutes();
+ fetchMock.get(fetchDashboardsEndpoint, mockDashboardData);
+ fetchMock.get(fetchChartEndpoint, { id: 1, dashboards: [1] });
+ fetchMock.get(`glob:*/api/v1/dashboard/${dashboardId}`, {
+ result: {
+ id: dashboardId,
+ dashboard_title: 'Managed Dashboard',
+ owners: [{ id: 1 }],
+ is_managed_externally: true,
+ },
+ });
+
+ const store = mockStore({
+ ...initialState,
+ explore: {
+ ...initialState.explore,
+ slice: {
+ ...initialState.explore.slice,
+ dashboards: [dashboardId],
+ },
+ },
+ });
+
+ const { queryByTestId } = setup(
+ {
+ ...defaultProps,
+ dashboardId,
+ },
+ store,
+ );
+
+ await waitFor(() => {
+ expect(
+ fetchMock.callHistory.calls(`glob:*/api/v1/dashboard/${dashboardId}`),
+ ).toHaveLength(1);
+ });
+
+ const selectInput = queryByTestId('mock-async-select') as HTMLInputElement;
+ expect(selectInput).toBeInTheDocument();
+ expect(
+ fetchMock.callHistory.calls(`glob:*/api/v1/dashboard/${dashboardId}/tabs`),
+ ).toHaveLength(0);
+
Review Comment:
Fair point. Extended the AsyncSelect mock to expose the selected value, so
the test now asserts the input stays empty instead of only checking that the
/tabs fetch never fired.
##########
tests/integration_tests/charts/commands_tests.py:
##########
@@ -393,6 +394,50 @@ def test_create_v1_response(self, mock_sm_g, mock_c_g,
mock_u_g):
db.session.delete(chart)
db.session.commit()
+ @patch("superset.utils.core.g")
+ @patch("superset.commands.chart.create.g")
+ @patch("superset.security.manager.g")
+ @pytest.mark.usefixtures("load_energy_table_with_slice")
+ def test_create_chart_rejects_externally_managed_dashboard(
+ self, mock_sm_g, mock_c_g, mock_u_g
+ ):
Review Comment:
Added type hints and a `-> None` return here.
##########
tests/integration_tests/charts/commands_tests.py:
##########
@@ -668,6 +713,43 @@ def test_update_chart_dashboard_security_admin_bypass(
db.session.delete(alpha_dashboard)
db.session.commit()
+ @patch("superset.commands.chart.update.g")
+ @patch("superset.utils.core.g")
+ @patch("superset.security.manager.g")
+ @pytest.mark.usefixtures("load_energy_table_with_slice")
+ def test_update_chart_rejects_new_externally_managed_dashboard(
+ self, mock_sm_g, mock_u_g, mock_c_g
+ ):
Review Comment:
Same fix here, added the type hints and `-> None`.
##########
superset-frontend/src/types/Dashboard.ts:
##########
@@ -36,6 +36,7 @@ export interface Dashboard {
owners: Owner[];
extra_owners?: Owner[];
roles: Role[];
+ is_managed_externally?: boolean;
Review Comment:
Good catch, made it required. The column is non-nullable on the backend and
`Chart.ts` already treats it that way.
--
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]
