Moodle
  1. Moodle
  2. MDL-31969

Add option to show/hide suspended users in grader report

    Details

    • Testing Instructions:
      Hide

      Prerequisite

      1. Course with min. one activity
      2. Few enrolled students with some suspended enrolment, and some with expired enrolments.

      Test 1

      1. Go to Grader report preference (Grade administration -> My report preferences -> Grader report)
      2. Set "Show only active enrolments" to Yes
      3. You should only see grades of active students (and not suspended users.)

      Test 2

      1. Go to Grader report preference (Grade administration -> My report preferences -> Grader report)
      2. Set "Show only active enrolments" to No
      3. You should only see grades of active and suspended (Greyed name) students.

      Test 4:

      1. Export grades (in all formats ods/xls/txt) with "Exclude suspended users" unchecked and make sure you see one more column "suspended" with appropriate "yes" value showing user enrolment status.
      2. Try exporting again with "Exclude suspended users" checked and you should only get grades for active users. Also suspended column should not be in export file.

      Test 5:

      1. Log in as admin
      2. Remove course:viewsuspendedusers capability for editing teacher
      3. Log in as editing teacher, and try Test 1, 2, 3 and 4 and make sure you can't show/hide/export suspended users.
      Show
      Prerequisite Course with min. one activity Few enrolled students with some suspended enrolment, and some with expired enrolments. Test 1 Go to Grader report preference (Grade administration -> My report preferences -> Grader report) Set "Show only active enrolments" to Yes You should only see grades of active students (and not suspended users.) Test 2 Go to Grader report preference (Grade administration -> My report preferences -> Grader report) Set "Show only active enrolments" to No You should only see grades of active and suspended (Greyed name) students. Test 4: Export grades (in all formats ods/xls/txt) with "Exclude suspended users" unchecked and make sure you see one more column "suspended" with appropriate "yes" value showing user enrolment status. Try exporting again with "Exclude suspended users" checked and you should only get grades for active users. Also suspended column should not be in export file. Test 5: Log in as admin Remove course:viewsuspendedusers capability for editing teacher Log in as editing teacher, and try Test 1, 2, 3 and 4 and make sure you can't show/hide/export suspended users.
    • Workaround:
      Hide

      change the code to something like -

      line 339,340 you'll see a comment that it wants to limit users with an active enrollment.

      Yet to do that, you would want something more like

      list($enrolledsql, $enrolledparams) = get_enrolled_sql($this->context,'',0,1);
      
      Show
      change the code to something like - line 339,340 you'll see a comment that it wants to limit users with an active enrollment. Yet to do that, you would want something more like list($enrolledsql, $enrolledparams) = get_enrolled_sql($ this ->context,'',0,1);
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-mdl-31969
    • Rank:
      38632

      Description

      When you go into the grader report, you will see suspended users (with the suspend icon).
      I don't believe it is the intent of the report to do that per the comments in the file
      (see the comment below on "//limit to users with an active enrollment")

      moodle / grade / report / grader / lib.php
      //limit to users with an active enrollment
      list($enrolledsql, $enrolledparams) = get_enrolled_sql($this->context);
      

      line 339,340 you'll see a comment that it wants to limit users with an active enrollment.

      Yet to do that, you would want something more like -

      list($enrolledsql, $enrolledparams) = get_enrolled_sql($this->context,'',0,1);
      

      get_enrolled_sql is in lib/accesslib.php

      function get_enrolled_sql(context $context, $withcapability = '', $groupid = 0, $onlyactive = false) {
      
        1. note onlyactive is set to false by default.

      Replication steps:

      1. in a course with users,
      2. go into Course administration -> users -> enrolled users
      3. click on the edit icon on the enrollment method column of one user and change user to suspended.
      4. now go into the gradebook, you will see the user, but there will be the pause/suspend icon.

        Issue Links

          Activity

          Hide
          steve miley added a comment -

          In user/index.php (to display participants) there is a line -

          index.php: list($esql, $params) = get_enrolled_sql($context, NULL, $currentgroup, true);

          The participants/people UI does not display suspended users.

          Show
          steve miley added a comment - In user/index.php (to display participants) there is a line - index.php: list($esql, $params) = get_enrolled_sql($context, NULL, $currentgroup, true); The participants/people UI does not display suspended users.
          Hide
          steve miley added a comment -

          The other comment I'll make, is that there IS CODE in the grader report to deal with suspended users. It seems, if there is a desire to not display, or display suspended users, then maybe that should be a grader report setting for the site or the course.

          The reason this is important to us, is that we are having our external DB plugin do the action of "disable course enrollment and remove roles" when a student drops from the class, if they come back, we want their grade history restored. IF we set it to "unenroll from course" they lose their grade history, which we don't want to happen.

          Show
          steve miley added a comment - The other comment I'll make, is that there IS CODE in the grader report to deal with suspended users. It seems, if there is a desire to not display, or display suspended users, then maybe that should be a grader report setting for the site or the course. The reason this is important to us, is that we are having our external DB plugin do the action of "disable course enrollment and remove roles" when a student drops from the class, if they come back, we want their grade history restored. IF we set it to "unenroll from course" they lose their grade history, which we don't want to happen.
          Hide
          Andrew Davis added a comment -

          I suspect that the comment "limit to users with an active enrollment" is incorrect. That said, a setting to allow teachers to show or hide suspended users would be handy. Most of the time I'd say teachers will want suspended users hidden entirely. There could well be situations where they will want a suspended user included in their grade aggregations and access to a convenient way to get to a suspended student's grades.

          Show
          Andrew Davis added a comment - I suspect that the comment "limit to users with an active enrollment" is incorrect. That said, a setting to allow teachers to show or hide suspended users would be handy. Most of the time I'd say teachers will want suspended users hidden entirely. There could well be situations where they will want a suspended user included in their grade aggregations and access to a convenient way to get to a suspended student's grades.
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this.

          If the report is exhibiting the current expected, I'm going to turn this around and make it an improvement.

          Show
          Michael de Raadt added a comment - Thanks for reporting this. If the report is exhibiting the current expected, I'm going to turn this around and make it an improvement.
          Hide
          Monash University VLE team added a comment -

          I agree with Andrew. Rather than just changing from show to hide, we would like the 'Require active enrolment' checkbox option from the export introduced for the grader report, preferably with the default set to on.

          Regards,
          Russell

          Show
          Monash University VLE team added a comment - I agree with Andrew. Rather than just changing from show to hide, we would like the 'Require active enrolment' checkbox option from the export introduced for the grader report, preferably with the default set to on. Regards, Russell
          Hide
          Rajesh Taneja added a comment -

          We are trying to decide similar functionality for other areas as well.
          It will be helpful if you can contribute your one cent to https://moodle.org/mod/forum/discuss.php?d=223822 or MDL-35731

          Show
          Rajesh Taneja added a comment - We are trying to decide similar functionality for other areas as well. It will be helpful if you can contribute your one cent to https://moodle.org/mod/forum/discuss.php?d=223822 or MDL-35731
          Hide
          Rajesh Taneja added a comment -

          User will be able to set preference to show/hide suspended users if he/she has course:viewsuspendedusers capability. By default it will be set for manager and editingteacher role only.

          Also, when grades are exported with suspended users a new column will be added to differentiate active and suspended user.

          Capability check was suggested in MDL-35731 and hence will be used in other places within course to show/hide suspended users.

          Show
          Rajesh Taneja added a comment - User will be able to set preference to show/hide suspended users if he/she has course:viewsuspendedusers capability. By default it will be set for manager and editingteacher role only. Also, when grades are exported with suspended users a new column will be added to differentiate active and suspended user. Capability check was suggested in MDL-35731 and hence will be used in other places within course to show/hide suspended users.
          Hide
          Andrew Davis added a comment -

          Perhaps give the variable $menususers a better name. I assume it's short for "menu suspended users". Maybe just expand the abbreviation. Scrolling through the code and seeing this "(count($menu) + count($menususers) - 1)" its not clear what is going on due to the cryptic variable name.

          I may be misunderstanding but it looks like the mform field "export_onlyactive" is being sent to the client then relied on later.

          $onlyactive        = optional_param('export_onlyactive', 0, PARAM_BOOL);
          .
          .
          .
          $export = new grade_export_ods($course, $groupid, $itemids, $export_feedback, $updatedgradesonly, $displaytype, $decimalpoints, $onlyactive, true);
          

          As this is now being restricted by 'moodle/course:viewsuspendedusers' it may be wise to recheck the capability to prevent someone modifying the form client side to gain access to suspended users.

          Show
          Andrew Davis added a comment - Perhaps give the variable $menususers a better name. I assume it's short for "menu suspended users". Maybe just expand the abbreviation. Scrolling through the code and seeing this "(count($menu) + count($menususers) - 1)" its not clear what is going on due to the cryptic variable name. I may be misunderstanding but it looks like the mform field "export_onlyactive" is being sent to the client then relied on later. $onlyactive = optional_param('export_onlyactive', 0, PARAM_BOOL); . . . $export = new grade_export_ods($course, $groupid, $itemids, $export_feedback, $updatedgradesonly, $displaytype, $decimalpoints, $onlyactive, true ); As this is now being restricted by 'moodle/course:viewsuspendedusers' it may be wise to recheck the capability to prevent someone modifying the form client side to gain access to suspended users.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the review Andrew,

          I have changed variable name.

          As per export_onlyactive variable is concerned, it is set as constant in form if user doesn't have permission and user can't manipulate constants in mform.
          I have added another commit to double check this. But this is not required.

          Can you please look at this again and let me know if this is good to go for integration.

          Show
          Rajesh Taneja added a comment - Thanks for the review Andrew, I have changed variable name. As per export_onlyactive variable is concerned, it is set as constant in form if user doesn't have permission and user can't manipulate constants in mform. I have added another commit to double check this. But this is not required. Can you please look at this again and let me know if this is good to go for integration.
          Hide
          Andrew Davis added a comment -

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [Y] Language
          [Y] Databases
          [Y] Testing
          [Y] Security
          [Y] Documentation
          [Y] Git
          [Y] Sanity check

          You are go for integration.

          Show
          Andrew Davis added a comment - [Y] Syntax [Y] Output [Y] Whitespace [Y] Language [Y] Databases [Y] Testing [Y] Security [Y] Documentation [Y] Git [Y] Sanity check You are go for integration.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Damyon Wiese added a comment -

          Hi Raj,

          Some comments:

          There are whitespace errors in "grade/export/lib.php".

          Also - with the patch installed I couldn't see suspended users in the gradebook unless I set my report preference to "No" specifically - the default didn't work even though No is the default.

          Also - the admin setting is not obvious that it is only the default setting for the user preference. The text implies that ticking the box will prevent users from seeing suspended enrolments everywhere - but teachers can just change their preferences and they will see them (this may even be considered a security issue for some). The help text for the setting just needs updating.

          The changes to the functions in accesslib.php particularly need to be documented in upgrade.txt

          Show
          Damyon Wiese added a comment - Hi Raj, Some comments: There are whitespace errors in "grade/export/lib.php". Also - with the patch installed I couldn't see suspended users in the gradebook unless I set my report preference to "No" specifically - the default didn't work even though No is the default. Also - the admin setting is not obvious that it is only the default setting for the user preference. The text implies that ticking the box will prevent users from seeing suspended enrolments everywhere - but teachers can just change their preferences and they will see them (this may even be considered a security issue for some). The help text for the setting just needs updating. The changes to the functions in accesslib.php particularly need to be documented in upgrade.txt
          Hide
          Damyon Wiese added a comment -

          Followup from the last comments - I think the change itself will be a huge improvement (especially with the linked issue) as currently the suspended user stuff is clunky and from my experience partners spend heaps of time fixing it themselves.

          Show
          Damyon Wiese added a comment - Followup from the last comments - I think the change itself will be a huge improvement (especially with the linked issue) as currently the suspended user stuff is clunky and from my experience partners spend heaps of time fixing it themselves.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Rajesh Taneja added a comment -

          Thanks Damyon,

          Fixed all suggested problems.

          Back for your review.

          Show
          Rajesh Taneja added a comment - Thanks Damyon, Fixed all suggested problems. Back for your review.
          Hide
          Damyon Wiese added a comment -

          Thanks Raj,

          This has been integrated to master now.

          I'll have a look to see if there is an issue about the name of the admin settings and add one if I can't find anything.

          Show
          Damyon Wiese added a comment - Thanks Raj, This has been integrated to master now. I'll have a look to see if there is an issue about the name of the admin settings and add one if I can't find anything.
          Hide
          Jason Fowler added a comment -

          Works as designed Raj, thanks!

          Show
          Jason Fowler added a comment - Works as designed Raj, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Your awesome contributions are now part of Moodle, your fav LMS out there.

          Closing this as fixed.

          Many thanks for all the hard work, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Your awesome contributions are now part of Moodle, your fav LMS out there. Closing this as fixed. Many thanks for all the hard work, ciao
          Hide
          Chris Gronewold added a comment -

          Will this be back ported to 2.3 or 2.4? We are currently on 2.3 and are wanting to upgrade to 2.4. This is a feature that would be highly used.

          Show
          Chris Gronewold added a comment - Will this be back ported to 2.3 or 2.4? We are currently on 2.3 and are wanting to upgrade to 2.4. This is a feature that would be highly used.
          Hide
          Rex Lorenzo added a comment -

          Would also like to request that this and MDL-35731 be backported to 2.3.

          Show
          Rex Lorenzo added a comment - Would also like to request that this and MDL-35731 be backported to 2.3.
          Hide
          Jason Fowler added a comment -

          Rex, as a rule, improvements aren't backported to stable branches, doing so has the potential to make them unstable...

          Show
          Jason Fowler added a comment - Rex, as a rule, improvements aren't backported to stable branches, doing so has the potential to make them unstable...

            People

            • Votes:
              4 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: