update: add leaderboard apis #452
Conversation
There was a problem hiding this comment.
@Ali-Salman29 thanks for creating this PR. I have added my feedback.
| model = User | ||
| fields = ('username', 'name') | ||
|
|
||
| def to_representation(self, instance): |
There was a problem hiding this comment.
I don't think its good idea to override to_representation for populating profile_image_url field I think you should create a field profile_image_url in serializer and then write get_profile_image_url method to populate it.
| score = serializers.IntegerField() | ||
| badges = BadgeAssertionSerializer(many=True) | ||
|
|
||
| def to_representation(self, instance): |
There was a problem hiding this comment.
I'm unable to understand the purpose of this to_representation override. Could you please explain?
lms/djangoapps/badges/api/views.py
Outdated
| course_badge_score = LeaderboardConfiguration.COURSE_BADGE_SCORE | ||
| event_badge_score = LeaderboardConfiguration.EVENT_BADGE_SCORE | ||
|
|
||
| leaderboard_data = ( |
There was a problem hiding this comment.
This seems very compute intensive query it might not work with large dataset. can we pre compute these on badge generation events to avoid this query scanning complete badge table?
Ali-Salman29
force-pushed
the
alisalman/leaderboard-apis
branch
from
c89e6ee to
d561d96
Compare
Ali-Salman29
force-pushed
the
alisalman/leaderboard-apis
branch
from
d561d96 to
f7de918
Compare
There was a problem hiding this comment.
@Ali-Salman29 overall looks good just remove the handler which updates leaderboard on configuration change and rely on management commands since we are not expecting the configuration to be changed too frequently.
|
|
||
|
|
||
| @receiver(post_save, sender=LeaderboardConfiguration) | ||
| def update_leaderboard_scores(sender, instance, **kwargs): |
There was a problem hiding this comment.
I don't think we should update leaderboard entries when configuration is changed. It can be very time consuming task and should be done via management command as you have created one below.
Description: The PR contains APIs for leaderboard customizations