Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add leaderboard and overview of achievements #3686

Merged
merged 5 commits into from
Jan 29, 2025
Merged

Add leaderboard and overview of achievements #3686

merged 5 commits into from
Jan 29, 2025

Conversation

jonasdeluna
Copy link
Member

No description provided.

@jonasdeluna jonasdeluna marked this pull request as draft November 10, 2024 12:55
@jonasdeluna jonasdeluna force-pushed the feat-add-achievement-leaderboard branch 4 times, most recently from ae41188 to 54f95ff Compare November 13, 2024 17:14
@jonasdeluna jonasdeluna marked this pull request as ready for review November 13, 2024 17:53
@jonasdeluna jonasdeluna changed the title WIP Add leaderboard and overview of achievements Add leaderboard and overview of achievements Nov 13, 2024
@jonasdeluna jonasdeluna force-pushed the feat-add-achievement-leaderboard branch 2 times, most recently from 585c84e to 9cb5d9c Compare November 13, 2024 20:38
@codecov Codecov
Copy link

codecov bot commented Nov 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 86.95652% with 9 lines in your changes missing coverage. Please review.

Project coverage is 89.66%. Comparing base (c2b431c) to head (dc4eda2).
Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
lego/apps/achievements/views.py 63.15% 7 Missing ⚠️
...ps/users/migrations/0045_user_achievement_score.py 81.81% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3686      +/-   ##
==========================================
- Coverage   89.67%   89.66%   -0.01%     
==========================================
  Files         703      707       +4     
  Lines       22536    22605      +69     
==========================================
+ Hits        20209    20269      +60     
- Misses       2327     2336       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonasdeluna jonasdeluna force-pushed the feat-add-achievement-leaderboard branch from 9cb5d9c to 0019f22 Compare November 13, 2024 20:49
eikhr
eikhr reviewed Nov 13, 2024
View reviewed changes
Copy link
Member

@eikhr eikhr left a comment

Choose a reason for hiding this comment

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

I think this might become slow with all users in prod, but I'm not sure how to improve it...

@jonasdeluna jonasdeluna force-pushed the feat-add-achievement-leaderboard branch 4 times, most recently from de955c3 to 21c8732 Compare November 14, 2024 12:29
@jonasdeluna jonasdeluna force-pushed the feat-add-achievement-leaderboard branch 2 times, most recently from 0cb28b5 to dbf9495 Compare January 15, 2025 17:01
danielyanghansen
danielyanghansen requested changes Jan 15, 2025
View reviewed changes
Copy link
Member

@danielyanghansen danielyanghansen left a comment

Choose a reason for hiding this comment

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

Overall looks good.
Pagination of the LeaderBoardViewSet.get_queryset would be nice, but can be done in a later PR.

lego/apps/achievements/utils/calculation_utils.py
delta = 0.1
# Remember to update this rarity list when adding new achievement
MAX_POSSIBLE_SCORE = sum(
max(rarity_list) + 1 + (max(rarity_list) * delta)
Copy link
Member

Choose a reason for hiding this comment

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

Why does this work? rarity_list doesn't seem to be defined at this point

Copy link
Member

Choose a reason for hiding this comment

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

I think this is going to give you a runtime error. If this code works, it seems to be because of unintended behavior

Copy link
Member Author

Choose a reason for hiding this comment

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

This is apparently due to a quirk in python as the defenition depends on a dictionary that already exists it apparently works

lego/apps/achievements/utils/calculation_utils.py
value = rarity_list[achievement.level]
score += value + 1 + (achievement.level * delta)

return score if MAX_POSSIBLE_SCORE else 0
Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure why the presence of MAX_POSSIBLE_SCORE would matter here

lego/apps/users/models.py Outdated
@@ -421,6 +422,12 @@ def delete(self, using=None, force=False):
event.unregister(event.registrations.get(user=self))
super(User, self).delete(using=using, force=force)

def save(self, *args, **kwargs):
from lego.apps.achievements.utils.calculation_utils import calculate_user_rank
Copy link
Member

Choose a reason for hiding this comment

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

Could you educate me on the benefits of a late import like this? 👼

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to avoid a circular import it unfortunately has to be done

danielyanghansen
danielyanghansen requested changes Jan 15, 2025
View reviewed changes
Copy link
Member

@danielyanghansen danielyanghansen left a comment

Choose a reason for hiding this comment

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

Overall looks good.
Pagination of the LeaderBoardViewSet.get_queryset would be nice, but can be done in a later PR.

@jonasdeluna jonasdeluna force-pushed the feat-add-achievement-leaderboard branch from e5a7c23 to a9e1cde Compare January 27, 2025 11:47
@jonasdeluna jonasdeluna force-pushed the feat-add-achievement-leaderboard branch from a9e1cde to 7c7f7ff Compare January 27, 2025 11:48
@jonasdeluna jonasdeluna force-pushed the feat-add-achievement-leaderboard branch from cf6e9c7 to dc4eda2 Compare January 29, 2025 19:17
@jonasdeluna jonasdeluna merged commit 36170fa into master Jan 29, 2025
1 of 3 checks passed
@jonasdeluna jonasdeluna deleted the feat-add-achievement-leaderboard branch January 29, 2025 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eikhr eikhr eikhr approved these changes

@danielyanghansen danielyanghansen danielyanghansen requested changes

@ivarnakken ivarnakken Awaiting requested review from ivarnakken

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jonasdeluna @eikhr @danielyanghansen