Making it harder to delete all rows from lists#2904
Making it harder to delete all rows from lists#2904XingY wants to merge 2 commits intorelease26.3-SNAPSHOTfrom
Conversation
labkey-nicka
left a comment
labkey-nicka
left a comment
There was a problem hiding this comment.
Overall, seems fine. I'd recommend trying to use more of a page component approach even in these LKS tests. Right now this feels like a mix of approaches.
| if (attachmentFileName.contains(IMG_FILE.getName())) | ||
| { | ||
| log("Hover over the thumbnail for the image and make sure the pop-up is as expected."); | ||
| // Mouse over the logo, migh help with the following mouse over the image. |
There was a problem hiding this comment.
Consider sharing this code with the two other usages in InlineImagesListTest
| clickButton("Cancel"); | ||
| } | ||
|
|
||
| private void selectLists(ManageListsGrid grid, List<String> listNames) |
There was a problem hiding this comment.
Consider putting this on ManageListsGrid. Something like:
public ManageListsGrid selectLists(Collection<String> listNames)
{
for (String listName : listNames)
checkCheckbox(getRowIndex("Name", listName));
return this;
}| // Select lists, verify delete menu is enabled and has 2 options | ||
| selectLists(grid, listNames); | ||
| var menuOptions = grid.getHeaderMenuOptions("Delete"); | ||
| Assert.assertEquals("Expected 2 delete menu options", |
There was a problem hiding this comment.
It seems like the preferred pattern these days is to use checker().withScreenshot().... I'm pointing this one out as an example but this goes for the whole file.
checker().withScreenshot().verifyEquals("Expected 2 delete menu options", List.of("Delete List", "Delete All Data from List"), menuOptions);| listsPage = BeginPage.beginAt(this, containerPath); | ||
| grid = listsPage.getGrid(); | ||
| selectLists(grid, listNames); | ||
| grid.clickHeaderMenu("Delete", true, "Delete List"); |
There was a problem hiding this comment.
Both the "Delete List" and "Delete All Data from List" menu item actions should be on ManageListsGrid. Delete list already is with deleteSelectedLists() but that implementation may need updating to align with how we sometimes generate a button and sometimes generate a dropdown now.
| selectLists(grid, listNames); | ||
| var menuOptions = grid.getHeaderMenuOptions("Delete"); | ||
| Assert.assertEquals("Expected 2 delete menu options", | ||
| List.of("Delete List", "Delete All Data from List"), menuOptions); |
There was a problem hiding this comment.
nit: The page where you land when clicking "Delete All Data from List" should have a page component associated with it.
2 participants
Rationale
Related Pull Requests
Changes