fix: delete multiple tables from lib pane #1705
Conversation
Signed-off-by: Irene Sanchez <irene.iscc777@gmail.com>
Signed-off-by: Irene Sanchez <irene.iscc777@gmail.com>
IreneS1
force-pushed
the
fix/delete-multi-lib-tables
branch
from
a911cc5 to
f4047d3
Compare
IreneS1
changed the title
| ? this.libraryDataProvider.treeView.selection | ||
| : [item]; | ||
|
|
||
| return items.filter(Boolean); |
There was a problem hiding this comment.
Is there ever a case where its would be falsy here? Do we need this .filter?
| } | ||
|
|
||
| private treeViewSelections(item: LibraryItem): LibraryItem[] { | ||
| const items = |
There was a problem hiding this comment.
I see this was copy/pasted from ContentNavigator. Could we make this a shared function that takes a treeview, perhaps in client/src/components/utils?
Right now, I think there's a bug with this (here and in content navigator). Putting this functionality in one place will make it easier to resolve.
In case you're curious, or want to tackle this in your PR (definitely out of scope), here's the bug:
- Create some deletable tables. I ended up running...
data work.class;
set sashelp.class;
run;
data work.classfit;
set sashelp.classfit;
run;
data work.air;
set sashelp.air;
run;
data work.aarfm;
set sashelp.aarfm;
run;
- Select
classandclassfit - With those two selected, right click on
aarfmand press delete
In this scenario, I would expect aarfm to be the item up for deletion. However, what you get instead is a prompt to delete class and classfit.
Like I said, this also happens with sas content and server files, so feel free to not resolve it in this PR and I can file a bug.
|
|
||
| try { | ||
| await this.libraryDataProvider.deleteTable(item); | ||
| if (selectedItems.length === 1) { |
There was a problem hiding this comment.
Instead of having two different paths here, how about we:
- Get rid of
LibraryDataProvider.deleteTable(you'll likely need to make a small update to tests) - Always call
LibraryDataProvider.deleteTables - Always show a prompt regardless if you're deleting one or more tables
| export const Messages = { | ||
| TableDeletionError: l10n.t("Unable to delete table {tableName}."), | ||
| TablesDeletionWarning: l10n.t( | ||
| "Are you sure you want to delete {count} table(s): {tableNames}?", |
There was a problem hiding this comment.
It might be worth asking @skyeaussie what message to use here. We may not need the count or extra parens around the s. Maybe something like...
Are you sure you want to delete the following tables: {tableNames}?
It looks like studio uses the following two messages:
Are you sure you want to delete the selected tables?
Are you sure you want to delete the table "{tableName}"
| deleteTableStub.restore(); | ||
| }); | ||
|
|
||
| it("deleteTables - deletes multiple tables successfully", async () => { |
2 participants
Summary:
Enhanced the delete functionality to support multiple table selection with a confirmation dialog that lists all selected tables. Once the user confirms, the tables are deleted. Single table deletion proceeds without a confirmation dialog.
Testing:
deleteTables - deletes multiple tables successfully
-Tests successful deletion of 2 tables
-Verifies API is called correct number of times
deleteTables - reports failures when some tables fail to delete
-Tests partial failure scenario (3 tables: succeed, fail, succeed)
-Verifies error message contains only failed table names
-Confirms all deletion attempts were made
Fixes: #1216