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

      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.

        Gliffy Diagrams

          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: