From 3b958c2b7956d438ad944e0c1ae39256d38fbb67 Mon Sep 17 00:00:00 2001 From: Jaimos Skriletz Date: Sat, 10 Feb 2024 16:16:25 -0700 Subject: [PATCH 1/6] Updates to Utils::FilterRecords::getFiltersForClass Update this method so it can be used for the filter menus on the stats and student progress page. This is done by adding a new option $include that lists which filters to include. In addition this adds translations to the visible string that is included for each filter. --- lib/WeBWorK/HTML/ScrollingRecordList.pm | 2 +- lib/WeBWorK/Utils/FilterRecords.pm | 85 ++++++++++++++----------- 2 files changed, 50 insertions(+), 37 deletions(-) diff --git a/lib/WeBWorK/HTML/ScrollingRecordList.pm b/lib/WeBWorK/HTML/ScrollingRecordList.pm index fedab5936b..d56d5ff72e 100644 --- a/lib/WeBWorK/HTML/ScrollingRecordList.pm +++ b/lib/WeBWorK/HTML/ScrollingRecordList.pm @@ -64,7 +64,7 @@ sub scrollingRecordList ($options, @records) { my $format_keywords = join('|', @format_keywords); @$sorts = grep { $_->[0] =~ /$format_keywords/ } @$sorts; - $filters = getFiltersForClass($c, @records); + $filters = getFiltersForClass($c, undef, @records); my @selected_filters; if (defined $c->param("$name!filter")) { diff --git a/lib/WeBWorK/Utils/FilterRecords.pm b/lib/WeBWorK/Utils/FilterRecords.pm index 3b50016f0a..a7c001a01b 100644 --- a/lib/WeBWorK/Utils/FilterRecords.pm +++ b/lib/WeBWorK/Utils/FilterRecords.pm @@ -22,26 +22,23 @@ WeBWorK::Utils::FilterRecords - utilities for filtering database records. =head1 SYNOPSIS - use WeBWorK::Utils::FilterRecords qw/getFiltersForClass/; - - # Get a list of filters - my $filters = getFiltersForClass(@users); - - use WeBWorK::Utils::FilterRecords qw/filterRecords/; + use WeBWorK::Utils::FilterRecords qw/getFiltersForClass filterRecords/; # Start with a list of records my @users = $db->getUsers($db->listUsers); - # Filter the records using a list of provided filters. - @filteredUsers = filterRecords([ 'section:1', 'recitation:2' ], @nsers); + # Get a list of all filters + my $filters = getFiltersForClass($c, undef, @users); - # Get all records (This isn't useful and just returns the passed in - # array of records. So don't actually do this.) - @filteredUsers = filterRecords(undef, @users); + # Alternative, get a list of section or recitation filters. + my $filters = getFiltersForClass($c, ['section', 'recitation'], @users); + + # Filter the records using a list of provided filters. + my @filteredUsers = filterRecords($c, 1, [ 'section:1', 'recitation:2' ], @users); =head1 DESCRIPTION -This module provides functions for filtering records from the database. +This module provides functions for filtering user or set records from the database. =cut @@ -62,27 +59,40 @@ our @EXPORT_OK = qw( =over -=item getFiltersForClass($c, @records) +=item getFiltersForClass($c, $include, @records) + +Given a list of database records, returns the filters available for those records. +C<$include> is an array reference that lists the filters to include. If this is +empty, all possible filters are returned. -Given a list of database records, returns the filters available for those -records. For all database records from the WeBWorK::DB::Record::User class -the filters are by section or recitation or by that user's permission level -in the permissionLevel table. For all database records from the -WeBWorK::DB::Record::Set class the filters are by assignment type and by -visibility. For other classes the only filter is no filter at all. +For user records (WeBWorK::DB::Record::User), filters can be by section, +recitation, status, or permission level in the permissionLevel table. The +possible C<$include> are: 'section', 'recitation', 'status', or 'permission'. + +For set records (WeBWorK::DB::Record::Set), filters can be assignment type, +or visibility. The possible C<$include> are: 'assignment_type', or 'visibility'. The return value is a reference to a list of two element lists. The first -element in each list is a a string description of the filter and the second +element in each list is a string description of the filter and the second element is the filter name. The return value is suitable for passing as the second value argument to the Mojolicious select_field tag helper method. =cut sub getFiltersForClass { - my ($c, @records) = @_; + my ($c, $include, @records) = @_; + my $blankName = "\x{27E8}" . $c->maketext('blank') . "\x{27E9}"; + + my %includes; + if (ref $include eq 'ARRAY') { + for (@$include) { + $includes{$_} = 1; + } + } my @filters; - push @filters, [ "\x{27E8}Display all possible records\x{27E9}" => 'all', selected => undef ]; + push @filters, + [ "\x{27E8}" . $c->maketext('Display all possible records') . "\x{27E9}" => 'all', selected => undef ]; if (ref $records[0] eq 'WeBWorK::DB::Record::User') { my (%sections, %recitations, %permissions, %roles); @@ -93,35 +103,38 @@ sub getFiltersForClass { ++$roles{ $user->status }; } - my %permissionName = reverse %{ $c->ce->{userRoles} }; - ++$permissions{ $permissionName{$_} } - for map { $_->permission } $c->db->getPermissionLevelsWhere({ user_id => { not_like => 'set_id:%' } }); + if (!%includes || $includes{permission}) { + my %permissionName = reverse %{ $c->ce->{userRoles} }; + ++$permissions{ $permissionName{$_} } + for map { $_->permission } $c->db->getPermissionLevelsWhere({ user_id => { not_like => 'set_id:%' } }); + } - if (keys %sections > 1) { + if (keys %sections > 1 && (!%includes || $includes{section})) { for my $sec (sortByName(undef, keys %sections)) { - push @filters, [ 'Section: ' . ($sec ne '' ? $sec : "\x{27E8}blank\x{27E9}") => "section:$sec" ]; + push @filters, [ $c->maketext('Section: [_1]', $sec ne '' ? $sec : $blankName) => "section:$sec" ]; } } - if (keys %recitations > 1) { + if (keys %recitations > 1 && (!%includes || $includes{recitation})) { for my $rec (sortByName(undef, keys %recitations)) { - push @filters, [ 'Recitation: ' . ($rec ne '' ? $rec : "\x{27E8}blank\x{27E9}") => "recitation:$rec" ]; + push @filters, + [ $c->maketext('Recitation: [_1]', $rec ne '' ? $rec : $blankName) => "recitation:$rec" ]; } } - if (keys %roles > 1) { + if (keys %roles > 1 && (!%includes || $includes{status})) { for my $role (sortByName(undef, keys %roles)) { my @statuses = keys %{ $c->ce->{statuses} }; for (@statuses) { - push @filters, [ "Enrollment Status: $_" => "status:$role" ] + push @filters, [ $c->maketext('Enrollment Status: [_1]', $_) => "status:$role" ] if ($c->ce->{statuses}{$_}{abbrevs}[0] eq $role); } } } - if (keys %permissions > 1) { + if (keys %permissions > 1 && (!%includes || $includes{permission})) { for my $perm (sortByName(undef, keys %permissions)) { - push @filters, [ "Permission Level: $perm" => "permission:$perm" ]; + push @filters, [ $c->maketext('Permission Level: [_1]', $perm) => "permission:$perm" ]; } } } elsif (ref $records[0] eq 'WeBWorK::DB::Record::Set') { @@ -133,15 +146,15 @@ sub getFiltersForClass { unless (defined $visibles{0} && $set->visible eq '' || defined $visibles{''} && $set->visible eq '0'); } - if (keys %assignment_types > 1) { + if (keys %assignment_types > 1 && (!%includes || $includes{assignment_type})) { for my $type (sortByName(undef, keys %assignment_types)) { push @filters, [ FIELD_PROPERTIES()->{assignment_type}{labels}{$type} => "assignment_type:$type" ]; } } - if (keys %visibles > 1) { + if (keys %visibles > 1 && (!%includes || $includes{visible})) { for my $vis (sortByName(undef, keys %visibles)) { - push @filters, [ ($vis ? 'Visible' : 'Not Visible') => "visible:$vis" ]; + push @filters, [ ($vis ? $c->maketext('Visible') : $c->maketext('Not Visible')) => "visible:$vis" ]; } } } From f2bf400d9aed305de99a48f98e4b98664e361bc4 Mon Sep 17 00:00:00 2001 From: Jaimos Skriletz Date: Sat, 10 Feb 2024 19:29:53 -0700 Subject: [PATCH 2/6] Add student filter to student progress page and update stats filter. This adds the ability to filter by section or recitation when viewing the student progress of a single set. This also updates the link parameters to ensure that any current filter, test display options, or sort is saved when updating the page view. This also updates the Stats.pm filter to use the Utils::filterRecords method instead of its own method. --- .../ContentGenerator/Instructor/Stats.pm | 44 ++++------- .../Instructor/StudentProgress.pm | 20 ++++- .../Instructor/Stats/problem_stats.html.ep | 2 +- .../Instructor/Stats/set_stats.html.ep | 2 +- .../StudentProgress/set_progress.html.ep | 74 +++++++++++++------ .../StudentFilterMenu/menu.html.ep} | 8 +- 6 files changed, 86 insertions(+), 64 deletions(-) rename templates/{ContentGenerator/Instructor/Stats/student_filter_menu.html.ep => HTML/StudentFilterMenu/menu.html.ep} (85%) diff --git a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm index d11cbb10a5..d71643e514 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm @@ -26,6 +26,7 @@ homework set (including sv graphs). use SVG; use WeBWorK::Utils qw(jitar_id_to_seq jitar_problem_adjusted_status format_set_name_display grade_set); +use WeBWorK::Utils::FilterRecords qw(getFiltersForClass filterRecords); sub initialize ($c) { my $db = $c->db; @@ -38,9 +39,9 @@ sub initialize ($c) { # Cache a list of all users except set level proctors and practice users, and restrict to the sections or # recitations that are allowed for the user if such restrictions are defined. This list is sorted by last_name, # then first_name, then user_id. This is used in multiple places in this module, and is guaranteed to be used at - # least once. So it is done here to prevent extra database access. + # least once. So it is done here to prevent extra database access. Filter out users not included in stats. $c->{student_records} = [ - $db->getUsersWhere( + grep { $ce->status_abbrev_has_behavior($_->status, 'include_in_stats') } $db->getUsersWhere( { user_id => [ -and => { not_like => 'set_id:%' }, { not_like => "$ce->{practiceUserPrefix}\%" } ], $ce->{viewable_sections}{$user} || $ce->{viewable_recitations}{$user} @@ -100,39 +101,18 @@ sub siblings ($c) { return $c->include('ContentGenerator/Instructor/Stats/siblings', header => $c->maketext('Statistics')); } -# Apply the currently selected filter to the the student records cached in initialize, and return a reference to the +# Apply the currently selected filter to the student records, and return a reference to the # list of students and a reference to a hash of section/recitation filters. sub filter_students ($c) { - my $ce = $c->ce; - my $db = $c->db; - my $user = $c->param('user'); + my $filter = $c->param('filter'); + my @students = $filter ? filterRecords($c, 0, [$filter], @{ $c->{student_records} }) : @{ $c->{student_records} }; - # Create a hash of sections and recitations, if there are any for the course. - # Filter out all records except for current/auditing students for stats. - # Filter out students not in selected section/recitation. - my $filter = $c->param('filter'); - my %filters; - my @outStudents; - for my $student (@{ $c->{student_records} }) { - # Only include current/auditing students in stats. - next - unless ($ce->status_abbrev_has_behavior($student->status, 'include_in_stats')); - - my $section = $student->section; - $filters{"section:$section"} = $c->maketext('Section [_1]', $section) - if $section && !$filters{"section:$section"}; - my $recitation = $student->recitation; - $filters{"recitation:$recitation"} = $c->maketext('Recitation [_1]', $recitation) - if $recitation && !$filters{"recitation:$recitation"}; - - # Only add users who match the selected section/recitation. - push(@outStudents, $student) - if (!$filter - || ($filter =~ /^section:(.*)$/ && $section eq $1) - || ($filter =~ /^recitation:(.*)$/ && $recitation eq $1)); - } + # convert the array from getFiltersForClass to a hash, after removing the first 'all' filter. + my $filters = getFiltersForClass($c, [ 'section', 'recitation' ], @{ $c->{student_records} }); + shift(@$filters); + $filters = { map { $_->[1] => $_->[0] } @$filters }; - return (\@outStudents, \%filters); + return (\@students, $filters); } sub set_stats ($c) { @@ -190,6 +170,7 @@ sub set_stats ($c) { # Only count top level problems for Jitar sets. my $num_problems = $isJitarSet ? scalar(keys %topLevelProblems) : scalar(@problems); + my $filter = $c->param('filter'); my ($students, $filters) = $c->filter_students; for my $studentRecord (@$students) { my $student = $studentRecord->user_id; @@ -381,6 +362,7 @@ sub problem_stats ($c) { my $db = $c->db; my $ce = $c->ce; my $user = $c->param('user'); + my $filter = $c->param('filter'); my $courseID = $c->stash('courseID'); my $problemID = $c->stash('problemID'); diff --git a/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm b/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm index 28fd8ec827..feaf6a5e93 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm @@ -24,6 +24,7 @@ WeBWorK::ContentGenerator::Instructor::StudentProgress - Display Student Progres use WeBWorK::Utils qw(jitar_id_to_seq wwRound grade_set format_set_name_display); use WeBWorK::Utils::Grades qw(list_set_versions); +use WeBWorK::Utils::FilterRecords qw(getFiltersForClass filterRecords); sub initialize ($c) { my $db = $c->db; @@ -36,9 +37,9 @@ sub initialize ($c) { # Cache a list of all users except set level proctors and practice users, and restrict to the sections or # recitations that are allowed for the user if such restrictions are defined. This list is sorted by last_name, # then first_name, then user_id. This is used in multiple places in this module, and is guaranteed to be used at - # least once. So it is done here to prevent extra database access. + # least once. So it is done here to prevent extra database access. Filter out users not included in stats. $c->{student_records} = [ - $db->getUsersWhere( + grep { $ce->status_abbrev_has_behavior($_->status, 'include_in_stats') } $db->getUsersWhere( { user_id => [ -and => { not_like => 'set_id:%' }, { not_like => "$ce->{practiceUserPrefix}\%" } ], $ce->{viewable_sections}{$user} || $ce->{viewable_recitations}{$user} @@ -115,10 +116,19 @@ sub displaySets ($c) { : (date => 0, testtime => 0, timeleft => 0, problems => 1, section => 1, recit => 1, login => 1); my $showBestOnly = $setIsVersioned ? $c->param('show_best_only') : 0; + my $filter = $c->param('filter'); + my @student_records = + $filter ? filterRecords($c, 0, [$filter], @{ $c->{student_records} }) : @{ $c->{student_records} }; + + # convert the array from getFiltersForClass to a hash, after removing the first 'all' filter. + my $filters = getFiltersForClass($c, [ 'section', 'recitation' ], @{ $c->{student_records} }); + shift(@$filters); + $filters = { map { $_->[1] => $_->[0] } @$filters }; + my @score_list; my @user_set_list; - for my $studentRecord (@{ $c->{student_records} }) { + for my $studentRecord (@student_records) { next unless $ce->status_abbrev_has_behavior($studentRecord->status, 'include_in_stats'); my $studentName = $studentRecord->user_id; @@ -277,7 +287,9 @@ sub displaySets ($c) { secondary_sort_method => $secondary_sort_method, ternary_sort_method => $ternary_sort_method, problems => \@problems, - user_set_list => \@user_set_list + user_set_list => \@user_set_list, + filters => $filters, + filter => $filter, ); } diff --git a/templates/ContentGenerator/Instructor/Stats/problem_stats.html.ep b/templates/ContentGenerator/Instructor/Stats/problem_stats.html.ep index 7ff784594b..4767239499 100644 --- a/templates/ContentGenerator/Instructor/Stats/problem_stats.html.ep +++ b/templates/ContentGenerator/Instructor/Stats/problem_stats.html.ep @@ -6,7 +6,7 @@ %
<%= maketext('Showing statistics for:') =%> - <%= include 'ContentGenerator/Instructor/Stats/student_filter_menu', filters => $filters =%> + <%= include 'HTML/StudentFilterMenu/menu', filters => $filters, params => {} =%> <%= include 'ContentGenerator/Instructor/Stats/problem_menu', problems => $problems =%>
% diff --git a/templates/ContentGenerator/Instructor/Stats/set_stats.html.ep b/templates/ContentGenerator/Instructor/Stats/set_stats.html.ep index 6659417475..89c1c80b33 100644 --- a/templates/ContentGenerator/Instructor/Stats/set_stats.html.ep +++ b/templates/ContentGenerator/Instructor/Stats/set_stats.html.ep @@ -5,7 +5,7 @@ % # Filter and problem selectors.
<%= maketext('Showing statistics for:') =%> - <%= include 'ContentGenerator/Instructor/Stats/student_filter_menu', filters => $filters =%> + <%= include 'HTML/StudentFilterMenu/menu', filters => $filters, params => {} =%> <%= include 'ContentGenerator/Instructor/Stats/problem_menu', problems => $problems =%>
% # Set information diff --git a/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep b/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep index 0bf142219b..4fe3361435 100644 --- a/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep +++ b/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep @@ -1,5 +1,33 @@ <%= include 'ContentGenerator/Base/set_status', set => $c->{setRecord} =%> % +% my %params = ( + % defined $primary_sort_method ? (primary_sort => $primary_sort_method) : (), + % defined $secondary_sort_method ? (secondary_sort => $secondary_sort_method) : (), + % defined $ternary_sort_method ? (ternary_sort => $ternary_sort_method) : (), + % defined $filter ? (filter => $filter) : (), + % # Preserve display options when the sort headers are clicked for gateway quizzes. + % $setIsVersioned + % ? ( + % show_best_only => $showBestOnly, + % show_date => $showColumns->{date}, + % show_testtime => $showColumns->{testtime}, + % show_timeleft => $showColumns->{timeleft}, + % show_problems => $showColumns->{problems}, + % show_section => $showColumns->{section}, + % show_recitation => $showColumns->{recit}, + % show_login => $showColumns->{login}, + % ) + % : () +% ); +% +% # Filter selector. +% if (%$filters) { +
+ <%= maketext('Showing progress for:') =%> + <%= include 'HTML/StudentFilterMenu/menu', filters => $filters, params => \%params =%> +
+% } +% % # In the case of gateway tests, add a form with checkboxes that allow customization of what is included in the % # display. % if ($setIsVersioned) { @@ -60,6 +88,18 @@ + % if (param('filter')) { + <%= hidden_field filter => param('filter') =%> + % } + % if ($primary_sort_method) { + <%= hidden_field primary_sort => $primary_sort_method =%> + % } + % if ($secondary_sort_method) { + <%= hidden_field secondary_sort => $secondary_sort_method =%> + % } + % if ($ternary_sort_method) { + <%= hidden_field ternary_sort => $ternary_sort_method =%> + % } <%= submit_button maketext('Update Display'), class => 'btn btn-primary' =%> <% end =%> @@ -106,23 +146,11 @@ % } % -% my %params = ( +% my %sort_params = ( + % %params, % # Shift past sort methods down in priority. - % defined $primary_sort_method ? (secondary_sort => $primary_sort_method) : (), - % defined $secondary_sort_method ? (ternary_sort => $secondary_sort_method) : (), - % # Preserve display options when the sort headers are clicked for gateway quizzes. - % $setIsVersioned - % ? ( - % show_best_only => $showBestOnly, - % show_date => $showColumns->{date}, - % show_testtime => $showColumns->{testtime}, - % show_timeleft => $showColumns->{timeleft}, - % show_problems => $showColumns->{problems}, - % show_section => $showColumns->{section}, - % show_recitation => $showColumns->{recit}, - % show_login => $showColumns->{login}, - % ) - % : () + % defined $primary_sort_method ? (secondary_sort => $primary_sort_method) : (), + % defined $secondary_sort_method ? (ternary_sort => $secondary_sort_method) : (), % ); % % # Start table output @@ -135,17 +163,17 @@ <%= maketext('Name') %>
<%= link_to maketext('First') => - $c->systemLink(url_for, params => { primary_sort => 'first_name', %params }) =%> + $c->systemLink(url_for, params => { %sort_params, primary_sort => 'first_name' }) =%>     <%= link_to maketext('Last') => - $c->systemLink(url_for, params => { primary_sort => 'last_name', %params }) =%> + $c->systemLink(url_for, params => { %sort_params, primary_sort => 'last_name' }) =%>
<%= link_to maketext('Email') => - $c->systemLink(url_for, params => { primary_sort => 'email_address', %params }) =%> + $c->systemLink(url_for, params => { %sort_params, primary_sort => 'email_address' }) =%> > <%= link_to maketext('Score') => - $c->systemLink(url_for, params => { primary_sort => 'score', %params }) =%> + $c->systemLink(url_for, params => { %sort_params, primary_sort => 'score' }) =%> ><%= maketext('Out Of') %> % # Additional columns that may be shown depending on if showing a gateway quiz and user selection. @@ -176,19 +204,19 @@ % if ($showColumns->{section}) { > <%= link_to maketext('Section') => - $c->systemLink(url_for, params => { primary_sort => 'section', %params }) =%> + $c->systemLink(url_for, params => { %sort_params, primary_sort => 'section' }) =%> % } % if ($showColumns->{recit}) { > <%= link_to maketext('Recitation') => - $c->systemLink(url_for, params => { primary_sort => 'recitation', %params }) =%> + $c->systemLink(url_for, params => { %sort_params, primary_sort => 'recitation' }) =%> % } % if ($showColumns->{login}) { > <%= link_to maketext('Login Name') => - $c->systemLink(url_for, params => { primary_sort => 'user_id', %params }) =%> + $c->systemLink(url_for, params => { %sort_params, primary_sort => 'user_id' }) =%> % } diff --git a/templates/ContentGenerator/Instructor/Stats/student_filter_menu.html.ep b/templates/HTML/StudentFilterMenu/menu.html.ep similarity index 85% rename from templates/ContentGenerator/Instructor/Stats/student_filter_menu.html.ep rename to templates/HTML/StudentFilterMenu/menu.html.ep index a0fa3c0674..5dcecaf5e6 100644 --- a/templates/ContentGenerator/Instructor/Stats/student_filter_menu.html.ep +++ b/templates/HTML/StudentFilterMenu/menu.html.ep @@ -2,20 +2,20 @@ % % # Create a section/recitation "filter by" dropdown if there are sections or recitations.
- <%= link_to param('filter') ? $filters->{param('filter')} : maketext('All sections') => '#', + <%= link_to param('filter') ? $filters->{param('filter')} : maketext('All students') => '#', id => 'filter', class => 'btn btn-primary dropdown-toggle', role => 'button', 'aria-expanded' => 'false', data => { bs_toggle => 'dropdown' } =%> -
\ + From 009ebaad93f0913d98215fc960ef026728079618 Mon Sep 17 00:00:00 2001 From: Jaimos Skriletz Date: Sun, 11 Feb 2024 09:33:24 -0700 Subject: [PATCH 3/6] Move student_filter_menu back to Stats template dir. --- .../ContentGenerator/Instructor/Stats/problem_stats.html.ep | 2 +- templates/ContentGenerator/Instructor/Stats/set_stats.html.ep | 2 +- .../Instructor/Stats/student_filter_menu.html.ep} | 0 .../Instructor/StudentProgress/set_progress.html.ep | 3 ++- 4 files changed, 4 insertions(+), 3 deletions(-) rename templates/{HTML/StudentFilterMenu/menu.html.ep => ContentGenerator/Instructor/Stats/student_filter_menu.html.ep} (100%) diff --git a/templates/ContentGenerator/Instructor/Stats/problem_stats.html.ep b/templates/ContentGenerator/Instructor/Stats/problem_stats.html.ep index 4767239499..56df432ca0 100644 --- a/templates/ContentGenerator/Instructor/Stats/problem_stats.html.ep +++ b/templates/ContentGenerator/Instructor/Stats/problem_stats.html.ep @@ -6,7 +6,7 @@ %
<%= maketext('Showing statistics for:') =%> - <%= include 'HTML/StudentFilterMenu/menu', filters => $filters, params => {} =%> + <%= include 'ContentGenerator/Instructor/Stats/student_filter_menu', filters => $filters, params => {} =%> <%= include 'ContentGenerator/Instructor/Stats/problem_menu', problems => $problems =%>
% diff --git a/templates/ContentGenerator/Instructor/Stats/set_stats.html.ep b/templates/ContentGenerator/Instructor/Stats/set_stats.html.ep index 89c1c80b33..ebdb7e3818 100644 --- a/templates/ContentGenerator/Instructor/Stats/set_stats.html.ep +++ b/templates/ContentGenerator/Instructor/Stats/set_stats.html.ep @@ -5,7 +5,7 @@ % # Filter and problem selectors.
<%= maketext('Showing statistics for:') =%> - <%= include 'HTML/StudentFilterMenu/menu', filters => $filters, params => {} =%> + <%= include 'ContentGenerator/Instructor/Stats/student_filter_menu', filters => $filters, params => {} =%> <%= include 'ContentGenerator/Instructor/Stats/problem_menu', problems => $problems =%>
% # Set information diff --git a/templates/HTML/StudentFilterMenu/menu.html.ep b/templates/ContentGenerator/Instructor/Stats/student_filter_menu.html.ep similarity index 100% rename from templates/HTML/StudentFilterMenu/menu.html.ep rename to templates/ContentGenerator/Instructor/Stats/student_filter_menu.html.ep diff --git a/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep b/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep index 4fe3361435..4a84d4d756 100644 --- a/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep +++ b/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep @@ -24,7 +24,8 @@ % if (%$filters) {
<%= maketext('Showing progress for:') =%> - <%= include 'HTML/StudentFilterMenu/menu', filters => $filters, params => \%params =%> + <%= include 'ContentGenerator/Instructor/Stats/student_filter_menu', + filters => $filters, params => \%params =%>
% } % From e9449f6189075e013dbd2daea6cd4c0eb4ce1c98 Mon Sep 17 00:00:00 2001 From: Jaimos Skriletz Date: Sun, 11 Feb 2024 12:38:05 -0700 Subject: [PATCH 4/6] Use the array from getFiltersForClass directly instead of converting to hash. --- .../ContentGenerator/Instructor/Stats.pm | 12 +++++----- .../Instructor/StudentProgress.pm | 9 ++++---- .../Stats/student_filter_menu.html.ep | 23 +++++++++---------- .../StudentProgress/set_progress.html.ep | 2 +- 4 files changed, 22 insertions(+), 24 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm index d71643e514..91bdc75eb9 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm @@ -102,15 +102,15 @@ sub siblings ($c) { } # Apply the currently selected filter to the student records, and return a reference to the -# list of students and a reference to a hash of section/recitation filters. +# list of students and a reference to the array of section/recitation filters. sub filter_students ($c) { - my $filter = $c->param('filter'); - my @students = $filter ? filterRecords($c, 0, [$filter], @{ $c->{student_records} }) : @{ $c->{student_records} }; + my $filter = $c->param('filter') || 'all'; + my @students = + $filter eq 'all' ? @{ $c->{student_records} } : filterRecords($c, 0, [$filter], @{ $c->{student_records} }); - # convert the array from getFiltersForClass to a hash, after removing the first 'all' filter. + # Change visible name of the first 'all' filter. my $filters = getFiltersForClass($c, [ 'section', 'recitation' ], @{ $c->{student_records} }); - shift(@$filters); - $filters = { map { $_->[1] => $_->[0] } @$filters }; + $filters->[0][0] = $c->maketext('All students'); return (\@students, $filters); } diff --git a/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm b/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm index feaf6a5e93..45af2e115c 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm @@ -116,14 +116,13 @@ sub displaySets ($c) { : (date => 0, testtime => 0, timeleft => 0, problems => 1, section => 1, recit => 1, login => 1); my $showBestOnly = $setIsVersioned ? $c->param('show_best_only') : 0; - my $filter = $c->param('filter'); + my $filter = $c->param('filter') || 'all'; my @student_records = - $filter ? filterRecords($c, 0, [$filter], @{ $c->{student_records} }) : @{ $c->{student_records} }; + $filter eq 'all' ? @{ $c->{student_records} } : filterRecords($c, 0, [$filter], @{ $c->{student_records} }); - # convert the array from getFiltersForClass to a hash, after removing the first 'all' filter. + # Change visible name of the first 'all' filter. my $filters = getFiltersForClass($c, [ 'section', 'recitation' ], @{ $c->{student_records} }); - shift(@$filters); - $filters = { map { $_->[1] => $_->[0] } @$filters }; + $filters->[0][0] = $c->maketext('All students'); my @score_list; my @user_set_list; diff --git a/templates/ContentGenerator/Instructor/Stats/student_filter_menu.html.ep b/templates/ContentGenerator/Instructor/Stats/student_filter_menu.html.ep index 5dcecaf5e6..98ba7f083e 100644 --- a/templates/ContentGenerator/Instructor/Stats/student_filter_menu.html.ep +++ b/templates/ContentGenerator/Instructor/Stats/student_filter_menu.html.ep @@ -1,20 +1,19 @@ -% last unless %$filters; +% last unless @$filters > 1; % % # Create a section/recitation "filter by" dropdown if there are sections or recitations.
- <%= link_to param('filter') ? $filters->{param('filter')} : maketext('All students') => '#', - id => 'filter', class => 'btn btn-primary dropdown-toggle', role => 'button', 'aria-expanded' => 'false', - data => { bs_toggle => 'dropdown' } =%> + % my $filter = param('filter') || 'all'; + % my $current_filter = $filters->[0][0]; + % for (@$filters) { + % $current_filter = $_->[0] if $_->[1] eq $filter; + % } + <%= link_to $current_filter => '#', id => 'filter', class => 'btn btn-primary dropdown-toggle', + role => 'button', 'aria-expanded' => 'false', data => { bs_toggle => 'dropdown' } =%> diff --git a/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep b/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep index 4a84d4d756..3f71ede160 100644 --- a/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep +++ b/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep @@ -21,7 +21,7 @@ % ); % % # Filter selector. -% if (%$filters) { +% if (@$filters > 1) {
<%= maketext('Showing progress for:') =%> <%= include 'ContentGenerator/Instructor/Stats/student_filter_menu', From abed71bc682728905a562afdabf5f2e0e07860c4 Mon Sep 17 00:00:00 2001 From: Jaimos Skriletz Date: Sun, 11 Feb 2024 12:50:59 -0700 Subject: [PATCH 5/6] Clean up some unused variables. --- lib/WeBWorK/ContentGenerator/Instructor/Stats.pm | 2 -- .../Instructor/StudentProgress/set_progress.html.ep | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm index 91bdc75eb9..efd891303f 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm @@ -170,7 +170,6 @@ sub set_stats ($c) { # Only count top level problems for Jitar sets. my $num_problems = $isJitarSet ? scalar(keys %topLevelProblems) : scalar(@problems); - my $filter = $c->param('filter'); my ($students, $filters) = $c->filter_students; for my $studentRecord (@$students) { my $student = $studentRecord->user_id; @@ -362,7 +361,6 @@ sub problem_stats ($c) { my $db = $c->db; my $ce = $c->ce; my $user = $c->param('user'); - my $filter = $c->param('filter'); my $courseID = $c->stash('courseID'); my $problemID = $c->stash('problemID'); diff --git a/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep b/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep index 3f71ede160..2c856bc55a 100644 --- a/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep +++ b/templates/ContentGenerator/Instructor/StudentProgress/set_progress.html.ep @@ -89,8 +89,8 @@
- % if (param('filter')) { - <%= hidden_field filter => param('filter') =%> + % if ($filter) { + <%= hidden_field filter => $filter =%> % } % if ($primary_sort_method) { <%= hidden_field primary_sort => $primary_sort_method =%> From 873760ded90bb48cab57999838a745588a0ab86e Mon Sep 17 00:00:00 2001 From: Jaimos Skriletz Date: Sun, 11 Feb 2024 17:57:52 -0700 Subject: [PATCH 6/6] Don't filter out include_in_stats to early. It should still be possible to see the stats or progress of users not included in stats, as they should only be filtered out when computing stats for the whole class or showing progress for a single set. This moves the include_in_stats filter to a more appropriate time. --- lib/WeBWorK/ContentGenerator/Instructor/Stats.pm | 14 ++++++++------ .../Instructor/StudentProgress.pm | 15 ++++++++------- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm index efd891303f..b880efe9d1 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm @@ -39,9 +39,9 @@ sub initialize ($c) { # Cache a list of all users except set level proctors and practice users, and restrict to the sections or # recitations that are allowed for the user if such restrictions are defined. This list is sorted by last_name, # then first_name, then user_id. This is used in multiple places in this module, and is guaranteed to be used at - # least once. So it is done here to prevent extra database access. Filter out users not included in stats. + # least once. So it is done here to prevent extra database access. $c->{student_records} = [ - grep { $ce->status_abbrev_has_behavior($_->status, 'include_in_stats') } $db->getUsersWhere( + $db->getUsersWhere( { user_id => [ -and => { not_like => 'set_id:%' }, { not_like => "$ce->{practiceUserPrefix}\%" } ], $ce->{viewable_sections}{$user} || $ce->{viewable_recitations}{$user} @@ -104,14 +104,16 @@ sub siblings ($c) { # Apply the currently selected filter to the student records, and return a reference to the # list of students and a reference to the array of section/recitation filters. sub filter_students ($c) { - my $filter = $c->param('filter') || 'all'; - my @students = - $filter eq 'all' ? @{ $c->{student_records} } : filterRecords($c, 0, [$filter], @{ $c->{student_records} }); + my $ce = $c->ce; + my $filter = $c->param('filter') || 'all'; + my @students = grep { $ce->status_abbrev_has_behavior($_->status, 'include_in_stats') } @{ $c->{student_records} }; # Change visible name of the first 'all' filter. - my $filters = getFiltersForClass($c, [ 'section', 'recitation' ], @{ $c->{student_records} }); + my $filters = getFiltersForClass($c, [ 'section', 'recitation' ], @students); $filters->[0][0] = $c->maketext('All students'); + @students = filterRecords($c, 0, [$filter], @students) unless $filter eq 'all'; + return (\@students, $filters); } diff --git a/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm b/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm index 45af2e115c..e4868ac791 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm @@ -37,9 +37,9 @@ sub initialize ($c) { # Cache a list of all users except set level proctors and practice users, and restrict to the sections or # recitations that are allowed for the user if such restrictions are defined. This list is sorted by last_name, # then first_name, then user_id. This is used in multiple places in this module, and is guaranteed to be used at - # least once. So it is done here to prevent extra database access. Filter out users not included in stats. + # least once. So it is done here to prevent extra database access. $c->{student_records} = [ - grep { $ce->status_abbrev_has_behavior($_->status, 'include_in_stats') } $db->getUsersWhere( + $db->getUsersWhere( { user_id => [ -and => { not_like => 'set_id:%' }, { not_like => "$ce->{practiceUserPrefix}\%" } ], $ce->{viewable_sections}{$user} || $ce->{viewable_recitations}{$user} @@ -116,20 +116,21 @@ sub displaySets ($c) { : (date => 0, testtime => 0, timeleft => 0, problems => 1, section => 1, recit => 1, login => 1); my $showBestOnly = $setIsVersioned ? $c->param('show_best_only') : 0; - my $filter = $c->param('filter') || 'all'; + # Only show students who are included in stats. my @student_records = - $filter eq 'all' ? @{ $c->{student_records} } : filterRecords($c, 0, [$filter], @{ $c->{student_records} }); + grep { $ce->status_abbrev_has_behavior($_->status, 'include_in_stats') } @{ $c->{student_records} }; # Change visible name of the first 'all' filter. - my $filters = getFiltersForClass($c, [ 'section', 'recitation' ], @{ $c->{student_records} }); + my $filter = $c->param('filter') || 'all'; + my $filters = getFiltersForClass($c, [ 'section', 'recitation' ], @student_records); $filters->[0][0] = $c->maketext('All students'); + @student_records = filterRecords($c, 0, [$filter], @student_records) unless $filter eq 'all'; + my @score_list; my @user_set_list; for my $studentRecord (@student_records) { - next unless $ce->status_abbrev_has_behavior($studentRecord->status, 'include_in_stats'); - my $studentName = $studentRecord->user_id; my ($allSetVersionNames, $notAssignedSet) = list_set_versions($db, $studentName, $c->stash('setID'), $setIsVersioned);