Fix restricted area endpoint by type with specific cache key #5150
Conversation
submarcos
changed the title
submarcos
changed the title
Geotrek-admin
|
||||||||||||||||||||||||||||
| Project |
Geotrek-admin
|
| Branch Review |
refs/pull/5150/merge
|
| Run status |
|
| Run duration | 02m 07s |
| Commit |
|
| Committer | Jean-Etienne Castagnede |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
22
|
| View all changes introduced in this branch ↗︎ | |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5150 +/- ##
=======================================
Coverage 98.47% 98.47%
=======================================
Files 272 272
Lines 22269 22285 +16
=======================================
+ Hits 21929 21945 +16
Misses 340 340 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a caching issue where restricted area GeoJSON endpoints were not properly differentiating cache entries by type. The fix adds a custom view_cache_key method to RestrictedAreaViewSet that incorporates the type_pk parameter into the cache key, and implements a new latest_updated method on the RestrictedArea model that can filter by type to get the appropriate timestamp for cache invalidation.
Key Changes:
- Adds
view_cache_keymethod toRestrictedAreaViewSetthat generates type-specific cache keys - Implements
latest_updatedclassmethod onRestrictedAreamodel with optional type filtering - Adds comprehensive model tests for the new
latest_updatedfunctionality
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| geotrek/zoning/views.py | Adds custom view_cache_key method to generate cache keys that include type_pk parameter |
| geotrek/zoning/models.py | Implements latest_updated classmethod with optional type_id filtering to support type-specific cache invalidation |
| geotrek/zoning/tests/test_models.py | Adds tests for latest_updated method behavior with and without type filtering |
| docs/changelog.rst | Documents the bug fix with issue reference #5136 |
| def test_latest_updated_is_different_by_type(self): | ||
| type_without_data = RestrictedAreaTypeFactory() | ||
| type_with_data = RestrictedAreaTypeFactory() | ||
| type_with_data_2 = RestrictedAreaTypeFactory() | ||
| RestrictedAreaFactory.create_batch(5, area_type=type_with_data) | ||
| RestrictedAreaFactory.create_batch(5, area_type=type_with_data_2) | ||
|
|
||
| self.assertIsNone(RestrictedArea.latest_updated(type_without_data.pk)) | ||
| self.assertEqual( | ||
| RestrictedArea.latest_updated(type_with_data.pk), | ||
| type_with_data.restrictedarea_set.only("date_update") | ||
| .latest("date_update") | ||
| .date_update, | ||
| ) | ||
| self.assertEqual( | ||
| RestrictedArea.latest_updated(type_with_data_2.pk), | ||
| type_with_data_2.restrictedarea_set.only("date_update") | ||
| .latest("date_update") | ||
| .date_update, | ||
| ) |
There was a problem hiding this comment.
While the new tests cover the latest_updated model method well, there's no test coverage for the view_cache_key method in RestrictedAreaViewSet. Tests should be added to verify that the cache key correctly incorporates the type_pk parameter and that different types generate different cache keys. This is critical since the whole purpose of this PR is to fix cache differentiation by type.
| try: | ||
| qs = cls.objects.all() | ||
| if type_id: | ||
| qs = cls.objects.filter(area_type_id=type_id) |
There was a problem hiding this comment.
The queryset initialization on line 41 is redundant because when type_id is provided, line 43 reinitializes the queryset from scratch using cls.objects.filter(), effectively discarding the assignment from line 41. This should be refactored to filter the initial queryset instead.
| qs = cls.objects.filter(area_type_id=type_id) | |
| qs = qs.filter(area_type_id=type_id) |
| def view_cache_key(self): | ||
| """Used by the ``view_cache_response_content`` decorator.""" | ||
| language = get_language() | ||
| geojson_lookup = None |
There was a problem hiding this comment.
The variable geojson_lookup is initialized to None but then immediately overwritten on line 41. This initialization is unnecessary and can be removed.
| geojson_lookup = None |
Description
Related Issue
Checklist