Fix favorite_lists returning null on cache miss and add regression coverage #3435
Conversation
📝 WalkthroughWalkthroughFixes favorites cache population and deduplication, preserves ordering, invalidates list and per-list caches and deletes empty DB objects on removal. Adds a test asserting the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (.cursorrules)
Files:
**/tests/**/*.py📄 CodeRabbit inference engine (.cursorrules)
Files:
🧬 Code graph analysis (1)care/emr/tests/test_favorites_api.py (1)
🔇 Additional comments (1)
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: 0
🧹 Nitpick comments (1)
care/emr/tests/test_favorites_api.py (1)
241-250: LGTM! Good regression coverage.The test correctly validates that the endpoint returns a proper list structure on first call after cache miss. The assertions cover status, field presence, non-null value, and type, which directly address the bug that was fixed.
Optional: Consider verifying cache population
While the test validates the response structure, you could also assert that the cache was populated after the first call to ensure the fix works end-to-end:
def test_favorite_lists_returns_list_on_first_call(self): # Ensure cache is empty cache.delete(self.favorite_list_cache_key) response = self.client.get(self.base_url + "favorite_lists/") self.assertEqual(response.status_code, 200, response.content) self.assertIn("lists", response.data) self.assertIsNotNone(response.data["lists"]) self.assertIsInstance(response.data["lists"], list) + + # Verify cache was populated + cached_data = cache.get(self.favorite_list_cache_key) + self.assertIsNotNone(cached_data, "Cache should be populated after first call") + self.assertIsInstance(cached_data, list)However, since
test_list_favorites_listalready validates cache behavior, this addition is purely optional.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/api/viewsets/favorites.pycare/emr/tests/test_favorites_api.pycare/users/api/viewsets/plug_config.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/users/api/viewsets/plug_config.pycare/emr/tests/test_favorites_api.pycare/emr/api/viewsets/favorites.py
**/tests/**/*.py
📄 CodeRabbit inference engine (.cursorrules)
Use Django’s built-in tools for testing (unittest and pytest-django) to ensure code quality and reliability.
Files:
care/emr/tests/test_favorites_api.py
🔇 Additional comments (2)
care/emr/api/viewsets/favorites.py (1)
59-59: LGTM! Critical bug fix.The assignment ensures that the computed
favorite_list_objis actually returned in the response on cache miss, rather than leavingfavorite_listsasNone. The fix is straightforward and resolves the issue described in the PR objectives.care/users/api/viewsets/plug_config.py (1)
22-22: LGTM! Correct cache miss handling.The change from
if not response:toif response is None:properly distinguishes between a cache miss and a cached empty list. This ensures that an empty queryset result ([]) is treated as a valid cache hit and not unnecessarily recomputed. The inline comment clearly documents the intent, which is appreciated (well, mostly).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
care/emr/api/viewsets/favorites.py (2)
74-95: Misleading docstring and cache inconsistency when creating new favorite lists.The docstring claims "updates the cache accordingly," but there are no cache operations in this method. More problematically, when
get_or_createcreates a new favorite list, thefavorite_lists_cache_keycache isn't invalidated. Subsequent calls to thefavorite_listsendpoint will return stale data (missing the newly created list) until the cache expires—quite the surprise for users.Add cache invalidation when a new list is created:
favorite_list_obj.favorites = list(dict.fromkeys(favorite_list_obj.favorites))[ : settings.MAX_FAVORITES_PER_LIST ] favorite_list_obj.save(update_fields=["favorites"]) + if _: # get_or_create returns (obj, created) + cache.delete( + favorite_lists_cache_key(user, self.FAVORITE_RESOURCE, self.retrieve_facility_obj(obj)) + ) return Response({})Also update the docstring to remove the cache claim:
""" Add the current object to the user's favorite list. Inserts the object at the beginning of the list, trims the list - to the maximum allowed size, and updates the cache accordingly. + to the maximum allowed size. """
33-70: Good fix for the cache miss, but there's a real cache invalidation issue to address.The
favorite_lists = favorite_list_objassignment on line 68 correctly ensures the computed value is returned on the first call. However, the broader cache consistency strategy has a gap that the existing code doesn't quite live up to its docstrings:
add_favoriteclaims to "update the cache accordingly" but never invalidatesfavorite_lists_cache_key. Adding a new favorite creates stale cache entries until expiration.remove_favoriteonly invalidatesfavorite_lists_cache_keywhen the list becomes empty, not when items are removed while the list still exists. Partial modifications also result in stale cache.Both methods need to invalidate
favorite_lists_cache_keyafter modifying favorites, regardless of whether the list becomes empty.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/api/viewsets/favorites.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/api/viewsets/favorites.py
🪛 Ruff (0.14.10)
care/emr/api/viewsets/favorites.py
32-32: Unused method argument: args
(ARG002)
73-73: Unused method argument: args
(ARG002)
73-73: Unused method argument: kwargs
(ARG002)
🔇 Additional comments (2)
care/emr/api/viewsets/favorites.py (2)
26-29: LGTM!The docstring is clear and concise.
99-138: Cache invalidation is properly handled via model save() method.The non-empty removal path correctly updates the cache when
favorite_list_obj.save()is called (line 137). TheUserResourceFavoritesmodel'ssave()method automatically invalidatesfavorite_list_object_cache_keyby callingcache.set()with the updated favorites list, making the suggested fix unnecessary. This is the same caching strategy used inadd_favoriteand is confirmed by test expectations at lines 169-172 of the test file.
|
All review comments are addressed and CI checks are passing. Kindly review and approve when you get time. Thank you! |
|
Also too much doc string/comments, remove what's not required |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
care/emr/api/viewsets/favorites.py (1)
44-58: Usedict.fromkeys()to preserve the ordering from the query.The query explicitly orders by
-modified_date(line 51), but wrapping the result inset()(line 45) discards that ordering since sets are unordered. For consistency with the model'srefresh_cachemethod (which usesdict.fromkeys()to deduplicate while preserving order), consider using the same approach here.🔎 Proposed fix
if favorite_lists is None: - favorite_lists = list( - set( - UserResourceFavorites.objects.filter( - user=user, - resource_type=self.FAVORITE_RESOURCE, - facility=facility, - ) - .order_by("-modified_date") - .values_list("favorite_list", flat=True) - ) + favorite_lists = list( + dict.fromkeys( + UserResourceFavorites.objects.filter( + user=user, + resource_type=self.FAVORITE_RESOURCE, + facility=facility, + ) + .order_by("-modified_date") + .values_list("favorite_list", flat=True) + ) ) cache.set( favorite_lists_cache_key(user, self.FAVORITE_RESOURCE, facility), favorite_lists, )Based on relevant code snippets, the model's
refresh_cachemethod uses this pattern for the same purpose.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/api/viewsets/favorites.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/api/viewsets/favorites.py
🧬 Code graph analysis (1)
care/emr/api/viewsets/favorites.py (1)
care/emr/models/favorites.py (1)
UserResourceFavorites(18-54)
🪛 Ruff (0.14.10)
care/emr/api/viewsets/favorites.py
99-99: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
care/emr/api/viewsets/favorites.py (3)
68-68: Minor formatting improvement.The additional blank lines improve code readability without changing functionality.
Also applies to: 75-75
90-90: Formatting improvements.The additional blank lines enhance readability without affecting logic.
Also applies to: 97-97, 100-100, 103-103, 118-118
104-117: Cache invalidation logic looks correct.When a favorite list becomes empty after removal, the code appropriately:
- Deletes the
favorite_listscache key to ensure the empty list is removed from the list of lists- Deletes the specific
favorite_list_objectcache key to clear stale per-list data- Removes the database record to complete the cleanup
This ensures subsequent calls to
favorite_listswon't include the now-deleted list.
|
Thanks for the review. I’ve simplified the variable usage and removed the extra comments as suggested. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
care/emr/api/viewsets/favorites.py (2)
101-103: Consider simplifying the removal logic.The current approach uses
dict.fromkeys()to deduplicate, then.pop()to remove. While functional, it's a bit roundabout since you're converting list → dict → list just to remove one item. If deduplication is needed here (though favorites should already be deduplicated fromadd_favorite), a more straightforward approach might be:favorite_list_obj.favorites.remove(obj.id) if obj.id in favorite_list_obj.favorites else Noneor if deduplication is truly necessary:
favorite_list_obj.favorites = [f for f in dict.fromkeys(favorite_list_obj.favorites) if f != obj.id]I suppose the current code works, so perhaps this is just... personal preference.
99-99: Static analysis suggests moving exception message to a custom exception class.Ruff (TRY003) suggests avoiding long messages directly in
raise ValidationError()calls. While "Favorite List not found" isn't particularly long, you could define a custom exception or use a constant if you're feeling... exceptionally compliant with linting rules.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/api/viewsets/favorites.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/api/viewsets/favorites.py
🧬 Code graph analysis (1)
care/emr/api/viewsets/favorites.py (1)
care/emr/models/favorites.py (1)
UserResourceFavorites(18-54)
🪛 Ruff (0.14.10)
care/emr/api/viewsets/favorites.py
99-99: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
care/emr/api/viewsets/favorites.py (3)
44-58: Cache miss fix looks correct.The assignment to
favorite_lists(line 44) ensures the endpoint returns an empty list instead of null on cold cache, which should prevent the frontend from encountering null iteration errors. The use ofdict.fromkeys()preserves insertion order while deduplicating, assuming Python 3.7+.
62-81: Formatting and logic look fine.The blank lines improve readability, and the deduplication logic correctly preserves order while enforcing the maximum favorites limit.
104-117: Cache invalidation and cleanup logic is solid.The cleanup when the favorite list becomes empty is well-handled: both cache keys are deleted, and the database object is removed. The early return prevents unnecessary saves, which is... thoughtful.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3435 +/- ##
===========================================
+ Coverage 74.59% 74.61% +0.01%
===========================================
Files 457 457
Lines 20885 20885
Branches 2153 2153
===========================================
+ Hits 15580 15583 +3
+ Misses 4842 4840 -2
+ Partials 463 462 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Fix lint error. Install pre-commit , it'll take care of linting and formating |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
care/emr/tests/test_favorites_api.py (1)
242-251: Consider removing the redundant comment and optionally enhancing test coverage.The comment on line 243 states what the next line already demonstrates quite clearly, and as noted in the PR discussion, excessive comments were flagged for removal. Additionally, while the test adequately covers the regression (ensuring
listsis not null), it could be more thorough:
- Assert that the returned list is actually empty:
self.assertEqual(response.data["lists"], [])- Verify cache population after the call (pattern used in
test_list_favorites_listat lines 227-232)The test is functional as-is, but these refinements would make it more complete and consistent with other tests in the suite.
🔎 Proposed refinements
def test_favorite_lists_returns_list_on_first_call(self): - # Ensure cache is empty cache.delete(self.favorite_list_cache_key) response = self.client.get(self.base_url + "favorite_lists/") self.assertEqual(response.status_code, 200, response.content) self.assertIn("lists", response.data) self.assertIsNotNone(response.data["lists"]) self.assertIsInstance(response.data["lists"], list) + self.assertEqual(response.data["lists"], [], "Should return empty list on cache miss with no favorites") + + data = cache.get(self.favorite_list_cache_key) + self.assertEqual(data, [], "Cache should be populated with empty list after first call")
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/emr/tests/test_favorites_api.pycare/users/api/viewsets/plug_config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- care/users/api/viewsets/plug_config.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/tests/test_favorites_api.py
**/tests/**/*.py
📄 CodeRabbit inference engine (.cursorrules)
Use Django’s built-in tools for testing (unittest and pytest-django) to ensure code quality and reliability.
Files:
care/emr/tests/test_favorites_api.py
2 participants
Fix favorite_lists API returning null on cold cache, causing frontend crash.
Problem:
Associated Issue:
The frontend expects an array and directly iterates over this response.
Returning null causes a runtime TypeError (null.map) and breaks the favorites page.
Proposed Changes:
Technical Details
Architecture Changes
Earlier flow:
favorite_listswas not updated.Fix:
favorite_lists = favorite_list_obj
Tests Added
Verification
All favorites tests pass locally.
Merge Checklist
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.