Moodle
  1. Moodle
  2. MDL-29772

Add "total number of penalties so far" info to question Adaptive behaviour.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.3, 2.2, 2.3
    • Fix Version/s: 2.1.4, 2.2.1
    • Component/s: Questions, Quiz
    • Labels:
    • Testing Instructions:
      Hide
      • Create a quiz with "How questions behave: Adaptive mode" (otherwise, with standard settings)
      • Add one question: "Short Answer" type, question text: "What is the great answer?", answer text: "42" (100%), Score for the question: 3
      • Log in as a student and attempt the quiz.
      • Enter the following answer in this sequence, click "Check" on the corresponding question after each entry.

      o What is the great answer? a
      Verify penalty message: "This submission attracted a penalty of 1.00"

      o What is the great answer? b
      Verify penalty message:
      in Moodle 2.1: "This submission attracted a penalty of 1.00."
      in Moodle 2.2 and Master: "This submission attracted a penalty of 1.00. Total penalty so far: 2.00"

      o What is the great answer? 42
      Verify that no penalty message is present.

      run unit tests at question/behaviour/adaptive and report any anomalies.

      Show
      Create a quiz with "How questions behave: Adaptive mode" (otherwise, with standard settings) Add one question: "Short Answer" type, question text: "What is the great answer?", answer text: "42" (100%), Score for the question: 3 Log in as a student and attempt the quiz. Enter the following answer in this sequence, click "Check" on the corresponding question after each entry. o What is the great answer? a Verify penalty message: "This submission attracted a penalty of 1.00" o What is the great answer? b Verify penalty message: in Moodle 2.1: "This submission attracted a penalty of 1.00." in Moodle 2.2 and Master: "This submission attracted a penalty of 1.00. Total penalty so far: 2.00" o What is the great answer? 42 Verify that no penalty message is present. run unit tests at question/behaviour/adaptive and report any anomalies.
    • Workaround:
      Hide

      Very easy: just add the following line to question/behaviour/adaptive/renderer.php, function feedback( )
      towards the end:
      $output .= "Total penalties so far: ".$gradedstep->get_behaviour_var('_try') * $qa->get_question()->penalty;
      return $output;

      Show
      Very easy: just add the following line to question/behaviour/adaptive/renderer.php, function feedback( ) towards the end: $output .= "Total penalties so far: ".$gradedstep->get_behaviour_var('_try') * $qa->get_question()->penalty; return $output;
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      19287

      Description

      In Adaptive mode (with penalty set), when student submits (Check button) successive incorrect answers, the "This submission attracted a penalty of..." message remains static on the screen. This may cause the student to believe that he is keeping that penalty for the whole of his successive tries. I suggest it would be much more informative to dynamically display the Total number of penalties incurred. That way, the student would understand better how many more tries he's got left before his final submission leaves him with a mark of zero.

        Activity

        Hide
        Henning Bostelmann added a comment -

        This patch introduces the following improvements to the penalty information in adaptive mode:

        (1) "Total penalty" message added as suggested by Joseph. Note: I've chosen to display this only when the total penalty is actually higher than the penalty incurred in the present try. Not sure whether this is the best way to do.

        (2) MDL-29780 fixed.

        (3) Fixed another bug: So far, penalties were always displayed as a fraction between 0 and 1, not as a fraction of the maximum score for the question. Unfortunately this was incorrectly coded in the unit tests as well.

        (4) Changed wording of 'gradingdetailsadjustment' as suggested by Tim in the discussion of MDL-28219.

        (5) Unit tests have been fixed and extended.

        Show
        Henning Bostelmann added a comment - This patch introduces the following improvements to the penalty information in adaptive mode: (1) "Total penalty" message added as suggested by Joseph. Note: I've chosen to display this only when the total penalty is actually higher than the penalty incurred in the present try. Not sure whether this is the best way to do. (2) MDL-29780 fixed. (3) Fixed another bug: So far, penalties were always displayed as a fraction between 0 and 1, not as a fraction of the maximum score for the question. Unfortunately this was incorrectly coded in the unit tests as well. (4) Changed wording of 'gradingdetailsadjustment' as suggested by Tim in the discussion of MDL-28219 . (5) Unit tests have been fixed and extended.
        Hide
        Henning Bostelmann added a comment -

        Hi Tim, any chance of getting this one reviewed?

        Show
        Henning Bostelmann added a comment - Hi Tim, any chance of getting this one reviewed?
        Hide
        Tim Hunt added a comment -

        This looks good to me. Submitting for integration.

        I think this should go into 2.2.1 and 2.3. (Sorry we missed the actual 2.2 release.) I don't think we should change this on the 2.1 stable branch.

        Show
        Tim Hunt added a comment - This looks good to me. Submitting for integration. I think this should go into 2.2.1 and 2.3. (Sorry we missed the actual 2.2 release.) I don't think we should change this on the 2.1 stable branch.
        Hide
        Henning Bostelmann added a comment -

        Hi Tim,

        thanks (as always) for reviewing.

        There's some actual bugs here that might be worth porting back to 2.1: First, MDL-29780; then, point (3) as mentioned above.

        Should I port these to 2.1? Maybe in MDL-29780, not in this bug? Sorry for muddling bugs and improvements here.

        Show
        Henning Bostelmann added a comment - Hi Tim, thanks (as always) for reviewing. There's some actual bugs here that might be worth porting back to 2.1: First, MDL-29780 ; then, point (3) as mentioned above. Should I port these to 2.1? Maybe in MDL-29780 , not in this bug? Sorry for muddling bugs and improvements here.
        Hide
        Tim Hunt added a comment -

        The new behaviour is clearly better, so we might get away with just putting it in 2.1, or it might really annoy someone. I don't know which way to call it. I'll post in the quiz forum and see if I can get any other opinions.

        Show
        Tim Hunt added a comment - The new behaviour is clearly better, so we might get away with just putting it in 2.1, or it might really annoy someone. I don't know which way to call it. I'll post in the quiz forum and see if I can get any other opinions.
        Show
        Tim Hunt added a comment - http://moodle.org/mod/forum/discuss.php?d=191851
        Hide
        Ray Lawrence added a comment -

        -1 For moving this back to 2.1

        Show
        Ray Lawrence added a comment - -1 For moving this back to 2.1
        Hide
        Oleg Sychev added a comment -

        -1 since it means two versions of adaptive with hint behaviour to deal with difference in two versions of adaptive behaviour

        The differences would arise, since penalty calculation with hints differs from penalty calculations without them, and just $gradedstep->get_behaviour_var('_try') * $qa->get_question()->penalty isn't total penalty when dialing with hints.

        I would need to have a clear version change to specify to users which version of adaptive with hint penalty they should use with which internal Moodle adapative behaviour.

        Show
        Oleg Sychev added a comment - -1 since it means two versions of adaptive with hint behaviour to deal with difference in two versions of adaptive behaviour The differences would arise, since penalty calculation with hints differs from penalty calculations without them, and just $gradedstep->get_behaviour_var('_try') * $qa->get_question()->penalty isn't total penalty when dialing with hints. I would need to have a clear version change to specify to users which version of adaptive with hint penalty they should use with which internal Moodle adapative behaviour.
        Hide
        Henning Bostelmann added a comment -

        Oleg,

        I can't quite follow - the "adaptive" behaviour doesn't use hints at all. It duplicates the behaviour of the Moodle 1.9 quiz, which didn't have the concept of hints. You may be referring to the "interactive" behaviour, which uses hints but is outside the scope of this change.

        Show
        Henning Bostelmann added a comment - Oleg, I can't quite follow - the "adaptive" behaviour doesn't use hints at all. It duplicates the behaviour of the Moodle 1.9 quiz, which didn't have the concept of hints. You may be referring to the "interactive" behaviour, which uses hints but is outside the scope of this change.
        Hide
        Oleg Sychev added a comment -

        Henning,

        you could better follow me if you look at the behaviours code (http://code.google.com/p/oasychev-moodle-plugins/source/checkout - you could browse the code even if you don't willing to use Hg) or read hinting part of http://docs.moodle.org/21/en/Preg_question_type . These are not new Moodle 2.1 "hints" at all (I asked Tim to rename them to something more specific, like "teacher-defined hints" or something to avoid confusion, but he ignoring it so far).

        These are response based hints that Joseph Rezeau REGEX and (later) my PREG question types had since Moodle 1.x times, they worked with adaptive quizzes traditionally and need it adaptive support at least for the sake of backward compatibility. For now it's next character hint, that allows student to see next possible character if student was stuck, for some penalty. That is why it changes the way penalty is calculated (some penalty for tries and some for hints) and why this change bother me.

        Show
        Oleg Sychev added a comment - Henning, you could better follow me if you look at the behaviours code ( http://code.google.com/p/oasychev-moodle-plugins/source/checkout - you could browse the code even if you don't willing to use Hg) or read hinting part of http://docs.moodle.org/21/en/Preg_question_type . These are not new Moodle 2.1 "hints" at all (I asked Tim to rename them to something more specific, like "teacher-defined hints" or something to avoid confusion, but he ignoring it so far). These are response based hints that Joseph Rezeau REGEX and (later) my PREG question types had since Moodle 1.x times, they worked with adaptive quizzes traditionally and need it adaptive support at least for the sake of backward compatibility. For now it's next character hint, that allows student to see next possible character if student was stuck, for some penalty. That is why it changes the way penalty is calculated (some penalty for tries and some for hints) and why this change bother me.
        Hide
        Oleg Sychev added a comment -

        Looking at the code more thoroughtly, I would be very much obliged if a calculation of current and total penalties for dispaly would be separated in special function, instead of been in the middle of renderer method penalty_info.

        This simple refactoring would make it much easier for child behaviours to overload the way how total penalty calculated without touching the way it is displayed (or change display without touching calculation).

        It may be even wise to place a function to calculate penalties ($currentpenalty and $totalpenalty) in the behaviour class, leaving renderer only display info. It's actually isn't renderer's work to calculate something (even if it's just two strings of code) and that way we could have unit-tests for penalty calculation function.

        Show
        Oleg Sychev added a comment - Looking at the code more thoroughtly, I would be very much obliged if a calculation of current and total penalties for dispaly would be separated in special function, instead of been in the middle of renderer method penalty_info. This simple refactoring would make it much easier for child behaviours to overload the way how total penalty calculated without touching the way it is displayed (or change display without touching calculation). It may be even wise to place a function to calculate penalties ($currentpenalty and $totalpenalty) in the behaviour class, leaving renderer only display info. It's actually isn't renderer's work to calculate something (even if it's just two strings of code) and that way we could have unit-tests for penalty calculation function.
        Hide
        Oleg Sychev added a comment -

        Also - that is clearly for Tim to decide - I don't actually like re-wording putting word "tries" in penalty info. I would prefer neutral strings, that could be reused with penalty for anything, not just tries.

        Show
        Oleg Sychev added a comment - Also - that is clearly for Tim to decide - I don't actually like re-wording putting word "tries" in penalty info. I would prefer neutral strings, that could be reused with penalty for anything, not just tries.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Henning Bostelmann added a comment -

        The discussion here has turned a bit from modifying the displayed output of the Core component (which the bug description is about) to refactoring the code in order to provide an API for Contrib code (which is what Oleg is proposing).

        Tim, it seems to me that the refactoring/API part would clearly be a separate issue, and in any case nothing for a stable branch.

        The present change is integration ready for Moodle 2.2 and 2.3. Regarding 2.1, if you want a bugfix-only backport, this would likely contain points (2) and (3) above, plus partially (5), and I can do it here, or separately in MDL-29780.

        Show
        Henning Bostelmann added a comment - The discussion here has turned a bit from modifying the displayed output of the Core component (which the bug description is about) to refactoring the code in order to provide an API for Contrib code (which is what Oleg is proposing). Tim, it seems to me that the refactoring/API part would clearly be a separate issue, and in any case nothing for a stable branch. The present change is integration ready for Moodle 2.2 and 2.3. Regarding 2.1, if you want a bugfix-only backport, this would likely contain points (2) and (3) above, plus partially (5), and I can do it here, or separately in MDL-29780 .
        Hide
        Tim Hunt added a comment -

        I agree. This fix in this bug is good, and should get integrated.

        The code can be improved more in future. Oleg is right that, according to general principles, we should not be computing things like penalties in the renderer, it would be better to call a method on the behaviour. However, we can re-factor to achieve that later. Particularly because we have good unit tests.

        Show
        Tim Hunt added a comment - I agree. This fix in this bug is good, and should get integrated. The code can be improved more in future. Oleg is right that, according to general principles, we should not be computing things like penalties in the renderer, it would be better to call a method on the behaviour. However, we can re-factor to achieve that later. Particularly because we have good unit tests.
        Hide
        Oleg Sychev added a comment -

        Well, Tim, it's up to you whether you think having calculations in the renderer is acceptable. I just brought the matter to you attention.

        However, you probably shoudn't count on me much on doing such refactoring. I could live without it - especially given overall bad state of adaptive mode code. DIY.

        Show
        Oleg Sychev added a comment - Well, Tim, it's up to you whether you think having calculations in the renderer is acceptable. I just brought the matter to you attention. However, you probably shoudn't count on me much on doing such refactoring. I could live without it - especially given overall bad state of adaptive mode code. DIY.
        Hide
        Henning Bostelmann added a comment -

        Added backport branch for MOODLE_21_STABLE, as discussed.

        Show
        Henning Bostelmann added a comment - Added backport branch for MOODLE_21_STABLE, as discussed.
        Hide
        Aparup Banerjee added a comment -

        Thanks everyone for their work and input!

        This has been integrated and is ready for testing (2.1+, 2.2+ master)

        Tester: please also test the unit tests are fine @ question/behaviour/adaptive

        Show
        Aparup Banerjee added a comment - Thanks everyone for their work and input! This has been integrated and is ready for testing (2.1+, 2.2+ master) Tester: please also test the unit tests are fine @ question/behaviour/adaptive
        Hide
        Ankit Agarwal added a comment -

        Hi,
        This works great on 22 and master.
        Not sure if am missing some setting or something but doesn't work for me on 21.
        I have set the quiz and question as per the instruction but when I enter an answer (example 'a') and submit, no feedback message is shown at all.
        Thanks

        Show
        Ankit Agarwal added a comment - Hi, This works great on 22 and master. Not sure if am missing some setting or something but doesn't work for me on 21. I have set the quiz and question as per the instruction but when I enter an answer (example 'a') and submit, no feedback message is shown at all. Thanks
        Hide
        Aparup Banerjee added a comment -

        The patch applied for 2.1 was significantly different from 2.2 onwards. We may need separate/subset testing instructions for 2.1.

        Show
        Aparup Banerjee added a comment - The patch applied for 2.1 was significantly different from 2.2 onwards. We may need separate/subset testing instructions for 2.1.
        Hide
        Henning Bostelmann added a comment -

        "No feedback message at all" is certainly not expected in 2.1. You should still see "This submission attracted a penalty of..." when entering a wrong answer first, like described in the testing instructions.

        I'll look into that and get back to you.

        Show
        Henning Bostelmann added a comment - "No feedback message at all" is certainly not expected in 2.1. You should still see "This submission attracted a penalty of..." when entering a wrong answer first, like described in the testing instructions. I'll look into that and get back to you.
        Hide
        Henning Bostelmann added a comment -

        Sorry, I can't reproduce the problem at the moment. (I tested on the branch MDL-29772-21 given above, as well as on the MOODLE_21_STABLE branch from the integration repository.) For me, it's working as expected.

        Are you sure that, in the quiz settings screen, you chose "Adaptive mode" rather than "Adaptive mode (no penalties)"?

        Are the unit tests working for you?

        Show
        Henning Bostelmann added a comment - Sorry, I can't reproduce the problem at the moment. (I tested on the branch MDL-29772 -21 given above, as well as on the MOODLE_21_STABLE branch from the integration repository.) For me, it's working as expected. Are you sure that, in the quiz settings screen, you chose "Adaptive mode" rather than "Adaptive mode (no penalties)"? Are the unit tests working for you?
        Hide
        Aparup Banerjee added a comment -

        Ankit, as promised, my test results for 2.1 after running through test sequence on "Adaptive mode" , (default mark=3) :
        test(a) & test(b) both have penalty message (1.00). last messsage -
        Marks for this submission: 3.00/3.00. With previous penalties this gives 1.00/3.00.
        (or 0.33/1.00 if i set to 1 mark)
        (no penalties) : gives me "Marks for this submission: 3.00/3.00." at end of test sequence.

        this test seems to work for me on 2.1.

        Show
        Aparup Banerjee added a comment - Ankit, as promised, my test results for 2.1 after running through test sequence on "Adaptive mode" , (default mark=3) : test(a) & test(b) both have penalty message (1.00). last messsage - Marks for this submission: 3.00/3.00. With previous penalties this gives 1.00/3.00. (or 0.33/1.00 if i set to 1 mark) (no penalties) : gives me "Marks for this submission: 3.00/3.00." at end of test sequence. this test seems to work for me on 2.1.
        Hide
        Joseph Rézeau added a comment -

        Aparup Banerjee added a comment - 13/Dec/11 5:11 PM
        when you say


        This has been integrated and is ready for testing (2.1+, 2.2+ master)


        What is the URL of the "integration repository"?
        I would like to test this.
        Joseph

        Show
        Joseph Rézeau added a comment - Aparup Banerjee added a comment - 13/Dec/11 5:11 PM when you say This has been integrated and is ready for testing (2.1+, 2.2+ master) What is the URL of the "integration repository"? I would like to test this. Joseph
        Hide
        Petr Škoda added a comment -

        git://git.moodle.org/integration.git

        Show
        Petr Škoda added a comment - git://git.moodle.org/integration.git
        Hide
        Joseph Rézeau added a comment -

        @Petr
        in my browser I get error: git://git.moodle.org/integration.git
        The requested URL /integration.git was not found on this server.

        Show
        Joseph Rézeau added a comment - @Petr in my browser I get error: git://git.moodle.org/integration.git The requested URL /integration.git was not found on this server.
        Hide
        Petr Škoda added a comment -

        URI staring with git:// can be used in git clients such as CLI git, GitTower, SourceTree, GitX, SmartGit, Netbeans, egit for Eclipse, etc. If you want web frontend for your web browser you can use http://git.moodle.org/gw

        Show
        Petr Škoda added a comment - URI staring with git:// can be used in git clients such as CLI git, GitTower, SourceTree, GitX, SmartGit, Netbeans, egit for Eclipse, etc. If you want web frontend for your web browser you can use http://git.moodle.org/gw
        Hide
        Joseph Rézeau added a comment -

        Tested on moodle 2.1 and 2.2. Works as expected.
        Joseph

        Show
        Joseph Rézeau added a comment - Tested on moodle 2.1 and 2.2. Works as expected. Joseph
        Hide
        Ankit Agarwal added a comment -

        Hi Guys,
        Thanks for the help with testing it out.
        I found the issue with my 21 branch, I had the feedback options unchecked under "During the attempt" option, so was getting no feedback.
        Anyways works fine now, sorry for all the rush.
        passing
        Thanks

        Show
        Ankit Agarwal added a comment - Hi Guys, Thanks for the help with testing it out. I found the issue with my 21 branch, I had the feedback options unchecked under "During the attempt" option, so was getting no feedback. Anyways works fine now, sorry for all the rush. passing Thanks
        Hide
        Sam Hemelryk added a comment -

        Moved back ready for testing again, as requested.

        Show
        Sam Hemelryk added a comment - Moved back ready for testing again, as requested.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

        Now... disconnect, relax and enjoy the next days, yay!

        Closing...ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks! Now... disconnect, relax and enjoy the next days, yay! Closing...ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: