Moodle
  1. Moodle
  2. MDL-26704

User outline report displaying dategraded instead of datesubmitted

    Details

    • Testing Instructions:
      Hide

      Previous notes:
      1) The change affects these modules, so it is expected to have some user with activity/grades on them: data, forum, glossary, lesson, quiz and scorm
      2) The tests will be performed as admin because, right now, due to another issue, teachers DON'T have access to individual user reports at all!

      Testing steps:
      1) Go to the participants page and pick some user having grades in the activities listed above.
      2) In the navigation menu, go to the Activity Reports -> Outline report for that user.
      3) Annotate all the grading dates shown in that page for the activities above.
      4) Go to the gradebook and override those grades or go to the activity and recalculate them
      5) Go back to the outline report for that user.
      6) TEST: Verify that the dates continue being the same (when the grade was initially created, the ones annotated in step 3) and they haven't been updated to the date when the override has happened (now).

      Final note:
      At the time of writing this, I'm not sure if the recalculation of grades (quiz, for example) keeps the original date of the grade or no, so in case recalculating makes step 6 above to fail, but override works I'd consider this passed.

      Show
      Previous notes: 1) The change affects these modules, so it is expected to have some user with activity/grades on them: data, forum, glossary, lesson, quiz and scorm 2) The tests will be performed as admin because, right now, due to another issue, teachers DON'T have access to individual user reports at all! Testing steps: 1) Go to the participants page and pick some user having grades in the activities listed above. 2) In the navigation menu, go to the Activity Reports -> Outline report for that user. 3) Annotate all the grading dates shown in that page for the activities above. 4) Go to the gradebook and override those grades or go to the activity and recalculate them 5) Go back to the outline report for that user. 6) TEST: Verify that the dates continue being the same (when the grade was initially created, the ones annotated in step 3) and they haven't been updated to the date when the override has happened (now). Final note: At the time of writing this, I'm not sure if the recalculation of grades (quiz, for example) keeps the original date of the grade or no, so in case recalculating makes step 6 above to fail, but override works I'd consider this passed.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Pull Master Branch:
      MDL-26704_user_outline_report_master
    • Rank:
      16306

      Description

      The outline report is meant to show user activity within a course - dategraded does not accurately reflect a users activity as it is affected by regrading, resulting in an inaccurate report on user activity.

      Bug introduced by 5e150ff30f9a71a4a3edf8358ec976e12e4dd281 after having previously been fixed in MDL-18285.

      Attached is a patch that changes dategraded to datesubmitted.

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment -

          It looks like were doomed to innaccurate reporting regardless of whether we use dategraded or datesubmitted. Here are the functions that determine those numbers.

          /**
               * Returns timestamp of submission related to this grade,
               * might be null if not submitted.
               * @return int
               */
              public function get_datesubmitted() {
                  //TODO: HACK - create new fields in 2.0
                  return $this->timecreated;
              }
          
              /**
               * Returns timestamp when last graded,
               * might be null if no grade present.
               * @return int
               */
              public function get_dategraded() {
                  //TODO: HACK - create new fields in 2.0
                  if (is_null($this->finalgrade) and is_null($this->feedback)) {
                      return null; // no grade == no date
                  } else if ($this->overridden) {
                      return $this->overridden;
                  } else {
                      return $this->timemodified;
                  }
              }
          

          date submitted is time created while date graded is time modified. If a student submits work then the only time its modified is by the teacher then always reporting time created in the user outline report will be accurate. As soon as students are able to have more than one go at an activity the report becomes innaccurate as it will still show the time/date of the first attempt.

          I'll post the details of git branches containing Matt's patch for the integrators in the event that this is decided to be more correct than what we do now.

          Show
          Andrew Davis added a comment - It looks like were doomed to innaccurate reporting regardless of whether we use dategraded or datesubmitted. Here are the functions that determine those numbers. /** * Returns timestamp of submission related to this grade, * might be null if not submitted. * @ return int */ public function get_datesubmitted() { //TODO: HACK - create new fields in 2.0 return $ this ->timecreated; } /** * Returns timestamp when last graded, * might be null if no grade present. * @ return int */ public function get_dategraded() { //TODO: HACK - create new fields in 2.0 if (is_null($ this ->finalgrade) and is_null($ this ->feedback)) { return null ; // no grade == no date } else if ($ this ->overridden) { return $ this ->overridden; } else { return $ this ->timemodified; } } date submitted is time created while date graded is time modified. If a student submits work then the only time its modified is by the teacher then always reporting time created in the user outline report will be accurate. As soon as students are able to have more than one go at an activity the report becomes innaccurate as it will still show the time/date of the first attempt. I'll post the details of git branches containing Matt's patch for the integrators in the event that this is decided to be more correct than what we do now.
          Hide
          Andrew Davis added a comment -

          Actually, no I won't. Instead I'll accidentally trash my git repo and have to throw those changes away. The changes are in the patch file. I can do over to get them into git if necessary.

          Show
          Andrew Davis added a comment - Actually, no I won't. Instead I'll accidentally trash my git repo and have to throw those changes away. The changes are in the patch file. I can do over to get them into git if necessary.
          Hide
          Matt Clarkson added a comment -

          The timecreated is probably a more accurate representation of student activity than the timemodified IMO since it is unaffected by regrades, so is the lesser of two inaccurate evils.

          Show
          Matt Clarkson added a comment - The timecreated is probably a more accurate representation of student activity than the timemodified IMO since it is unaffected by regrades, so is the lesser of two inaccurate evils.
          Hide
          Andrew Davis added a comment -

          Ive tried to make the code involved smart enough to make some sort of decision about which to use. See https://github.com/andyjdavis/moodle/commit/5e2b4d4207cf06584f489d8baf4de0b340b2912c and let me know what you think.

          For some reason with forums (at least) date submitted can be null while date graded has a value which is odd.

          Show
          Andrew Davis added a comment - Ive tried to make the code involved smart enough to make some sort of decision about which to use. See https://github.com/andyjdavis/moodle/commit/5e2b4d4207cf06584f489d8baf4de0b340b2912c and let me know what you think. For some reason with forums (at least) date submitted can be null while date graded has a value which is odd.
          Hide
          Andrew Davis added a comment -

          Ive created versions of this for 1.9, 2 stable and 2.1. Im after a peer review now and also, Matt, if you have any feedback let me know

          Show
          Andrew Davis added a comment - Ive created versions of this for 1.9, 2 stable and 2.1. Im after a peer review now and also, Matt, if you have any feedback let me know
          Hide
          Aparup Banerjee added a comment -

          some works needs to be done to centralise the code a bit , perhaps into gradebook api?

          Show
          Aparup Banerjee added a comment - some works needs to be done to centralise the code a bit , perhaps into gradebook api?
          Hide
          Andrew Davis added a comment - - edited

          Given the amount of repetitive code in this Apu suggested moving some into the gradebook api. The functions involved already load the gradebook api ie

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

          However moving it into the grade API brings with it a presumption that the values it returns will be, for lack of a better word, correct. The code as it currently stands uses a pretty rough approximation that is going to be wrong much of the time. There is a way to make it correct all the time. Something like the following.

          Call it something like grade_get_timemodified($gradegradeid, $userid=null) where $userid is the user you care about. get the last time this grade was modified BY this particular user.

          You then retrieve the last row from grade_grades_history for that grade grade instance where grade_grades_history.usermodified == $userid.

          The downside of this is that it is an extra query per grade item to go find the correct time modified.

          Show
          Andrew Davis added a comment - - edited Given the amount of repetitive code in this Apu suggested moving some into the gradebook api. The functions involved already load the gradebook api ie require_once( "$CFG->libdir/gradelib.php" ); However moving it into the grade API brings with it a presumption that the values it returns will be, for lack of a better word, correct. The code as it currently stands uses a pretty rough approximation that is going to be wrong much of the time. There is a way to make it correct all the time. Something like the following. Call it something like grade_get_timemodified($gradegradeid, $userid=null) where $userid is the user you care about. get the last time this grade was modified BY this particular user. You then retrieve the last row from grade_grades_history for that grade grade instance where grade_grades_history.usermodified == $userid. The downside of this is that it is an extra query per grade item to go find the correct time modified.
          Hide
          Andrew Davis added a comment -

          There has been some discussion about this issue. The code as it currently stands is repetitive and does not produce correct data under all circumstances. To centralize it it could be moved into the gradebook API however I wouldnt be comfortable doing that unless its correct all the time. Getting that correctness requires per item queries into the history table that are undesirable. As such, although its not super, I think we should stick with the current uncentralized code.

          That way any inaccuracy in the name of performance is at least limited to a single report rather than being in the gradebook API where people will come to rely on it.

          Show
          Andrew Davis added a comment - There has been some discussion about this issue. The code as it currently stands is repetitive and does not produce correct data under all circumstances. To centralize it it could be moved into the gradebook API however I wouldnt be comfortable doing that unless its correct all the time. Getting that correctness requires per item queries into the history table that are undesirable. As such, although its not super, I think we should stick with the current uncentralized code. That way any inaccuracy in the name of performance is at least limited to a single report rather than being in the gradebook API where people will come to rely on it.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Silly question, apart from any repetitive/centralised disquisition... is this condition really necessary (just guessing if ALL grades have that information available always):

          empty($grade->datesubmitted)
          

          Anyway, I'm going to integrate this now, don't think the question above is an stopper at all. Just for your consideration.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Silly question, apart from any repetitive/centralised disquisition... is this condition really necessary (just guessing if ALL grades have that information available always): empty($grade->datesubmitted) Anyway, I'm going to integrate this now, don't think the question above is an stopper at all. Just for your consideration. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been integrated, thanks!

          Note: I've added one extra commit both on 20_STABLE and master containing this TODO comment:

          //TODO: move this copied & pasted code somewhere in the grades API. See MDL-26704
          

          Perhaps it could be some calculated column returned as part ot the grade_grade or static function or whatever, but we need to take rid of this dupe code sometime in the future.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been integrated, thanks! Note: I've added one extra commit both on 20_STABLE and master containing this TODO comment: //TODO: move this copied & pasted code somewhere in the grades API. See MDL-26704 Perhaps it could be some calculated column returned as part ot the grade_grade or static function or whatever, but we need to take rid of this dupe code sometime in the future. Ciao
          Hide
          Helen Foster added a comment -

          Going to test with a graded assignment activity. Anyone feel free to suggest further testing steps if necessary.

          Show
          Helen Foster added a comment - Going to test with a graded assignment activity. Anyone feel free to suggest further testing steps if necessary.
          Hide
          Helen Foster added a comment -

          Apologies if I don't understand what the issue is about.

          To get to user outline report I have to login as admin then go to: Home ► Courses ► Features Demo ► Participants ► Sam Student ► Activity reports ► Outline report

          I can't view outline reports at all when logged in as a teacher (possible separate bug?)

          The outline report shows date graded next to the grade. If the assignment has not yet been graded, nothing is shown.

          Show
          Helen Foster added a comment - Apologies if I don't understand what the issue is about. To get to user outline report I have to login as admin then go to: Home ► Courses ► Features Demo ► Participants ► Sam Student ► Activity reports ► Outline report I can't view outline reports at all when logged in as a teacher (possible separate bug?) The outline report shows date graded next to the grade. If the assignment has not yet been graded, nothing is shown.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          After talking with Helen, starting one new testing loop.

          I'm going to add test instructions now (grr, I should have rejected this without proper instructions).

          And also one new issue must be created as part of this because current implementation in navigationlib prevents teachers for getting user reports (individual) completely!!

          Show
          Eloy Lafuente (stronk7) added a comment - After talking with Helen, starting one new testing loop. I'm going to add test instructions now (grr, I should have rejected this without proper instructions). And also one new issue must be created as part of this because current implementation in navigationlib prevents teachers for getting user reports (individual) completely!!
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Testing istructions updated.

          Also, here it's one change I've tried here in order to get teachers again being able to view individual reports. For Sam's consideration, I really don't know if that's the way to fix it.

          https://github.com/stronk7/moodle/compare/master...navigationlib_userreports

          Note it seems to work but I really don't like those plugins harcoded there, instead we should extend the course reports API to be able to tell navigation stuff about:

          • The possibility to run the report for 1 user
          • The perms to check

          Ciao

          Edited: So +1 to test this exclusively as admin, then create followup issue with the teachers problem (if it does not exist already, seems so obvious) and there verify/complete the proposed patch.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Testing istructions updated. Also, here it's one change I've tried here in order to get teachers again being able to view individual reports. For Sam's consideration, I really don't know if that's the way to fix it. https://github.com/stronk7/moodle/compare/master...navigationlib_userreports Note it seems to work but I really don't like those plugins harcoded there, instead we should extend the course reports API to be able to tell navigation stuff about: The possibility to run the report for 1 user The perms to check Ciao Edited: So +1 to test this exclusively as admin, then create followup issue with the teachers problem (if it does not exist already, seems so obvious) and there verify/complete the proposed patch.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Offtopic: Also, plz, don't include commits and reverts and merges in the proposed branches to integrate (as has happened in the 19_STABLE branch). IMO we should only accept linear developments, so rebasing and squashing before requesting integration is a good habit. TIA!

          Show
          Eloy Lafuente (stronk7) added a comment - Offtopic: Also, plz, don't include commits and reverts and merges in the proposed branches to integrate (as has happened in the 19_STABLE branch). IMO we should only accept linear developments, so rebasing and squashing before requesting integration is a good habit. TIA!
          Hide
          Helen Foster added a comment -

          Thanks Eloy, I've reported the problem of teachers not being able to view outline reports as MDL-27541.

          Show
          Helen Foster added a comment - Thanks Eloy, I've reported the problem of teachers not being able to view outline reports as MDL-27541 .
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks, Helen!

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks, Helen!
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          You patch looks good presently and I agree - really the reports API should be extended to perform proper permission checks. Doing so we could just iterate the collection of course reports and add if the perm tests pass... much easier.
          Given that you already have a solution I think it should go it - this issue won't get looked at before 2.1 and it is certainly an improvement.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy, You patch looks good presently and I agree - really the reports API should be extended to perform proper permission checks. Doing so we could just iterate the collection of course reports and add if the perm tests pass... much easier. Given that you already have a solution I think it should go it - this issue won't get looked at before 2.1 and it is certainly an improvement. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Tomaz found a bug with this while testing.
          Code is referencing $options->scaleid however in the code it is $params['scaleid'].
          I'll make an emergency commit to fix this soon, given this is an obvious problem it should be fine.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Tomaz found a bug with this while testing. Code is referencing $options->scaleid however in the code it is $params ['scaleid'] . I'll make an emergency commit to fix this soon, given this is an obvious problem it should be fine. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Ok I've commit fixes for this now:

          This is ready for testing again - please note you will need to update your repo again.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ok I've commit fixes for this now: master: http://git.moodle.org/gw?p=integration.git;a=commit;h=74cf2d00158a2c7ae2529f21787ea65e91c5d82d MOODLE_20_STABLE: http://git.moodle.org/gw?p=integration.git;a=commit;h=bcb785f MOODLE_19_STABLE wasn't affected. This is ready for testing again - please note you will need to update your repo again. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Tested as admin and passed - MDL-27541 will see it fixed for teachers next week.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Tested as admin and passed - MDL-27541 will see it fixed for teachers next week. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing this, it's already part of upstream, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Closing this, it's already part of upstream, thanks!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: