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..33b86fb40b 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/Stats.pm @@ -22,30 +22,7 @@ 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 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; @@ -57,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; @@ -67,9 +68,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,12 +78,11 @@ sub page_title ($c) { ); } - return $c->maketext('Statistics'); + return $c->maketext('Set Statistics'); } 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/lib/WeBWorK/Utils/Routes.pm b/lib/WeBWorK/Utils/Routes.pm index bb5101069c..d1b42c73dc 100644 --- a/lib/WeBWorK/Utils/Routes.pm +++ b/lib/WeBWorK/Utils/Routes.pm @@ -86,7 +86,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 @@ -483,7 +482,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' }, @@ -498,11 +497,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) { - + % }
    • % # 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/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_/) { - - % } elsif (current_route eq 'instructor_problem_statistics') { +

    <%= maketext('Statistics') %>

    + % if (current_route eq 'instructor_problem_statistics') { % } else { - + % # 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. +% + 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/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' =%> + % } +
    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.') =%>