Skip to content

fix: rating statistics very slow#362

Merged
shadinaif merged 10 commits intomainfrom
mkurkar/fix-rating-statistics
Dec 2, 2025
Merged

fix: rating statistics very slow#362
shadinaif merged 10 commits intomainfrom
mkurkar/fix-rating-statistics

Conversation

@mkurkar
Copy link
Collaborator

@mkurkar mkurkar commented Nov 30, 2025

This pull request refactors the course ratings API to support per-tenant queries and introduces caching for course ratings results. The changes improve performance by caching results per tenant, enforce stricter API input validation, and update related tests and settings to support the new behavior.

API and Business Logic Changes

  • The get_courses_ratings function now takes a tenant_id argument instead of permission info, computes ratings for a single tenant, and caches results per tenant using a generated cache key and a configurable timeout (FX_CACHE_TIMEOUT_COURSES_RATINGS). [1] [2] [3]
  • The GlobalRatingView API endpoint now requires exactly one tenant ID via the tenant_ids query parameter, validates access, and returns ratings only for the specified tenant. It raises appropriate exceptions for missing, multiple, invalid, or unauthorized tenant IDs.

Settings and Constants

  • Added FX_CACHE_TIMEOUT_COURSES_RATINGS (default 15 minutes) to both production and test settings, controlling the cache timeout for course ratings per tenant. [1] [2]
  • Introduced the shared constant RATING_RANGE = range(1, 6) for rating calculations and response formatting. [1] [2]

Testing Updates

  • Updated unit and integration tests to use the new single-tenant ratings API signature and to verify correct error handling for invalid or unauthorized tenant access, missing or multiple tenant IDs, and direct function calls. [1] [2] [3] [4]

Imports and Minor Refactoring

  • Updated imports in several files to support new helpers and constants, and improved docstrings for clarity. [1] [2] [3] [4]

References:
[1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14]

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses performance issues with course rating statistics by implementing per-tenant caching and refactoring the API to support single-tenant queries. The changes shift from computing ratings across all accessible tenants to computing and caching results for one tenant at a time, significantly improving response times for the ratings endpoint.

Key Changes

  • Refactored get_courses_ratings to accept tenant_id parameter instead of fx_permission_info, with results cached per tenant using the cache_dict decorator
  • Modified GlobalRatingView to require exactly one tenant ID via query parameter, with validation to ensure users only access authorized tenants
  • Added FX_CACHE_TIMEOUT_COURSES_RATINGS setting (default 15 minutes) to control cache expiration for rating statistics

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
futurex_openedx_extensions/dashboard/statistics/courses.py Refactored get_courses_ratings to use tenant_id parameter and added caching with cache key generation; introduced RATING_RANGE constant
futurex_openedx_extensions/dashboard/views.py Updated GlobalRatingView to enforce single-tenant queries with validation and authorization checks; added RATING_RANGE constant and improved error handling
futurex_openedx_extensions/dashboard/settings/common_production.py Added FX_CACHE_TIMEOUT_COURSES_RATINGS configuration setting
test_utils/test_settings_common.py Added FX_CACHE_TIMEOUT_COURSES_RATINGS test configuration
tests/test_dashboard/test_statistics/test_courses.py Updated tests to use new tenant_id parameter signature
tests/test_dashboard/test_views.py Enhanced tests to cover new single-tenant requirement, parameter validation, and error cases; added PermissionDenied import

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Collaborator

@shadinaif shadinaif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @mkurkar , please address the requested changes and let me know when it's done

@mkurkar mkurkar self-assigned this Nov 30, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Collaborator

@shadinaif shadinaif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @mkurkar , please remove the redundant tests, then it's ready to be merged

@mkurkar mkurkar requested a review from shadinaif December 1, 2025 18:36
Copy link
Collaborator

@shadinaif shadinaif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @mkurkar . I've added a very small change here, please take a look

@shadinaif shadinaif force-pushed the mkurkar/fix-rating-statistics branch from 9ed124f to bf92300 Compare December 2, 2025 06:28
@shadinaif shadinaif merged commit ebb886d into main Dec 2, 2025
3 checks passed
@shadinaif shadinaif deleted the mkurkar/fix-rating-statistics branch December 2, 2025 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants