Moodle
  1. Moodle
  2. MDL-37600

Update qtype_match tables to match the coding guidelines

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.5
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      You need to test all aspects of match questions.

      1. Create, edit and delete match questions.

      2. Preview a match question.

      3. Export match questions as Moodle XML, and re-import and check that all the options are correctly preserved.

      4. Backup and restore a course with match questions, and check that all the settings are correctly preserved.

      5. In a Moodle 2.0 install, create a quiz including some match questions. Attempt that quiz with some student accounts. Backup the course including user data, and restore it to your master install. Verify the question definitions and attempt data is transferred correctly.

      6. In am Moodle 1.9 install, create a quiz including some match questions. Back up that course, and restore it into your master install. Verify that the question definitions are transferred correctly.

      Show
      You need to test all aspects of match questions. 1. Create, edit and delete match questions. 2. Preview a match question. 3. Export match questions as Moodle XML, and re-import and check that all the options are correctly preserved. 4. Backup and restore a course with match questions, and check that all the settings are correctly preserved. 5. In a Moodle 2.0 install, create a quiz including some match questions. Attempt that quiz with some student accounts. Backup the course including user data, and restore it to your master install. Verify the question definitions and attempt data is transferred correctly. 6. In am Moodle 1.9 install, create a quiz including some match questions. Back up that course, and restore it into your master install. Verify that the question definitions are transferred correctly.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      47288

      Description

      • Rename table question_match -> qtype_match_options.
      • Rename table question_match_sub -> qtype_match_subquestions.
      • Rename column question -> questionid in both tables.
      • Since we have to drop and re-create the foreign key, make qtype_match_options.questionid a foreign-unique key.
      • Drop the unnecessary column qtype_match_options.subquestions.
      • Drop the unnecessary column qtype_match_subquestions.code.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          MDL-17812 is the equivalent issue for shortanswer.

          Show
          Tim Hunt added a comment - MDL-17812 is the equivalent issue for shortanswer.
          Hide
          Tim Hunt added a comment -

          Yay! hideous testing instructions complete, and it works for me. Now for peer review.

          Show
          Tim Hunt added a comment - Yay! hideous testing instructions complete, and it works for me. Now for peer review.
          Hide
          Tim Hunt added a comment -

          Also, separate commit added to fix all the code-checker issues.

          Show
          Tim Hunt added a comment - Also, separate commit added to fix all the code-checker issues.
          Hide
          Frédéric Massart added a comment -

          Hi Tim, I had a look through and the code looks good. For some reason I decided to run the Unit Tests, and they failed, here is the error:

          There was 1 failure:
          
          1) qtype_match_attempt_upgrader_test::test_match_deferredfeedback_history622220
          Failed asserting that two objects are equal.
          --- Expected
          +++ Actual
          @@ @@
           stdClass Object (
               'behaviour' => 'deferredfeedback'
          -    'questionid' => 11135
          +    'questionid' => '11135'
               'variant' => 1
          -    'maxmark' => 1
          +    'maxmark' => '1'
               'minfraction' => 0
               'flagged' => 0
               'questionsummary' => 'Whichofthefollowingstatements...False}'
               'rightanswer' => 'Subject gateways provide link...> True'
          -    'responsesummary' => 'Subjectgatewaysoffermorevarietythansearchengines->False;Subjectgatewayscanprovideamoredirectroutetowebsitescontainingacademiccontent->True;Subjectgatewaysindexwebsitesautomatically->True;Subjectgatewaysprovidelinkstositesthathavebeenqualitychecked->True'
          -    'timemodified' => 1200507467
          +    'responsesummary' => 'Subjectgatewaysoffermorevarietythansearchengines->True;Subjectgatewayscanprovideamoredirectroutetowebsitescontainingacademiccontent->True;Subjectgatewaysindexwebsitesautomatically->True;Subjectgatewaysprovidelinkstositesthathavebeenqualitychecked->True'
          +    'timemodified' => '1200507467'
          
          @@ @@
                       'fraction' => 0.75
          -            'timecreated' => 1200507467
          -            'userid' => 6584
          +            'timecreated' => '1200507467'
          +            'userid' => '6584'
                       'data' => Array (
          -                'sub0' => 1
          +                'sub0' => 2
                           'sub1' => 2
                           'sub2' => 2
                           'sub3' => 2
          -                '-finish' => 1
          +                '-finish' => '1'
                       )
                   )
               )
           )
          
          /home/fred/www/repositories/sm/moodle/question/engine/upgrade/tests/helper.php:139
          /home/fred/www/repositories/sm/moodle/question/type/match/tests/upgradelibnewqe_test.php:818
          /home/fred/www/repositories/sm/moodle/lib/phpunit/classes/advanced_testcase.php:76
          /usr/bin/phpunit:46
          

          Would that have some value to try to upgrade from 2.4 to 2.5 during testing?

          Show
          Frédéric Massart added a comment - Hi Tim, I had a look through and the code looks good. For some reason I decided to run the Unit Tests, and they failed, here is the error: There was 1 failure: 1) qtype_match_attempt_upgrader_test::test_match_deferredfeedback_history622220 Failed asserting that two objects are equal. --- Expected +++ Actual @@ @@ stdClass Object ( 'behaviour' => 'deferredfeedback' - 'questionid' => 11135 + 'questionid' => '11135' 'variant' => 1 - 'maxmark' => 1 + 'maxmark' => '1' 'minfraction' => 0 'flagged' => 0 'questionsummary' => 'Whichofthefollowingstatements...False}' 'rightanswer' => 'Subject gateways provide link...> True' - 'responsesummary' => 'Subjectgatewaysoffermorevarietythansearchengines->False;Subjectgatewayscanprovideamoredirectroutetowebsitescontainingacademiccontent->True;Subjectgatewaysindexwebsitesautomatically->True;Subjectgatewaysprovidelinkstositesthathavebeenqualitychecked->True' - 'timemodified' => 1200507467 + 'responsesummary' => 'Subjectgatewaysoffermorevarietythansearchengines->True;Subjectgatewayscanprovideamoredirectroutetowebsitescontainingacademiccontent->True;Subjectgatewaysindexwebsitesautomatically->True;Subjectgatewaysprovidelinkstositesthathavebeenqualitychecked->True' + 'timemodified' => '1200507467' @@ @@ 'fraction' => 0.75 - 'timecreated' => 1200507467 - 'userid' => 6584 + 'timecreated' => '1200507467' + 'userid' => '6584' 'data' => Array ( - 'sub0' => 1 + 'sub0' => 2 'sub1' => 2 'sub2' => 2 'sub3' => 2 - '-finish' => 1 + '-finish' => '1' ) ) ) ) /home/fred/www/repositories/sm/moodle/question/engine/upgrade/tests/helper.php:139 /home/fred/www/repositories/sm/moodle/question/type/match/tests/upgradelibnewqe_test.php:818 /home/fred/www/repositories/sm/moodle/lib/phpunit/classes/advanced_testcase.php:76 /usr/bin/phpunit:46 Would that have some value to try to upgrade from 2.4 to 2.5 during testing?
          Hide
          Tim Hunt added a comment -

          Thanks for the review Fred.

          That is an odd unit test failure. I will look into it.

          The strange thing is, I have not seen that when I run the tests. Weird. It looks like PHPunit has started considering strings and ints different.

          I don't think it is worth testing 2.4 -> 2.5 upgrade separately. Testing master last week -> master this week will be just the same as far as the qtype_match plugin is concerned, and many other people will test 2.4 -> 2.5 before the 2.5 release, so no need to waste tester time now.

          Show
          Tim Hunt added a comment - Thanks for the review Fred. That is an odd unit test failure. I will look into it. The strange thing is, I have not seen that when I run the tests. Weird. It looks like PHPunit has started considering strings and ints different. I don't think it is worth testing 2.4 -> 2.5 upgrade separately. Testing master last week -> master this week will be just the same as far as the qtype_match plugin is concerned, and many other people will test 2.4 -> 2.5 before the 2.5 release, so no need to waste tester time now.
          Hide
          Tim Hunt added a comment -

          Acutally, there was one value in the unit test that was wrong. Strangely, when one int did not match, then PHPunit showed lots of other irrelevant int/string differences, but once the test passed, those were not shown.

          Test fixed. Submitting for integration now.

          Show
          Tim Hunt added a comment - Acutally, there was one value in the unit test that was wrong. Strangely, when one int did not match, then PHPunit showed lots of other irrelevant int/string differences, but once the test passed, those were not shown. Test fixed. Submitting for integration now.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, thanks Tim!

          Show
          Dan Poltawski added a comment - Integrated to master, thanks Tim!
          Hide
          Dan Poltawski added a comment -

          CI server currently says:

          Table question_match only available in first DB
          Table question_match_sub only available in first DB
          Table qtype_match_options only available in second DB
          Table qtype_match_subquestions only available in second DB

          Show
          Dan Poltawski added a comment - CI server currently says: Table question_match only available in first DB Table question_match_sub only available in first DB Table qtype_match_options only available in second DB Table qtype_match_subquestions only available in second DB
          Hide
          Dan Poltawski added a comment -

          It resolved itself. I've created an issue to work out whats going wrong with the CI server: MDLSITE-2100

          Show
          Dan Poltawski added a comment - It resolved itself. I've created an issue to work out whats going wrong with the CI server: MDLSITE-2100
          Hide
          Tim Hunt added a comment -

          Remember that exactly the same transient failure happened last week with MDL-17812.

          Show
          Tim Hunt added a comment - Remember that exactly the same transient failure happened last week with MDL-17812 .
          Hide
          Dan Poltawski added a comment -

          Tim, yep - I know, thats why I created MDLSITE-2100

          Show
          Dan Poltawski added a comment - Tim, yep - I know, thats why I created MDLSITE-2100
          Hide
          David Monllaó added a comment -

          It passes, tested only in master, all the values of the match questions are still there

          Show
          David Monllaó added a comment - It passes, tested only in master, all the values of the match questions are still there
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

          (and won't be revisiting it unless some regression is found)

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: