Moodle
  1. Moodle
  2. MDL-27322

Suspended Students in exported gradebook, indistinguishable from Active Students

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.2, 2.3
    • Fix Version/s: 2.3
    • Component/s: Gradebook
    • Labels:
    • Testing Instructions:
      Hide

      You'll need two students enrolled in a course and at least one gradable activity. The 2 (or more) students and the 1 (or more) activities must have an ID number set. You can set one on the activity settings and the user's profile.

      Go to the enrolment screen, edit one of the student's enrolments and suspend it. You want 1 suspended student and 1 active student.

      Perform two exports in each format (txt, odf, xml etc). One with "require active enrolment" ticked and one with it not ticked. Check that the suspended user is or is not included as appropriate.

      Show
      You'll need two students enrolled in a course and at least one gradable activity. The 2 (or more) students and the 1 (or more) activities must have an ID number set. You can set one on the activity settings and the user's profile. Go to the enrolment screen, edit one of the student's enrolments and suspend it. You want 1 suspended student and 1 active student. Perform two exports in each format (txt, odf, xml etc). One with "require active enrolment" ticked and one with it not ticked. Check that the suspended user is or is not included as appropriate.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-27322_export
    • Rank:
      17015

      Description

      Now that we are able to visually determine which users are suspended and which are active in the gradebook, it'd make sense to have a field (perhaps as a user-selectable option) in the gradebook export feature to be able to export the enrolment status of the student as well.

      For users that like to export their gradebook and fill it in elsewhere before then importing it, this would be really handy.

        Issue Links

          Activity

          Hide
          Michael Blake added a comment -

          This issue has been requested by a MP as it affects their client. Please give it priority.

          Show
          Michael Blake added a comment - This issue has been requested by a MP as it affects their client. Please give it priority.
          Hide
          Petr Škoda added a comment -

          Technically it should be very easy to add extra column with "enrolment status", the only problem might be backwards compatibility with systems that do not expect extra element in XML or column in CSV. Alternatively there could be a separate option to include only active users.

          Show
          Petr Škoda added a comment - Technically it should be very easy to add extra column with "enrolment status", the only problem might be backwards compatibility with systems that do not expect extra element in XML or column in CSV. Alternatively there could be a separate option to include only active users.
          Hide
          Andrew Davis added a comment - - edited

          Adding a branch. It contains changes to make a checkbox appear when you're doing an export. It allows you to restrict the export to users with an active enrolment. I did start down the path of still exporting users with a suspended enrolment but marking them but that turned out to be significantly more complex. Hopefully this is adequate.

          Show
          Andrew Davis added a comment - - edited Adding a branch. It contains changes to make a checkbox appear when you're doing an export. It allows you to restrict the export to users with an active enrolment. I did start down the path of still exporting users with a suspended enrolment but marking them but that turned out to be significantly more complex. Hopefully this is adequate.
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          The code here looks good thanks.
          Just a couple of notes I made:

          1. It would be nice if the new checkbox had a help text associated with it.
          2. I'm not a big fan of adding a new argument to the middle of a list of arguments for a function. Given this is master only, that the final arguments are never used in core, and the arguments are in a sensible order perhaps its fine. I do wonder whether there should be some sort of debugging aid however. Perhaps you could you perform an is_bool check on that argument and if its not a bool reorder the args as original and print a debugging message informing people they need to upgrade their gradeexport plugins and any other uses of the graded_uses_iterator. Actually I'm still not convinced it is a wise idea.

          The other thing I think needs to be considered is this change in relation to this issue.
          It sounds to me like this issue was asking for the enrolment status to be exported along with other user data, and perhaps an option to toggle its export (which would be smart as those with parsing systems could have it off to ensure things worked.
          The change you've made allows users to export only users with active enrolments however which while it may meet the needs is different from what is being asked.
          Perhaps Adam you could let us know whether this would meet your needs given you are the creator of this issue?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, The code here looks good thanks. Just a couple of notes I made: It would be nice if the new checkbox had a help text associated with it. I'm not a big fan of adding a new argument to the middle of a list of arguments for a function. Given this is master only, that the final arguments are never used in core, and the arguments are in a sensible order perhaps its fine. I do wonder whether there should be some sort of debugging aid however. Perhaps you could you perform an is_bool check on that argument and if its not a bool reorder the args as original and print a debugging message informing people they need to upgrade their gradeexport plugins and any other uses of the graded_uses_iterator. Actually I'm still not convinced it is a wise idea. The other thing I think needs to be considered is this change in relation to this issue. It sounds to me like this issue was asking for the enrolment status to be exported along with other user data, and perhaps an option to toggle its export (which would be smart as those with parsing systems could have it off to ensure things worked. The change you've made allows users to export only users with active enrolments however which while it may meet the needs is different from what is being asked. Perhaps Adam you could let us know whether this would meet your needs given you are the creator of this issue? Cheers Sam
          Hide
          Adam Olley added a comment -

          Hi Sam/Andrew,

          While it'd be nice to include the enrolment status in the csv itself, so long as we have a way to export actively enrolled users instead of all users, then that at least is a usable workaround.

          One question however, is default behaviour, I assume it'll be to output all users (since that's the current behaviour).

          Regards,
          Adam.

          Show
          Adam Olley added a comment - Hi Sam/Andrew, While it'd be nice to include the enrolment status in the csv itself, so long as we have a way to export actively enrolled users instead of all users, then that at least is a usable workaround. One question however, is default behaviour, I assume it'll be to output all users (since that's the current behaviour). Regards, Adam.
          Hide
          Andrew Davis added a comment -

          Sam, I've added a help button.

          I'm experimenting with putting the only active flag at the end of the arguments for the grade_user_iterator constructor. Its not pretty either way. If I put it in the middle I'll need to do some data vetting. If I put it on the end everywhere that wants to supply the only active flag will then also have to supply the 4 sort parameters which also seems kind of crappy. That's probably the least crappy option though.

          Admin, yes, by default it will still output all users.

          It would be good to export each student's enrolment status but that turned out to be somewhat complex. I will open a new issue and come back and revisit this as I would like to see that implemented.

          Show
          Andrew Davis added a comment - Sam, I've added a help button. I'm experimenting with putting the only active flag at the end of the arguments for the grade_user_iterator constructor. Its not pretty either way. If I put it in the middle I'll need to do some data vetting. If I put it on the end everywhere that wants to supply the only active flag will then also have to supply the 4 sort parameters which also seems kind of crappy. That's probably the least crappy option though. Admin, yes, by default it will still output all users. It would be good to export each student's enrolment status but that turned out to be somewhat complex. I will open a new issue and come back and revisit this as I would like to see that implemented.
          Hide
          Andrew Davis added a comment -

          Another option would be to pass the only active flag in to init() instead of the constructor. That feels a little weird as everything else goes into the constructor. Here are the 3 options

          New param in the middle

          $gui = new graded_users_iterator($this->course, $this->columns, $this->groupid, $this->onlyactive);
          $gui->init();

          New param at the end

          $gui = new graded_users_iterator($this->course, $this->columns, $this->groupid, 'lastname', 'ASC', 'firstname', 'ASC', $this->onlyactive);
          $gui->init();

          New param supplied to init() instead

          $gui = new graded_users_iterator($this->course, $this->columns, $this->groupid);
          $gui->init($this->onlyactive);
          Show
          Andrew Davis added a comment - Another option would be to pass the only active flag in to init() instead of the constructor. That feels a little weird as everything else goes into the constructor. Here are the 3 options New param in the middle $gui = new graded_users_iterator($ this ->course, $ this ->columns, $ this ->groupid, $ this ->onlyactive); $gui->init(); New param at the end $gui = new graded_users_iterator($ this ->course, $ this ->columns, $ this ->groupid, 'lastname', 'ASC', 'firstname', 'ASC', $ this ->onlyactive); $gui->init(); New param supplied to init() instead $gui = new graded_users_iterator($ this ->course, $ this ->columns, $ this ->groupid); $gui->init($ this ->onlyactive);
          Hide
          Andrew Davis added a comment - - edited

          For the sake of completeness (and to remind myself) here is why exporting the actual enrolment status is annoyingly difficult.

          graded_user_iterator calls get_enrolled_sql() which does not include the enrolment status. I could probably add it there but that would involve it being returned for thousands of queries that dont need it. As an added wrinkle a user can potentially have multiple enrolments in a course.

          The graded_user_iterator then uses $DB->get_recordset_sql() to retrieve the users. Afaik because its using get_recordset_sql(), which returns an iterator, instead of get_records(), which returns an array, we cant do something like the following. This is what the grader report is doing. Note how it grabs all the users IDs in the first line using array_keys() then runs a single query.

          list($usql, $uparams) = $DB->get_in_or_equal(array_keys($this->users), SQL_PARAMS_NAMED, 'usid0');
          ...
                      //add a flag to each user indicating whether their enrolment is active
                      $sql = "SELECT ue.userid
                                FROM {user_enrolments} ue
                                JOIN {enrol} e ON e.id = ue.enrolid
                               WHERE ue.userid $usql
                                     AND ue.status = :uestatus
                                     AND e.status = :estatus
                                     AND e.courseid = :courseid
                            GROUP BY ue.userid";
                      $coursecontext = get_course_context($this->context);
                      $params = array_merge($uparams, array('estatus'=>ENROL_INSTANCE_ENABLED, 'uestatus'=>ENROL_USER_ACTIVE, 'courseid'=>$coursecontext->instanceid));
                      $useractiveenrolments = $DB->get_records_sql($sql, $params);
          
                      foreach ($this->users as $user) {
                          $this->users[$user->id]->suspendedenrolment = !array_key_exists($user->id, $useractiveenrolments);
                      }
          

          The exporters have a recordset object so would have to either iterate over each user querying the DB for the enrolment status individually or iterate over the users, build an array, run the query, then reset the recordset. Maybe that wouldnt be so bad.

          Show
          Andrew Davis added a comment - - edited For the sake of completeness (and to remind myself) here is why exporting the actual enrolment status is annoyingly difficult. graded_user_iterator calls get_enrolled_sql() which does not include the enrolment status. I could probably add it there but that would involve it being returned for thousands of queries that dont need it. As an added wrinkle a user can potentially have multiple enrolments in a course. The graded_user_iterator then uses $DB->get_recordset_sql() to retrieve the users. Afaik because its using get_recordset_sql(), which returns an iterator, instead of get_records(), which returns an array, we cant do something like the following. This is what the grader report is doing. Note how it grabs all the users IDs in the first line using array_keys() then runs a single query. list($usql, $uparams) = $DB->get_in_or_equal(array_keys($ this ->users), SQL_PARAMS_NAMED, 'usid0'); ... //add a flag to each user indicating whether their enrolment is active $sql = "SELECT ue.userid FROM {user_enrolments} ue JOIN {enrol} e ON e.id = ue.enrolid WHERE ue.userid $usql AND ue.status = :uestatus AND e.status = :estatus AND e.courseid = :courseid GROUP BY ue.userid"; $coursecontext = get_course_context($ this ->context); $params = array_merge($uparams, array('estatus'=>ENROL_INSTANCE_ENABLED, 'uestatus'=>ENROL_USER_ACTIVE, 'courseid'=>$coursecontext->instanceid)); $useractiveenrolments = $DB->get_records_sql($sql, $params); foreach ($ this ->users as $user) { $ this ->users[$user->id]->suspendedenrolment = !array_key_exists($user->id, $useractiveenrolments); } The exporters have a recordset object so would have to either iterate over each user querying the DB for the enrolment status individually or iterate over the users, build an array, run the query, then reset the recordset. Maybe that wouldnt be so bad.
          Hide
          Andrew Davis added a comment -

          Actually, there appears to be no way to reset the recordset so we'd have to iterate over it to get user IDs, query the DB to get enrolment statuses then re-call init() on the recordset, essentially throw it away and start over

          Show
          Andrew Davis added a comment - Actually, there appears to be no way to reset the recordset so we'd have to iterate over it to get user IDs, query the DB to get enrolment statuses then re-call init() on the recordset, essentially throw it away and start over
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          Having now read over all of the comments here are my thoughts.

          Certainly it sounds like everyone is for including the enrolment status in the exported data as well as having a setting to do so (as Petr points out to avoid problems with other existing programs).
          I also think that your present solution in regards to exporting only users with active enrolments is a good idea.
          So two good ideas and presently one solution.

          So in regards to your solution for exporting active enrolments:
          I think if were going to have just this solution then making onlyactive the forth argument is the way to go, and that __construct should be tolerant of it being a string and reordering args and printing a debug message.
          However if we are going to have two new arguments, one to export users with active enrolments, and another to export enrolment status for each user then perhaps there is one more approach that should be considered.

          $gui = new graded_users_iterator($this->course, $this->columns, $this->groupid);
          $gui->include_active_enrolments_only($default = true);
          $gui->include_enrolment_status($default = true);
          

          That way we avoid adding more arguments and cluttering the constructor, and we set a precedence for adding new features to the iterator in the future. We could also convert current args 4 ~ 7 to use methods like these and reduce the complexity of the constructor as well (given they're not used in core that may be a good idea).

          In regards to the second idea about exporting enrolment status I have a thought on how that might be done.
          Assuming you implement a solution like the above adding a method consider the following code.

          class graded_users_iterator {
              protected $includeenrolmentstatus = false;
              protected $activelyenrolledusers = null;
              public function include_enrolment_status($default = true) {
                  $this->includeenrolmentstatus = $default;
              }
              public function init() {
                  // ... current code here
                  if ($this->includeenrolmentstatus && !$this->userswithactiveenrolmentonly) {
                      list($enrolledsql, $enrolledparams) = get_enrolled_sql($coursecontext);
                      $usersrs = $DB->get_recordset_sql($enrolledsql, $enrolledparams);
                      $this->activelyenrolledusers = array();
                      foreach ($usersrs as $user) {
                          $this->activelyenrolledusers[] = $user;
                      }
                      $usersrs->close();
                  }
              }
              public function next_user() {
                  // ...somewhere in the middle...
                  if ($this->includeenrolmentstatus) {
                      if ($this->userswithactiveenrolmentonly || in_array($user->id, $this->activelyenrolledusers)) {
                          $user->enrolmentstatus = 'active';
                      } else {
                          $user->enrolmentstatus = 'inactive';
                      }
                  }   
              }
          }
          

          It is using a separate query to get the user ids of all actively enrolled users if required and then checking it if we need to include enrolment status.

          Anyway, back to this particular issue I think it would be great to see at least solution 1 land before 2.3 release and we can always create a separate issue for the inclusion of enrolment status, however I think the solution for the first idea should factor in the upcoming second idea.
          Or you can solve them both in this issue.

          I will leave that part up to you.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Having now read over all of the comments here are my thoughts. Certainly it sounds like everyone is for including the enrolment status in the exported data as well as having a setting to do so (as Petr points out to avoid problems with other existing programs). I also think that your present solution in regards to exporting only users with active enrolments is a good idea. So two good ideas and presently one solution. So in regards to your solution for exporting active enrolments: I think if were going to have just this solution then making onlyactive the forth argument is the way to go, and that __construct should be tolerant of it being a string and reordering args and printing a debug message. However if we are going to have two new arguments, one to export users with active enrolments, and another to export enrolment status for each user then perhaps there is one more approach that should be considered. $gui = new graded_users_iterator($ this ->course, $ this ->columns, $ this ->groupid); $gui->include_active_enrolments_only($ default = true ); $gui->include_enrolment_status($ default = true ); That way we avoid adding more arguments and cluttering the constructor, and we set a precedence for adding new features to the iterator in the future. We could also convert current args 4 ~ 7 to use methods like these and reduce the complexity of the constructor as well (given they're not used in core that may be a good idea). In regards to the second idea about exporting enrolment status I have a thought on how that might be done. Assuming you implement a solution like the above adding a method consider the following code. class graded_users_iterator { protected $includeenrolmentstatus = false ; protected $activelyenrolledusers = null ; public function include_enrolment_status($ default = true ) { $ this ->includeenrolmentstatus = $ default ; } public function init() { // ... current code here if ($ this ->includeenrolmentstatus && !$ this ->userswithactiveenrolmentonly) { list($enrolledsql, $enrolledparams) = get_enrolled_sql($coursecontext); $usersrs = $DB->get_recordset_sql($enrolledsql, $enrolledparams); $ this ->activelyenrolledusers = array(); foreach ($usersrs as $user) { $ this ->activelyenrolledusers[] = $user; } $usersrs->close(); } } public function next_user() { // ...somewhere in the middle... if ($ this ->includeenrolmentstatus) { if ($ this ->userswithactiveenrolmentonly || in_array($user->id, $ this ->activelyenrolledusers)) { $user->enrolmentstatus = 'active'; } else { $user->enrolmentstatus = 'inactive'; } } } } It is using a separate query to get the user ids of all actively enrolled users if required and then checking it if we need to include enrolment status. Anyway, back to this particular issue I think it would be great to see at least solution 1 land before 2.3 release and we can always create a separate issue for the inclusion of enrolment status, however I think the solution for the first idea should factor in the upcoming second idea. Or you can solve them both in this issue. I will leave that part up to you. Cheers Sam
          Hide
          Andrew Davis added a comment -

          I've added a new function to set the "only active" flag rather than altering the constructor. I've also created and linked 2 new MDLs to do additional work (allow exporting of enrolment status and general refactoring).

          Show
          Andrew Davis added a comment - I've added a new function to set the "only active" flag rather than altering the constructor. I've also created and linked 2 new MDLs to do additional work (allow exporting of enrolment status and general refactoring).
          Hide
          Andrew Davis added a comment -

          Putting this up for peer review. Note that the help button on the export form is non-functional for an as yet unknown reason. If you open the help link in a new tab you get the help text but the ajaxy overlay help thing stubbornly refuses to appear.

          Show
          Andrew Davis added a comment - Putting this up for peer review. Note that the help button on the export form is non-functional for an as yet unknown reason. If you open the help link in a new tab you get the help text but the ajaxy overlay help thing stubbornly refuses to appear.
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew, changes look good once more.
          Couple of small things I think should be fixed up before this goes up for integration:

          1. graded_users_iterator::onlyactive needs to be defaulted to false, and should probably be made protected so that users are forced to use require_only_active just in case we need to do more processing in there in the future.
          2. debugging call in require_active_enrolment should probably be using DEBUG_DEVELOPER given its likely developer triggered and the message references code.
          3. Probably should prefix public to require_active_enrolment to make it explicit, despite other functions not being explicit its always advantageous and part of the coding style to define the scope of new methods.

          Otherwise spot on!

          One more note actually MDL-32864 could those who want enrolment status included in exported data please vote on that issue. It may pay to bump up the priority there Andrew to reflect that there is a plan, and mark the issue as a partner issue.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, changes look good once more. Couple of small things I think should be fixed up before this goes up for integration: graded_users_iterator::onlyactive needs to be defaulted to false, and should probably be made protected so that users are forced to use require_only_active just in case we need to do more processing in there in the future. debugging call in require_active_enrolment should probably be using DEBUG_DEVELOPER given its likely developer triggered and the message references code. Probably should prefix public to require_active_enrolment to make it explicit, despite other functions not being explicit its always advantageous and part of the coding style to define the scope of new methods. Otherwise spot on! One more note actually MDL-32864 could those who want enrolment status included in exported data please vote on that issue. It may pay to bump up the priority there Andrew to reflect that there is a plan, and mark the issue as a partner issue. Cheers Sam
          Hide
          Andrew Davis added a comment -

          Ok, I've fixed that all and added the labels to MDL-32864 and increased its priority.

          Show
          Andrew Davis added a comment - Ok, I've fixed that all and added the labels to MDL-32864 and increased its priority.
          Hide
          Sam Hemelryk added a comment -

          Cool, put it up for integration when you're ready then

          Show
          Sam Hemelryk added a comment - Cool, put it up for integration when you're ready then
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Dan Poltawski added a comment -

          Thanks Andrew i've integrated this now.

          Show
          Dan Poltawski added a comment - Thanks Andrew i've integrated this now.
          Hide
          Frédéric Massart added a comment -

          Successfully tested on master

          Show
          Frédéric Massart added a comment - Successfully tested on master
          Hide
          Eloy Lafuente (stronk7) added a comment -

          U P S T R E A M I Z E D !

          Many thanks for the hard work, closing this as fixed.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks for the hard work, closing this as fixed. Ciao

            People

            • Votes:
              5 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: