Details

    • Testing Instructions:
      Hide

      Un edited forms containing the combined feedback fields should contain the relevant pre filled text as follows
      The field 'For any correct response' should contain "Your answer is correct."
      The field 'For any partially correct response' should contain 'Your answer is partially correct'
      The field 'For any incorrect response' should contain "Your answer is incorrect".

      Relevant questions types are calculated multichoice, select missing words, Multiple choice, Matching, Drag and drop onto text, Drag and drop onto image,

      Show
      Un edited forms containing the combined feedback fields should contain the relevant pre filled text as follows The field 'For any correct response' should contain "Your answer is correct." The field 'For any partially correct response' should contain 'Your answer is partially correct' The field 'For any incorrect response' should contain "Your answer is incorrect". Relevant questions types are calculated multichoice, select missing words, Multiple choice, Matching, Drag and drop onto text, Drag and drop onto image,
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
    • Rank:
      38319

      Description

      There are a few question types that involve selecting multiple responses and these have a combined feedback section.
      pre-fill 'For any correct response' with "Your answer is correct."
      pre-fill 'For any partially correct response' with 'Your answer is partially correct'
      pre-fill 'For any incorrect response'with "Your answer is incorrect".

      Handling different languages
      Use the standard approach with languages. English only handled initially. The pre fill value will be supplied through the standard Moodle language files.

      The reason for the pre fill example is that the size of the text boxes implies too much information is needed. The hint gives a better example. It also means we can reduce the size of the text box.

      The only change required is in adding the pre fill hint. If the hint is not changed before the question type is saved the hint text will be saved as the appropriate response.

        Issue Links

          Activity

          Hide
          Colin Chambers added a comment -

          Changes committed and pushed. Now awaiting review.

          Show
          Colin Chambers added a comment - Changes committed and pushed. Now awaiting review.
          Hide
          Tim Hunt added a comment -

          Looks good, just a couple of things to consider:

          1. You don't need the line $element = null;

          2. Should you add a comment explaining that you are using setValue, not setDefault, because the latter does not work for editors.

          3. I think a funcitonality change like this should only go into master, not 2.2, so I think we only need the master branch.

          Show
          Tim Hunt added a comment - Looks good, just a couple of things to consider: 1. You don't need the line $element = null; 2. Should you add a comment explaining that you are using setValue, not setDefault, because the latter does not work for editors. 3. I think a funcitonality change like this should only go into master, not 2.2, so I think we only need the master branch.
          Hide
          Colin Chambers added a comment -

          Good points. Tim previously asked 'I would expect this one to use $mform->setDefault() instead of $element->setValue().'. My answer was 'You would if editor elements accepted that default. I believe I tried it and they didn't. Hence having to go the route I did.'

          You don't specifically need the line $element = null; I do this for performance reasons when using loops. Without this line the memory space for $element must be allocated for each iteration of the loop. With the line the same memory location is used. Saving the need to clear and allocate any. This is the case in javascript anyhow. that's where I've tested it. I'm assuming PHP works the same way. I can understand it's not a big concern here but it's a habit of mine. I'm interested in your opinion. Maybe eaccelerator and similar tools remove the need for this or maybe I'm mistaken.

          Master branch only is fine with me.

          Show
          Colin Chambers added a comment - Good points. Tim previously asked 'I would expect this one to use $mform->setDefault() instead of $element->setValue().'. My answer was 'You would if editor elements accepted that default. I believe I tried it and they didn't. Hence having to go the route I did.' You don't specifically need the line $element = null; I do this for performance reasons when using loops. Without this line the memory space for $element must be allocated for each iteration of the loop. With the line the same memory location is used. Saving the need to clear and allocate any. This is the case in javascript anyhow. that's where I've tested it. I'm assuming PHP works the same way. I can understand it's not a big concern here but it's a habit of mine. I'm interested in your opinion. Maybe eaccelerator and similar tools remove the need for this or maybe I'm mistaken. Master branch only is fine with me.
          Hide
          Tim Hunt added a comment -

          1. The performance of a Moodle page is pretty much determined by the number of DB queries it does. This sort of PHP micro-optimisation is essentially irrelevant. Also, with garbage collectors, your intuition may well be wrong. So for me, robustness of the code is more important, so I would rather see the scope of $element being just inside the loop - even though PHP scope does not really work like that.

          2. I really meant a comment in the code, not in the bug. Sorry, I did not realise I was being ambiguous

          Show
          Tim Hunt added a comment - 1. The performance of a Moodle page is pretty much determined by the number of DB queries it does. This sort of PHP micro-optimisation is essentially irrelevant. Also, with garbage collectors, your intuition may well be wrong. So for me, robustness of the code is more important, so I would rather see the scope of $element being just inside the loop - even though PHP scope does not really work like that. 2. I really meant a comment in the code, not in the bug. Sorry, I did not realise I was being ambiguous
          Show
          Colin Chambers added a comment - fixed comments as commit https://github.com/colchambers/moodle/commit/dbb1024ae5cefe3c4882a519166c348a34696325
          Hide
          Tim Hunt added a comment -

          This looks good now. I think it is better to wait and submit this to Moodle 2.4 along with the rest of the editing form changes, rather than trying to squeeze it into 2.3 after the code-freeze, So I will leave this in peer-review state for now.

          Show
          Tim Hunt added a comment - This looks good now. I think it is better to wait and submit this to Moodle 2.4 along with the rest of the editing form changes, rather than trying to squeeze it into 2.3 after the code-freeze, So I will leave this in peer-review state for now.
          Hide
          Tim Hunt added a comment -

          Col, This is ready to go. I think there is just one more thing you need to do before I click the 'Submit for integration' button. Please could you rebase this commit on top of the latest master from git.moodle.org.

          Show
          Tim Hunt added a comment - Col, This is ready to go. I think there is just one more thing you need to do before I click the 'Submit for integration' button. Please could you rebase this commit on top of the latest master from git.moodle.org.
          Hide
          Colin Chambers added a comment -

          I'll also add full stops to each feedback.

          Show
          Colin Chambers added a comment - I'll also add full stops to each feedback.
          Hide
          Colin Chambers added a comment -

          Combined feedback does not have "Show the number of correct responses" ticked but Drag and Drop into text and onto images do have it ticked and I like it. We should be consistent and tick the box when the form is new.

          Create a new feature request if this feature is not quickly to implement and prevents integration.

          Show
          Colin Chambers added a comment - Combined feedback does not have "Show the number of correct responses" ticked but Drag and Drop into text and onto images do have it ticked and I like it. We should be consistent and tick the box when the form is new. Create a new feature request if this feature is not quickly to implement and prevents integration.
          Hide
          Colin Chambers added a comment -

          Rebased on master and added full stops. Latest commit https://github.com/colchambers/moodle/commit/544153fb9360b6dc16738e644802c8f1983deac5

          New bug MDL-37465 for "Show the number of correct responses" ticked by default

          Show
          Colin Chambers added a comment - Rebased on master and added full stops. Latest commit https://github.com/colchambers/moodle/commit/544153fb9360b6dc16738e644802c8f1983deac5 New bug MDL-37465 for "Show the number of correct responses" ticked by default
          Hide
          Tim Hunt added a comment -

          +1. Submitting for integration now.

          Show
          Tim Hunt added a comment - +1. Submitting for integration now.
          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
          Eloy Lafuente (stronk7) added a comment -

          Integrated (master only), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
          Hide
          Michael de Raadt added a comment - - edited

          Before starting this test, I thought I should comment that the testing instructions are lacking details. Certain assumptions are made and not communicated. Where should the test start? Should only the listed question types be tested?

          Show
          Michael de Raadt added a comment - - edited Before starting this test, I thought I should comment that the testing instructions are lacking details. Certain assumptions are made and not communicated. Where should the test start? Should only the listed question types be tested?
          Hide
          Michael de Raadt added a comment -

          Test result: Success.

          Tested in Master only.

          I tested this using a new quiz adding a multiple choice, calculated multiple choice and matching question. I also added a few other question types. The remaining question types suggested in the testing instructions are not standard and were not tested.

          The default values for correct, incorrect and partially correct answers were added to new question forms. I also tried overriding the default feedback and that stuck. I ran this through a few students giving correct, incorrect and partially correct answers and the responses seemed appropriate.

          Show
          Michael de Raadt added a comment - Test result: Success. Tested in Master only. I tested this using a new quiz adding a multiple choice, calculated multiple choice and matching question. I also added a few other question types. The remaining question types suggested in the testing instructions are not standard and were not tested. The default values for correct, incorrect and partially correct answers were added to new question forms. I also tried overriding the default feedback and that stuck. I ran this through a few students giving correct, incorrect and partially correct answers and the responses seemed appropriate.
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!
          Hide
          Colin Chambers added a comment -

          Thanks Guys. Nice to finally get this in.

          Michael. Thanks for describing your testing more fully. I never get feedback within the OU so it doesn't encourage me to add much to the testing instructions. To do it properly takes time. I'm also new to working on quiz and questions so I can't say I know all the angles to test. Apologies.

          Your example makes it clearer what you guys need. Is there a way I can replicate some of your testing approach here? A set of standard questions and users you test against?

          We have a good list we've built but it's OU specific. It's nice to see what you guys use too.

          Show
          Colin Chambers added a comment - Thanks Guys. Nice to finally get this in. Michael. Thanks for describing your testing more fully. I never get feedback within the OU so it doesn't encourage me to add much to the testing instructions. To do it properly takes time. I'm also new to working on quiz and questions so I can't say I know all the angles to test. Apologies. Your example makes it clearer what you guys need. Is there a way I can replicate some of your testing approach here? A set of standard questions and users you test against? We have a good list we've built but it's OU specific. It's nice to see what you guys use too.
          Hide
          Michael de Raadt added a comment -

          Hi, Colin.

          My apologies if I came across too strongly in my initial testing comment. It must have been a long testing day.

          We don't have guidelines for writing functional tests. We're on the cusp of switching to a system for automating functional tests, which has its own language.

          In the meantime, here is an example of an issue with some good testing instructions: MDL-32639. I guess the key is to write for a user who is competent, but has not been following the issue you are working on.

          Thanks for your work on this issue. Hopefully we will see more of your work.

          Show
          Michael de Raadt added a comment - Hi, Colin. My apologies if I came across too strongly in my initial testing comment. It must have been a long testing day. We don't have guidelines for writing functional tests. We're on the cusp of switching to a system for automating functional tests, which has its own language. In the meantime, here is an example of an issue with some good testing instructions: MDL-32639 . I guess the key is to write for a user who is competent, but has not been following the issue you are working on. Thanks for your work on this issue. Hopefully we will see more of your work.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: