From e4f27baa7b8d4cdfd20b02988c769ef0b537487f Mon Sep 17 00:00:00 2001 From: Jaimos Skriletz Date: Tue, 17 Feb 2026 19:31:42 -0700 Subject: [PATCH 1/2] Remove student statistics. The student statistics and student progress page are identical. Remove the student statistics version of the page, since the page doesn't provide any summary of the data, and only shows a student's number of attempts and grade. --- .../ContentGenerator/Instructor/Index.pm | 7 ----- .../ContentGenerator/Instructor/Stats.pm | 10 ++----- lib/WeBWorK/Utils/Routes.pm | 8 +---- templates/ContentGenerator/Base/links.html.ep | 30 ++----------------- .../ContentGenerator/Instructor/Stats.html.ep | 18 ++++++----- .../Instructor/StudentProgress.html.ep | 8 ++--- .../{Stats => StudentProgress}/index.html.ep | 13 +++----- .../student_progress.html.ep} | 3 -- templates/HelpFiles/InstructorStats.html.ep | 13 +++----- 9 files changed, 27 insertions(+), 83 deletions(-) rename templates/ContentGenerator/Instructor/{Stats => StudentProgress}/index.html.ep (59%) rename templates/ContentGenerator/Instructor/{Stats/student_stats.html.ep => StudentProgress/student_progress.html.ep} (85%) diff --git a/lib/WeBWorK/ContentGenerator/Instructor/Index.pm b/lib/WeBWorK/ContentGenerator/Instructor/Index.pm index 2ac5e5e94e..ac0f00f2be 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/Index.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/Index.pm @@ -80,13 +80,6 @@ sub pre_header_initialize ($c) { } else { push @error, E_ONE_SET; } - } elsif (defined $c->param('user_stats')) { - if ($nusers == 1) { - $route = 'instructor_user_statistics'; - $args{userID} = $firstUserID; - } else { - push @error, E_ONE_USER; - } } elsif (defined $c->param('set_stats')) { if ($nsets == 1) { $route = 'instructor_set_statistics'; diff --git a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm index ea59388414..5b147efb66 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm @@ -43,9 +43,7 @@ sub initialize ($c) { ) ]; - if ($c->current_route eq 'instructor_user_statistics') { - $c->{studentID} = $c->stash('userID'); - } elsif ($c->current_route =~ /^instructor_(set|problem)_statistics$/) { + if ($c->current_route =~ /^instructor_(set|problem)_statistics$/) { my $setRecord = $db->getGlobalSet($c->stash('setID')); return unless $setRecord; $c->{setRecord} = $setRecord; @@ -67,9 +65,7 @@ sub page_title ($c) { my $setID = $c->stash('setID') || ''; - if ($c->current_route eq 'instructor_user_statistics') { - return $c->maketext('Statistics for student [_1]', $c->{studentID}); - } elsif ($c->current_route eq 'instructor_set_statistics') { + if ($c->current_route eq 'instructor_set_statistics') { return $c->maketext('Statistics for [_1]', $c->tag('span', dir => 'ltr', format_set_name_display($setID))); } elsif ($c->current_route eq 'instructor_problem_statistics') { return $c->maketext( @@ -79,7 +75,7 @@ sub page_title ($c) { ); } - return $c->maketext('Statistics'); + return $c->maketext('Set Statistics'); } sub siblings ($c) { diff --git a/lib/WeBWorK/Utils/Routes.pm b/lib/WeBWorK/Utils/Routes.pm index b8fda7d559..e24f33ce12 100644 --- a/lib/WeBWorK/Utils/Routes.pm +++ b/lib/WeBWorK/Utils/Routes.pm @@ -87,7 +87,6 @@ PLEASE FOR THE LOVE OF GOD UPDATE THIS IF YOU CHANGE THE ROUTES BELOW!!! instructor_statistics /$courseID/instructor/stats instructor_set_statistics /$courseID/instructor/stats/set/$setID instructor_problem_statistics /$courseID/instructor/stats/set/$setID/$problemID - instructor_user_statistics /$courseID/instructor/stats/student/$userID instructor_progress /$courseID/instructor/progress instructor_set_progress /$courseID/instructor/progress/set/$setID @@ -491,7 +490,7 @@ my %routeParameters = ( }, instructor_statistics => { title => x('Statistics'), - children => [qw(instructor_set_statistics instructor_user_statistics)], + children => [qw(instructor_set_statistics)], module => 'Instructor::Stats', path => '/stats' }, @@ -506,11 +505,6 @@ my %routeParameters = ( module => 'Instructor::Stats', path => '/' }, - instructor_user_statistics => { - title => '[_1]', - module => 'Instructor::Stats', - path => '/student/#userID' - }, instructor_progress => { title => x('Student Progress'), children => [qw(instructor_set_progress instructor_user_progress)], diff --git a/templates/ContentGenerator/Base/links.html.ep b/templates/ContentGenerator/Base/links.html.ep index f45f2f9b2e..171206b58b 100644 --- a/templates/ContentGenerator/Base/links.html.ep +++ b/templates/ContentGenerator/Base/links.html.ep @@ -221,31 +221,8 @@ % # Statistics
  • <%= $makelink->('instructor_statistics') %> - % if ($userID ne $eUserID || defined $setID || defined $urlUserID) { -
      - % if (defined $urlUserID) { -
    • - <%= $makelink->( - 'instructor_user_statistics', - text => $urlUserID, - captures => { userID => $urlUserID }, - link_attrs => { dir => 'ltr' } - ) %> -
      • - % } - % if ($userID ne $eUserID && (!defined $urlUserID || $urlUserID ne $eUserID)) { -
    • - <%= $makelink->( - 'instructor_user_statistics', - text => $eUserID, - captures => { userID => $eUserID }, - active => current_route eq 'instructor_user_statistics' - && !defined $urlUserID, - link_attrs => { dir => 'ltr' } - ) %> -
      • - % } - % if (defined $setID) { + % if (defined $setID) { +
      • <%= $makelink->( 'instructor_set_statistics', @@ -270,8 +247,7 @@
      % } - % } -
    + % }
    • % # Student Progress diff --git a/templates/ContentGenerator/Instructor/Stats.html.ep b/templates/ContentGenerator/Instructor/Stats.html.ep index 19c234d292..f1ee404562 100644 --- a/templates/ContentGenerator/Instructor/Stats.html.ep +++ b/templates/ContentGenerator/Instructor/Stats.html.ep @@ -1,20 +1,22 @@ % use WeBWorK::Utils qw(getAssetURL); +% use WeBWorK::Utils::Sets qw(format_set_name_display); % % unless ($authz->hasPermissions(param('user'), 'access_instructor_tools')) {
    <%= maketext('You are not authorized to access instructor tools') %>
    % last; % } % -% if (current_route eq 'instructor_user_statistics') { - % # Stats and StudentProgress share this template. - <%= include 'ContentGenerator/Instructor/Stats/student_stats' =%> -% } elsif (current_route eq 'instructor_set_statistics') { +% if (current_route eq 'instructor_set_statistics') { <%= $c->set_stats =%> % } elsif (current_route eq 'instructor_problem_statistics') { <%= $c->problem_stats =%> % } else { - % # Stats and StudentProgress share this template also. - <%= include 'ContentGenerator/Instructor/Stats/index', - set_header => maketext('View statistics by set'), - student_header => maketext('View statistics by student') =%> + % } diff --git a/templates/ContentGenerator/Instructor/StudentProgress.html.ep b/templates/ContentGenerator/Instructor/StudentProgress.html.ep index d135d8e5c5..4b02941e92 100644 --- a/templates/ContentGenerator/Instructor/StudentProgress.html.ep +++ b/templates/ContentGenerator/Instructor/StudentProgress.html.ep @@ -4,13 +4,9 @@ % } % % if (current_route eq 'instructor_user_progress') { - % # Stats and StudentProgress share this template. - <%= include 'ContentGenerator/Instructor/Stats/student_stats' =%> + <%= include 'ContentGenerator/Instructor/StudentProgress/student_progress' =%> % } elsif (current_route eq 'instructor_set_progress') { <%= $c->displaySets =%> % } else { - % # Stats and StudentProgress share this template also. - <%= include 'ContentGenerator/Instructor/Stats/index', - set_header => maketext('View student progress by set'), - student_header => maketext('View student progress by student') =%> + <%= include 'ContentGenerator/Instructor/StudentProgress/index' =%> % } diff --git a/templates/ContentGenerator/Instructor/Stats/index.html.ep b/templates/ContentGenerator/Instructor/StudentProgress/index.html.ep similarity index 59% rename from templates/ContentGenerator/Instructor/Stats/index.html.ep rename to templates/ContentGenerator/Instructor/StudentProgress/index.html.ep index 96016b1bfa..142abaa394 100644 --- a/templates/ContentGenerator/Instructor/Stats/index.html.ep +++ b/templates/ContentGenerator/Instructor/StudentProgress/index.html.ep @@ -1,22 +1,17 @@ -% # Note that this template is used by both WeBWorK::ContentGenerator::Instructor::Stats and -% # WeBWorK::ContentGenerator::Instructor::StudentProgress. -% % use WeBWorK::Utils::Sets qw(format_set_name_display); % -% my $type = current_route =~ s/instructor_//r; -% % stash->{footerWidthClass} = 'col-lg-10 col-sm-12'; %
    -

    <%= $set_header %>

    +

    <%= maketext('View student progress by set') %>

      % for ($db->listGlobalSetsWhere({}, 'set_id')) {
    • <%= link_to format_set_name_display($_->[0]) => - $c->systemLink(url_for("instructor_set_$type", setID => $_->[0])) =%> + $c->systemLink(url_for("instructor_set_progress", setID => $_->[0])) =%>
      • % }
    @@ -26,12 +21,12 @@
    -

    <%= $student_header %>

    +

    <%= maketext('View student progress by student') %>

      % for (@{ $c->{student_records} }) {
    • <%= link_to $_->last_name . ', ' . $_->first_name . ' (' . $_->user_id . ')' => - $c->systemLink(url_for("instructor_user_$type", userID => $_->user_id)) =%> + $c->systemLink(url_for("instructor_user_progress", userID => $_->user_id)) =%>
      • % }
    diff --git a/templates/ContentGenerator/Instructor/Stats/student_stats.html.ep b/templates/ContentGenerator/Instructor/StudentProgress/student_progress.html.ep similarity index 85% rename from templates/ContentGenerator/Instructor/Stats/student_stats.html.ep rename to templates/ContentGenerator/Instructor/StudentProgress/student_progress.html.ep index 6f50080c39..913d3e410f 100644 --- a/templates/ContentGenerator/Instructor/Stats/student_stats.html.ep +++ b/templates/ContentGenerator/Instructor/StudentProgress/student_progress.html.ep @@ -1,6 +1,3 @@ -% # Note that this template is used by both WeBWorK::ContentGenerator::Instructor::Stats and -% # WeBWorK::ContentGenerator::Instructor::StudentProgress. -% % use WeBWorK::ContentGenerator::Grades; % % my $studentRecord = $db->getUser($c->{studentID}); diff --git a/templates/HelpFiles/InstructorStats.html.ep b/templates/HelpFiles/InstructorStats.html.ep index c3434a1ccd..49a3f62517 100644 --- a/templates/HelpFiles/InstructorStats.html.ep +++ b/templates/HelpFiles/InstructorStats.html.ep @@ -1,14 +1,9 @@ % layout 'help_macro'; % title maketext('Statistics Help'); % -

    - <%= maketext('The main page allows access to set and student statistics. When selecting a student, their grades ' - . 'page is shown, which lists set totals and per problem grades for each set assigned to the student. When ' - . 'selecting a set, various statistics and histograms are shown for the overall grades of students who are ' - . 'currently enrolled or auditing the course.') =%> -

    - <%= maketext('When viewing set statistics, the drop down menus can be used to show stats for individual sections, ' - . 'recitations, or problems. The overall results include all students who are assigned to the set, while the ' - . 'individual problem results only include active (have attempted the problem) students.') =%> + <%= maketext('Display full set or problem statistics. The main page lists all sets to view. When viewing ' + . 'set statistics, the drop down menus can be used to show stats for individual sections, recitations, or ' + . 'problems. The overall results include all students who are assigned to the set, while the individual ' + . 'problem results only include active (have attempted the problem) students.') =%>

    From 55221885a8a1ece690cd6d577690fff4e0fe136b Mon Sep 17 00:00:00 2001 From: Jaimos Skriletz Date: Tue, 17 Feb 2026 21:06:12 -0700 Subject: [PATCH 2/2] No longer share siblings template between Stats and StudentProgress. The only shared code is a new template which just links to all sets. The problem links or student links are put in each pages siblings template. --- .../ContentGenerator/Instructor/Stats.pm | 48 ++++++++++--------- .../Instructor/StudentProgress.pm | 3 +- .../Instructor/Stats/siblings.html.ep | 43 ++--------------- .../Stats/siblings_set_list.html.ep | 18 +++++++ .../StudentProgress/siblings.html.ep | 30 ++++++++++++ 5 files changed, 79 insertions(+), 63 deletions(-) create mode 100644 templates/ContentGenerator/Instructor/Stats/siblings_set_list.html.ep create mode 100644 templates/ContentGenerator/Instructor/StudentProgress/siblings.html.ep diff --git a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm index 5b147efb66..33b86fb40b 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm @@ -22,27 +22,6 @@ sub initialize ($c) { # Check permissions return unless $c->authz->hasPermissions($user, 'access_instructor_tools'); - # 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. - $c->{student_records} = [ - $db->getUsersWhere( - { - user_id => [ -and => { not_like => 'set_id:%' }, { not_like => "$ce->{practiceUserPrefix}\%" } ], - $ce->{viewable_sections}{$user} || $ce->{viewable_recitations}{$user} - ? ( - -or => [ - $ce->{viewable_sections}{$user} ? (section => $ce->{viewable_sections}{$user}) : (), - $ce->{viewable_recitations}{$user} ? (recitation => $ce->{viewable_recitations}{$user}) : () - ] - ) - : () - }, - [qw/last_name first_name user_id/] - ) - ]; - if ($c->current_route =~ /^instructor_(set|problem)_statistics$/) { my $setRecord = $db->getGlobalSet($c->stash('setID')); return unless $setRecord; @@ -55,6 +34,30 @@ sub initialize ($c) { return unless $problemRecord; $c->{problemRecord} = $problemRecord; } + + # 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 used + # on every page except the main page, so it is done here to prevent extra database access. + $c->{student_records} = [ + $db->getUsersWhere( + { + user_id => + [ -and => { not_like => 'set_id:%' }, { not_like => "$ce->{practiceUserPrefix}\%" } ], + $ce->{viewable_sections}{$user} || $ce->{viewable_recitations}{$user} + ? ( + -or => [ + $ce->{viewable_sections}{$user} ? (section => $ce->{viewable_sections}{$user}) : (), + $ce->{viewable_recitations}{$user} + ? (recitation => $ce->{viewable_recitations}{$user}) + : () + ] + ) + : () + }, + [qw/last_name first_name user_id/] + ) + ]; } return; @@ -79,8 +82,7 @@ sub page_title ($c) { } sub siblings ($c) { - # Stats and StudentProgress share this template. - return $c->include('ContentGenerator/Instructor/Stats/siblings', header => $c->maketext('Statistics')); + return $c->include('ContentGenerator/Instructor/Stats/siblings'); } # Apply the currently selected filter to the student records, and return a reference to the diff --git a/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm b/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm index 6acc9cc579..0871948c44 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/StudentProgress.pm @@ -68,8 +68,7 @@ sub page_title ($c) { } sub siblings ($c) { - # Stats and StudentProgress share this template. - return $c->include('ContentGenerator/Instructor/Stats/siblings', header => $c->maketext('Student Progress')); + return $c->include('ContentGenerator/Instructor/StudentProgress/siblings'); } # Display student progress table diff --git a/templates/ContentGenerator/Instructor/Stats/siblings.html.ep b/templates/ContentGenerator/Instructor/Stats/siblings.html.ep index 68827f455e..41bd165a6a 100644 --- a/templates/ContentGenerator/Instructor/Stats/siblings.html.ep +++ b/templates/ContentGenerator/Instructor/Stats/siblings.html.ep @@ -1,6 +1,3 @@ -% # Note that this template is used by both WeBWorK::ContentGenerator::Instructor::Stats and -% # WeBWorK::ContentGenerator::Instructor::StudentProgress. -% % use WeBWorK::Utils::JITAR qw(jitar_id_to_seq); % use WeBWorK::Utils::Sets qw(format_set_name_display); % @@ -9,24 +6,8 @@ % } %
    -

    <%= $header %>

    - % if (current_route =~ /^instructor_user_/) { -
      - % for (@{ $c->{student_records} }) { -
    • - <%= tag 'a', - $_->user_id eq $c->{studentID} - ? (class => 'nav-link active') - : ( - href => $c->systemLink(url_for(current_route, userID => $_->user_id)), - class => 'nav-link' - ), begin =%> - <%= $_->last_name =%>, <%= $_->first_name %> (<%= $_->user_id %>) - <% end =%> -
      • - % } -
    - % } elsif (current_route eq 'instructor_problem_statistics') { +

    <%= maketext('Statistics') %>

    + % if (current_route eq 'instructor_problem_statistics') {
      % for (map { $_->[1] } $db->listGlobalProblemsWhere({ set_id => stash->{setID} }, 'problem_id')) {
    • @@ -46,22 +27,8 @@ % }
    % } else { -
      - % for (map { $_->[0] } $db->listGlobalSetsWhere({}, 'set_id')) { -
    • - <%= tag 'a', - defined stash('setID') && $_ eq stash('setID') ? (class => 'nav-link active') - : ( - href => $c->systemLink( - url_for(current_route =~ s/instructor_(progress|statistics)/instructor_set_$1/r, - setID => $_), - params => param('filter') ? { filter => param('filter') } : {} - ), - class => 'nav-link' - ), - format_set_name_display($_) =%> -
      • - % } -
    + % # Shared with StudentProgress siblings. + <%= include 'ContentGenerator/Instructor/Stats/siblings_set_list', + set_link_route => 'instructor_set_statistics' =%> % }
    diff --git a/templates/ContentGenerator/Instructor/Stats/siblings_set_list.html.ep b/templates/ContentGenerator/Instructor/Stats/siblings_set_list.html.ep new file mode 100644 index 0000000000..e69605b056 --- /dev/null +++ b/templates/ContentGenerator/Instructor/Stats/siblings_set_list.html.ep @@ -0,0 +1,18 @@ +% # This is shared between Stats and StudentProgress siblings page to list all sets. +% +
      + % for (map { $_->[0] } $db->listGlobalSetsWhere({}, 'set_id')) { +
    • + <%= tag 'a', + defined stash('setID') && $_ eq stash('setID') ? (class => 'nav-link active') + : ( + href => $c->systemLink( + url_for($set_link_route, setID => $_), + params => param('filter') ? { filter => param('filter') } : {} + ), + class => 'nav-link' + ), + format_set_name_display($_) =%> +
      • + % } +
    diff --git a/templates/ContentGenerator/Instructor/StudentProgress/siblings.html.ep b/templates/ContentGenerator/Instructor/StudentProgress/siblings.html.ep new file mode 100644 index 0000000000..8d77a079f9 --- /dev/null +++ b/templates/ContentGenerator/Instructor/StudentProgress/siblings.html.ep @@ -0,0 +1,30 @@ +% use WeBWorK::Utils::Sets qw(format_set_name_display); +% +% unless ($authz->hasPermissions(param('user'), 'access_instructor_tools')) { + % last; +% } +% +
    +

    <%= maketext('Student Progress') %>

    + % if (current_route eq 'instructor_user_progress') { +
      + % for (@{ $c->{student_records} }) { +
    • + <%= tag 'a', + $_->user_id eq $c->{studentID} + ? (class => 'nav-link active') + : ( + href => $c->systemLink(url_for(current_route, userID => $_->user_id)), + class => 'nav-link' + ), begin =%> + <%= $_->last_name =%>, <%= $_->first_name %> (<%= $_->user_id %>) + <% end =%> +
      • + % } +
    + % } else { + % # Shared with Stats siblings. + <%= include 'ContentGenerator/Instructor/Stats/siblings_set_list', + set_link_route => 'instructor_set_progress' =%> + % } +