Details

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.

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.

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.

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.

Petr Skoda
added a comment - 21/Aug/11 9:29 PM 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.

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.

Andrew Davis
added a comment - 02/May/12 7:23 PM - 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.

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?

Sam Hemelryk
added a comment - 07/May/12 12:25 PM 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

Adam Olley
added a comment - 07/May/12 1:12 PM 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.

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.

Andrew Davis
added a comment - 08/May/12 9:55 AM 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.

Andrew Davis
added a comment - 08/May/12 10:01 AM 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);

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.

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.

Andrew Davis
added a comment - 08/May/12 10:24 AM - 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.

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

Andrew Davis
added a comment - 08/May/12 11:04 AM 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

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.

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.

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.

Sam Hemelryk
added a comment - 08/May/12 11:57 AM 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

You'll need two students enrolled in a course and at least one gradable activity. Go to the enrolment screen, edit one of their enrolments and suspend it.

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.

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 and 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.

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).

Andrew Davis
added a comment - 09/May/12 10:23 AM 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).

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 and 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.

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.

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.

Andrew Davis
added a comment - 09/May/12 10:45 AM 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.

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.

Sam Hemelryk
added a comment - 10/May/12 7:08 AM 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

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.

Eloy Lafuente (stronk7)
added a comment - 12/May/12 1:35 AM 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