Moodle
  1. Moodle
  2. MDL-37784

Adaptive Mode tells students they have been penalized even when set to no penalties

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.4.4, 2.5
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      This should be tested in MOODLE_24_STABLE and master branchs only. MOODLE_23_STABLE don't have this problem.

      1. Create a multiple choice question:
        Which one is an animal?
        Choice 1: the cat Grade 100% Feedback: Yes, the cat is an animal.
        Choice 2: the dinosaur Grade 50% Feedback: Yes, but it's an extinct animal.
        Choice 3: the bus Grade 0% Feedback: No way!
      2. Preview question in Adaptive mode.
      3. Select radio button for the dinosaur and click the Check button.
      4. Check that you get this feedback :
        Partially correct / Marks for this submission: 0.5/1.0. This submission attracted a penalty of 0.3.
      5. Now select Adaptive mode (no penalties) and click Start again with these options.
      6. Select radio button for the dinosaur and click the Check button.
      7. Check that you get this feedback :
        Partially correct / Marks for this submission: 0.5/1.0.
        (i.e. feedback no longer displays irrelevant penalty information).
      8. To confirm that things work as expected with the patch, continue and select the correct choice (the cat) and verify that still no penalty information is displayed in Adaptive (no penalties) mode.
      Show
      This should be tested in MOODLE_24_STABLE and master branchs only. MOODLE_23_STABLE don't have this problem. Create a multiple choice question: Which one is an animal? Choice 1: the cat Grade 100% Feedback: Yes, the cat is an animal. Choice 2: the dinosaur Grade 50% Feedback: Yes, but it's an extinct animal. Choice 3: the bus Grade 0% Feedback: No way! Preview question in Adaptive mode. Select radio button for the dinosaur and click the Check button. Check that you get this feedback : Partially correct / Marks for this submission: 0.5/1.0. This submission attracted a penalty of 0.3. Now select Adaptive mode (no penalties) and click Start again with these options. Select radio button for the dinosaur and click the Check button. Check that you get this feedback : Partially correct / Marks for this submission: 0.5/1.0. (i.e. feedback no longer displays irrelevant penalty information). To confirm that things work as expected with the patch, continue and select the correct choice ( the cat ) and verify that still no penalty information is displayed in Adaptive (no penalties) mode.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:

      Description

      When checking an answer in an adaptive mode (no penalties) quiz, students are shown that they have been penalized in the feedback area. However, no penalties are actually applied to their grade.

        Gliffy Diagrams

        1. quiz_adaptivenopenalty_jr.patch
          0.9 kB
          Joseph Rézeau
        2. quiz_adaptivenopenalty_jr02.patch
          1 kB
          Joseph Rézeau
        1. 1st answer.PNG
          11 kB
        2. 2nd answer.PNG
          12 kB
        3. 3rd answer.PNG
          9 kB
        4. screenshot-1 by Joseph.jpg
          55 kB

          Issue Links

            Activity

            Hide
            Tim Hunt added a comment -

            I am sure this used to work, and I cannot think of any changes in this area. Still, something must have broken it.

            Show
            Tim Hunt added a comment - I am sure this used to work, and I cannot think of any changes in this area. Still, something must have broken it.
            Hide
            Joseph Rézeau added a comment -

            Bug still exists in current moodle 2.4 and in forthcoming moodle 2.5! I am attaching a screencapture showing it.
            As mentioned by the OP, if correct answer is submitted later on, grade is however correctly set to 100%.

            I expect the reason this bug has not been reported more widely is that the "adaptive mode (no penalties)" is rarely used by moodlers. Still it would be nice if it could be fixed.

            Show
            Joseph Rézeau added a comment - Bug still exists in current moodle 2.4 and in forthcoming moodle 2.5! I am attaching a screencapture showing it. As mentioned by the OP, if correct answer is submitted later on, grade is however correctly set to 100%. I expect the reason this bug has not been reported more widely is that the "adaptive mode (no penalties)" is rarely used by moodlers. Still it would be nice if it could be fixed.
            Hide
            Joseph Rézeau added a comment -

            The attached patch should fix it. Function penalty_info has been removed from behaviours and replaced by other functions, e.g. function grading_details.
            Tested on moodle 2.4.

            Show
            Joseph Rézeau added a comment - The attached patch should fix it. Function penalty_info has been removed from behaviours and replaced by other functions, e.g. function grading_details. Tested on moodle 2.4.
            Hide
            Jean-Michel Vedrine added a comment - - edited

            Looking at older Moodle versions, it seems to me that Joseph's patch is in the right direction: penalty_info is not defined in the parent class and is never called.

            Show
            Jean-Michel Vedrine added a comment - - edited Looking at older Moodle versions, it seems to me that Joseph's patch is in the right direction: penalty_info is not defined in the parent class and is never called.
            Hide
            Joseph Rézeau added a comment -

            Thanks for your support, Jean-Michel.
            @Tim, I think this small bug ought really to be fixed before moodle 2.5 is released.

            Show
            Joseph Rézeau added a comment - Thanks for your support, Jean-Michel. @Tim, I think this small bug ought really to be fixed before moodle 2.5 is released.
            Hide
            Tim Hunt added a comment -

            Well, a patch "along the right lines" is not the same as a patch that is right, with unit tests and testing instructions to prove it.

            Actually, I don't think this patch is along the right lines. It will mean that adaptive no penalties will never display grading information. What about the case where the submission scored 5/10, but a previous submission scored 9/10. Surely we should display something in that case, so students know what is going on?

            Show
            Tim Hunt added a comment - Well, a patch "along the right lines" is not the same as a patch that is right, with unit tests and testing instructions to prove it. Actually, I don't think this patch is along the right lines. It will mean that adaptive no penalties will never display grading information. What about the case where the submission scored 5/10, but a previous submission scored 9/10. Surely we should display something in that case, so students know what is going on?
            Hide
            Jean-Michel Vedrine added a comment -

            Hello Tim,
            This why I wrote "is in the right direction", we both know that if I was sure it was the right patch I would have worded it differently and done a github branch
            The only thing I was sure is that penalty_info is buggy or is a leftover from some refactoring, because it is never called.
            In fact I was about to study how grading_details works or should be working in that case because in fact I don't understand why it is not working for adaptivenopenalty behaviour.
            More info later !

            Show
            Jean-Michel Vedrine added a comment - Hello Tim, This why I wrote "is in the right direction", we both know that if I was sure it was the right patch I would have worded it differently and done a github branch The only thing I was sure is that penalty_info is buggy or is a leftover from some refactoring, because it is never called. In fact I was about to study how grading_details works or should be working in that case because in fact I don't understand why it is not working for adaptivenopenalty behaviour. More info later !
            Hide
            Joseph Rézeau added a comment - - edited

            @Tim, you are correct, my patch does prevent the - unwanted - display of penalty, but it also prevents the - needed - grading info!
            I'm sure Jean-Michel will come up with the right solution...

            Show
            Joseph Rézeau added a comment - - edited @Tim, you are correct, my patch does prevent the - unwanted - display of penalty, but it also prevents the - needed - grading info! I'm sure Jean-Michel will come up with the right solution...
            Hide
            Joseph Rézeau added a comment - - edited

            This new patch should do the trick. (quiz_adaptivenopenalty_jr02.patch)
            Joseph

            EDIT pushed patch to my github for testing:
            https://github.com/rezeau/moodle/tree/MDL-37784_24

            Show
            Joseph Rézeau added a comment - - edited This new patch should do the trick. (quiz_adaptivenopenalty_jr02.patch) Joseph EDIT pushed patch to my github for testing: https://github.com/rezeau/moodle/tree/MDL-37784_24
            Hide
            Tim Hunt added a comment -

            Right, so I broke this here: https://github.com/moodle/moodle/commit/a3e6ad77d272207

            Jamie's second patch looks like the logic is right.

            However, rather than me just looking at the patch, and guessing that it is right, please can we have some unit tests that verify:
            1. That the logic is right in all cases.
            2. That in future, we don't break this again.

            You can use the unit tests at https://github.com/moodle/moodle/commit/a3e6ad77d272207#diff-3 as an example.

            (Actually, those are not great unit tests, becuase it is really hard to see what the expected behaviour is. Perhaps we should add comments expaning out all the get_string calls in the epxected values. Still they are infinitely better than no unit tests.)

            Show
            Tim Hunt added a comment - Right, so I broke this here: https://github.com/moodle/moodle/commit/a3e6ad77d272207 Jamie's second patch looks like the logic is right. However, rather than me just looking at the patch, and guessing that it is right, please can we have some unit tests that verify: 1. That the logic is right in all cases. 2. That in future, we don't break this again. You can use the unit tests at https://github.com/moodle/moodle/commit/a3e6ad77d272207#diff-3 as an example. (Actually, those are not great unit tests, becuase it is really hard to see what the expected behaviour is. Perhaps we should add comments expaning out all the get_string calls in the epxected values. Still they are infinitely better than no unit tests.)
            Hide
            Jean-Michel Vedrine added a comment -

            Hello Tim and Joseph,

            • unfortunately I forgot to look at this thread and missed Joseph's second patch so I did one independently
            • fortunately this is the same fix
            • fortunately it include unit tests

            Sorry I didn't let Joseph discover the joy of unit testing (In factt I started by writing the tests, verified they ere failing, and wrote the fix after that and was happy that now all tests were OK)

            Show
            Jean-Michel Vedrine added a comment - Hello Tim and Joseph, unfortunately I forgot to look at this thread and missed Joseph's second patch so I did one independently fortunately this is the same fix fortunately it include unit tests Sorry I didn't let Joseph discover the joy of unit testing (In factt I started by writing the tests, verified they ere failing, and wrote the fix after that and was happy that now all tests were OK)
            Hide
            Jean-Michel Vedrine added a comment -

            Well in fact Joseph can still discover the joy of unit testing because I used a walkthrough test and you pointed him in another direction, maybe it would be better to have both tests ?

            Show
            Jean-Michel Vedrine added a comment - Well in fact Joseph can still discover the joy of unit testing because I used a walkthrough test and you pointed him in another direction, maybe it would be better to have both tests ?
            Hide
            Tim Hunt added a comment -

            Well, if the two of you agree on a fix, and we have some unit tests, then I am happy.

            We just need some testing instructions before this can be submitted for integration.

            Also, MDL-34066 was a regression in 2.4. I think we should back-port this fix.

            Show
            Tim Hunt added a comment - Well, if the two of you agree on a fix, and we have some unit tests, then I am happy. We just need some testing instructions before this can be submitted for integration. Also, MDL-34066 was a regression in 2.4. I think we should back-port this fix.
            Hide
            Joseph Rézeau added a comment -

            Hi Jean-Michel,

            It's great that we both found the same fix. I am not a great fan of unit tests, and have never quite understood their value (shame on me).

            But I can write testing instructions. IN order to write those instructions in the correct place, in this tracker issue, do I go to the More Actions tab and click the Create Test Session item?

            Show
            Joseph Rézeau added a comment - Hi Jean-Michel, It's great that we both found the same fix. I am not a great fan of unit tests, and have never quite understood their value (shame on me). But I can write testing instructions. IN order to write those instructions in the correct place, in this tracker issue, do I go to the More Actions tab and click the Create Test Session item?
            Hide
            Tim Hunt added a comment -

            No. You edit this issue, and put them in the Testing instructions box.

            Show
            Tim Hunt added a comment - No. You edit this issue, and put them in the Testing instructions box.
            Hide
            Jean-Michel Vedrine added a comment -

            Hello Joseph,
            After some practice I really think that tests are helpful, not only to be sure something will not be broken again in the future without anybody noticing, but also during development.
            For instance I think it is a lot easier to write the code to convert old attempt data to question engine 2 using a test driven approach and I don't think I could have done it for the formulas question type if I had used another method.
            Also look at how I did it for this issue

            1. we have a problem: something about penalty is displayed on screen when it should not
            2. write some tests to assert things as they should be, in the present case assert that no information about penalty is in the output
            3. verify that currently these tests don't pass
            4. begin to work on the code
            5. when you think you have solved the problem, run the tests again
            6. if tests are OK, then most probably you can be confident you have really solved the problem
            7. if not, back to the drawing board

            Another good example is when I was working on grading formulas questions as multipart questions, I designed sample test questions and wrote how I was thinking they should be graded during an adaptive attempt, then I wrote the grading code and verified all worked as expected (of course it wasn't working as expected the first time then I was forced to think what was wrong, either my "manual" grading or my grading code, or in some occasion, both )

            Show
            Jean-Michel Vedrine added a comment - Hello Joseph, After some practice I really think that tests are helpful, not only to be sure something will not be broken again in the future without anybody noticing, but also during development. For instance I think it is a lot easier to write the code to convert old attempt data to question engine 2 using a test driven approach and I don't think I could have done it for the formulas question type if I had used another method. Also look at how I did it for this issue we have a problem: something about penalty is displayed on screen when it should not write some tests to assert things as they should be , in the present case assert that no information about penalty is in the output verify that currently these tests don't pass begin to work on the code when you think you have solved the problem, run the tests again if tests are OK, then most probably you can be confident you have really solved the problem if not, back to the drawing board Another good example is when I was working on grading formulas questions as multipart questions, I designed sample test questions and wrote how I was thinking they should be graded during an adaptive attempt, then I wrote the grading code and verified all worked as expected (of course it wasn't working as expected the first time then I was forced to think what was wrong, either my "manual" grading or my grading code, or in some occasion, both )
            Hide
            Jean-Michel Vedrine added a comment -

            To write the testing instructions, just edit this issue and write something in the "testing instructions" field

            Show
            Jean-Michel Vedrine added a comment - To write the testing instructions, just edit this issue and write something in the "testing instructions" field
            Hide
            Joseph Rézeau added a comment -

            Jean-Michel, thanks for trying to convert me to unit tests. The thing is I don't really know how to begin. If you would just post the test you wrote for the bug at hand it would help.
            Joseph

            Show
            Joseph Rézeau added a comment - Jean-Michel, thanks for trying to convert me to unit tests. The thing is I don't really know how to begin. If you would just post the test you wrote for the bug at hand it would help. Joseph
            Hide
            Joseph Rézeau added a comment -

            Thanks for putting the tests on your github, Jean-Michel!

            Show
            Joseph Rézeau added a comment - Thanks for putting the tests on your github, Jean-Michel!
            Hide
            Jean-Michel Vedrine added a comment -

            Hello Joseph and Tim,
            I have backported to MOODLE_24_STABLE (23_STABLE is irrelevant as MDL-34066 was fixed in 24_STABLE only)
            I have slightly edited Joseph's testing instructions as I don't think it is useful to test that the problem exists, so only test that the fix is working.
            I think this is ready for peer review.
            Tim, I think Joseph should be credited for that bugfix, not me, as he was the one that provided the fix, and also the one that insisted for fixing that bug

            Show
            Jean-Michel Vedrine added a comment - Hello Joseph and Tim, I have backported to MOODLE_24_STABLE (23_STABLE is irrelevant as MDL-34066 was fixed in 24_STABLE only) I have slightly edited Joseph's testing instructions as I don't think it is useful to test that the problem exists, so only test that the fix is working. I think this is ready for peer review. Tim, I think Joseph should be credited for that bugfix, not me, as he was the one that provided the fix, and also the one that insisted for fixing that bug
            Hide
            Jean-Michel Vedrine added a comment -

            Assigning to Joseph

            Show
            Jean-Michel Vedrine added a comment - Assigning to Joseph
            Hide
            Jean-Michel Vedrine added a comment -

            Assigned to Joseph and starting peer review

            Show
            Jean-Michel Vedrine added a comment - Assigned to Joseph and starting peer review
            Hide
            Tim Hunt added a comment -

            OK, so the issue is assigned to Joseph, and the commit has Jean-Michel's name on it. Overall, that seems like a fair division of credit.

            And the patch looks good, so submitting for integration.

            Show
            Tim Hunt added a comment - OK, so the issue is assigned to Joseph, and the commit has Jean-Michel's name on it. Overall, that seems like a fair division of credit. And the patch looks good, so submitting for integration.
            Hide
            Jean-Michel Vedrine added a comment - - edited

            Thanks Tim, I hope this makes Joshua "a happy camper" (see https://moodle.org/mod/forum/discuss.php?d=226739&parent=986278)

            Show
            Jean-Michel Vedrine added a comment - - edited Thanks Tim, I hope this makes Joshua "a happy camper" (see https://moodle.org/mod/forum/discuss.php?d=226739&parent=986278 )
            Hide
            Tim Hunt added a comment -

            Well, there you go, your reward for fixing this bug is that you have learned an obscure American-English idiom.

            Show
            Tim Hunt added a comment - Well, there you go, your reward for fixing this bug is that you have learned an obscure American-English idiom.
            Hide
            Jean-Michel Vedrine added a comment -

            Not that bad!

            Show
            Jean-Michel Vedrine added a comment - Not that bad!
            Hide
            Joseph Rézeau added a comment -

            Jean-Michel " I think Joseph should be credited for that bugfix, not me,..."
            I'm perfectly happy with credit for this bug fix going to Jean-Michel. That should make him a "happy camper" too.
            "Rise and Shine all you happy campers it's Groundhog Day!" From one of my favorite "cult films": http://www.imdb.com/title/tt0107048/

            Show
            Joseph Rézeau added a comment - Jean-Michel " I think Joseph should be credited for that bugfix, not me,..." I'm perfectly happy with credit for this bug fix going to Jean-Michel. That should make him a "happy camper" too. "Rise and Shine all you happy campers it's Groundhog Day!" From one of my favorite "cult films": http://www.imdb.com/title/tt0107048/
            Hide
            Joshua Bragg added a comment -

            Thanks guys. I appreciate the hard work. And yes I'm now a happy camper and pleased as punch too.

            Show
            Joshua Bragg added a comment - Thanks guys. I appreciate the hard work. And yes I'm now a happy camper and pleased as punch too.
            Hide
            Jean-Michel Vedrine added a comment -

            Thanks Joshua, I know Mr Punch and this english idiom very well and in fact if I had enough money I would buy a lot of Mr Punch puppets to add to my puppets collection and also if I was not fearing to swallow it I would try to use a swazzle.
            That's the way to do it !

            Show
            Jean-Michel Vedrine added a comment - Thanks Joshua, I know Mr Punch and this english idiom very well and in fact if I had enough money I would buy a lot of Mr Punch puppets to add to my puppets collection and also if I was not fearing to swallow it I would try to use a swazzle. That's the way to do it !
            Hide
            Dan Poltawski added a comment -

            Integrated to 2.4 and master - thanks!

            Show
            Dan Poltawski added a comment - Integrated to 2.4 and master - thanks!
            Hide
            Andrew Davis added a comment -

            Tests fine in master. Testing 2.4 now.

            Show
            Andrew Davis added a comment - Tests fine in master. Testing 2.4 now.
            Hide
            Andrew Davis added a comment -

            Seems to be working fine in 2.4 as well. Passing.

            Show
            Andrew Davis added a comment - Seems to be working fine in 2.4 as well. Passing.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I feel myself really alone tonight! So was time to push your fixes upstream!

            "Lest we forget. We will remember them."

            Thanks and ciao!

            Show
            Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: