EnxDev commented on code in PR #44392:
URL: https://github.com/apache/superset/pull/44392#discussion_r4062718903
##########
superset-frontend/src/pages/DatabaseList/index.tsx:
##########
@@ -1098,16 +1198,37 @@ function DatabaseList({
<DeleteModal
description={
<>
- <p>
- {t('The %s', databaseLabelLower())}{' '}
- <b>{databaseCurrentlyDeleting.database_name}</b>{' '}
- {t(
- '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.',
- databaseCurrentlyDeleting.charts.count,
- databaseCurrentlyDeleting.dashboards.count,
- databaseCurrentlyDeleting.sqllab_tab_count,
- )}
- </p>
+ {/* Datasets block the delete outright (the backend refuses while
+ any dataset still references the database), so the dataset
+ case must not promise a destructive outcome that cannot
+ happen -- it has to say the delete is blocked and name what
+ is blocking it. */}
+ {databaseCurrentlyDeleting.datasets.count >= 1 ? (
+ <p>
+ {t('The %s', databaseLabelLower())}{' '}
+ <b>{databaseCurrentlyDeleting.database_name}</b>{' '}
+ {tn(
+ 'cannot be deleted because %s dataset is still attached to
it. Delete or move that dataset first.',
+ 'cannot be deleted because %s datasets are still attached
to it. Delete or move those datasets first.',
Review Comment:
Could we make the cleanup guidance cover archived datasets here? With
`SOFT_DELETE` enabled (the default), removing a dataset from the dataset list
archives it, so it still contributes to this count and blocks the connection
delete. Once only archived datasets remain, the normal dataset list is empty,
and “Delete or move” gives the user no next step. Disabling the button also
means they never see `DatabaseDeleteSoftDeletedDatasetsExistFailedError`, which
explains the purge flow. Pointing them to **Recently archived → Delete
permanently**, and covering the archived-only case in the modal tests, would
make this actionable.
##########
superset-frontend/src/pages/DatabaseList/index.tsx:
##########
@@ -1206,6 +1327,7 @@ function DatabaseList({
}}
onHide={() => setDatabaseCurrentlyDeleting(null)}
open
+ disablePrimaryButton={databaseCurrentlyDeleting.datasets.count >= 1}
Review Comment:
Small UX suggestion: could the blocked state use an informational modal with
a Close action and no confirmation input? `DeleteModal` still renders and
focuses “Type DELETE to confirm”, even though typing it can never enable the
button here. The new disabled-action test confirms that behavior. Hiding that
prompt while datasets are attached would avoid inviting the user to complete a
step that cannot do anything.
--
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]
