Moodle
  1. Moodle
  2. MDL-28183

Add num parts correct and clear wrong options to multianswer (cloze)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.3.3, 2.4
    • Fix Version/s: 2.4.1
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide
      1. Create at least one multianswer (Cloze) question. You can use some examples for Moodle docs: http://docs.moodle.org/24/en/Cloze. Example 2 is a good candidate because it has many subquestions and include samples of all subquestions types (it is specially important to verify that horizontal and vertical multichoice subquestions (MCH and MCV) are working because there were some problems with them during development of the code). Additionnaly, you can use Example 1 too of you want also to test with a smaller question.
      2. Don't forget to add several hints to your question(s), for instance "This is the first hint", "This is second hint", ...
        The number of tries allowed in interactive with multiple tries is equal to the number of hints + 1. So if you ceate 2 hints you will have 3 tries allowed.
      3. For each hint try some combination of checked/unchecked for the 2 checkboxes "Show the number of correct responses" and "Clear incorrect responses" ideally all combinations should be tested.
      4. Save and preview the question(s) setting "How questions behave" to "Interactive with multiple tries"
      5. Verify that for each try when you click on the "Test" button the number of correctly answered subquestions is displayed after "You have correctly selected" if and only if "Show the number of correct responses" was checked for the corresponding hint
      6. Verify that after clicking on the "Try again" button, if and only if the "Clear incorrect responses" checkbox was checked for the corresponding hint, all the subquestions answers that were wrong (but no other ones) are correctly cleared (shortanswer and numerical boxes should be empty, radio buttons should not be selected, select should be set to empty option).
      7. Try exporting your subquestions using the Moodle XML format and re-iporting them back and verify that hint's have been correctly exported and imported.
      Show
      Create at least one multianswer (Cloze) question. You can use some examples for Moodle docs: http://docs.moodle.org/24/en/Cloze . Example 2 is a good candidate because it has many subquestions and include samples of all subquestions types (it is specially important to verify that horizontal and vertical multichoice subquestions (MCH and MCV) are working because there were some problems with them during development of the code). Additionnaly, you can use Example 1 too of you want also to test with a smaller question. Don't forget to add several hints to your question(s), for instance "This is the first hint", "This is second hint", ... The number of tries allowed in interactive with multiple tries is equal to the number of hints + 1. So if you ceate 2 hints you will have 3 tries allowed. For each hint try some combination of checked/unchecked for the 2 checkboxes "Show the number of correct responses" and "Clear incorrect responses" ideally all combinations should be tested. Save and preview the question(s) setting "How questions behave" to "Interactive with multiple tries" Verify that for each try when you click on the "Test" button the number of correctly answered subquestions is displayed after "You have correctly selected" if and only if "Show the number of correct responses" was checked for the corresponding hint Verify that after clicking on the "Try again" button, if and only if the "Clear incorrect responses" checkbox was checked for the corresponding hint, all the subquestions answers that were wrong (but no other ones) are correctly cleared (shortanswer and numerical boxes should be empty, radio buttons should not be selected, select should be set to empty option). Try exporting your subquestions using the Moodle XML format and re-iporting them back and verify that hint's have been correctly exported and imported.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      17797

      Description

      In interactive with multiple tries mode question types with multiples parts can offer 2 options: "Show the number of correct responses" and "Clear incorrect responses". The multianswer (Cloze) question type should offer these 2 options like the match question type does.
      Tim's original description:
      Apologies that this was missing from the 2.1 release. That was an oversight on my part.

        Issue Links

          Activity

          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim this issue is certainly not a priority. So maybe you will be happy if somebody else create a git branch and you only have to give advices and correct mistakes ?
          This code is just a first try because I never really studied how multianswer works in QE2.
          Not sure that I catched all the bits that need changing.
          Of course we need some unit tests too.
          Would it be better to test the 2 new functions in question_test or do an overall test in walkthrough_test ?

          Show
          Jean-Michel Vedrine added a comment - Hello Tim this issue is certainly not a priority. So maybe you will be happy if somebody else create a git branch and you only have to give advices and correct mistakes ? This code is just a first try because I never really studied how multianswer works in QE2. Not sure that I catched all the bits that need changing. Of course we need some unit tests too. Would it be better to test the 2 new functions in question_test or do an overall test in walkthrough_test ?
          Hide
          Tim Hunt added a comment -

          Ideally you would create both sorts of tests. Low-level unit tests of the question class, and then walkthrough_tests, which are not strictly speaking unit tests, but very useful, none the less.

          Show
          Tim Hunt added a comment - Ideally you would create both sorts of tests. Low-level unit tests of the question class, and then walkthrough_tests, which are not strictly speaking unit tests, but very useful, none the less.
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks will try to do both. I have added some low level tests.

          Show
          Jean-Michel Vedrine added a comment - Thanks will try to do both. I have added some low level tests.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          I was trying to use the multianswer question with four multichoice subquestions defined in make_multianswer_question_fourmc in question/type/multianswer/tests/helper.php
          to do a walkthrough test and was puzzled because some things were not working as expected.
          So I was thinking my code has some problems then I discovered that the subquestions have fraction with type boolean because of the $data['California'] == 'OK' and $data['Arizona'] == 'OK' used when creating them. So for instance the get_correct_response function was returning array(). Of course it didn't caused any trouble the way it was used in function test_deferred_feedback in qbehaviour_interactive_walkthrough_test but I have some trouble using it as it is.
          Do you prefer I use another question defined in the helper or correct this question so that subquestions fraction are numeric so I can use it ?
          Have a nice week-end.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I was trying to use the multianswer question with four multichoice subquestions defined in make_multianswer_question_fourmc in question/type/multianswer/tests/helper.php to do a walkthrough test and was puzzled because some things were not working as expected. So I was thinking my code has some problems then I discovered that the subquestions have fraction with type boolean because of the $data ['California'] == 'OK' and $data ['Arizona'] == 'OK' used when creating them. So for instance the get_correct_response function was returning array(). Of course it didn't caused any trouble the way it was used in function test_deferred_feedback in qbehaviour_interactive_walkthrough_test but I have some trouble using it as it is. Do you prefer I use another question defined in the helper or correct this question so that subquestions fraction are numeric so I can use it ? Have a nice week-end.
          Hide
          Jean-Michel Vedrine added a comment -

          I have commited an interactive walkthrough test to github.
          There is still a problem remaining in my test (I think the problem is in the test not in the clear_wrong_from_response function because if I try the exact same multianswer question live the wrong responses are cleared after the first check when I click on the Try again button) the fragment
          // Do try again.
          $this->process_submission(array('-tryagain' => 1));

          // Verify.
          $this->check_current_state(question_state::$todo);
          $this->check_current_mark(null);
          $this->check_current_output(
          $this->get_contains_marked_out_of_summary(),
          $this->get_contains_select_expectation('sub1_answer', $choices, 1, true),
          $this->get_contains_select_expectation('sub2_answer', $choices, 1, true),
          $this->get_contains_select_expectation('sub3_answer', $choices, 1, true),
          $this->get_contains_select_expectation('sub4_answer', $choices, 1, true),
          $this->get_contains_submit_button_expectation(true),
          $this->get_does_not_contain_correctness_expectation(),
          $this->get_does_not_contain_feedback_expectation(),
          $this->get_tries_remaining_expectation(2),
          $this->get_no_hint_visible_expectation());
          Should fail because two of the four select should not have any selected tag.

          Show
          Jean-Michel Vedrine added a comment - I have commited an interactive walkthrough test to github. There is still a problem remaining in my test (I think the problem is in the test not in the clear_wrong_from_response function because if I try the exact same multianswer question live the wrong responses are cleared after the first check when I click on the Try again button) the fragment // Do try again. $this->process_submission(array('-tryagain' => 1)); // Verify. $this->check_current_state(question_state::$todo); $this->check_current_mark(null); $this->check_current_output( $this->get_contains_marked_out_of_summary(), $this->get_contains_select_expectation('sub1_answer', $choices, 1, true), $this->get_contains_select_expectation('sub2_answer', $choices, 1, true), $this->get_contains_select_expectation('sub3_answer', $choices, 1, true), $this->get_contains_select_expectation('sub4_answer', $choices, 1, true), $this->get_contains_submit_button_expectation(true), $this->get_does_not_contain_correctness_expectation(), $this->get_does_not_contain_feedback_expectation(), $this->get_tries_remaining_expectation(2), $this->get_no_hint_visible_expectation()); Should fail because two of the four select should not have any selected tag.
          Hide
          Jean-Michel Vedrine added a comment -

          Stupid me ! If I had looked at how it works !!

          Show
          Jean-Michel Vedrine added a comment - Stupid me ! If I had looked at how it works !!
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          This is beginning to look good. I have the walkthrough test working but I really don't know if I am testing all that should be tested. Review it when you can, it's not a high priority issue.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, This is beginning to look good. I have the walkthrough test working but I really don't know if I am testing all that should be tested. Review it when you can, it's not a high priority issue.
          Hide
          Tim Hunt added a comment -

          Fixing the test helper class is right, and I see you have done that.

          I don't think your walkthrough test handles clear wrong properly yet. test_match_clear_wrong in question/type/match/tests/walkthrough_test.php might give you some clues.

          Show
          Tim Hunt added a comment - Fixing the test helper class is right, and I see you have done that. I don't think your walkthrough test handles clear wrong properly yet. test_match_clear_wrong in question/type/match/tests/walkthrough_test.php might give you some clues.
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks that was the example of code I missed !!

          Show
          Jean-Michel Vedrine added a comment - Thanks that was the example of code I missed !!
          Hide
          Jean-Michel Vedrine added a comment -

          Is it better now ?

          Show
          Jean-Michel Vedrine added a comment - Is it better now ?
          Hide
          Tim Hunt added a comment -

          This looks great. I have not yet downloaded the code and tried it, I have just reviewed the changes and the unit tests, but it certainly looks right. I will try to test it tomorrow at work, then this can be submitted for integration.

          The only things I noticed were really minor. None of these are significant enough that you need to fix them:

          1. In test_get_num_parts_right, you could add at least one assert on $numparts.

          2. I would write a cast with a space after the (float) bit. However, the coding style (http://docs.moodle.org/dev/Coding_style) does not express an opinion, so you don't need to change that.

          3. While you are in the area, you could fix the \ No newline at end of file in question/type/multianswer/tests/question_test.php

          4. As far as I can see, you only ever use reset($rightchoice), so I think the tests would be clearer if you did $rightchoice = reset($rightchoice); once at the start.

          Finally, before this is submitted for integration, please can you use git rebase -i to squash it down to a single commit with a nice commit comment.

          Show
          Tim Hunt added a comment - This looks great. I have not yet downloaded the code and tried it, I have just reviewed the changes and the unit tests, but it certainly looks right. I will try to test it tomorrow at work, then this can be submitted for integration. The only things I noticed were really minor. None of these are significant enough that you need to fix them: 1. In test_get_num_parts_right, you could add at least one assert on $numparts. 2. I would write a cast with a space after the (float) bit. However, the coding style ( http://docs.moodle.org/dev/Coding_style ) does not express an opinion, so you don't need to change that. 3. While you are in the area, you could fix the \ No newline at end of file in question/type/multianswer/tests/question_test.php 4. As far as I can see, you only ever use reset($rightchoice), so I think the tests would be clearer if you did $rightchoice = reset($rightchoice); once at the start. Finally, before this is submitted for integration, please can you use git rebase -i to squash it down to a single commit with a nice commit comment.
          Hide
          Tim Hunt added a comment -

          Now I am looking at MDL-36955, and I see you have done $right = reset($rightchoice); there

          Show
          Tim Hunt added a comment - Now I am looking at MDL-36955 , and I see you have done $right = reset($rightchoice); there
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Hello Tim,
          I have integrated all the results of your review in the code I think
          I have re-run the tests and the codechecker just to be sure
          I have squashed all commits into one

          Is this only master (then it should wait the end of the synchronisation period isn't it), or 2.4 and master or should I prepare other branchs ? I ask that because of your comment in issue's description: "Apologies that this was missing from the 2.1 release. That was an oversight on my part."
          We also need some testing instructions. Is it enough to ask to create a question with 2 hints, tests with various combinations of shownum parts correct and clear wrong checked/unchecked and also test XML export/import ?

          Show
          Jean-Michel Vedrine added a comment - - edited Hello Tim, I have integrated all the results of your review in the code I think I have re-run the tests and the codechecker just to be sure I have squashed all commits into one Is this only master (then it should wait the end of the synchronisation period isn't it), or 2.4 and master or should I prepare other branchs ? I ask that because of your comment in issue's description: "Apologies that this was missing from the 2.1 release. That was an oversight on my part." We also need some testing instructions. Is it enough to ask to create a question with 2 hints, tests with various combinations of shownum parts correct and clear wrong checked/unchecked and also test XML export/import ?
          Hide
          Tim Hunt added a comment -

          I think this had to be 2.5-only. Changing the way that questions are graded (even when making it better) cannot really be done on a stable branch.

          I think we are getting near the end of the synchronisation period, so no harm in getting this submitted for integration. The integrators have a way of handling these issues.

          Show
          Tim Hunt added a comment - I think this had to be 2.5-only. Changing the way that questions are graded (even when making it better) cannot really be done on a stable branch. I think we are getting near the end of the synchronisation period, so no harm in getting this submitted for integration. The integrators have a way of handling these issues.
          Hide
          Tim Hunt added a comment -

          Testing instructions: What you suggest is fine. The person doing the testing will appreciate instructions that are as short as possible (but they still have to be reasonably complete.)

          Show
          Tim Hunt added a comment - Testing instructions: What you suggest is fine. The person doing the testing will appreciate instructions that are as short as possible (but they still have to be reasonably complete.)
          Hide
          Jean-Michel Vedrine added a comment -

          I was thinking that maybe MDL-28183 could be backported (maybe to 2.4 only ?) as it doesn't change the grading, just offer new options to question's creators.
          But clearly MDL-36955 has to be 2.5 only as you said.
          As we have based MDL-36955 on MDL-28183 this is a possibility, if you agree of course !
          I must admit I find display of wrong answers and clearing of wrong responses very nice (and my students agree too !)

          Show
          Jean-Michel Vedrine added a comment - I was thinking that maybe MDL-28183 could be backported (maybe to 2.4 only ?) as it doesn't change the grading, just offer new options to question's creators. But clearly MDL-36955 has to be 2.5 only as you said. As we have based MDL-36955 on MDL-28183 this is a possibility, if you agree of course ! I must admit I find display of wrong answers and clearing of wrong responses very nice (and my students agree too !)
          Hide
          Tim Hunt added a comment -

          I agree, it would be safe and nice to get this slipped in to 2.4 too. Let us request that and see what the integrators say.

          I think all we need before this is submitted for integration is some testing instructions.

          Also, in an ideal world, the first line of the commit comment would be shorter. (http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages)

          Show
          Tim Hunt added a comment - I agree, it would be safe and nice to get this slipped in to 2.4 too. Let us request that and see what the integrators say. I think all we need before this is submitted for integration is some testing instructions. Also, in an ideal world, the first line of the commit comment would be shorter. ( http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages )
          Hide
          Tim Hunt added a comment -

          Sorry but I found a bug. Clear wrong does not work with MCV or MCH sub-questions.

          Show
          Tim Hunt added a comment - Sorry but I found a bug. Clear wrong does not work with MCV or MCH sub-questions.
          Hide
          Jean-Michel Vedrine added a comment -

          Grr at one stage I thought "you must test that with all subquestions types", then I forgot about it!

          Show
          Jean-Michel Vedrine added a comment - Grr at one stage I thought "you must test that with all subquestions types", then I forgot about it!
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          I had to spend a few hours finishing a 60 MCQ paper test (final exam) for my students. A lot more boring than working on clear_wrong_from_response
          Looking at the multichoice renderer an specially at qtype_multianswer_multichoice_vertical_renderer give me an idea to solve the problem but I don't know if you will love it (well in fact I think you WON'T love it !)

          public function clear_wrong_from_response(array $response) {
                  foreach ($this->subquestions as $i => $subq) {
                      $substep = $this->get_substep(null, $i);
                      $subresp = $substep->filter_array($response);
                      list($subfraction, $newstate) = $subq->grade_response($subresp);
                      if ($newstate != question_state::$gradedright) {
                          foreach ($subresp as $ind => $resp) {
                              if (($subq->layout == qtype_multichoice_base::LAYOUT_VERTICAL) 
                                      || ($subq->layout == qtype_multichoice_base::LAYOUT_HORIZONTAL)) {
                                  $response[$substep->add_prefix($ind)] = '-1';
                              } else {
                                  $response[$substep->add_prefix($ind)] = '';
                              }
                          }
                      }
                  }
                  return $response;
              }
          
          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I had to spend a few hours finishing a 60 MCQ paper test (final exam) for my students. A lot more boring than working on clear_wrong_from_response Looking at the multichoice renderer an specially at qtype_multianswer_multichoice_vertical_renderer give me an idea to solve the problem but I don't know if you will love it (well in fact I think you WON'T love it !) public function clear_wrong_from_response(array $response) { foreach ($ this ->subquestions as $i => $subq) { $substep = $ this ->get_substep( null , $i); $subresp = $substep->filter_array($response); list($subfraction, $newstate) = $subq->grade_response($subresp); if ($newstate != question_state::$gradedright) { foreach ($subresp as $ind => $resp) { if (($subq->layout == qtype_multichoice_base::LAYOUT_VERTICAL) || ($subq->layout == qtype_multichoice_base::LAYOUT_HORIZONTAL)) { $response[$substep->add_prefix($ind)] = '-1'; } else { $response[$substep->add_prefix($ind)] = ''; } } } } return $response; }
          Hide
          Tim Hunt added a comment -

          So, the problem is, qtype_multichoice_single_question::get_expected_data says that it expects PARAM_INT, which means that '' -> 0, which is a valid choice. Therefore, your idea to send -1 to clear the wrong response is the least-bad option that will work. At least, I cannot think of anything better.

          Show
          Tim Hunt added a comment - So, the problem is, qtype_multichoice_single_question::get_expected_data says that it expects PARAM_INT, which means that '' -> 0, which is a valid choice. Therefore, your idea to send -1 to clear the wrong response is the least-bad option that will work. At least, I cannot think of anything better.
          Hide
          Jean-Michel Vedrine added a comment -

          Yep the problem is exactly that. So anything that will give an integer no part of answer's index will do the job (In fact when I understood the problem, my first test was done with a positive integer bigger than the number of answers)
          To modify the commit message A Google search learned me there is a commit --amend but my first idea was that e rebase -i could doo the job too
          After that I suppose I will have to rebase again MDL-36955 on this one ?

          Show
          Jean-Michel Vedrine added a comment - Yep the problem is exactly that. So anything that will give an integer no part of answer's index will do the job (In fact when I understood the problem, my first test was done with a positive integer bigger than the number of answers) To modify the commit message A Google search learned me there is a commit --amend but my first idea was that e rebase -i could doo the job too After that I suppose I will have to rebase again MDL-36955 on this one ?
          Hide
          Tim Hunt added a comment -

          Yes. That is all correct.

          Show
          Tim Hunt added a comment - Yes. That is all correct.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          I have committed the fix for MCH and MCV, tried to improve the commit comment and added testing instructions.
          I have also created a 2.4 git branch to be nice to integrators in the secret hope they will accept to backport this to MOODLE_24_STABLE.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I have committed the fix for MCH and MCV, tried to improve the commit comment and added testing instructions. I have also created a 2.4 git branch to be nice to integrators in the secret hope they will accept to backport this to MOODLE_24_STABLE.
          Hide
          Tim Hunt added a comment -

          Yay! We got there. Thanks Jean-Michel. Submitting for integration now.

          Show
          Tim Hunt added a comment - Yay! We got there. Thanks Jean-Michel. Submitting for integration now.
          Hide
          Emma Richardson added a comment - - edited

          What are we thinking on time frame - I was getting ready to upgrade to 2.4 but have to have this grading schema and it looks like the patch that Tim wrote before will not work with the 2.4 version of multianswer (code is looking very different!). Or would you be willing to send me the fixes so that I can implement them on my site?

          Just realized I was looking at the wrong file so think the old patch will work in the meantime but would love to have the clear wrong answers and this patch anyway!

          Show
          Emma Richardson added a comment - - edited What are we thinking on time frame - I was getting ready to upgrade to 2.4 but have to have this grading schema and it looks like the patch that Tim wrote before will not work with the 2.4 version of multianswer (code is looking very different!). Or would you be willing to send me the fixes so that I can implement them on my site? Just realized I was looking at the wrong file so think the old patch will work in the meantime but would love to have the clear wrong answers and this patch anyway!
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Hello Emma,
          There is a mandatory order between this issue and MDL-36955 Multianswer grading penalties do not address subparts. This one must be integrated before and MDL-36955 after.
          But this is only important for developers and integrators.
          The best solution for you I think is to upgrade to 2.4+ now and use the patch Tim gave in the forum until these 2 issues are closed, then upgrade you Moodle again to the 2.4.1+ weekly release when these 2 issues will be closed. At that time if integrators are OK to backport MDL-28183 to the 2.4 stable branch you will benefit not only of the subparts grading scheme but also of the "Clear incorrect responses" and "Show the number of correct responses" features.
          But you will have to re-apply Tim's patch at each Moodle upgrade until Moodle 2.5 is out in June 2013 because MDL-36955 will not be integrated in a stable branch (absolutely no hope because this is a change in grading). Look at the 2 tracker issues, in this one there is both a MOODLE_24_STABLE branch and a master branch, but in MDL-36955 there is only a master branch (master means 2.5dev)
          Don't worry, the code to grade multianswer questions in MDL-36955 Multianswer grading penalties do not address subparts is exactly the same as Tim's code given in forum, I only added unit tests which are only important for developers and integrators but not for users like you. So when Moodle 2.5 will be released (if MDL-36955 is accepted ) the transition for you will be easy.

          Show
          Jean-Michel Vedrine added a comment - - edited Hello Emma, There is a mandatory order between this issue and MDL-36955 Multianswer grading penalties do not address subparts. This one must be integrated before and MDL-36955 after. But this is only important for developers and integrators. The best solution for you I think is to upgrade to 2.4+ now and use the patch Tim gave in the forum until these 2 issues are closed, then upgrade you Moodle again to the 2.4.1+ weekly release when these 2 issues will be closed. At that time if integrators are OK to backport MDL-28183 to the 2.4 stable branch you will benefit not only of the subparts grading scheme but also of the "Clear incorrect responses" and "Show the number of correct responses" features. But you will have to re-apply Tim's patch at each Moodle upgrade until Moodle 2.5 is out in June 2013 because MDL-36955 will not be integrated in a stable branch (absolutely no hope because this is a change in grading). Look at the 2 tracker issues, in this one there is both a MOODLE_24_STABLE branch and a master branch, but in MDL-36955 there is only a master branch (master means 2.5dev) Don't worry, the code to grade multianswer questions in MDL-36955 Multianswer grading penalties do not address subparts is exactly the same as Tim's code given in forum, I only added unit tests which are only important for developers and integrators but not for users like you. So when Moodle 2.5 will be released (if MDL-36955 is accepted ) the transition for you will be easy.
          Hide
          Dan Poltawski added a comment -

          Hi Guys,

          You've sweet-talked me with phpunit tests into backporting this to 2.4 :-P.

          However, I have to warn you at the last integration meeting we agreed to get much stricter on our policy on backporting, from the meeting notes "Stricter policy on backports - any exceptions need to be discussed and voted by integrators". We want to do this because we feel we're being inconsistent about applying these rules (so nobody knows where they stand) and there is some pushback about the volume of change on the stable branches.

          Show
          Dan Poltawski added a comment - Hi Guys, You've sweet-talked me with phpunit tests into backporting this to 2.4 :-P. However, I have to warn you at the last integration meeting we agreed to get much stricter on our policy on backporting, from the meeting notes "Stricter policy on backports - any exceptions need to be discussed and voted by integrators". We want to do this because we feel we're being inconsistent about applying these rules (so nobody knows where they stand) and there is some pushback about the volume of change on the stable branches.
          Hide
          Dan Poltawski added a comment -

          Integrated (with a trailing whitespace fix) in 2.4 and master, thanks!

          Show
          Dan Poltawski added a comment - Integrated (with a trailing whitespace fix) in 2.4 and master, thanks!
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks Dan, sorry for the whitespace, I did run codechecker but unfortunately did changes after that. I really appreciate your decision to backport this to Moodle 2.4 and I think Moodle users too will be quite happy to not have to wait 6 months for this feature.

          Show
          Jean-Michel Vedrine added a comment - Thanks Dan, sorry for the whitespace, I did run codechecker but unfortunately did changes after that. I really appreciate your decision to backport this to Moodle 2.4 and I think Moodle users too will be quite happy to not have to wait 6 months for this feature.
          Hide
          Dan Poltawski added a comment -

          Failed:

          qtype_multianswer_question_test.test_clear_wrong_from_response

          Failing for the past 1 build (Since #111 )
          Took 66 ms.
          Stacktrace

          qtype_multianswer_question_test::test_clear_wrong_from_response
          Undefined property: qtype_shortanswer_question::$layout

          /var/lib/jenkins/git_repositories/MOODLE_24_STABLE/question/type/multianswer/question.php:241
          /var/lib/jenkins/git_repositories/MOODLE_24_STABLE/question/type/multianswer/tests/question_test.php:165
          /var/lib/jenkins/git_repositories/MOODLE_24_STABLE/lib/phpunit/classes/advanced_testcase.php:76

          Show
          Dan Poltawski added a comment - Failed: qtype_multianswer_question_test.test_clear_wrong_from_response Failing for the past 1 build (Since #111 ) Took 66 ms. Stacktrace qtype_multianswer_question_test::test_clear_wrong_from_response Undefined property: qtype_shortanswer_question::$layout /var/lib/jenkins/git_repositories/MOODLE_24_STABLE/question/type/multianswer/question.php:241 /var/lib/jenkins/git_repositories/MOODLE_24_STABLE/question/type/multianswer/tests/question_test.php:165 /var/lib/jenkins/git_repositories/MOODLE_24_STABLE/lib/phpunit/classes/advanced_testcase.php:76
          Hide
          Dan Poltawski added a comment -

          (In case I wasn't clear, this is currently breaking phpunit on both branches )

          Show
          Dan Poltawski added a comment - (In case I wasn't clear, this is currently breaking phpunit on both branches )
          Hide
          Jean-Michel Vedrine added a comment -

          I think the fix is to change lines 241-242 in question/type/multianswer/question.php to

          if ($subq->qtype == 'multichoice' && ($subq->layout == qtype_multichoice_base::LAYOUT_VERTICAL 
                                      || $subq->layout == qtype_multichoice_base::LAYOUT_HORIZONTAL)) {
          

          Whith this fix tests are ok.
          But I don't understand why I didn't spotted this problem. I am quite sure I did run the tests several times!
          But maybe I didn't run then again just before submitting to integration. Sorry.
          I had a failed issue once but I didn't remember the exact procedure. Should I push a new commit to github ?

          Show
          Jean-Michel Vedrine added a comment - I think the fix is to change lines 241-242 in question/type/multianswer/question.php to if ($subq->qtype == 'multichoice' && ($subq->layout == qtype_multichoice_base::LAYOUT_VERTICAL || $subq->layout == qtype_multichoice_base::LAYOUT_HORIZONTAL)) { Whith this fix tests are ok. But I don't understand why I didn't spotted this problem. I am quite sure I did run the tests several times! But maybe I didn't run then again just before submitting to integration. Sorry. I had a failed issue once but I didn't remember the exact procedure. Should I push a new commit to github ?
          Hide
          Dan Poltawski added a comment -

          Hi Jean-Michel,

          Could you add a commit on top of your branches and I will pull them both in.

          Show
          Dan Poltawski added a comment - Hi Jean-Michel, Could you add a commit on top of your branches and I will pull them both in.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Dan,
          Will do that immediately. Thanks.

          Show
          Jean-Michel Vedrine added a comment - Hello Dan, Will do that immediately. Thanks.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Hello Dan, commits for both branches pushed to github after running tests on both .
          Sorry for the added work for you. Will try to do better next time.

          Show
          Jean-Michel Vedrine added a comment - - edited Hello Dan, commits for both branches pushed to github after running tests on both . Sorry for the added work for you. Will try to do better next time.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          new commit cherry-picked and applied to 24 and master, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - new commit cherry-picked and applied to 24 and master, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (confirmed that the qtype_multianswer_question_test.test_clear_wrong_from_response test failure is not happening anymore)

          Show
          Eloy Lafuente (stronk7) added a comment - (confirmed that the qtype_multianswer_question_test.test_clear_wrong_from_response test failure is not happening anymore)
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks Eloy.

          Show
          Jean-Michel Vedrine added a comment - Thanks Eloy.
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Jean-Michel.

          Cloze question with different hint options work as expected.
          Export/Import works fine too.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Jean-Michel. Cloze question with different hint options work as expected. Export/Import works fine too.
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks Rajesh.

          Show
          Jean-Michel Vedrine added a comment - Thanks Rajesh.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And your fantastic code has met core, hope they become good friends for a long period.

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!

            People

            • Votes:
              5 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: