Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Treat empty as null values to fix case search #2779

Merged
merged 6 commits into from
Jul 31, 2024
Merged

Treat empty as null values to fix case search #2779

merged 6 commits into from
Jul 31, 2024

Conversation

shubham1g5
Copy link
Contributor

@shubham1g5 shubham1g5 commented Jul 5, 2024 •
edited
Loading

Summary

https://dimagi.atlassian.net/browse/QA-6604

This is a workaround until we start supporting blank value search on mobile similar to what we did on Web Apps

Feature Flag

CASE_SEARCH_CLAIM

Safety Assurance

  • If the PR is high risk, "High Risk" label is set
  • I have confidence that this PR will not introduce a regression for the reasons below
  • Do we need to enhance manual QA test coverage ? If yes, "QA Note" label is set correctly

Automated test coverage

not adding tests right now as it's a temp workaround we wish to remove soon

cross-request: dimagi/commcare-core#1421

@shubham1g5 shubham1g5 requested a review from avazirna July 5, 2024 09:52
@shubham1g5 shubham1g5 mentioned this pull request Jul 8, 2024
Merged
3 tasks
@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

2 similar comments
@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

@shubham1g5 shubham1g5 force-pushed the handleEmptyAsNullInCaseSearch branch from dbcf064 to 01c557e Compare July 9, 2024 05:45
@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

3 similar comments
@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

@shubham1g5 shubham1g5 force-pushed the handleEmptyAsNullInCaseSearch branch from 01c557e to 162f5c4 Compare July 9, 2024 14:07
@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

@shubham1g5 shubham1g5 force-pushed the handleEmptyAsNullInCaseSearch branch from 3901f00 to b259746 Compare July 10, 2024 06:52
@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

2 similar comments
@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

@shubham1g5 shubham1g5 force-pushed the handleEmptyAsNullInCaseSearch branch from 6ba917f to 55d1edd Compare July 10, 2024 08:25
@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

4 similar comments
@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

@shubham1g5
Copy link
Contributor Author

@damagatchi retest this please

@shubham1g5 shubham1g5 added skip-integration-tests Skip android tests. and removed skip-integration-tests Skip android tests. labels Jul 11, 2024
@avazirna
Copy link
Contributor

@damagatchi retest this please

4 similar comments
@avazirna
Copy link
Contributor

@damagatchi retest this please

@avazirna
Copy link
Contributor

@damagatchi retest this please

@avazirna
Copy link
Contributor

@damagatchi retest this please

@avazirna
Copy link
Contributor

@damagatchi retest this please

@avazirna
Copy link
Contributor

@shubham1g5 can you review the changes I made?

shubham1g5
shubham1g5 commented Jul 29, 2024
View reviewed changes
app/src/org/commcare/activities/QueryRequestUiController.kt Outdated
if (position > 0) {
val choices = queryPrompt.itemsetBinding!!.choices
val selectChoice = choices[position - 1]
value = selectChoice.value
updateAnswerAndRefresh(queryPrompt, selectChoice.value)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@avazirna what prompted this change ? What if user clears the spinner choice and selects nothing ?

Copy link
Contributor

@avazirna avazirna Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shubham1g5 because of an infinite loop, when the data source is set for the first time and onItemSelected is called, it calls the updateAnswerAndRefresh with an empty String value; and in that method, when oldAnswer == null || !oldAnswer.contentEquals(answer) is true, we are now setting the user answer value to null which means that when we call refreshView() and refresh the data source again and onItemSelected is triggered again with an empty String, oldAnswer == null || !oldAnswer.contentEquals(answer) continues to evaluating to true, and so on. And you are right, this option misses the scenario when the user changes back to an, I thought the index started at 1. Let me fix that, there is another scenario I tested with.

@shubham1g5
Copy link
Contributor Author

can you review the changes I made?

curious how did we resolve the Jenkins getting stuck issue with this PR ?

@avazirna avazirna force-pushed the handleEmptyAsNullInCaseSearch branch from 53398c0 to 509a3a6 Compare July 29, 2024 10:33
@avazirna avazirna force-pushed the handleEmptyAsNullInCaseSearch branch from 509a3a6 to 8baf5d0 Compare July 29, 2024 11:48
@avazirna
Copy link
Contributor

@shubham1g5 can I have another look at this?

@shubham1g5
Copy link
Contributor Author

@avazirna looks good to me, consider it approved from my side.

@avazirna avazirna merged commit 356e463 into commcare_2.54 Jul 31, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avazirna avazirna avazirna left review comments

Assignees
No one assigned
Labels
cross requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shubham1g5 @avazirna