feat: add learning hours count to the stats api #169
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tehreem-sadat
force-pushed
the
tehreem/feat_add_learning_hours_stats_to_stats_api
branch
from
038f0ce to
6d56c74
Compare
shadinaif
requested changes
Dec 20, 2024
| STAT_LEARNING_HOURS = 'learning_hours' | ||
|
|
||
| valid_stats = [ | ||
| STAT_CERTIFICATES, STAT_COURSES, STAT_ENROLLMENTS, STAT_HIDDEN_COURSES, STAT_LEARNERS, STAT_LEARNING_HOURS |
There was a problem hiding this comment.
Suggested change
| STAT_CERTIFICATES, STAT_COURSES, STAT_ENROLLMENTS, STAT_HIDDEN_COURSES, STAT_LEARNERS, STAT_LEARNING_HOURS | |
| STAT_CERTIFICATES, STAT_COURSES, STAT_ENROLLMENTS, STAT_HIDDEN_COURSES, STAT_LEARNERS, STAT_LEARNING_HOURS, |
| settings.FX_DEFAULT_COURSE_EFFORT = getattr( | ||
| settings, | ||
| 'FX_DEFAULT_COURSE_EFFORT', | ||
| '50' |
There was a problem hiding this comment.
this value was communicated with Nelc team
Suggested change
| '50' | |
| '12', |
| ('30', 30 * 2, 'Valid, only hours provided (no minutes)'), | ||
| ('5:120', 10 * 2, 'Invalid, minutes exceed 60, use default value'), | ||
| ('invalid', 10 * 2, 'Invalid, non-numeric value for hours, use default value'), | ||
| ('20:invalid', 10 * 2, 'Invalid, non-numeric value for minutes, use default value'), |
There was a problem hiding this comment.
two more tests, please
Suggested change
| ('20:invalid', 10 * 2, 'Invalid, non-numeric value for minutes, use default value'), | |
| ('20:invalid', 10 * 2, 'Invalid, non-numeric value for minutes, use default value'), | |
| ('0:25', 10 * 2, 'Invalid, less than 30 minutes'), | |
| ('0:-55', 10 * 2, 'Invalid, less than 30 minutes'), |
There was a problem hiding this comment.
that's a good catch there.
Comment on lines
75
to
77
| return round(total_hours, 1) | ||
| except (ValueError, IndexError): | ||
| return settings.FX_DEFAULT_COURSE_EFFORT |
There was a problem hiding this comment.
we want to make it silent for the user, but noisy for us in the logs
Suggested change
| return round(total_hours, 1) | |
| except (ValueError, IndexError): | |
| return settings.FX_DEFAULT_COURSE_EFFORT | |
| if total_hours < 0.5: | |
| raise ValueError('course effort value is too small') | |
| return round(total_hours, 1) | |
| except (ValueError, IndexError) as exc: | |
| log.exception('Invalid course-effort for course %s. Assuming default value (%s hours). Error: %s', course_id, settings.FX_DEFAULT_COURSE_EFFORT, str(exc)) | |
| return settings.FX_DEFAULT_COURSE_EFFORT |
|
|
||
|
|
||
| def get_learning_hours_count( | ||
| fx_permission_info: dict, visible_courses_filter: bool | None = True, active_courses_filter: bool | None = None |
There was a problem hiding this comment.
✅ same default values for argument as in get_certificates_count. Thank you!
| ).values('course_effort', 'certificates_count') | ||
| ) | ||
|
|
||
| return sum( |
There was a problem hiding this comment.
✅ this is great, let's see the performance on production. We can cache it if we find it slowing down the dashboard
| @@ -32,6 +32,7 @@ class CourseOverview(models.Model): | |||
| self_paced = models.BooleanField(default=False) | |||
| course_image_url = models.TextField() | |||
| visible_to_staff_only = models.BooleanField(default=False) | |||
| effort = models.CharField(max_length=255) | |||
There was a problem hiding this comment.
always remember, please: mocks are our hidden enemies in tests. We need to make them similar to the original as much as we can
Suggested change
| effort = models.CharField(max_length=255) | |
| effort = models.TextField(null=True) |
shadinaif
approved these changes
Dec 20, 2024
shadinaif
deleted the
tehreem/feat_add_learning_hours_stats_to_stats_api
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related issue: #67
Add learning hours stats count to the Stats API.