Moodle
  1. Moodle
  2. MDL-27386

lesson essay grading mform needs to be properly initialised , $mform->score isn't working

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.3
    • Fix Version/s: 2.0.4
    • Component/s: Lesson
    • Labels:
      None
    • Rank:
      17073

      Description

      lesson essay grading form needs to be initialised properly so that $mform->score carries a value in the 'get_data' instance of mform.

      some work was done but as yet incomplete and lacking testing @ https://github.com/nebgor/moodle/compare/mistress...to-be-created-MDL-based-on-MDL-26788

      this may actually be a bug that could be resolve within mform itself.

        Issue Links

          Activity

          Hide
          Rossiani Wijaya added a comment -

          Hi Apu,

          I made changes to your patch. When you have a chance, could you please take a look and review it?

          Diff url: https://github.com/rwijaya/moodle/compare/master...MDL-27386_HEAD

          Thanks

          Show
          Rossiani Wijaya added a comment - Hi Apu, I made changes to your patch. When you have a chance, could you please take a look and review it? Diff url: https://github.com/rwijaya/moodle/compare/master...MDL-27386_HEAD Thanks
          Hide
          Aparup Banerjee added a comment -

          Hi Rossi,
          I've had a look at the patch it works great for me. Looks fine too.
          cheers,
          Aparup.

          ps: could this have been situation that mform could have handled?

          Show
          Aparup Banerjee added a comment - Hi Rossi, I've had a look at the patch it works great for me. Looks fine too. cheers, Aparup. ps: could this have been situation that mform could have handled?
          Hide
          Rossiani Wijaya added a comment -

          Apu,

          mform is not working here because the option value hasn't been initialize.

          Create patch for 2.0 and 2.1 (see details above for diff url).

          Show
          Rossiani Wijaya added a comment - Apu, mform is not working here because the option value hasn't been initialize. Create patch for 2.0 and 2.1 (see details above for diff url).
          Hide
          Rossiani Wijaya added a comment -

          Assigning Andrew as peer review.

          Show
          Rossiani Wijaya added a comment - Assigning Andrew as peer review.
          Hide
          Andrew Davis added a comment -

          Hi. The area around line 75 of mod/lesson/essay.php doesnt look quite right.

          if (!$user = $DB->get_record('user', array('id' => $attempt->userid))) {
                      print_error('cannotfinduser', 'lesson');
                  }
                  if (!$answer = $DB->record_exists('lesson_answers', array('lessonid' => $lesson->id, 'pageid' => $attempt->pageid))) {
                      print_error('cannotfindanswer', 'lesson');
                  }
          

          As record_exists() returns a boolean you can just do this

          if (!$DB->record_exists('lesson_answers', array('lessonid' => $lesson->id, 'pageid' => $attempt->pageid))) {
                      print_error('cannotfindanswer', 'lesson');
                  }
          

          Also, there is a variable called $answer set on line 52. Is overwriting the answer object with a boolean what you actually wanted to do?

          Show
          Andrew Davis added a comment - Hi. The area around line 75 of mod/lesson/essay.php doesnt look quite right. if (!$user = $DB->get_record('user', array('id' => $attempt->userid))) { print_error('cannotfinduser', 'lesson'); } if (!$answer = $DB->record_exists('lesson_answers', array('lessonid' => $lesson->id, 'pageid' => $attempt->pageid))) { print_error('cannotfindanswer', 'lesson'); } As record_exists() returns a boolean you can just do this if (!$DB->record_exists('lesson_answers', array('lessonid' => $lesson->id, 'pageid' => $attempt->pageid))) { print_error('cannotfindanswer', 'lesson'); } Also, there is a variable called $answer set on line 52. Is overwriting the answer object with a boolean what you actually wanted to do?
          Hide
          Aparup Banerjee added a comment -

          Hi Rossi,
          i've just had a glance, thinking the 'update' case needs to check if $attempt object is set else possible fatal.

          Show
          Aparup Banerjee added a comment - Hi Rossi, i've just had a glance, thinking the 'update' case needs to check if $attempt object is set else possible fatal.
          Hide
          Rossiani Wijaya added a comment -

          Hi Andrew and Apu,

          Thank you for reviewing.

          On line 52, the value of $answer has been assign to populate $scoreoptions and unused afterward.

          Line 75, $answer used to captured lesson_answers data but its actually just need to check for the existence of lesson answer. and $answer should not be used there.

          Looking at the code again, some call to DB can be eliminate to improve performance. Also, adding Apu's suggestion to add $attempt object check to prevent fatal error.

          Repost both 2.0 and 2.1 patches.

          Show
          Rossiani Wijaya added a comment - Hi Andrew and Apu, Thank you for reviewing. On line 52, the value of $answer has been assign to populate $scoreoptions and unused afterward. Line 75, $answer used to captured lesson_answers data but its actually just need to check for the existence of lesson answer. and $answer should not be used there. Looking at the code again, some call to DB can be eliminate to improve performance. Also, adding Apu's suggestion to add $attempt object check to prevent fatal error. Repost both 2.0 and 2.1 patches.
          Hide
          Aparup Banerjee added a comment - - edited

          Hi Rosie: Looks good enough for me! (sorry, i thought from email that i was supposed to review this. it very confusing, now seeing that Andrew is the reviewer. lol)

          Show
          Aparup Banerjee added a comment - - edited Hi Rosie: Looks good enough for me! (sorry, i thought from email that i was supposed to review this. it very confusing, now seeing that Andrew is the reviewer. lol)
          Hide
          Rossiani Wijaya added a comment -

          Hi Apu,

          Thank you for testing it and sorry for the confusion. I assigned Andrew as peer review this morning because I made some changes to the code based on his comments last Friday.

          Show
          Rossiani Wijaya added a comment - Hi Apu, Thank you for testing it and sorry for the confusion. I assigned Andrew as peer review this morning because I made some changes to the code based on his comments last Friday.
          Hide
          Andrew Davis added a comment -

          Ive had another look and it looks far more clear

          Show
          Andrew Davis added a comment - Ive had another look and it looks far more clear
          Hide
          Rossiani Wijaya added a comment -

          Great. Thanks Andrew.

          Show
          Rossiani Wijaya added a comment - Great. Thanks Andrew.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been integrated, thanks!

          Note: I'd suggest to look for and/or create one new issue about how the detailed report in the lesson is working. It seems that there are various things to review there, for example:

          • How/what does it show if no user is specified.
          • How essay answers are shown (missing format-xxxx support?)
          • Not to show not visited pages...

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been integrated, thanks! Note: I'd suggest to look for and/or create one new issue about how the detailed report in the lesson is working. It seems that there are various things to review there, for example: How/what does it show if no user is specified. How essay answers are shown (missing format-xxxx support?) Not to show not visited pages... Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've created MDL-27554 as followup for the things to improve in the detailed report.

          Show
          Eloy Lafuente (stronk7) added a comment - I've created MDL-27554 as followup for the things to improve in the detailed report.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Getting this for testing...

          Show
          Eloy Lafuente (stronk7) added a comment - Getting this for testing...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Tested both under 20_STABLE and master, the score is now properly stored and lesson final grade and detailed report shows it (and points earned) properly. Passing.

          Show
          Eloy Lafuente (stronk7) added a comment - Tested both under 20_STABLE and master, the score is now properly stored and lesson final grade and detailed report shows it (and points earned) properly. Passing.
          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:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: