Moodle
  1. Moodle
  2. MDL-34793

Adding an assignment grade through gradebook with feedback on, breaks student view.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3.1
    • Fix Version/s: 2.3.2
    • Component/s: Assignment
    • Labels:
    • Testing Instructions:
      Hide
      1. Create an assignment, turn at least one of the feedback methods on.
      2. Go into the gradebook, turn editing on, add a grade for that student in the gradebook, save.
      3. Become the student.
      4. Click on the assignment in the course.
      5. You should see the assignment page, indicating no submission, but a reported grade
      Show
      Create an assignment, turn at least one of the feedback methods on. Go into the gradebook, turn editing on, add a grade for that student in the gradebook, save. Become the student. Click on the assignment in the course. You should see the assignment page, indicating no submission, but a reported grade
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
    • Rank:
      43288

      Description

      To reproduce:

      You need a course with a student.

      1. Create an assignment, turn at least one of the feedback methods on.
      2. Go into the gradebook, turn editing on, add a grade for that student in the gradebook, save.
      3. Become the student.
      4. Click on the assignment in the course.
      5. You will get a "Coding error detected, it must be fixed by a programmer: PHP catchable fatal error"

      Here is the stack trace in the 20120802 master branch:
      Debug info: Argument 1 passed to assign_feedback_comments::is_empty() must be an instance of stdClass, boolean given, called in /usr/local/apache2/htdocs/m_master/mod/assign/renderer.php on line 324 and defined
      Error code: codingerror
      Stack trace:
      line 397 of /lib/setuplib.php: coding_exception thrown
      line 316 of /mod/assign/feedback/comments/locallib.php: call to default_error_handler()
      line 324 of /mod/assign/renderer.php: call to assign_feedback_comments->is_empty()
      line 215 of /lib/outputrenderers.php: call to mod_assign_renderer->render_assign_feedback_status()
      line 2116 of /mod/assign/locallib.php: call to plugin_renderer_base->render()
      line 2152 of /mod/assign/locallib.php: call to assign->view_student_summary()
      line 382 of /mod/assign/locallib.php: call to assign->view_submission_page()
      line 53 of /mod/assign/view.php: call to assign->view()

      My general findings:
      When you add a grade through the gradebook, and grade_grades entry is made, but assign_grades is empty. In mod/assign/locallib.php view_student_summary(), the assign_grade item it fetch and put into $grade and the gradebook grade is fetched and placed into $gradebookgrade. Later on, if $gradebookgrade->grade exists, a assign_feedback_status is made, but with the assign_grade grade ($grade) which is false, because there is no assign_grade entry.

      That is then passed to mod/assign/renderer.php render_assign_feedback_status(). In there, the $grade is passed to assign_feedback_XXX::is_empty(), which is expecting a object, not the boolean false.

        Issue Links

          Activity

          Hide
          Eric Merrill added a comment -

          This is my proposed solution - basically to check if the $status->grade is a object before doing the is_empty check.

          Show
          Eric Merrill added a comment - This is my proposed solution - basically to check if the $status->grade is a object before doing the is_empty check.
          Hide
          Damyon Wiese added a comment -

          Hi Eric,

          Thanks for taking the time to hunt this down - I like the patch but I would prefer !empty() instead of isobject() as I think it makes it more obvious what it is checking for. Also could you split that if statement up as that line is getting long.

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Eric, Thanks for taking the time to hunt this down - I like the patch but I would prefer !empty() instead of isobject() as I think it makes it more obvious what it is checking for. Also could you split that if statement up as that line is getting long. Thanks, Damyon
          Hide
          Eric Merrill added a comment -

          I updated the patch to !empty(). I didn't split the if because with this edit, it's 129 char, which is under the 132 soft limit (and well shorter than the surrounding lines).

          Show
          Eric Merrill added a comment - I updated the patch to !empty(). I didn't split the if because with this edit, it's 129 char, which is under the 132 soft limit (and well shorter than the surrounding lines).
          Hide
          Damyon Wiese added a comment -

          Looks good - Thanks Eric

          Show
          Damyon Wiese added a comment - Looks good - Thanks Eric
          Hide
          Eloy Lafuente (stronk7) added a comment -

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

          TIA and ciao

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

          Hi Eric,

          Looks fine but I think your commit message is slightly out of date as its not checking for an object anymore?

          Show
          Dan Poltawski added a comment - Hi Eric, Looks fine but I think your commit message is slightly out of date as its not checking for an object anymore?
          Hide
          Eric Merrill added a comment -

          Hrm, good catch. Normally I know how I would fix that, but what should I do since it's already in integration?

          Show
          Eric Merrill added a comment - Hrm, good catch. Normally I know how I would fix that, but what should I do since it's already in integration?
          Hide
          Dan Poltawski added a comment -

          I've not pulled it into integration (I don't think!) so you can just ammend the commit?

          Show
          Dan Poltawski added a comment - I've not pulled it into integration (I don't think!) so you can just ammend the commit?
          Hide
          Eric Merrill added a comment -

          Done and done.

          Show
          Eric Merrill added a comment - Done and done.
          Hide
          Dan Poltawski added a comment -

          Hmm, now it seems to have changed to is_object in the code?

          Show
          Dan Poltawski added a comment - Hmm, now it seems to have changed to is_object in the code?
          Hide
          Eric Merrill added a comment -

          Clearly I had a rough day with git. I think I got it all squared away, have another look.

          Sorry about that.
          -Eric

          Show
          Eric Merrill added a comment - Clearly I had a rough day with git. I think I got it all squared away, have another look. Sorry about that. -Eric
          Hide
          Dan Poltawski added a comment -

          Thanks Eric - integrated to 23 and master.

          Show
          Dan Poltawski added a comment - Thanks Eric - integrated to 23 and master.
          Hide
          Tim Barker added a comment -

          Tested as per the testing instructions, congrats, the test passed!

          Show
          Tim Barker added a comment - Tested as per the testing instructions, congrats, the test passed!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For the good and the bad... this is now part of Moodle and people around the world will start using it immediately, what a responsibility!

          Many thanks for your collaboration, yay!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - For the good and the bad... this is now part of Moodle and people around the world will start using it immediately, what a responsibility! Many thanks for your collaboration, yay! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: