Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Show sqllab state when deleting databases #17331

Merged
merged 7 commits into from
Nov 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ fetchMock.get(databaseRelatedEndpoint, {
count: 0,
result: [],
},
sqllab_tab_states: {
count: 0,
result: [],
},
});

describe('DatabaseList', () => {
Expand Down Expand Up @@ -131,7 +135,7 @@ describe('DatabaseList', () => {
await waitForComponentToPaint(wrapper);

expect(wrapper.find(DeleteModal).props().description).toMatchInlineSnapshot(
`"The database db 0 is linked to 0 charts that appear on 0 dashboards. Are you sure you want to continue? Deleting the database will break those objects."`,
`"The database db 0 is linked to 0 charts that appear on 0 dashboards and users have 0 SQL Lab tabs using this database open. Are you sure you want to continue? Deleting the database will break those objects."`,
);

act(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const CONFIRM_OVERWRITE_MESSAGE = t(
interface DatabaseDeleteObject extends DatabaseObject {
chart_count: number;
dashboard_count: number;
sqllab_tab_count: number;
}
interface DatabaseListProps {
addDangerToast: (msg: string) => void;
Expand Down Expand Up @@ -122,6 +123,7 @@ function DatabaseList({ addDangerToast, addSuccessToast }: DatabaseListProps) {
...database,
chart_count: json.charts.count,
dashboard_count: json.dashboards.count,
sqllab_tab_count: json.sqllab_tab_states.count,
});
})
.catch(
Expand Down Expand Up @@ -437,10 +439,11 @@ function DatabaseList({ addDangerToast, addSuccessToast }: DatabaseListProps) {
{databaseCurrentlyDeleting && (
<DeleteModal
description={t(
'The database %s is linked to %s charts that appear on %s dashboards. Are you sure you want to continue? Deleting the database will break those objects.',
'The database %s is linked to %s charts that appear on %s dashboards and users have %s SQL Lab tabs using this database open. Are you sure you want to continue? Deleting the database will break those objects.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to skip this message if all the counts are 0, something like:

const message = (
  databaseCurrentlyDeleting.chart_count +
  databaseCurrentlyDeleting.dashboard_count +
  databaseCurrentlyDeleting.sqllab_tab_count) === 0
  ? 'There are no charts, dashboards or queries associated with this database, it should be safe to delete.'
  : 'The database %s is linked to ...';

databaseCurrentlyDeleting.database_name,
databaseCurrentlyDeleting.chart_count,
databaseCurrentlyDeleting.dashboard_count,
databaseCurrentlyDeleting.sqllab_tab_count,
)}
onConfirm={() => {
if (databaseCurrentlyDeleting) {
Expand Down
8 changes: 8 additions & 0 deletions superset/databases/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -689,10 +689,18 @@ def related_objects(self, pk: int) -> Response:
}
for dashboard in data["dashboards"]
]
sqllab_tab_states = [
{"id": tab_state.id, "label": tab_state.label, "active": tab_state.active}
for tab_state in data["sqllab_tab_states"]
]
return self.response(
200,
charts={"count": len(charts), "result": charts},
dashboards={"count": len(dashboards), "result": dashboards},
sqllab_tab_states={
"count": len(sqllab_tab_states),
"result": sqllab_tab_states,
},
)

@expose("/export/", methods=["GET"])
Expand Down
10 changes: 9 additions & 1 deletion superset/databases/dao.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from superset.models.core import Database
from superset.models.dashboard import Dashboard
from superset.models.slice import Slice
from superset.models.sql_lab import TabState

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -87,4 +88,11 @@ def get_related_objects(cls, database_id: int) -> Dict[str, Any]:
.distinct()
.all()
)
return dict(charts=charts, dashboards=dashboards)

sqllab_tab_states = (
db.session.query(TabState).filter(TabState.database_id == database_id).all()
)

return dict(
charts=charts, dashboards=dashboards, sqllab_tab_states=sqllab_tab_states
)
10 changes: 10 additions & 0 deletions tests/integration_tests/databases/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -1989,3 +1989,13 @@ def test_validate_parameters_invalid_port_range(self, is_hostname_valid):
},
]
}

def test_get_related_objects(self):
example_db = get_example_database()
self.login(username="admin")
uri = f"api/v1/database/{example_db.id}/related_objects/"
rv = self.client.get(uri)
assert rv.status_code == 200
assert "charts" in rv.json
assert "dashboards" in rv.json
assert "sqllab_tab_states" in rv.json