fix: modularize admin dashboard statistics with reusable helpers #555
Conversation
- extract yearly stats logic into `stats_helpers_yearly.py` - extract metrics-related logic into `stats_helper_metrics.py` - extract chart-related logic into `stats_helper_chart.py` - refactor admin dashboard to use new helpers for stats and metrics
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the admin dashboard statistics functionality by extracting logic into three reusable helper modules for better code organization and maintainability.
- Modularizes yearly statistics logic into dedicated helper functions with improved caching
- Extracts metrics calculation into a decorator-based caching system
- Separates chart-related data processing into its own module
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 16 comments.
| File | Description |
|---|---|
stats_helpers_yearly.py |
New module handling yearly statistics with cache validation and async updates |
stats_helper_metrics.py |
New module providing cached metrics functions with decorator pattern |
stats_helper_chart.py |
New module for monthly donor statistics used in chart generation |
admin_dashboard.py |
Refactored to use new helper modules, removing inline statistics logic |
Comments suppressed due to low confidence (1)
backend/donations/views/dashboard/stats_helper_metrics.py:22
- Variable name CACHE_KEY should follow snake_case convention. It should be renamed to cache_key.
CACHE_KEY = _cache_key_for_metric(func.__name__)
| "month": month, | ||
| } | ||
|
|
||
| cache.set(cache_key, stat) |
There was a problem hiding this comment.
Missing timeout parameter in cache.set() call. This should include a timeout value like the other cache operations in the codebase for consistency.
| cache.set(cache_key, stat) | |
| cache.set(cache_key, stat, timeout=300) # Cache expires after 5 minutes |
Copilot uses AI. Check for mistakes.
| "timestamp": now(), | ||
| } | ||
|
|
||
| cache.set(_cache_key_for_metric("current_year_redirections"), result) |
There was a problem hiding this comment.
Missing timeout parameter in cache.set() call. This should include a timeout value for consistency with other cache operations.
| cache.set(_cache_key_for_metric("current_year_redirections"), result) | |
| cache.set(_cache_key_for_metric("current_year_redirections"), result, timeout=settings.TIMEOUT_CACHE_NORMAL) |
Copilot uses AI. Check for mistakes.
| "timestamp": now(), | ||
| } | ||
|
|
||
| cache.set(_cache_key_for_metric("all_redirections"), result) |
There was a problem hiding this comment.
Missing timeout parameter in cache.set() call. This should include a timeout value for consistency with other cache operations.
| cache.set(_cache_key_for_metric("all_redirections"), result) | |
| cache.set(_cache_key_for_metric("all_redirections"), result, timeout=settings.TIMEOUT_CACHE_NORMAL) |
Copilot uses AI. Check for mistakes.
| "timestamp": now(), | ||
| } | ||
|
|
||
| cache.set(_cache_key_for_metric("all_active_ngos"), result) |
There was a problem hiding this comment.
Missing timeout parameter in cache.set() call. This should include a timeout value for consistency with other cache operations.
| cache.set(_cache_key_for_metric("all_active_ngos"), result) | |
| cache.set(_cache_key_for_metric("all_active_ngos"), result, timeout=settings.TIMEOUT_CACHE_NORMAL) |
Copilot uses AI. Check for mistakes.
| "timestamp": now(), | ||
| } | ||
|
|
||
| cache.set(_cache_key_for_metric("ngos_active_in_current_year"), result) |
There was a problem hiding this comment.
Missing timeout parameter in cache.set() call. This should include a timeout value for consistency with other cache operations.
| cache.set(_cache_key_for_metric("ngos_active_in_current_year"), result) | |
| cache.set(_cache_key_for_metric("ngos_active_in_current_year"), result, timeout=settings.TIMEOUT_CACHE_NORMAL) |
Copilot uses AI. Check for mistakes.
| "footer": _create_stat_link( | ||
| url=f'{reverse("admin:donations_ngo_changelist")}?is_active=1', text=_("View all") | ||
| ), | ||
| "timestamp": now(), | ||
| "timestamp": all_active_ngos["timestamp"], |
There was a problem hiding this comment.
Missing function call parentheses. Should be all_active_ngos()["timestamp"] to actually invoke the function.
| "timestamp": all_active_ngos["timestamp"], | |
| "timestamp": all_active_ngos()["timestamp"], |
Copilot uses AI. Check for mistakes.
| "metric": ngos_active_in_current_year["metric"], | ||
| "footer": _create_stat_link(url=f'{reverse("admin:donations_ngo_changelist")}', text=_("View all")), | ||
| "timestamp": now(), | ||
| "timestamp": ngos_active_in_current_year["timestamp"], |
There was a problem hiding this comment.
Missing function call parentheses. Should be ngos_active_in_current_year()["metric"] to actually invoke the function.
Copilot uses AI. Check for mistakes.
| "footer": _create_stat_link(url=f'{reverse("admin:donations_ngo_changelist")}', text=_("View all")), | ||
| "timestamp": now(), | ||
| "timestamp": ngos_active_in_current_year["timestamp"], |
There was a problem hiding this comment.
Missing function call parentheses. Should be ngos_active_in_current_year()["timestamp"] to actually invoke the function.
| "timestamp": ngos_active_in_current_year["timestamp"], | |
| "timestamp": ngos_active_in_current_year()["timestamp"], |
Copilot uses AI. Check for mistakes.
| }, | ||
| { | ||
| "title": _("NGOs from NGO Hub"), | ||
| "icon": "foundation", | ||
| "metric": Ngo.ngo_hub.count(), | ||
| "metric": ngos_with_ngo_hub["metric"], |
There was a problem hiding this comment.
Missing function call parentheses. Should be ngos_with_ngo_hub()["metric"] to actually invoke the function.
| "metric": ngos_with_ngo_hub["metric"], | |
| "metric": ngos_with_ngo_hub()["metric"], |
Copilot uses AI. Check for mistakes.
| "footer": _create_stat_link( | ||
| url=f'{reverse("admin:donations_ngo_changelist")}?is_active=1&has_ngohub=1', text=_("View all") | ||
| ), | ||
| "timestamp": now(), | ||
| "timestamp": ngos_with_ngo_hub["timestamp"], |
There was a problem hiding this comment.
Missing function call parentheses. Should be ngos_with_ngo_hub()["timestamp"] to actually invoke the function.
| "timestamp": ngos_with_ngo_hub["timestamp"], | |
| "timestamp": ngos_with_ngo_hub()["timestamp"], |
Copilot uses AI. Check for mistakes.
- add `cache_set` function in `stats_helpers.utils` - replace direct `cache.set` calls with `cache_set` across stats helpers - update imports to reflect the new utility module structure
stats_helpers_yearly.pystats_helper_metrics.pystats_helper_chart.py