Fix case list search #1439
Fix case list search #1439
Conversation
|
@avazirna Can we add more details on around when the issue happens ? or How can QA repro it in a sample app if they want to ? (I could not see these details in the linked ticket as well) |
| } | ||
| } | ||
| } catch (IOException e) { | ||
| Logger.exception("Error while getting sort field", e); | ||
| } | ||
| return null; | ||
| return sortData[i]; |
There was a problem hiding this comment.
when does the default behaviour get triggered here ? Seems like when mEntityStorageCache == null ? (My assumption is it should never be null for AsyncEntity for cc-android)
There was a problem hiding this comment.
We cache the properties when we load the case list so the default value is used here when sortData[i] != null
There was a problem hiding this comment.
I'm not super comfortable with this change as currently documented.
The PR description mentions that the null condition was introduced in error, but doesn't reference the change or clarify why the change was wrong, and it doesn't spell out why the method is currently falling through to the null condition and why sortData[i] is a safe fallback in all cases.
There's still at least one null return condition above on line 142, so if this method should never return null, this change also isn't really sufficient. In that case sortData[i] is also explicitly null, so it seems problematic to presume that this will ~always result in a safe return.
There was a problem hiding this comment.
@ctsims I added more details to the PR description, let me know if it's clearer now. The null on line 142 seems right, when there is no data, we don't need to continue the execution.
There was a problem hiding this comment.
@avazirna Looking at the previous version of this class it seems like we should be returning null here (in case of an IOException encountered) and instead should be returning sortData[i] after the closing of if (sortData[i] == null) { } block on line 174.
There was a problem hiding this comment.
@shubham1g5 sortData is initialised with null values so in case an IOException is thrown, the current approach should return the element in sortData, which can be null or an existing value. For readability sake, I'm happy to follow your lead.
| } | ||
| } | ||
| } catch (IOException e) { | ||
| Logger.exception("Error while getting sort field", e); | ||
| } | ||
| return null; | ||
| return sortData[i]; |
There was a problem hiding this comment.
I'm not super comfortable with this change as currently documented.
The PR description mentions that the null condition was introduced in error, but doesn't reference the change or clarify why the change was wrong, and it doesn't spell out why the method is currently falling through to the null condition and why sortData[i] is a safe fallback in all cases.
There's still at least one null return condition above on line 142, so if this method should never return null, this change also isn't really sufficient. In that case sortData[i] is also explicitly null, so it seems problematic to presume that this will ~always result in a safe return.
avazirna
force-pushed
the
fix-case-list-search
branch
from
f4921d0 to
9e50d74
Compare
Product Description
Users have reported issues with the case list search feature after updating to CommCare 2.54, claiming that it's not filtering cases as expected. After some investigation, it was noted that the issue is related to AsyncEntities used by features such as
Case Tile templatesandLazy load case list. Currently, when a user tries to search for a case, even when a case with properties matching the search key is present, the case list comes out empty, trying to delete the search key also results in an empty case list:issue_case_list_search.mp4
Details about the bug
When filtering case data, the method EntitySortUtil.sortEntities() is called, within this method, CommCare loops through all case list properties of each case and retrieves data by using getNormalizedField(). In the AsyncEntity implementation, this method calls getSortField() which is supposed to return previously loaded sort data or retrieve it from the cache, and here lies the issue, when a value exists, the method returns null instead of the existing value, causing the entire list to be empty. It seems that during a previous refactoring work, the sort field was mistakenly set to default to
null, this PR corrects that.Ticket: https://dimagi.atlassian.net/browse/SUPPORT-22191
Safety Assurance
Safety story
Not applicable, maybe a note to QA to ensure that this is included in the scope of the Regression tests plan.
Rollback instructions
Review
Duplicate PR
Automatically duplicate this PR as defined in contributing.md.