Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-27386

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

          Attachments

            Issue Links

              Activity

              Hide
              rwijaya 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
              rwijaya 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
              nebgor 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
              nebgor 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
              rwijaya 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
              rwijaya 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
              rwijaya Rossiani Wijaya added a comment -

              Assigning Andrew as peer review.

              Show
              rwijaya Rossiani Wijaya added a comment - Assigning Andrew as peer review.
              Hide
              andyjdavis 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
              andyjdavis 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
              nebgor 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
              nebgor 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
              rwijaya 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
              rwijaya 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
              nebgor 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
              nebgor 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
              rwijaya 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
              rwijaya 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
              andyjdavis Andrew Davis added a comment -

              Ive had another look and it looks far more clear

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

              Great. Thanks Andrew.

              Show
              rwijaya Rossiani Wijaya added a comment - Great. Thanks Andrew.
              Hide
              stronk7 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
              stronk7 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

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

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

              Getting this for testing...

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Getting this for testing...
              Hide
              stronk7 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
              stronk7 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

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

              Show
              stronk7 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:
                    Fix Release Date:
                    1/Aug/11