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 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      47518

      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.

      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: