-
-
Notifications
You must be signed in to change notification settings - Fork 165
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.
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
Add leaderboard for achievements. #2579
base: develop
Are you sure you want to change the base?
Conversation
6ebf2c3
to
66f2ceb
Compare
66f2ceb
to
d2141f3
Compare
d2141f3
to
95843d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several changes that are needed here. I also suggested some changes that should improve loading the needed data from the database some.
Also, to take the "Leaderboard" to "Achievement Leaderboard" comments further, I think you should rename |
@drgrice1 thanks for your suggestions. I'll add them later. I did initially call this the "Achievement Leaderboard", but felt the name looked to long in the menu, and then I put "Leaderboard" indented under the "Achievements" link, but felt the offset looked odd, and ended up just calling it "Leaderboard" with no offset. I'll go change the name back, just sharing my thought process. |
Yeah, I did note that "Achievements Leaderboard" does become the longest name in the site navigation. But it seems to still fit okay. |
95843d5
to
1ce79e9
Compare
@drgrice1 thanks again, your suggestions improve load speed quite a bit here. Still takes a couple of seconds, but 3 seconds is faster than the 15 or so seconds I was getting. |
1ce79e9
to
30586c1
Compare
30586c1
to
3ad85d9
Compare
my @badges; | ||
for my $achievement (@achievements) { | ||
# Skip level achievements and only show earned achievements. | ||
last if $achievement->category eq 'level'; | ||
|
||
my $userBadge = $db->getUserAchievement($user->user_id, $achievement->achievement_id); | ||
push(@badges, $achievement) if $userBadge && $achievement->enabled && $userBadge->earned; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is another improvement that should speed this up a little more. Change this to
my @badges; | |
for my $achievement (@achievements) { | |
# Skip level achievements and only show earned achievements. | |
last if $achievement->category eq 'level'; | |
my $userBadge = $db->getUserAchievement($user->user_id, $achievement->achievement_id); | |
push(@badges, $achievement) if $userBadge && $achievement->enabled && $userBadge->earned; | |
} | |
my %userAchievements = map { $_->achievement_id => $_ } $db->getUserAchievementsWhere({ | |
user_id => $user->user_id, | |
achievement_id => [ map { $_->achievement_id } grep { $_->category ne 'level' } @achievements ], | |
}); | |
my @badges; | |
for my $achievement (@achievements) { | |
# Skip level achievements and only show earned achievements. | |
last if $achievement->category eq 'level'; | |
push(@badges, $achievement) | |
if $userAchievements{ $achievement->achievement_id } | |
&& $achievement->enabled | |
&& $userAchievements{ $achievement->achievement_id }->earned; | |
} |
This gets all user achievements except those in the "level" category at once instead of getting them one at a time in the loop. You can add && $_->category ne 'one_time'
if you like to also exclude the "extensions" achievement that isn't used.
I think that this code and the code in lib/WeBWorK/ContentGenerator/Achievements.pm
needs to be revisited though. Calling last
as is done here on line 69 and on line 143 of that file is messed up. The assumption that "level" achievements are last is really unintelligent design. The id's of the achievements can be edited by the instructor, and if the instructor changes the id of a "level" achievement to be smaller than the id of the first achievement, then it messes everything up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree when I saw that as well, but that would require a bit of a redesign on how level achievements work, since they always need to be processed last. I wonder if we could just update the sortAchievements
method to always put the level achievements at the end of the list independent of their number.
I cant think of any reason why one would want to process any other achievement after the level achievements, since if a student earned any of those achievements and got enough points to pass a level boundary, the level won't be awarded until the next time the level achievements are processed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that an instructor might not understand how achievements work, and change the id's in such a way that it messes things up. It is just a poor design. It shouldn't be hard to rework things to ensure level achievements are processed last. I'm my opinion there shouldn't even be a numerical id that is visible and even worse editable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The level achievements should be ordered within their group though, so I don't know how making the id hidden and not editable would let instructors add/remove levels. Though I guess there could be a separate list of level achievements that are managed and ordered in a different way. That would take a bit of work beyond just updating the sort to always include them last.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general achievements need to be completely redesigned for webwork3 anyway. Using actual Perl code for these, and another "safe compartment" is not the way to go to begin with.
3ad85d9
to
46b6785
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good now.
This is a casual question, not a request to change anything. But would it be difficult to make it so that there is just "Achievements" in the menu, and then if you go to that page, you can toggle between seeing your own achievements and seeing the leaderboard? (Assuming you have permission to see the leaderboard.) |
In the leaderboard, are the badges handled well in terms of accessibility? Is it right that they are in an |
There are some bootstrap styling options you could consider for the table for readability. I like the following but I defer to your opinion.
|
This lays out the rows ordered by rank. Would it be helpful to be able to sort the rows by the user name too? |
This could be done. I don't think we would want to use tabs, and load the leaderboard each time a user visits the achievement page, as this would require loading all achievements for all users each time. So the another option is to have a link to the leaderboard on the achievements page vs have a link in the main menu. |
Here is what is rendered. I'm unsure how the
|
<a class="help-popup" role="button" tabindex="0" | ||
data-bs-placement="top" data-bs-toggle="popover" data-bs-html="true" | ||
data-bs-content="<strong><%= $badge->{name} %></strong><br><%= $badge->{description} %>"> | ||
<%= image $badge->{icon} | ||
? "$ce->{courseURLs}{achievements}/$badge->{icon}" | ||
: "$ce->{webworkURLs}{htdocs}/images/defaulticon.png", | ||
alt => $c->maketext('[_1] Icon', $badge->{name}), | ||
width => 50 %> | ||
</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that @Alex-Jordan is right that these should not use an a
tag with the role button
. I think that instead this should be
<a class="help-popup" role="button" tabindex="0" | |
data-bs-placement="top" data-bs-toggle="popover" data-bs-html="true" | |
data-bs-content="<strong><%= $badge->{name} %></strong><br><%= $badge->{description} %>"> | |
<%= image $badge->{icon} | |
? "$ce->{courseURLs}{achievements}/$badge->{icon}" | |
: "$ce->{webworkURLs}{htdocs}/images/defaulticon.png", | |
alt => $c->maketext('[_1] Icon', $badge->{name}), | |
width => 50 %> | |
</a> | |
<button class="btn btn-sm btn-link help-popup" type="button" | |
data-bs-placement="top" data-bs-toggle="popover" data-bs-html="true" | |
data-bs-content="<strong><%= $badge->{name} %></strong><br><%= $badge->{description} %>"> | |
<%= image $badge->{icon} | |
? "$ce->{courseURLs}{achievements}/$badge->{icon}" | |
: "$ce->{webworkURLs}{htdocs}/images/defaulticon.png", | |
alt => $c->maketext('[_1] Icon', $badge->{name}), | |
width => 50 %> | |
</button> |
At the very least, that looks better. The popovers align better relative to the icon, and the focus appearance is nicer. The buttons are a little bigger, but I think that looks fine for these. Custom styling could be applied if desired to make them smaller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. The only thing I didn't like was the default padding making the badges space out a bit more, so I added the p-0
class to remove the padding and it looks better to me.
Hmm. I just saw something interesting looking at both the timing log and the morbo trace log. The achievements leaderboard is loading faster on my machine than the achievements page. The leaderboard takes about half the time to load. Admittedly I don't have a lot of achievements data on my local machine (really only data for my instructor user and one test student), but there is still more that the leaderboard is loading. So it stands to reason that the leaderboard should take longer. It seems the achievement page needs to be cleaned up. Looking at the code I see that that is one of the pages that didn't get as much attention when I did some of the heavy database access revision stuff several years ago. Many of the tips I gave you for speeding this page up need to be applied to the achievements page also. |
I initially thought about this, but wasn't sure how useful it would end up being. To me the usernames should be hidden from students anyways, and it might add a minor use for instructors to find a particular student, but wasn't sure it was worth the extra complication due to this.
I am not the biggest fan of |
This adds a leaderboard for achievements, which ranks users from the greatest to the least number of achievement points along with showing the badges of all earned achievements. The default use of this is to provide a summary page for professors to see how many achievement points students have earned along with which badges they have earned. The default permission level to view the leaderboard and see usernames on the leaderboard is professor. The permission level for viewing the leaderboard and viewing names on the leaderboard can be changed under course configuration to allow students to see the leaderboard. It is noted that since achievement points are often closely related to grades, that this should be considered before allowing students access.
Rename "Leaderboard" to "AchievementsLeaderboard". Implement the code and database improvements to make the page load faster.
Thanks to drgrice1.
132b3d2
to
14f44e0
Compare
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good to me at this point.
This adds a leaderboard for achievements, which ranks users from the greatest to the least number of achievement points along with showing the badges of all earned achievements.
The default use of this is to provide a summary page for professors to see how many achievement points students have earned along with which badges they have earned. The default permission level to view the leaderboard and see usernames on the leaderboard is professor.
The permission level for viewing the leaderboard and viewing names on the leaderboard can be changed under course configuration to allow students to see the leaderboard. It is noted that since achievement points are often closely related to grades, that this should be considered before allowing students access.
Note, this is currently a little slow since it loops over all students, and for each student loops over all achievements making multiple database calls to get the user achievement records. In my class of 40 students with 80 achievements, it takes about 5 seconds to generate the page, and could be slower for much larger classes. I would like to speed this up, but unsure on how to more efficiently make database calls to get all the achievement data needed to build the leaderboard.