Moodle
  1. Moodle
  2. MDL-36024

Allow grade recovery for multiple users

    Details

    • Testing Instructions:
      Hide

      To test this you will need two courses. Course A should have some manually-enrolled students. Course B should have a course meta link with Course A. Students in both courses will need to have some grades. Set External unenrol action in Site administration > Plugins > Enrolments > Course meta link to "Unenrol user from course."

      Manual grade recovery

      1. Set Recover Grades Default to true.
      2. Un-enrol a student (with grades) from Course A.
      3. Manually re-enrol the student. Verify in the interface that the box to recover grades is checked and leave it checked.
      4. Verify that the student's grades returned.
      5. Un-enrol the same student from Course A.
      6. Manually re-enrol the student but uncheck the recover grades box.
      7. Verify that the student's grades did not return.
      8. Set "Recover Grades Default" to false.
      9. Un-enrol a student (with grades) from Course A.
      10. Manually re-enrol the student. Verify in the interface that the box to recover grades is un-checked and check it.
      11. Verify that the student's grades returned.
      12. Un-enrol the same student from Course A.
      13. Manually re-enrol the student. Verify in the interface that the box to recover grades is un-checked.
      14. Verify that the student's grades did not return.

      Course Metalink

      1. Set Recover Grades Default to true.
      2. Select a student in Course B and make sure that student has grades in Course B.
      3. Un-enrol the student from Course A and verify that the student was un-enroled from Course B.
      4. Re-enrol the student in Course A.
      5. Verify that the student's grades returned in Course B.
      6. Set Recover Grades Default to false.
      7. Un-enrol the student from Course A and verify that the student was un-enroled from Course B.
      8. Re-enrol the student in Course A.
      9. Verify that the student's grades did not return in Course B.
      Show
      To test this you will need two courses. Course A should have some manually-enrolled students. Course B should have a course meta link with Course A. Students in both courses will need to have some grades. Set External unenrol action in Site administration > Plugins > Enrolments > Course meta link to "Unenrol user from course." Manual grade recovery Set Recover Grades Default to true. Un-enrol a student (with grades) from Course A. Manually re-enrol the student. Verify in the interface that the box to recover grades is checked and leave it checked. Verify that the student's grades returned. Un-enrol the same student from Course A. Manually re-enrol the student but uncheck the recover grades box. Verify that the student's grades did not return. Set "Recover Grades Default" to false. Un-enrol a student (with grades) from Course A. Manually re-enrol the student. Verify in the interface that the box to recover grades is un-checked and check it. Verify that the student's grades returned. Un-enrol the same student from Course A. Manually re-enrol the student. Verify in the interface that the box to recover grades is un-checked. Verify that the student's grades did not return. Course Metalink Set Recover Grades Default to true. Select a student in Course B and make sure that student has grades in Course B. Un-enrol the student from Course A and verify that the student was un-enroled from Course B. Re-enrol the student in Course A. Verify that the student's grades returned in Course B. Set Recover Grades Default to false. Un-enrol the student from Course A and verify that the student was un-enroled from Course B. Re-enrol the student in Course A. Verify that the student's grades did not return in Course B.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36024-master
    • Rank:
      44784

      Description

      When you add a user manually, you have the possibility to 'recover old grades'. This only seems to work when you add users manually.

      Since our upgrade (1.9 -> 2.3) users are no longer added to a course manually, but we use either sitegroup or metacourse. This results in massive loss of old grades.

      We have now turned 'recover all grades' on by default, but still we can not see the oldgrades in the gradebook. If we remove a user from the course, add them manually to the course with 'recover old grades' checked, the grades reappear.

      I file this as a bug. What we need is a 'recover old grades' for the entire website, or for more then one course.

      1. grader_report.png
        63 kB
      2. overall_grade_enrol.png
        50 kB
      3. overall_grade_unenrol.png
        46 kB
      4. student_courseB_unenrol.png
        42 kB
      5. student_summary.png
        53 kB

        Issue Links

          Activity

          Hide
          Richard van Iwaarden added a comment -

          This bug has priority 'Minor' which says 'workaround possible'.

          Can someone tell me how to work around this problem? We are experiencing massive loss of grades for thousants of students for 2 months now. If a workaround is possible I would like to know about it?

          Show
          Richard van Iwaarden added a comment - This bug has priority 'Minor' which says 'workaround possible'. Can someone tell me how to work around this problem? We are experiencing massive loss of grades for thousants of students for 2 months now. If a workaround is possible I would like to know about it?
          Hide
          Richard van Iwaarden added a comment - - edited

          Anyone? It is kind of a big problem when you loose hundreds of grades in your system and there's no one who can help you out. This particular problem seems to be the end of Moodle for some schools that I'm running Moodle for. I can't blame them: losing grades in your system is not acceptable.

          Show
          Richard van Iwaarden added a comment - - edited Anyone? It is kind of a big problem when you loose hundreds of grades in your system and there's no one who can help you out. This particular problem seems to be the end of Moodle for some schools that I'm running Moodle for. I can't blame them: losing grades in your system is not acceptable.
          Hide
          Charles Fulton added a comment -

          Hi Richard, what's your enrollment scenario that you're losing so many grades? Some enrollment methods (like LMB) include this feature already. Are students unenrolling themselves by accident?

          Show
          Charles Fulton added a comment - Hi Richard, what's your enrollment scenario that you're losing so many grades? Some enrollment methods (like LMB) include this feature already. Are students unenrolling themselves by accident?
          Hide
          Richard van Iwaarden added a comment - - edited

          We use sitegroups to enroll our students in one course. That one course is the parent of many child courses. So we use sitegroups and metacourses for enrollment.

          We must be sure that if a student unenrolls from a course this way and re-enrolls later, the grades should be intact. I don't see the point in Moodle if the grades get lost this way - I don't understand what's the benefit. In Moodle 1.9 this never happened.

          Show
          Richard van Iwaarden added a comment - - edited We use sitegroups to enroll our students in one course. That one course is the parent of many child courses. So we use sitegroups and metacourses for enrollment. We must be sure that if a student unenrolls from a course this way and re-enrolls later, the grades should be intact. I don't see the point in Moodle if the grades get lost this way - I don't understand what's the benefit. In Moodle 1.9 this never happened.
          Hide
          Charles Fulton added a comment -

          See MDL-25718 for the discussion about why it was implemented this ay. I believe performance was a major concern. When you say site groups, do you mean cohorts? How do the students get added to them, and how do they get removed?

          Show
          Charles Fulton added a comment - See MDL-25718 for the discussion about why it was implemented this ay. I believe performance was a major concern. When you say site groups, do you mean cohorts? How do the students get added to them, and how do they get removed?
          Hide
          Richard van Iwaarden added a comment -

          Yes, I do mean cohorts, in my language it's translated as site groups. But it's the same thing.

          All users got unenrolled during our 1.9 -> 2.2 -> 2.3 upgrade this summer. This had to do with the fact that we could no longer use metacourses for our enrollment system and had to switch to sitegroups/cohorts. So they were all unenrolled (metacourses) during summer holiday and re-enrolled 2 days later (sitegroups/cohorts)

          Our Moodle is connected to Microsoft Active directory for filling cohorts/sitegroups. Students can switch a couple of times during their study from study program. This means they get unenrolled en re-enrolled to some courses.

          Basically: we just don't want Moodle to forget grades, or have an option to retrieve grades. It has been made with manual enrollment to retrieve the grades, why not by the automatic re-enrollment? So it might take performance, but when you have 5000 students enrolled in 8000 courses you don't want this manually done!

          Show
          Richard van Iwaarden added a comment - Yes, I do mean cohorts, in my language it's translated as site groups. But it's the same thing. All users got unenrolled during our 1.9 -> 2.2 -> 2.3 upgrade this summer. This had to do with the fact that we could no longer use metacourses for our enrollment system and had to switch to sitegroups/cohorts. So they were all unenrolled (metacourses) during summer holiday and re-enrolled 2 days later (sitegroups/cohorts) Our Moodle is connected to Microsoft Active directory for filling cohorts/sitegroups. Students can switch a couple of times during their study from study program. This means they get unenrolled en re-enrolled to some courses. Basically: we just don't want Moodle to forget grades, or have an option to retrieve grades. It has been made with manual enrollment to retrieve the grades, why not by the automatic re-enrollment? So it might take performance, but when you have 5000 students enrolled in 8000 courses you don't want this manually done!
          Hide
          Tabitha Roder added a comment -

          We are missing some grades. We wrote an enrolment script (Peoplesoft users). We are missing grades in a number of courses, for both quizzes and assignment types (so far found single upload file assignment issue). We upgraded from 1.9.7 and are now on 2.2.4. All the users with issues were existing users before the upgrade.
          This is a critical issue for us as the repercussions for grading errors can result in student fails and student complaints.

          Show
          Tabitha Roder added a comment - We are missing some grades. We wrote an enrolment script (Peoplesoft users). We are missing grades in a number of courses, for both quizzes and assignment types (so far found single upload file assignment issue). We upgraded from 1.9.7 and are now on 2.2.4. All the users with issues were existing users before the upgrade. This is a critical issue for us as the repercussions for grading errors can result in student fails and student complaints.
          Hide
          Richard van Iwaarden added a comment -

          Thanks Andrew, for changing the priority to major.

          Show
          Richard van Iwaarden added a comment - Thanks Andrew, for changing the priority to major.
          Hide
          Richard van Iwaarden added a comment - - edited

          Tabitha wrote: This is a critical issue for us as the repercussions for grading errors can result in student fails and student complaints.

          It's starting to become critical here too. Management starts talking about leaving Moodle and I can't blame them. It hink we lost the grades of 2000 students. Try explaining this to teachers, parents, students. How are we going to give these students diplomas?

          Moodle has failed hard this time.

          Show
          Richard van Iwaarden added a comment - - edited Tabitha wrote: This is a critical issue for us as the repercussions for grading errors can result in student fails and student complaints. It's starting to become critical here too. Management starts talking about leaving Moodle and I can't blame them. It hink we lost the grades of 2000 students. Try explaining this to teachers, parents, students. How are we going to give these students diplomas? Moodle has failed hard this time.
          Hide
          Lindy Klein added a comment -

          We have a client who is experiencing a similar issue, and it certainly gets my vote for a fix BUT:

          It's worth noting that Moodle is an online learning environment first and foremost. It wasn't built to do student information management, so when a student unenrols, it cleans up the space for the next student.

          ALSO: What you see through Moodle's interface is not necessarily the totality of the information Moodle has stored in its database. I'm not a programmer, but my understanding is Moodle has all the grades information in its tables for each student - so that it can recover grades on re-enrolment. What's missing is the method for getting the information to the interface, not the information itself. Could someone with a little more database understanding help confirm this one way or the other please?

          Show
          Lindy Klein added a comment - We have a client who is experiencing a similar issue, and it certainly gets my vote for a fix BUT: It's worth noting that Moodle is an online learning environment first and foremost. It wasn't built to do student information management, so when a student unenrols, it cleans up the space for the next student. ALSO: What you see through Moodle's interface is not necessarily the totality of the information Moodle has stored in its database. I'm not a programmer, but my understanding is Moodle has all the grades information in its tables for each student - so that it can recover grades on re-enrolment. What's missing is the method for getting the information to the interface, not the information itself. Could someone with a little more database understanding help confirm this one way or the other please?
          Hide
          Andrew Davis added a comment -

          Lindy, you are correct in that the grade information should all be there in the grade history tables. Moodle just isn't currently using that stored information to its fullest.

          We should certainly look at making more use of it. I'm slightly wary of routinely relying on "undeleted" grades as any flaw in that process is likely to be subtle and hard to spot. I guess that just comes down to thoroughness of testing.

          Show
          Andrew Davis added a comment - Lindy, you are correct in that the grade information should all be there in the grade history tables. Moodle just isn't currently using that stored information to its fullest. We should certainly look at making more use of it. I'm slightly wary of routinely relying on "undeleted" grades as any flaw in that process is likely to be subtle and hard to spot. I guess that just comes down to thoroughness of testing.
          Hide
          Lindy Klein added a comment -

          Andrew, thanks for the confirmation

          I don't understand what you're referring to with "undeleted" grades - is this the grades information in the relevant Moodle table/s being flagged as "deleted" so it doesn't show in the interface (and vice versa)?

          I'm sorry for my lack of coding expertise here, but what I would envisage is a check when a student enrols into a course, for whether they've been there before, and if they have, should the grades information be repopulated with the last known grades. I would think this should be able to be turned on/off for each enrolment method, but at the moment, I'd focus on the Moodle only enrolment methods - cohorts, self-enrol and csv (bearing in mind manual individual enrolment already has a method).

          Our client has indicated a preference for self-enrolment. I'm also thinking csv would be good, as perhaps this could be included as a yes/no field per course enrolment, ie course1, role1, recover1.

          I don't understand the coding well enough to know what's involved in this fix, but am happy to help where I can.

          Show
          Lindy Klein added a comment - Andrew, thanks for the confirmation I don't understand what you're referring to with "undeleted" grades - is this the grades information in the relevant Moodle table/s being flagged as "deleted" so it doesn't show in the interface (and vice versa)? I'm sorry for my lack of coding expertise here, but what I would envisage is a check when a student enrols into a course, for whether they've been there before, and if they have, should the grades information be repopulated with the last known grades. I would think this should be able to be turned on/off for each enrolment method, but at the moment, I'd focus on the Moodle only enrolment methods - cohorts, self-enrol and csv (bearing in mind manual individual enrolment already has a method). Our client has indicated a preference for self-enrolment. I'm also thinking csv would be good, as perhaps this could be included as a yes/no field per course enrolment, ie course1, role1, recover1. I don't understand the coding well enough to know what's involved in this fix, but am happy to help where I can.
          Hide
          Andrew Davis added a comment -

          Currently all grading events are recorded in the grade history tables. With manual enrolment what we're doing now, from memory, is reconstructing grades based on that history. That's what I meant when I said undeleting.

          Show
          Andrew Davis added a comment - Currently all grading events are recorded in the grade history tables. With manual enrolment what we're doing now, from memory, is reconstructing grades based on that history. That's what I meant when I said undeleting.
          Hide
          Scott Krajewski added a comment -

          I will add that we are experiencing this as well (we run database enrollment plugin). We have often found that doing an override on a grade entry, saving, and then removing the override and saving (or else locking and unlocking the grade) makes the grade re-appear in most cases.

          Show
          Scott Krajewski added a comment - I will add that we are experiencing this as well (we run database enrollment plugin). We have often found that doing an override on a grade entry, saving, and then removing the override and saving (or else locking and unlocking the grade) makes the grade re-appear in most cases.
          Hide
          Alan Kmiecik added a comment -

          I have numerous instances of grades being dropped from grade book in Moodle 2.3. Have been seeing it for months now but can not put any kind of cause with it. This function could help restore.

          Also, have hundreds of unenroled students that need re-enroled. Flat File plugin kind of works but does not restore grades (even if restore grades option is turned on).

          Need function mentioned here (https://moodle.org/mod/forum/discuss.php?d=213256#p945955) but that "fix" causes error.

          Given the drain caused by student inquiry as to where their grades are, I would rate this a 'critical' need.

          Thanks

          Show
          Alan Kmiecik added a comment - I have numerous instances of grades being dropped from grade book in Moodle 2.3. Have been seeing it for months now but can not put any kind of cause with it. This function could help restore. Also, have hundreds of unenroled students that need re-enroled. Flat File plugin kind of works but does not restore grades (even if restore grades option is turned on). Need function mentioned here ( https://moodle.org/mod/forum/discuss.php?d=213256#p945955 ) but that "fix" causes error. Given the drain caused by student inquiry as to where their grades are, I would rate this a 'critical' need. Thanks
          Hide
          Scott Krajewski added a comment -

          Here's something I'm testing right now with database enrollment. So far so good:
          in
          enrol/database/lib.php
          in
          public function sync_enrolments
          add just after globals (we need the grade function loaded)

          require_once($CFG->libdir.'/gradelib.php');

          then about line 500 in the block commented as
          // enrol all users and sync roles
          add
          if (grade_recover_history_grades($userid,$course->id)) {
          if ($verbose)

          { mtrace(" recovering grades: $userid ==> $course->shortname"); }

          }

          just before
          if ($verbose)

          { mtrace(" enrolling: $userid ==> $course->shortname as ".$allroles[$roleid]->shortname); }

          If you do something similar for flat file in private function process_records I bet it'll work right after this->enrol_user. Don't forget to get the gradelib.php in the function.

          Show
          Scott Krajewski added a comment - Here's something I'm testing right now with database enrollment. So far so good: in enrol/database/lib.php in public function sync_enrolments add just after globals (we need the grade function loaded) require_once($CFG->libdir.'/gradelib.php'); then about line 500 in the block commented as // enrol all users and sync roles add if (grade_recover_history_grades($userid,$course->id)) { if ($verbose) { mtrace(" recovering grades: $userid ==> $course->shortname"); } } just before if ($verbose) { mtrace(" enrolling: $userid ==> $course->shortname as ".$allroles[$roleid]->shortname); } If you do something similar for flat file in private function process_records I bet it'll work right after this->enrol_user. Don't forget to get the gradelib.php in the function.
          Hide
          Richard van Iwaarden added a comment -

          But does that mean you have to unrole all students first, and re-enrol them to pick up these changes?

          Show
          Richard van Iwaarden added a comment - But does that mean you have to unrole all students first, and re-enrol them to pick up these changes?
          Hide
          Luis de Vasconcelos added a comment -

          What kind or grades have gone missing - for what activities? If you have missing grades from the quiz module what happens if you do a regrade for the affected quiz? Do the grades reappear in the gradebook after the regrade?

          Show
          Luis de Vasconcelos added a comment - What kind or grades have gone missing - for what activities? If you have missing grades from the quiz module what happens if you do a regrade for the affected quiz? Do the grades reappear in the gradebook after the regrade?
          Hide
          Alan Kmiecik added a comment -

          Do (did) I have to un-enrol and then re-enrol? No. Usually a manual re-enrol worked.

          What kind of grades were missing? Quizzes and on-line text.

          Did not have to do a regrade. The scores were still in the system. Most recovered with re-enrol. Clicking on students activity in grade book showed quizzed and on-line text grade/feedback.

          Show
          Alan Kmiecik added a comment - Do (did) I have to un-enrol and then re-enrol? No. Usually a manual re-enrol worked. What kind of grades were missing? Quizzes and on-line text. Did not have to do a regrade. The scores were still in the system. Most recovered with re-enrol. Clicking on students activity in grade book showed quizzed and on-line text grade/feedback.
          Hide
          Richard van Iwaarden added a comment - - edited

          I can not manually re-enrol students... if I start now it will take untill my retirement to complete this task.

          Show
          Richard van Iwaarden added a comment - - edited I can not manually re-enrol students... if I start now it will take untill my retirement to complete this task.
          Hide
          Scott Krajewski added a comment -

          @Richard - this fix only applies when enrollment is done like when the database plugin updates enrollment. It won't help students with missing grades now. We've been re-entering grades from the grade history table when a missing is reported to us.

          @Alan - it's anything and everything, quizzes, gradebook items, assignments. Often editing the grade and turning the lock or override on then off again makes the grade re-appear. I have multiple students that are being dropped and re-added to courses which results in grades vanishing. I am looking for the cause. It has not been the database enrollment plugin that has dropped them (everything is logged) but the plugin does re-enroll them because they are not enrolled but should be. I've got database triggers going so I can see what is happening to the enrollment table.

          Show
          Scott Krajewski added a comment - @Richard - this fix only applies when enrollment is done like when the database plugin updates enrollment. It won't help students with missing grades now. We've been re-entering grades from the grade history table when a missing is reported to us. @Alan - it's anything and everything, quizzes, gradebook items, assignments. Often editing the grade and turning the lock or override on then off again makes the grade re-appear. I have multiple students that are being dropped and re-added to courses which results in grades vanishing. I am looking for the cause. It has not been the database enrollment plugin that has dropped them (everything is logged) but the plugin does re-enroll them because they are not enrolled but should be. I've got database triggers going so I can see what is happening to the enrollment table.
          Hide
          Scott Krajewski added a comment -

          I'll add that you'll need to add this to public function sync_user_enrolments($user) as well.
          add both
          require_once($CFG->libdir.'/gradelib.php');
          after the globals and
          grade_recover_history_grades($user->id,$courseid);
          around line 219 in the foreach labelled // enrol user into courses and sync roles

          Show
          Scott Krajewski added a comment - I'll add that you'll need to add this to public function sync_user_enrolments($user) as well. add both require_once($CFG->libdir.'/gradelib.php'); after the globals and grade_recover_history_grades($user->id,$courseid); around line 219 in the foreach labelled // enrol user into courses and sync roles
          Hide
          Hubert Simms added a comment -

          We are using Moodle 2.3.4 and just this weekend we had a major issue as outlined here. We are using the database sync. plugin to sync. enrollments from at table in MSSQL. On Saturday the script ran at the scheduled time and there was a problem with the source table. Then anyone that logged in, did not get access to any courses. Once the issue was fixed on the MSSQL side of things, we are finding that anyone that logged in during the two hour time, lost the grades from the gradebook.
          Removing them from the course and adding them seems to bring the grades back. However, it seems that the issue was with 500+ students times 5+ courses which makes it tedious and not practical.

          Can anyone confirm that Scott's solution above actually works? We are getting desperate.

          Show
          Hubert Simms added a comment - We are using Moodle 2.3.4 and just this weekend we had a major issue as outlined here. We are using the database sync. plugin to sync. enrollments from at table in MSSQL. On Saturday the script ran at the scheduled time and there was a problem with the source table. Then anyone that logged in, did not get access to any courses. Once the issue was fixed on the MSSQL side of things, we are finding that anyone that logged in during the two hour time, lost the grades from the gradebook. Removing them from the course and adding them seems to bring the grades back. However, it seems that the issue was with 500+ students times 5+ courses which makes it tedious and not practical. Can anyone confirm that Scott's solution above actually works? We are getting desperate.
          Hide
          Scott Krajewski added a comment -

          We are running this successfully on our production server with ~4000 students.

          I don't think I mentioned it here yet, but the root of the problem for us was that random students were logging in during the 45 second window when our table was updating with SIS enrollment data. If they were initiating a login in that tiny window they would not see their courses (and would be unenrolled as a sync was triggered by the login process). They could log out and log back in to trigger another sync but the grades would be gone. We've tracked cases of these since I made the change and grades are OK.

          We refresh the table hourly by the way.

          Show
          Scott Krajewski added a comment - We are running this successfully on our production server with ~4000 students. I don't think I mentioned it here yet, but the root of the problem for us was that random students were logging in during the 45 second window when our table was updating with SIS enrollment data. If they were initiating a login in that tiny window they would not see their courses (and would be unenrolled as a sync was triggered by the login process). They could log out and log back in to trigger another sync but the grades would be gone. We've tracked cases of these since I made the change and grades are OK. We refresh the table hourly by the way.
          Hide
          Scott Krajewski added a comment -

          @Hubert – I'll add a suggestion that you insert a sanity check in your updating of the enrollment table. If we get no SIS data or get a set of SIS data that has a significantly different number of rows we do not update and log the event. Since we update hourly the delta in rows is moderate so any major delta in rows suggests likely an issue. Worst case we cry wolf and have older data until I can look into it.

          Show
          Scott Krajewski added a comment - @Hubert – I'll add a suggestion that you insert a sanity check in your updating of the enrollment table. If we get no SIS data or get a set of SIS data that has a significantly different number of rows we do not update and log the event. Since we update hourly the delta in rows is moderate so any major delta in rows suggests likely an issue. Worst case we cry wolf and have older data until I can look into it.
          Hide
          Charles Fulton added a comment -

          I'm going to throw a patch into the mix here: this extends the existing manual grade recovery behavior to all core enrol plugins by making it part of enrol_user (I've modified the manual ajax code to use this as well). In my own limited testing with a course meta link it behaved as expected.

          Show
          Charles Fulton added a comment - I'm going to throw a patch into the mix here: this extends the existing manual grade recovery behavior to all core enrol plugins by making it part of enrol_user (I've modified the manual ajax code to use this as well). In my own limited testing with a course meta link it behaved as expected.
          Hide
          Robert Puffer added a comment -

          This is such a pita. I hope it can be included in the next release.

          Show
          Robert Puffer added a comment - This is such a pita. I hope it can be included in the next release.
          Hide
          Adam Olley added a comment -

          +1 on Charles patch from me (except for the uppercased NULL, but the function already had uppercased null's in its signature, so can't really fault it for that).

          Show
          Adam Olley added a comment - +1 on Charles patch from me (except for the uppercased NULL, but the function already had uppercased null's in its signature, so can't really fault it for that).
          Hide
          Jean-Philippe Gaudreau added a comment -

          +1 on the patch too. Charles, are you able to take the lead on this task and assign it to yourself since your patch looks great ?

          If you do, here are some minor things that I think should be modified :

          • enrol/manual/ajax.php
            • Line 39 : replace NULL for null (coding style in lowercases)
          • lib/enrollib.php
            • Rename parameter $grades for $recovergrades as it would make more sense.
            • Line 1250 : replace NULL for null
            • Line 1264 : Check should be made on "!isset" instead of "empty" because false value is considered to be empty (see http://php.net/manual/en/function.empty.php)
          • When applying your patch, you see a bug (strict standard exception) coming from enrol/guest/lib.php that redefine enrol_user method. Code should be as follows :
                public function enrol_user(stdClass $instance, $userid, $roleid = null, $timestart = 0, $timeend = 0, $status = null,
                                           $recovergrades = null) {
            

          Please tell me if you intend to take the lead, otherwise I'm willing to push it foward.

          Show
          Jean-Philippe Gaudreau added a comment - +1 on the patch too. Charles, are you able to take the lead on this task and assign it to yourself since your patch looks great ? If you do, here are some minor things that I think should be modified : enrol/manual/ajax.php Line 39 : replace NULL for null (coding style in lowercases) lib/enrollib.php Rename parameter $grades for $recovergrades as it would make more sense. Line 1250 : replace NULL for null Line 1264 : Check should be made on "!isset" instead of "empty" because false value is considered to be empty (see http://php.net/manual/en/function.empty.php ) When applying your patch, you see a bug (strict standard exception) coming from enrol/guest/lib.php that redefine enrol_user method. Code should be as follows : public function enrol_user(stdClass $instance, $userid, $roleid = null , $timestart = 0, $timeend = 0, $status = null , $recovergrades = null ) { Please tell me if you intend to take the lead, otherwise I'm willing to push it foward.
          Hide
          Charles Fulton added a comment -

          Thanks Jean-Philippe, I'll run with this one for now. I've made the changes you suggested and updated the branch.

          Show
          Charles Fulton added a comment - Thanks Jean-Philippe, I'll run with this one for now. I've made the changes you suggested and updated the branch.
          Hide
          Andrew Davis added a comment -

          I think there's an error in lib/enrollib.php There function parameter and the flag being set is called $recovergrades however the if statement uses $grades. $grades doesnt appear to be being set anywhere.

          This issue is missing testing instructions. Make sure to include the existing testing of the existing individual user grade restoration to make sure that all still works fine.

          Show
          Andrew Davis added a comment - I think there's an error in lib/enrollib.php There function parameter and the flag being set is called $recovergrades however the if statement uses $grades. $grades doesnt appear to be being set anywhere. This issue is missing testing instructions. Make sure to include the existing testing of the existing individual user grade restoration to make sure that all still works fine.
          Hide
          Charles Fulton added a comment -

          Andy: yes, sorry, I missed an instance when renaming. Fixed. Testing instructions added for the manual and metalink scenarios. I wouldn't think it's necessary to test external DB explicitly; the instructions would be similar.

          Show
          Charles Fulton added a comment - Andy: yes, sorry, I missed an instance when renaming. Fixed. Testing instructions added for the manual and metalink scenarios. I wouldn't think it's necessary to test external DB explicitly; the instructions would be similar.
          Hide
          Andrew Davis added a comment -

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

          Looks good. Putting this up for integration.

          Show
          Andrew Davis added a comment - [Y] Syntax [NA] Output [Y] Whitespace [NA] Language [NA] Databases [Y] Testing [Y] Security [NA] Documentation [Y] Git [Y] Sanity check Looks good. Putting this up for integration.
          Hide
          Andrew Davis added a comment -

          Is this fix master only?

          Show
          Andrew Davis added a comment - Is this fix master only?
          Hide
          Damyon Wiese added a comment -

          IMO - this is definitely a bug and deserving of a backport. It does need to be tested really well though - so I would really like to see comprehensive unit tests for this (including stable branches).

          Show
          Damyon Wiese added a comment - IMO - this is definitely a bug and deserving of a backport. It does need to be tested really well though - so I would really like to see comprehensive unit tests for this (including stable branches).
          Hide
          Charles Fulton added a comment -

          I've added branches for 2.3 and 2.4. I think this is somewhere between a bug and an improvement, and I'm unsure of introducing a signature change to a function on a stable branch.

          Show
          Charles Fulton added a comment - I've added branches for 2.3 and 2.4. I think this is somewhere between a bug and an improvement, and I'm unsure of introducing a signature change to a function on a stable branch.
          Hide
          Dan Poltawski added a comment -

          Petr maintains the enrolment plugins, adding him here to look over it.

          Show
          Dan Poltawski added a comment - Petr maintains the enrolment plugins, adding him here to look over it.
          Hide
          Petr Škoda added a comment - - edited

          +1 for master, thanks

          some comments:
          1/ where is the admin setting?
          2/ $CFG->recovergradesdefault might not be set before finishing upgrade, it might be better to use !empty($CFG->recovergradesdefault) there

          Show
          Petr Škoda added a comment - - edited +1 for master, thanks some comments: 1/ where is the admin setting? 2/ $CFG->recovergradesdefault might not be set before finishing upgrade, it might be better to use !empty($CFG->recovergradesdefault) there
          Hide
          Charles Fulton added a comment -

          The setting is in Site Administration > Grades > General settings. It was added in MDL-31158.

          Show
          Charles Fulton added a comment - The setting is in Site Administration > Grades > General settings. It was added in MDL-31158 .
          Hide
          Petr Škoda added a comment -

          ah, thanks

          Show
          Petr Škoda added a comment - ah, thanks
          Hide
          Robert Puffer added a comment -

          +1 for backport. This is a bug fix, its always been a bug as it departs (dramatically) from how Moodle 1.9 performed and causes MAJOR pita a lot more often than you'd think when someone gets unintentionally unenrolled. Thanks to Charles for creating the fix across the enrollment plugin board.

          Show
          Robert Puffer added a comment - +1 for backport. This is a bug fix, its always been a bug as it departs (dramatically) from how Moodle 1.9 performed and causes MAJOR pita a lot more often than you'd think when someone gets unintentionally unenrolled. Thanks to Charles for creating the fix across the enrollment plugin board.
          Hide
          Petr Škoda added a comment -

          did you grep for "function enrol_user(" in master? It looks like there are some more that need tweaking.

          Show
          Petr Škoda added a comment - did you grep for "function enrol_user(" in master? It looks like there are some more that need tweaking.
          Hide
          Charles Fulton added a comment -

          Thanks Petr; updated master branch to account for flatfile enrolment. I think that covers the core use cases.

          Show
          Charles Fulton added a comment - Thanks Petr; updated master branch to account for flatfile enrolment. I think that covers the core use cases.
          Hide
          Petr Škoda added a comment - - edited

          hmm, it should be also noted in enrol/upgrade.txt because it could give PHP notices in contrib enrol plugins, that is one of the major reasons for not backporting it

          Show
          Petr Škoda added a comment - - edited hmm, it should be also noted in enrol/upgrade.txt because it could give PHP notices in contrib enrol plugins, that is one of the major reasons for not backporting it
          Hide
          Charles Fulton added a comment -

          Added a note to upgrade.txt (master only).

          Show
          Charles Fulton added a comment - Added a note to upgrade.txt (master only).
          Hide
          Damyon Wiese added a comment -

          Just want to get some more opinions on backporting this.

          I also think it could really do with a unit test.

          Petr - what did you mean by contrib enrol plugins generating warnings? I can't see how that would happen.

          Show
          Damyon Wiese added a comment - Just want to get some more opinions on backporting this. I also think it could really do with a unit test. Petr - what did you mean by contrib enrol plugins generating warnings? I can't see how that would happen.
          Hide
          Dan Poltawski added a comment -

          Hmm, also not following how it would generate a warning.

          Show
          Dan Poltawski added a comment - Hmm, also not following how it would generate a warning.
          Hide
          Petr Škoda added a comment -

          See the flatfile for example, if anything redefines the enrol method it needs to use the same number of parameters.

          Show
          Petr Škoda added a comment - See the flatfile for example, if anything redefines the enrol method it needs to use the same number of parameters.
          Hide
          Damyon Wiese added a comment -

          Thanks Petr - makes sense now,

          We could rename the enrol_user function to something else like "enrol_user_recover_grades" and then have an enrol_user wrapper function which calls it.

          This would allow backporting with no api change - it would be good to decide on this before integrating the current patch as it affects what we put in upgrade.txt (ie we would be adding a new function for stables and then deprecating it immediately on master).

          Just explaining a little better:

          On 24 we could have:

          enrol_user_recover_grades(..., $recovergrades=null) {
          ...
          }
          
          enrol_user(...) {
              return enrol_user_recover_grades(...)
          }
          

          And on master

          enrol_user_recover_grades(..., $recovergrades=null) {
             debugging('This function has been deprecated. Call enrol_user instead.');
             return enrol_user(..., $recovergrades);
          ...
          }
          
          enrol_user(..., $recovergrades) {
              ...
          }
          
          
          upgrade.txt
          * enrol_user_recover_grades is deprecated, call enrol_user instead.
          

          Will get some more opinions on this.

          Show
          Damyon Wiese added a comment - Thanks Petr - makes sense now, We could rename the enrol_user function to something else like "enrol_user_recover_grades" and then have an enrol_user wrapper function which calls it. This would allow backporting with no api change - it would be good to decide on this before integrating the current patch as it affects what we put in upgrade.txt (ie we would be adding a new function for stables and then deprecating it immediately on master). Just explaining a little better: On 24 we could have: enrol_user_recover_grades(..., $recovergrades= null ) { ... } enrol_user(...) { return enrol_user_recover_grades(...) } And on master enrol_user_recover_grades(..., $recovergrades= null ) { debugging('This function has been deprecated. Call enrol_user instead.'); return enrol_user(..., $recovergrades); ... } enrol_user(..., $recovergrades) { ... } upgrade.txt * enrol_user_recover_grades is deprecated, call enrol_user instead. Will get some more opinions on this.
          Hide
          Petr Škoda added a comment -

          Hi Damyon, the whole point is to keep the same method name. If really really necessary it could be implemented without changing the API by directly accessing method argument that is not defined. I am afraid this is not a good backport candidate for other reasons too because there might be hidden bugs in the grade recovery code or plugins, anybody who needs this may easily modify their PHP files to recover the grades unconditionally...

          Show
          Petr Škoda added a comment - Hi Damyon, the whole point is to keep the same method name. If really really necessary it could be implemented without changing the API by directly accessing method argument that is not defined. I am afraid this is not a good backport candidate for other reasons too because there might be hidden bugs in the grade recovery code or plugins, anybody who needs this may easily modify their PHP files to recover the grades unconditionally...
          Hide
          Dan Poltawski added a comment -

          Yep, I think its better to keep it clean -1 to backport

          Show
          Dan Poltawski added a comment - Yep, I think its better to keep it clean -1 to backport
          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
          Charles Fulton added a comment -

          Thanks everybody; rebased on current master.

          Show
          Charles Fulton added a comment - Thanks everybody; rebased on current master.
          Hide
          Damyon Wiese added a comment -

          Thanks all,

          Integrated to master only.

          Show
          Damyon Wiese added a comment - Thanks all, Integrated to master only.
          Hide
          Damyon Wiese added a comment -

          Added docs required label - the documentation is still accurate, but could also mention that since 2.5, users enrolled with methods other than manual will have their grades restored depending on the value of the admin setting "recovergradesdefault".

          http://docs.moodle.org/24/en/Unenrolment#Unenrolment_and_grade_history

          Show
          Damyon Wiese added a comment - Added docs required label - the documentation is still accurate, but could also mention that since 2.5, users enrolled with methods other than manual will have their grades restored depending on the value of the admin setting "recovergradesdefault". http://docs.moodle.org/24/en/Unenrolment#Unenrolment_and_grade_history
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as I have added Damyon's comment here http://docs.moodle.org/25/en/Unenrolment#Unenrolment_and_grade_history thanks.

          Show
          Mary Cooch added a comment - Removing docs_required label as I have added Damyon's comment here http://docs.moodle.org/25/en/Unenrolment#Unenrolment_and_grade_history thanks.
          Hide
          Rossiani Wijaya added a comment -

          Hi Charles,

          While testing this, I noticed the following:

          1. Student had an attempt in courseB but it's not recorded in grader report (screenshot: grader_report.png and student_summary.png)
          2. un-enrol student from courseA, student still listed in courseB with un-enrol button displayed (screenshot: student_courseB_unenrol.png)
          3. un-enrol student from courseA, student name is not listed in grader report in courseB however the overall average remain the same. (screenshot: overall_grade_enrol.png and overall_grade_unenrol.png)
          4. Test instruction for Course metalink step #9 - student's grades displayed in Course B.

          Could you confirm the above errors please?

          Thanks.

          Rosie

          Show
          Rossiani Wijaya added a comment - Hi Charles, While testing this, I noticed the following: Student had an attempt in courseB but it's not recorded in grader report (screenshot: grader_report.png and student_summary.png) un-enrol student from courseA, student still listed in courseB with un-enrol button displayed (screenshot: student_courseB_unenrol.png) un-enrol student from courseA, student name is not listed in grader report in courseB however the overall average remain the same. (screenshot: overall_grade_enrol.png and overall_grade_unenrol.png) Test instruction for Course metalink step #9 - student's grades displayed in Course B. Could you confirm the above errors please? Thanks. Rosie
          Hide
          Rossiani Wijaya added a comment -

          Failing test

          Show
          Rossiani Wijaya added a comment - Failing test
          Hide
          Charles Fulton added a comment -

          Hi Rossi, I'll try to dig into these later today. As a note, #2 also occurs in a vanilla 2.5 instance. This is some new behavior with meta link unenrolment with which I'm not familiar.

          Show
          Charles Fulton added a comment - Hi Rossi, I'll try to dig into these later today. As a note, #2 also occurs in a vanilla 2.5 instance. This is some new behavior with meta link unenrolment with which I'm not familiar.
          Hide
          Charles Fulton added a comment -

          Didn't account for MDL-29684 in the testing instructions for meta links. Haven't looked at the other issues yet.

          Show
          Charles Fulton added a comment - Didn't account for MDL-29684 in the testing instructions for meta links. Haven't looked at the other issues yet.
          Hide
          Charles Fulton added a comment -

          Here's the behavior I observed with the quiz. If the patch isn't applied, you don't get your grades restored but the attempt is present, with a grade recorded in the attempt but not in the gradebook. When I restored my patch the actual grade returned as well. I need to dig deeper but I think there's a separate issue with quiz attempts.

          Show
          Charles Fulton added a comment - Here's the behavior I observed with the quiz. If the patch isn't applied, you don't get your grades restored but the attempt is present, with a grade recorded in the attempt but not in the gradebook. When I restored my patch the actual grade returned as well. I need to dig deeper but I think there's a separate issue with quiz attempts.
          Hide
          Adam Olley added a comment -

          Charles: That sounds like normal behaviour to me. Assignments/quizzes tend to keep track of their own copy of the submission/attempt grade which doesn't get moved off to another table when the user is unenrolled like the gradebook data does.

          Show
          Adam Olley added a comment - Charles: That sounds like normal behaviour to me. Assignments/quizzes tend to keep track of their own copy of the submission/attempt grade which doesn't get moved off to another table when the user is unenrolled like the gradebook data does.
          Hide
          Robert Puffer added a comment -

          Yeah, what's the sense of that and how many "total rewrites" will we go through before we get the centralized grades facility discussed since 1.3?

          Show
          Robert Puffer added a comment - Yeah, what's the sense of that and how many "total rewrites" will we go through before we get the centralized grades facility discussed since 1.3?
          Hide
          Damyon Wiese added a comment -

          To clarify:

          1. is expected behaviour, student data like quiz attempts and assignment submissions are not automatically deleted when a student is unenrolled (current behaviour)

          2 & 3 - I tried but count not reproduce - getting Rosie to get some more info.

          4. The testing instructions are correct (and worked for me on master) - the meta-enrolment that gets restored uses the "Recover Grades Default" setting to determine if it should recover the grades or not - the manual enrolment "recover grades" setting does not affect the meta-enrolment.

          Show
          Damyon Wiese added a comment - To clarify: 1. is expected behaviour, student data like quiz attempts and assignment submissions are not automatically deleted when a student is unenrolled (current behaviour) 2 & 3 - I tried but count not reproduce - getting Rosie to get some more info. 4. The testing instructions are correct (and worked for me on master) - the meta-enrolment that gets restored uses the "Recover Grades Default" setting to determine if it should recover the grades or not - the manual enrolment "recover grades" setting does not affect the meta-enrolment.
          Hide
          Rossiani Wijaya added a comment -

          Points 2 & 3 - works as expected after Seting External unenrol action to "Unenrol user from course."

          Point 4 - Not sure why it didn't work before.

          Re-tested this issue again and it works great.

          Test passed

          Show
          Rossiani Wijaya added a comment - Points 2 & 3 - works as expected after Seting External unenrol action to "Unenrol user from course." Point 4 - Not sure why it didn't work before. Re-tested this issue again and it works great. Test passed
          Hide
          Dan Poltawski added a comment -

          Thanks! You're changes are now spread to the world through this git and our source control repositories.

          No time to rest though, we've got days to make 2.5 the best yet!

          ciao

          Show
          Dan Poltawski added a comment - Thanks! You're changes are now spread to the world through this git and our source control repositories. No time to rest though, we've got days to make 2.5 the best yet! ciao
          Hide
          Farhan Karmali added a comment -

          In Moodle 2.6 , using enrollment method - 'Self' , Even though the site configuration is set to recover grades, It does not work , can anything check?

          Show
          Farhan Karmali added a comment - In Moodle 2.6 , using enrollment method - 'Self' , Even though the site configuration is set to recover grades, It does not work , can anything check?

            People

            • Votes:
              36 Vote for this issue
              Watchers:
              33 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: