Only prime cache, if it is available#1538
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe ChangesCache Guard in Sort Field Computation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
| if (isCancelled) return; | ||
| AsyncEntity e = (AsyncEntity)entities.get(i); | ||
| for (int col = 0; col < e.getNumFields(); ++col) { | ||
| if (mEntityCache != null) { |
There was a problem hiding this comment.
Not sure if we would want put this behind a feature flag for now.
There was a problem hiding this comment.
Looks like commcare-core doesn't have access to feature flags, is that right? We could pass through a boolean if we needed to - might be nice to easily toggle on and off for performance testing, but coordinating across three repos is tedious, and Shubham seemed optimistic about this change, so maybe not worth the effort. Dunno, I could go either way.
There was a problem hiding this comment.
yeah I am quite confident for this to be rolloed out without a feature flag.
Verifies that prepareEntities does not call getSortField during the cache priming loop when no EntityStorageCache is present, which is always the case in formplayer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@shubham1g5 Do you have a suggestion how to test the mobile app for regressions (making sure sort works as intended)? |
|
@MartinRiese I don't think there is a need to do, the cache is always not-null for mobile when we use |
| import static org.junit.Assert.assertEquals; | ||
|
|
||
| /** | ||
| * Regression test for USH-6551: sort field expressions must not be evaluated during the cache |
There was a problem hiding this comment.
Curious if it's a common practice on USS to add Jira ticket # in code comments, we have refrained from doing so on mobile team in past as the code is open source and putting these refrences make it cryptic for anyone external looking into the code.
Product Description
Add a check in the cache priming code. Only do the priming, if there is a cache set.
Technical Summary
Safety Assurance
Safety story
Deploy to staging and test variations:
Automated test coverage
Added a test to check for future regressions which doubles as check on the fix.
QA Plan
Special deploy instructions
Rollback instructions
Review
Summary by CodeRabbit