-
Notifications
You must be signed in to change notification settings - Fork 9
Tests for multi choice value #2872
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
base: develop
Are you sure you want to change the base?
Changes from all commits
333c490
fabe096
349d08d
0ab67be
fd9d700
bf42d09
1745585
f349d82
92000dd
c3c8266
e5ec9db
33683e0
9a5773a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -236,6 +236,10 @@ else if (validator instanceof FieldDefinition.TextChoiceValidator textChoiceVali | |
| throw new IllegalArgumentException("TextChoice fields cannot have additional validators."); | ||
| } | ||
| fieldRow.setTextChoiceValues(textChoiceValidator.getValues()); | ||
| if(textChoiceValidator.getMultipleSelections()) | ||
| { | ||
| fieldRow.setAllowMultipleSelections(textChoiceValidator.getMultipleSelections()); | ||
| } | ||
|
Comment on lines
+239
to
+242
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would make it impossible to disable multi-select through this method.
Comment on lines
+239
to
+242
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will only set it if textChoiceValidator.getMultipleSelections is true, correct? This is ok if you are creating a field, but wouldn't work as expected if the test is editing an existing field and wants to change to field from a multi-choice to a single choice. |
||
| } | ||
| else | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -507,11 +507,14 @@ public WebElement setCellValue(int row, CharSequence columnIdentifier, Object va | |
|
|
||
| if (value instanceof List) | ||
| { | ||
|
|
||
| // If this is a list assume that it will need a lookup. | ||
| List<String> values = (List) value; | ||
|
|
||
| ReactSelect lookupSelect = elementCache().lookupSelect(gridCell); | ||
|
|
||
| lookupSelect.clearSelection(); | ||
|
|
||
|
Comment on lines
+516
to
+517
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this work if a test is trying to add to an existing selections? |
||
| lookupSelect.open(); | ||
|
|
||
| for (String _value : values) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,12 @@ | |
| import static org.hamcrest.CoreMatchers.is; | ||
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.junit.Assert.assertEquals; | ||
| import static org.labkey.remoteapi.query.Filter.Operator.CONTAINS_ALL; | ||
| import static org.labkey.remoteapi.query.Filter.Operator.CONTAINS_ANY; | ||
| import static org.labkey.remoteapi.query.Filter.Operator.CONTAINS_EXACTLY; | ||
| import static org.labkey.remoteapi.query.Filter.Operator.CONTAINS_NONE; | ||
| import static org.labkey.remoteapi.query.Filter.Operator.DOES_NOT_CONTAIN_EXACTLY; | ||
| import static org.labkey.remoteapi.query.Filter.Operator.IN; | ||
| import static org.labkey.test.WebDriverWrapper.waitFor; | ||
|
|
||
| public class ResponsiveGrid<T extends ResponsiveGrid<?>> extends WebDriverComponent<ResponsiveGrid<T>.ElementCache> implements UpdatingComponent | ||
|
|
@@ -234,15 +240,18 @@ public String filterColumnExpectingError(CharSequence columnIdentifier, Filter.O | |
|
|
||
| private GridFilterModal initFilterColumn(CharSequence columnIdentifier, Filter.Operator operator, Object value) | ||
| { | ||
| List<Filter.Operator> listOperators = List.of(IN, CONTAINS_ALL, CONTAINS_ANY, CONTAINS_EXACTLY, CONTAINS_NONE, | ||
| DOES_NOT_CONTAIN_EXACTLY); | ||
| clickColumnMenuItem(columnIdentifier, "Filter...", false); | ||
| GridFilterModal filterModal = new GridFilterModal(getDriver(), this); | ||
| if (operator != null) | ||
| { | ||
| if (operator.equals(Filter.Operator.IN) && value instanceof List<?>) | ||
| if (listOperators.contains(operator) && value instanceof List<?>) | ||
| { | ||
| List<String> values = (List<String>) value; | ||
| filterModal.selectFacetTab().selectValue(values.get(0)); | ||
| filterModal.selectFacetTab().checkValues(values.toArray(String[]::new)); | ||
| filterModal.selectFacetTab().selectFilter(operator.getDisplayValue()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this will (or could) break functionality for other column types; the facet panel usually doesn't have a filter type. |
||
| } | ||
| else | ||
| filterModal.selectExpressionTab().setFilter(new FilterExpressionPanel.Expression(operator, value)); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| import org.labkey.test.components.WebDriverComponent; | ||
| import org.labkey.test.components.html.Checkbox; | ||
| import org.labkey.test.components.html.Input; | ||
| import org.labkey.test.components.react.ReactSelect; | ||
| import org.labkey.test.components.ui.FilterStatusValue; | ||
| import org.openqa.selenium.WebDriver; | ||
| import org.openqa.selenium.WebElement; | ||
|
|
@@ -48,6 +49,15 @@ public void selectValue(String value) | |
| elementCache().findCheckboxLabel(value).click(); | ||
| } | ||
|
|
||
| /** | ||
| * Select a single facet value by clicking its label. Should replace all existing selections. | ||
| * @param value desired value | ||
| */ | ||
| public void selectFilter(String value) | ||
| { | ||
| elementCache().filterTypeSelects.select(value); | ||
| } | ||
|
Comment on lines
+52
to
+59
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update this comment to describe this method (looks leftover from the This could also take a |
||
|
|
||
| /** | ||
| * Check single facet value by label to see if it is checked or not. | ||
| * @param value desired value | ||
|
|
@@ -123,6 +133,8 @@ protected class ElementCache extends Component<?>.ElementCache | |
| { | ||
| protected final Input filterInput = | ||
| Input(Locator.id("filter-faceted__typeahead-input"), getDriver()).findWhenNeeded(this); | ||
| protected final ReactSelect filterTypeSelects = | ||
| new ReactSelect.ReactSelectFinder(getDriver()).index(0).findWhenNeeded(this); | ||
| protected final WebElement checkboxSection = | ||
| Locator.byClass("labkey-wizard-pills").index(0).refindWhenNeeded(this); | ||
| protected final Locator.XPathLocator checkboxLabelLoc | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,7 +82,7 @@ private void tryInsert(Map<String, String> values) | |
| { | ||
| for (Map.Entry<String, String> entry : values.entrySet()) | ||
| { | ||
| WebElement fieldInput = Locator.name(EscapeUtil.getFormFieldName(entry.getKey())).findElement(getDriver()); | ||
| WebElement fieldInput = Locator.nameContaining(EscapeUtil.getFormFieldName(entry.getKey())).findElement(getDriver()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use a more precise locator and add a comment explaining why this can't match the name exactly. (See |
||
| String type = fieldInput.getAttribute("type"); | ||
| switch (type) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,7 +15,6 @@ | |||||||
| import org.labkey.test.util.EscapeUtil; | ||||||||
| import org.openqa.selenium.WebDriver; | ||||||||
| import org.openqa.selenium.WebElement; | ||||||||
|
|
||||||||
| import java.io.File; | ||||||||
| import java.util.HashMap; | ||||||||
| import java.util.Map; | ||||||||
|
|
@@ -99,7 +98,7 @@ public UpdateQueryRowPage setField(String fieldName, String value) | |||||||
| WebElement field = elementCache().findField(fieldName); | ||||||||
| if (field.getTagName().equals("select")) | ||||||||
| { | ||||||||
| setField(fieldName, OptionSelect.SelectOption.textOption(value)); | ||||||||
| selectOptionByText(field, value); | ||||||||
| } | ||||||||
| else | ||||||||
| { | ||||||||
|
|
@@ -186,7 +185,7 @@ WebElement findField(String name) | |||||||
| { | ||||||||
| if (!fieldMap.containsKey(name)) | ||||||||
| { | ||||||||
| fieldMap.put(name, Locator.name(EscapeUtil.getFormFieldName(name)).findElement(this)); | ||||||||
| fieldMap.put(name, Locator.nameContaining(EscapeUtil.getFormFieldName(name)).findElement(this)); | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use a more precise locator and add a comment explaining why this can't match the name exactly.
Suggested change
|
||||||||
| } | ||||||||
| return fieldMap.get(name); | ||||||||
| } | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -475,6 +475,13 @@ public FieldDefinition setTextChoiceValues(List<String> values) | |
| return this; | ||
| } | ||
|
|
||
| public FieldDefinition setMultiChoiceValues(List<String> values) | ||
| { | ||
| Assert.assertEquals("Invalid field type for text choice values.", ColumnType.TextChoice, getType()); | ||
| setValidators(List.of(new FieldDefinition.TextChoiceValidator(values).setMultipleSelections())); | ||
| return this; | ||
| } | ||
|
Comment on lines
+478
to
+483
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be a separate |
||
|
|
||
| public ExpSchema.DerivationDataScopeType getAliquotOption() | ||
| { | ||
| return _aliquotOption; | ||
|
|
@@ -1112,6 +1119,8 @@ public static class TextChoiceValidator extends FieldValidator<TextChoiceValidat | |
| { | ||
| private final List<String> _values; | ||
|
|
||
| private Boolean multipleSelections = false; | ||
|
|
||
| public TextChoiceValidator(List<String> values) | ||
| { | ||
| // The TextChoice validator only has a name and no description or message. | ||
|
|
@@ -1143,6 +1152,17 @@ public List<String> getValues() | |
| return _values; | ||
| } | ||
|
|
||
| public TextChoiceValidator setMultipleSelections() | ||
| { | ||
| this.multipleSelections = true; | ||
| return this; | ||
| } | ||
|
|
||
| public Boolean getMultipleSelections() | ||
| { | ||
| return this.multipleSelections; | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -84,6 +84,7 @@ | |||||||||||||
| import java.util.List; | ||||||||||||||
| import java.util.Map; | ||||||||||||||
| import java.util.Set; | ||||||||||||||
| import java.util.stream.Collectors; | ||||||||||||||
|
|
||||||||||||||
| import static org.junit.Assert.assertEquals; | ||||||||||||||
| import static org.junit.Assert.assertFalse; | ||||||||||||||
|
|
@@ -1688,6 +1689,42 @@ public void testAutoIncrementKeyEncoded() | |||||||||||||
| _listHelper.deleteList(); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| @Test | ||||||||||||||
| public void testMultiChoiceValues() | ||||||||||||||
| { | ||||||||||||||
|
Comment on lines
+1693
to
+1694
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should only run the test on Postgres.
Suggested change
|
||||||||||||||
| // setup a list with an auto-increment key and multi text choice field | ||||||||||||||
| String encodedListName = "multiChoiceList"; | ||||||||||||||
| String keyName = "'><script>alert(\":(\")</script>'"; | ||||||||||||||
| String columnName = "MultiChoiceField"; | ||||||||||||||
|
Comment on lines
+1696
to
+1698
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using randomly generated field and list names. (I'm not sure whether ListHelper can handle the tricky characters but it's worth a try)
Suggested change
|
||||||||||||||
| List<String> tcValues = List.of("~`!@#$%^&*()_+=[]{}\\|';:\"<>?,./", "1", "2"); | ||||||||||||||
| _listHelper.createList(PROJECT_VERIFY, encodedListName, keyName, col(columnName, ColumnType.TextChoice) | ||||||||||||||
| .setMultiChoiceValues(tcValues)); | ||||||||||||||
| _listHelper.goToList(encodedListName); | ||||||||||||||
|
|
||||||||||||||
| DataRegionTable table = new DataRegionTable("query", getDriver()); | ||||||||||||||
| table.clickInsertNewRow(); | ||||||||||||||
| String valuesToChoose = tcValues.subList(1, 3).stream() | ||||||||||||||
| .sorted() | ||||||||||||||
| .collect(Collectors.joining(" ")); | ||||||||||||||
| Locator loc = Locator.nameContaining(EscapeUtil.getFormFieldName(columnName)); | ||||||||||||||
| selectOptionByText(loc, valuesToChoose); | ||||||||||||||
|
Comment on lines
+1705
to
+1710
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||
|
|
||||||||||||||
| clickButton("Submit"); | ||||||||||||||
| checker().verifyEquals("Multi choice value not as expected", valuesToChoose, table.getDataAsText(0, columnName)); | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| table.clickEditRow(0); | ||||||||||||||
| valuesToChoose = tcValues.subList(1, 3).stream() | ||||||||||||||
| .sorted() | ||||||||||||||
| .collect(Collectors.joining(" ")); | ||||||||||||||
| selectOptionByText(loc, valuesToChoose); | ||||||||||||||
| clickButton("Submit"); | ||||||||||||||
|
|
||||||||||||||
| // verify the multi choice value is persisted | ||||||||||||||
| checker().verifyEquals("Multi choice value not as expected", valuesToChoose, table.getDataAsText(0, columnName)); | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Take a screenshot.
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| _listHelper.deleteList(); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private List<String> getQueryFormFieldNames() | ||||||||||||||
| { | ||||||||||||||
| return Locator.tag("input").attributeStartsWith("name", "quf_") | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is less precise than we like for broadly available helpers; it would get confused by selection options that have overlapping text. I would suggest having a separate method for multi-select (
selectOptionsByText(WebElement selectElement, List<String> values).