Moodle
  1. Moodle
  2. MDL-45127

Wrong score calculation in lesson

    Details

    • Testing Instructions:
      Hide

      Custom score improvement.

      1. Go to a course and create a lesson activity with the following settings.
        • Appearance - Display ongoing score: yes
        • Appearance - Slideshow: yes
        • Appearance - Maximum number of answers: 4
        • Flow control - Allow student review: yes
        • Flow control - Maximum number of attempts: 3
        • Grade - Custom scoring: No
        • Grade - Re-takes allowed: Yes
      2. Click save and display.
      3. Add a question page. Don't select essay.
      4. Fill in the required question details.
      5. in the Answer 1 section check that the "Score" box is disabled.
      6. Click on the help button next to "Score". Check that the help makes sense.
      7. Have the Jump for the correct answer go to "Next page"
      8. Fill in Answer 2 with a likely incorrect answer and have the Jump go to "This page"
      9. Repeat steps 3 to 8 to create two more questions.
      10. Log is as a student and run through the lesson.
      11. Check that when answering incorrectly you return to the question again and when answering correctly you move on to the next question.
      12. Log in as a teacher or Admin.
      13. Edit the lesson settings and set Grade - Custom scoring: Yes.
      14. Edit the questions and check that the "Score" box is now enabled.
      15. Enter scores in for correct and incorrect answers.
      16. Log in as a student and run through the lesson again. Check that things run as expected.

      Lesson review

      1. As a student follow the "Review lesson" link.
      2. Keep clicking the Next page and continue buttons until you reach the end of the lesson again.
      3. Make sure that the score has not changed at all.
      Show
      Custom score improvement. Go to a course and create a lesson activity with the following settings. Appearance - Display ongoing score: yes Appearance - Slideshow: yes Appearance - Maximum number of answers: 4 Flow control - Allow student review: yes Flow control - Maximum number of attempts: 3 Grade - Custom scoring: No Grade - Re-takes allowed: Yes Click save and display. Add a question page. Don't select essay. Fill in the required question details. in the Answer 1 section check that the "Score" box is disabled. Click on the help button next to "Score". Check that the help makes sense. Have the Jump for the correct answer go to "Next page" Fill in Answer 2 with a likely incorrect answer and have the Jump go to "This page" Repeat steps 3 to 8 to create two more questions. Log is as a student and run through the lesson. Check that when answering incorrectly you return to the question again and when answering correctly you move on to the next question. Log in as a teacher or Admin. Edit the lesson settings and set Grade - Custom scoring: Yes. Edit the questions and check that the "Score" box is now enabled. Enter scores in for correct and incorrect answers. Log in as a student and run through the lesson again. Check that things run as expected. Lesson review As a student follow the "Review lesson" link. Keep clicking the Next page and continue buttons until you reach the end of the lesson again. Make sure that the score has not changed at all.
    • Affected Branches:
      MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull 2.6 Branch:
      wip-MDL-45127-26
    • Pull Master Branch:
      wip-MDL-45127-master

      Description

      While testing MDLQA-6748, it was discovered that lesson scores are not correctly calculated.
      Steps to reproduce:

      1. Backup attached lesson activity in a course
      2. Edit lesson and set grade point 100
      3. Log in as student and attempt lesson
        • 1 (You have answered 1 correctly out of 1 attempts.)
        • 2 (You have answered 2 correctly out of 2 attempts.) - This is wrong.
        • 0 (You have answered 2 correctly out of 3 attempts.)
        • Press "No, I just want to go to next question"
        • 0 (You have answered 2 correctly out of 4 attempts.)
        • Press "Yes, I'd like to try again"
        • 1 (You have answered 3 correctly out of 5 attempts.)
        • Your score is 3 (out of 6).

      In Moodle 25

      1. Step 2: Show you have given wrong answer and pressing "No..." takes you to next question without giving extra point (You have earned 1 point(s) out of 2 point(s) thus far.)
      2. Message (You have...) doesn't keep on increasing attempts, it just goes up-to 3 (Number of questions) and says (You have earned 0 point(s) out of 1 point(s) thus far.)

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Rajesh Taneja added a comment -

            Increase priority, as it seems to be broken functionality. Will add more information.

            Show
            Rajesh Taneja added a comment - Increase priority, as it seems to be broken functionality. Will add more information.
            Hide
            Joseph Rézeau added a comment -

            I tried to test this but the attached lesson is not clear (should be in English, not French) and the testing instructions are not clear at all...
            Joseph

            Show
            Joseph Rézeau added a comment - I tried to test this but the attached lesson is not clear (should be in English, not French) and the testing instructions are not clear at all... Joseph
            Hide
            Adrian Greeve added a comment -

            Thanks Joseph for your enthusiasm.

            I haven't actually finished with this issue. I just posted up my branches so that I wouldn't lose my current work if something happened to my computer. I would be extremely grateful to have you test the issue once the status of this issue changes to "Waiting for testing".

            Thanks again,

            Show
            Adrian Greeve added a comment - Thanks Joseph for your enthusiasm. I haven't actually finished with this issue. I just posted up my branches so that I wouldn't lose my current work if something happened to my computer. I would be extremely grateful to have you test the issue once the status of this issue changes to "Waiting for testing". Thanks again,
            Hide
            CiBoT added a comment -

            Results for MDL-45127

            • Remote repository: git://github.com/abgreeve/moodle.git
            Show
            CiBoT added a comment - Results for MDL-45127 Remote repository: git://github.com/abgreeve/moodle.git Remote branch wip- MDL-45127 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3213 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3213/artifact/work/smurf.html
            Hide
            John Okely added a comment -

            [] Syntax
            [] Whitespace
            [] Output
            [] Language
            [] Databases
            [] Testing (instructions and automated tests)
            [] Security
            [] Documentation
            [] Git
            [] Third party code
            [] Sanity check
            

            Show
            John Okely added a comment - [] Syntax [] Whitespace [] Output [] Language [] Databases [] Testing (instructions and automated tests) [] Security [] Documentation [] Git [] Third party code [] Sanity check
            Hide
            John Okely added a comment -

             @@ -102,6 +102,9 @@
                          if ($attempt->pageid == $page->id) {
                              $found = true; // if found current page
                              $temppageid = $attempt->pageid;
             +                // If this is the last element in the array then there is no next element to loop to.
             +                // This will be overridden if there is another element.
             +                $result->newpageid = LESSON_EOL;
                          }
                      }
                  }
            

            Looking at the code from pure logic side of things, this new line will always be hit. So it could be placed before the for loop, since it just providing a default value. Would make the code slightly simpler to understand.

            Show
            John Okely added a comment - @@ -102,6 +102,9 @@ if ($attempt->pageid == $page->id) { $found = true; // if found current page $temppageid = $attempt->pageid; + // If this is the last element in the array then there is no next element to loop to. + // This will be overridden if there is another element. + $result->newpageid = LESSON_EOL; } } } Looking at the code from pure logic side of things, this new line will always be hit. So it could be placed before the for loop, since it just providing a default value. Would make the code slightly simpler to understand.
            Hide
            John Okely added a comment -

            +$string['score_help'] = 'Score is only used when custom scoring is enabled. In all other cases a jump to a different page is considered a correct answer. If the response is incorrect then jump back to this page.';

            Perhaps could be rephrased to:

            Score is only used when custom scoring is enabled. In all other cases a jump to a different page is considered a correct answer. If the response is incorrect then set the jump setting to "this page".

            Show
            John Okely added a comment - +$string['score_help'] = 'Score is only used when custom scoring is enabled. In all other cases a jump to a different page is considered a correct answer. If the response is incorrect then jump back to this page.'; Perhaps could be rephrased to: Score is only used when custom scoring is enabled. In all other cases a jump to a different page is considered a correct answer. If the response is incorrect then set the jump setting to "this page".
            Hide
            Adrian Greeve added a comment -

            Thanks John for having a look at this for me.

            • I've moved the default value outside of the foreach loop.
            • I have also changed the string here as well. I'll add Helen as a watcher and see if she has any suggestions as to how this help dialogue could be improved.

            Helen Foster Could please have a look at the new string "score_help". I still feel that this could be written in a more concise and clear way.

            Thanks.

            Show
            Adrian Greeve added a comment - Thanks John for having a look at this for me. I've moved the default value outside of the foreach loop. I have also changed the string here as well. I'll add Helen as a watcher and see if she has any suggestions as to how this help dialogue could be improved. Helen Foster Could please have a look at the new string "score_help". I still feel that this could be written in a more concise and clear way. Thanks.
            Hide
            John Okely added a comment - - edited

            If this goes ahead we will need to change the docs to mention that the 'score' text box will only appear when editing a question if custom scoring is enabled. 'docs_required' added as a tag

            Show
            John Okely added a comment - - edited If this goes ahead we will need to change the docs to mention that the 'score' text box will only appear when editing a question if custom scoring is enabled. 'docs_required' added as a tag
            Hide
            John Okely added a comment -

            Peer review complete. Ready for someone else to pick up as a secondary review.

            Show
            John Okely added a comment - Peer review complete. Ready for someone else to pick up as a secondary review.
            Hide
            Frédéric Massart added a comment -

            Hi Adrian,

            tackling down Lesson bugs is an act of bravery, orz. I just have a few comments, but they do not affect the logic which you seem to have fixed perfectly.

            1. I understand the need for defining a default value for newpageid as a new attempt is no more created and thus we could end the foreach without a value.
            2. Can I suggest you to use the methods freeze or hardFreeze to disable the score field? If not, then you would need to set "disabled" => $disabled, as array keys should be used.
            3. It is very unlikely to happen, but if you were to preview your lesson with a retake of 0, then you could create an attempt. Perhaps you could wrap the whole insert_record block within something like if (!$userisreviewing).
            4. Perhaps the testing instructions should cover more scenarios, is it worth repeating some of the previous QA tests?
            5. I guess some Behat tests here could be useful, but I understand the need to fix that quickly, so we can probably leave them aside for now... if a good testing is made.

            Feel free to push for integration whenever you are ready.

            Cheers,
            Fred

            Show
            Frédéric Massart added a comment - Hi Adrian, tackling down Lesson bugs is an act of bravery, orz. I just have a few comments, but they do not affect the logic which you seem to have fixed perfectly. I understand the need for defining a default value for newpageid as a new attempt is no more created and thus we could end the foreach without a value. Can I suggest you to use the methods freeze or hardFreeze to disable the score field? If not, then you would need to set "disabled" => $disabled, as array keys should be used. It is very unlikely to happen, but if you were to preview your lesson with a retake of 0, then you could create an attempt. Perhaps you could wrap the whole insert_record block within something like if (!$userisreviewing) . Perhaps the testing instructions should cover more scenarios, is it worth repeating some of the previous QA tests? I guess some Behat tests here could be useful, but I understand the need to fix that quickly, so we can probably leave them aside for now... if a good testing is made. Feel free to push for integration whenever you are ready. Cheers, Fred
            Hide
            Adrian Greeve added a comment -

            Thanks Fred for the review.

            1. Thanks
            2. I have changed this to use freeze
            3. I changed this over as well.
            4. I have created a behat test. We still have the current testing instructions and the QA test instructions are different again. Happy to add more if integrators feel this is necessary.
            5. Behat tests!

            Submitting for integration.

            Show
            Adrian Greeve added a comment - Thanks Fred for the review. Thanks I have changed this to use freeze I changed this over as well. I have created a behat test. We still have the current testing instructions and the QA test instructions are different again. Happy to add more if integrators feel this is necessary. Behat tests! Submitting for integration.
            Hide
            Damyon Wiese added a comment -

            Hi Adrian - the behat test on this issue has multiple "Then" steps. It should have only one and the rest should be "And".

            Show
            Damyon Wiese added a comment - Hi Adrian - the behat test on this issue has multiple "Then" steps. It should have only one and the rest should be "And".
            Hide
            Helen Foster added a comment -

            Thanks Adrian for asking me to take a look at the new string.

            I suggest you remove the sentences

            In all other cases a jump to a different page is considered a correct answer. If the expected response is incorrect then set the jump setting to "this page".

            since they are not about the score setting, and instead add info about the score i.e.

            Score is only used when custom scoring is enabled. Each answer can then be given a numerical point value (positive or negative).

            Show
            Helen Foster added a comment - Thanks Adrian for asking me to take a look at the new string. I suggest you remove the sentences In all other cases a jump to a different page is considered a correct answer. If the expected response is incorrect then set the jump setting to "this page". since they are not about the score setting, and instead add info about the score i.e. Score is only used when custom scoring is enabled. Each answer can then be given a numerical point value (positive or negative).
            Hide
            Adrian Greeve added a comment -

            Hello!

            I have updated the score_help string. Thank again for your help Helen.

            I have also update the behat test to remove those pesky "Then" steps and replaced them with "And"s.

            Thanks.

            Show
            Adrian Greeve added a comment - Hello! I have updated the score_help string. Thank again for your help Helen. I have also update the behat test to remove those pesky "Then" steps and replaced them with "And"s. Thanks.
            Hide
            Marina Glancy added a comment -

            Thanks Adrian, integrated in 2.5, 2.6 and master.

            New behat tests pass after the fix and fail before the fix.

            Show
            Marina Glancy added a comment - Thanks Adrian, integrated in 2.5, 2.6 and master. New behat tests pass after the fix and fail before the fix.
            Hide
            Rajesh Taneja added a comment -

            Thanks for working on this Adrian,

            Lesson is strange with all possible options and behave different with different settings.
            Grade behaviour with:

            1. Custom scoring: It use points and max points = no. of questions.
            2. Grade with no custom scoring: Use attempts as max value.

            Works as expected... Passing...

            Show
            Rajesh Taneja added a comment - Thanks for working on this Adrian, Lesson is strange with all possible options and behave different with different settings. Grade behaviour with: Custom scoring: It use points and max points = no. of questions. Grade with no custom scoring: Use attempts as max value. Works as expected... Passing...
            Hide
            Eloy Lafuente (stronk7) added a comment -

            M A N Y T H A N K S ! !

            This is now part of Moodle, the LMS beast eating developers for breakfast.

            Closing as fixed, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - M A N Y T H A N K S ! ! This is now part of Moodle, the LMS beast eating developers for breakfast. Closing as fixed, ciao
            Hide
            Mary Cooch added a comment -

            Just checking here, re docs_required label: do I understand that now lesson scoring works as expected? In which case, perhaps we don't need any docs?

            Show
            Mary Cooch added a comment - Just checking here, re docs_required label: do I understand that now lesson scoring works as expected? In which case, perhaps we don't need any docs?
            Hide
            Mary Cooch added a comment -

            (Housekeeping) Removing docs_required label -unless anyone strongly objects!

            Show
            Mary Cooch added a comment - (Housekeeping) Removing docs_required label -unless anyone strongly objects!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: