Fix duplicate options returning same value in ComboboxWidget #3515
Conversation
📝 WalkthroughWalkthroughThe change refactors combobox-related classes to use ComboItem instead of String across three files. ComboboxAdapter now extends ArrayAdapter, with constructors, getItem, filtering, and internal arrays converted to ComboItem[]. Combobox accepts and processes Vector and uses ComboItem.getDisplayText() where appropriate. ComboboxWidget tracks choices as Vector, adds selectedComboItem state, provides getComboItemChoicesWithEmptyFirstSlot, maps selection values to ComboItem, and updates answer extraction, clearing, and initialization to operate on ComboItem objects. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/org/commcare/views/Combobox.java (1)
100-106: Fix capitalization check after type migration.
choicesis nowVector<ComboItem>, sochoices.contains(enteredText)will never match. This changes behavior and can cause unnecessary resets. Prefer comparing display text (or rely solely onchoicesAllLowerCase).🛠️ Suggested fix
- if (enteredText != null && !choices.contains(enteredText) && - choicesAllLowerCase.contains(enteredText.toLowerCase())) { - int index = choicesAllLowerCase.indexOf(enteredText.toLowerCase()); - setText(choices.get(index).getDisplayText()); - } + if (enteredText != null && choicesAllLowerCase.contains(enteredText.toLowerCase())) { + int index = choicesAllLowerCase.indexOf(enteredText.toLowerCase()); + String displayText = choices.get(index).getDisplayText(); + if (!displayText.equals(enteredText)) { + setText(displayText); + } + }
🤖 Fix all issues with AI agents
In `@app/src/org/commcare/views/widgets/ComboboxWidget.java`:
- Around line 121-138: fillInPreviousAnswer currently assumes
getComboItemFromSelectionValue returns a non-null ComboItem and directly calls
selectedComboItem.getDisplayText(), which can NPE if choices changed; update
fillInPreviousAnswer to check the result of
getComboItemFromSelectionValue(selectionValue) for null before assigning to
selectedComboItem and calling comboBox.setText, and handle the null case (e.g.,
leave selectedComboItem null and skip setting comboBox text or set a safe
fallback) so comboBox.setText and selectedComboItem are only used when a valid
ComboItem is returned.
- Around line 147-158: getAnswer() currently only uses selectedComboItem (set on
click or prior fill) so typed input is ignored; change logic in
ComboboxWidget.getAnswer() to read the current text from the combo box when
selectedComboItem is null or not found in choiceComboItems, compare that text
against the available choices (choices vector / SelectChoice labels or values),
and if a match is found return a SelectOneData with that matching Selection; if
the text is empty return null, otherwise return InvalidData with the entered
text wrapped in SelectOneData. Refer to symbols:
comboBox.autoCorrectCapitalization(), selectedComboItem, choiceComboItems,
choices, SelectOneData, Selection, and InvalidData to locate and update the
code.
| comboBox.autoCorrectCapitalization(); | ||
| String enteredText = comboBox.getText().toString(); | ||
| if (choiceTexts.contains(enteredText)) { | ||
| int i = choiceTexts.indexOf(enteredText); | ||
| return new SelectOneData(new Selection(choices.elementAt(i))); | ||
| } else if ("".equals(enteredText)) { | ||
| return null; | ||
| if (selectedComboItem != null && choiceComboItems.contains(selectedComboItem)) { | ||
| int selectChoiceIndex = selectedComboItem.getSelectChoiceIndex(); | ||
| return new SelectOneData(new Selection(choices.elementAt(selectChoiceIndex))); | ||
| } else { | ||
| return new InvalidData("The text entered is not a valid answer choice", | ||
| new SelectOneData(new Selection(enteredText))); | ||
| String enteredText = selectedComboItem!=null ? selectedComboItem.getDisplayText() : ""; | ||
| if ("".equals(enteredText)) { | ||
| return null; | ||
| } else { | ||
| return new InvalidData("The text entered is not a valid answer choice", | ||
| new SelectOneData(new Selection(enteredText))); | ||
| } |
There was a problem hiding this comment.
Typed entries are ignored unless user clicks a dropdown item.
getAnswer() relies on selectedComboItem, which is only set on click or previous-answer fill. If a user types a valid option (or edits after selection), this returns null/invalid. This is a functional regression.
🛠️ Suggested fix
- comboBox.autoCorrectCapitalization();
- if (selectedComboItem != null && choiceComboItems.contains(selectedComboItem)) {
+ comboBox.autoCorrectCapitalization();
+ String enteredText = comboBox.getText().toString();
+ if (selectedComboItem == null || !enteredText.equals(selectedComboItem.getDisplayText())) {
+ selectedComboItem = null;
+ for (ComboItem item : choiceComboItems) {
+ if (item.getDisplayText().equals(enteredText)) {
+ selectedComboItem = item;
+ break;
+ }
+ }
+ }
+ if (selectedComboItem != null && choiceComboItems.contains(selectedComboItem)) {
int selectChoiceIndex = selectedComboItem.getSelectChoiceIndex();
return new SelectOneData(new Selection(choices.elementAt(selectChoiceIndex)));
} else {
- String enteredText = selectedComboItem!=null ? selectedComboItem.getDisplayText() : "";
if ("".equals(enteredText)) {
return null;
} else {🤖 Prompt for AI Agents
In `@app/src/org/commcare/views/widgets/ComboboxWidget.java` around lines 147 -
158, getAnswer() currently only uses selectedComboItem (set on click or prior
fill) so typed input is ignored; change logic in ComboboxWidget.getAnswer() to
read the current text from the combo box when selectedComboItem is null or not
found in choiceComboItems, compare that text against the available choices
(choices vector / SelectChoice labels or values), and if a match is found return
a SelectOneData with that matching Selection; if the text is empty return null,
otherwise return InvalidData with the entered text wrapped in SelectOneData.
Refer to symbols: comboBox.autoCorrectCapitalization(), selectedComboItem,
choiceComboItems, choices, SelectOneData, Selection, and InvalidData to locate
and update the code.
There was a problem hiding this comment.
This crossed my mind and I'm glad you flagged it too. We can return the first option when the text matches a valid option
There was a problem hiding this comment.
@avazirna Great, I'm glad we're aligned on this! The "first match" approach makes sense, especially given that this PR is about supporting duplicate options.
Just to clarify the logic: when getAnswer() is called, if selectedComboItem is null or the entered text doesn't match it, the code should iterate through choiceComboItems to find the first ComboItem whose getDisplayText() matches the entered text. If found, return the corresponding SelectOneData; otherwise, follow the existing invalid/empty logic.
This ensures that both click-selection and typed-entry work correctly, and when there are duplicate display texts, the first matching option is consistently used.
Let me know if you'd like me to plan the implementation or if you've got it covered! 🚀
avazirna
force-pushed
the
fix-duplicate-options-issue-in-combobox-widget
branch
from
2446234 to
78f3ca4
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
avazirna
changed the title
|
@damagatchi retest this please |
| } | ||
|
|
||
| private ComboItem getComboItemFromSelectionValue(String selectionValue) { | ||
| for (ComboItem cboIt: choiceComboItems) { |
There was a problem hiding this comment.
Nit: can we name this variable comboItem since cboIt is not a common contraction
|
@avazirna can we add the Safety Story section to the PR description describing how you've personally tested these changes and are confident that this solves the issue |
Product Description
This PR addresses an issue with the
ComboboxWidgetwhere, when duplicate options exist in the dropdown list, the returned value is always from the first option:comboboxwidget_duplicates_issue.mp4
cross-request: dimagi/commcare-core#1516
Ticket: https://dimagi.atlassian.net/browse/SAAS-18514
Technical Summary
The root cause of this behavior was a design limitation: the
ComboboxAdapterwas based onString, so after a selection is made, it fetches the value by comparing the selected text with the text of each the question choices and returning the first match found. This fix makes the ComboboxAdapter to be based on a new model,ComboItem.Safety Assurance
QA Plan
This will need to go through QA.
Labels and Review