Moodle
  1. Moodle
  2. MDL-25492

Blackboard V6+ question import is broken

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      IMPORTANT NOTICE

      This needs to be tested in all target branches
      All tests should be conducted with developer debugging set to enabled to verify that no error or warning is displayed during imports.
      This is especially important because even when it was working in Moodle 1.9 this import format produced a lot of notices when importing some files.
      Purge all caches, to force lang string renewal.

      WHAT ARE WE TESTING ?

      Go to Question Bank -> Import.
      This issue is about fixing the "Blackboard V6+" question import file format.
      Don't be confused, this issue is just about the "Blackboard V6+" file format and not the "Blackboard" file format.
      Clicking on the help icon for this format format should display : Blackboard V6+ format enables questions saved in all Blackboard export formats to be imported via a dat or zip file. For zip files, images import is supported.
      But in fact we are not only fixing this format which was broken since Moodle 2.0, we are also extending it so that it is able to import all types of "Blackboard" files.
      So once this is integrated, it could be made the only Blackboard question import format available in the question bank.
      This will be a lot easier for Moodle users.

      FIRST PART : VERIFY I DIDN'T BREAK OTHER AREAS

      The first thing to test is that nothing is broken in questions import/export as this change is not limited to the Blackboard V6+ format code.
      So try to import and export various questions using both "Moodle XML format" and "Gift format" file formats to ensure all is working as before.
      In each part I give in the title the type of file being imported, but this information is only interesting for me, to classify the example files. The import procedure is exactly the same for all types : just select the file format "Blackboard V6+" file format and upload the file. No difference.

      PART TWO TEST IMPORT OF AN "ASSESSEMENT QTI" .DAT FILE

      Try to import the question/format/blackboard_six/tests/fixtures/sample_blackboard_qti.dat file, using the "Blackboard V6+" file format.
      The import should not produce any warning and cleanly import 6 questions (1 essay, 1 multichoice single answer, 1 multichoice multianswer, 1 fill in the blank (shortanswer), 1 true/false and 1 matching).
      This is the same file that is used in phpunit tests.
      Test that each of these 6 questions is working, that you can edit or preview it without any warning or problem.
      In the matching question, verify that you get get 3 subquestions (cat, frog, newt) and 3 choices (mammal, insect, amphibian). Opening it for editing you should see 4 subquestions one of them with empty question and insect as answer.

      PART THREE TEST IMPORT OF A "QUESTION POOL" .DAT FILE

      Try to import the question/format/blackboard_six/tests/fixtures/sample_blackboard_pool.dat file, using the "Blackboard V6+" file format.
      The import should not produce any warning and cleanly import 6 questions (1 essay, 1 multichoice single answer, 1 multichoice multianswer, 1 fill in the blank (shortanswer), 1 true/false and 1 matching).
      This is the same file that is used in phpunit tests.
      Test that each of these 6 questions is working, that you can edit or preview it without any warning or problem.
      In the matching question, verify that you get get 3 subquestions (cat, frog, newt) and 3 choices (mammal, insect, amphibian). Opening it for editing you should see 4 subquestions one of them with empty question and insect as answer.

      PART FOUR TEST RECODING OF ENTITIES

      Download the apostrophe.zip file attached to this issue (I don't remember where I got it but it's a user file). Try to import this file file using the "Blackboard V6+" file format. Verify no warnings are displayed during import.
      This should import only 1 MCQ question. Open it for editing. Both in the title and question text you should see Mendel's classic pea and looking at the html source code of the question text you should not see an entity instead of the apostrophe between Mendel and s.

      PART FIVE TEST IMPORT OF AN "ASSESSEMENT QTI" .ZIP ARCHIVE WITH SOME IMAGES

      Download the testgen_images.ZIP file attached to the present issue
      Try to import this file file using the "Blackboard V6+" file format. Verify no warnings are displayed during import. You should also get no notification of missing image.
      This file should import 1 MCQ question with name : Describe this image. Verify that both in preview and in editing the question text and each choice display a book image (the right answer is not what you may think first).
      Looking at the question text the image src should be a valid draftfile url ending with

      ppg__questions with image1127111308__f1q1g1.jpg


      Looking at the choice with the image that looks the same, the img src should be a valid draftfile url ending with

      ppg__questions with image1127111308__f1q1g3.jpg

      PART SIX : TEST IMPORT OF A "QUESTION POOL" .ZIP ARCHIVE WITH SOME IMAGES

      Download the 6.4 greeks.zip file from http://moodle.tccsa.net/tccsa2/mod/resource/view.php?id=635 1072 ko. Try to import this file file using the "Blackboard V6+" file format. Verify no warnings are displayed during import. You should also get no notification of missing image.
      This file should import 12 MCQ questions. One of these questions has a name that display as * but is in fact * followed by a non breaking space entity. This name is wierd but is the expected result as we don't want any image included in question's names.
      Preview this question, it should diplay an image with some text in the question's text (text in the image begin by atlas), verify it is working as expected, the right answer is Greeks.
      Open the same question for editing and look at the html source of the question text. The image src should be a valid draftfile url with

      ppg__examview__6-4-greeks__mc009-1.jpg

      as last part, alt should be just "mc009-1.jpg".
      Preview the question whose name start with "Ancients Greeks used myths about ..." and verify that for both correct and incorrect responses a clipart of Zeus is displayed. Open this question for editing and verify the draftfile url in combined feedback for any correct response ends with

      ppg__examview__6-4-greeks__mc010-1.jpg

      (this test is distinct from the one we did for question text because question text is a special case so we must ensure all is OK in both cases)
      Download the alg2_sequences_and_series_benchmark.zip file from http://moodle.org/pluginfile.php/134/mod_forum/attachment/621559/alg2_sequences_and_series_benchmark.zip . Try to import this file file using the "Blackboard V6+" file format. Verify no warnings are displayed during import. You should also get no notification of missing image.
      It should import 12 MCQ questions. Verify that 2 of these questions have names "imported question q8" and "imported question q12". Open these questions for editing and verify that their question text is just an img tag inside a <p> tag. This test that the code to set a default name when there is no plain text in question text is working and also the id of the question is correctly extracted from the file.
      Open the question "What is the fifth term of the expansion of ?" and verify that each of the 4 choices is just an image inside a <p> tag. For the answer that reads 420 x^4^, the image draftfile url should ends with

      ppg__examview__alg2_sec_and_series__mc003-5.jpg

      PART SEVEN : TEST IMPORT OF AN "UNDEPLOYED ASSESSEMENT QTI" .ZIP ARCHIVE

      This part is to verify that MDL-23893 is fixed (see forum http://moodle.org/mod/forum/discuss.php?d=155794, the sample file used come from this thread).
      Download the file L1Group.zip (this file is issued from this forum thread but I have attached it to the present issue for commodity) attached to the present issue and try to import it using the "Blackboard V6+" format.
      This should import 22 MCQ questions written in some oriental language. You are not required to do anything with these questions, the test is just to verify that they are imported because previously no questions were found in the archive.

      PART EIGHT TEST IMPORT OF AN "ASSESSEMENT QTI" ZIP ARCHIVE WITH MULTIPLE QUESTIONS RESSOURCES FILES IN IT

      This part is to verify that MDL-10314 is fixed, but I will use a rather big file to also test performance of the code. Be sure your Moodle server settings allow for enough memory and that the max execution time of php scripts is set to > 4 minutes before attempting this test.
      First try to download the file as it was uploaded by user : download David_0538753099_249380.zip file uploaded 27/Jan/11 8:59 AM 479 kB from the present issue. This big archive weight more than 10 Mo when unzipped.
      Try to import this file using the using the "Blackboard V6+" file format.
      You should get an error message : Error importing question Error while parsing the IMS manifest document Cannot read import file (or file is empty)
      This is the error message displayed to user but as you have debugging activated you should see that the problem is "Input is not proper UTF-8". This file contains invalid UTF-8 characters.
      Download the David_0538753099_249380_corrected.zip file. This is exactly the same archive but I have corrected the invalid UTF-8 characters problem.
      Try to import it using the "Blackboard V6+" import format. You should get 1950 questions if your server is able to swallow it.
      This test that now the import format is able to find all resource files in the archive. Previously only 145 questions were imported.

      Thanks a lot for all that testing. I will be more than happy if all is working as expected.

      Show
      IMPORTANT NOTICE This needs to be tested in all target branches All tests should be conducted with developer debugging set to enabled to verify that no error or warning is displayed during imports. This is especially important because even when it was working in Moodle 1.9 this import format produced a lot of notices when importing some files. Purge all caches, to force lang string renewal. WHAT ARE WE TESTING ? Go to Question Bank -> Import. This issue is about fixing the "Blackboard V6+" question import file format. Don't be confused, this issue is just about the "Blackboard V6+" file format and not the "Blackboard" file format. Clicking on the help icon for this format format should display : Blackboard V6+ format enables questions saved in all Blackboard export formats to be imported via a dat or zip file. For zip files, images import is supported. But in fact we are not only fixing this format which was broken since Moodle 2.0, we are also extending it so that it is able to import all types of "Blackboard" files. So once this is integrated, it could be made the only Blackboard question import format available in the question bank. This will be a lot easier for Moodle users. FIRST PART : VERIFY I DIDN'T BREAK OTHER AREAS The first thing to test is that nothing is broken in questions import/export as this change is not limited to the Blackboard V6+ format code. So try to import and export various questions using both "Moodle XML format" and "Gift format" file formats to ensure all is working as before. In each part I give in the title the type of file being imported, but this information is only interesting for me, to classify the example files. The import procedure is exactly the same for all types : just select the file format "Blackboard V6+" file format and upload the file. No difference. PART TWO TEST IMPORT OF AN "ASSESSEMENT QTI" .DAT FILE Try to import the question/format/blackboard_six/tests/fixtures/sample_blackboard_qti.dat file, using the "Blackboard V6+" file format. The import should not produce any warning and cleanly import 6 questions (1 essay, 1 multichoice single answer, 1 multichoice multianswer, 1 fill in the blank (shortanswer), 1 true/false and 1 matching). This is the same file that is used in phpunit tests. Test that each of these 6 questions is working, that you can edit or preview it without any warning or problem. In the matching question, verify that you get get 3 subquestions (cat, frog, newt) and 3 choices (mammal, insect, amphibian). Opening it for editing you should see 4 subquestions one of them with empty question and insect as answer. PART THREE TEST IMPORT OF A "QUESTION POOL" .DAT FILE Try to import the question/format/blackboard_six/tests/fixtures/sample_blackboard_pool.dat file, using the "Blackboard V6+" file format. The import should not produce any warning and cleanly import 6 questions (1 essay, 1 multichoice single answer, 1 multichoice multianswer, 1 fill in the blank (shortanswer), 1 true/false and 1 matching). This is the same file that is used in phpunit tests. Test that each of these 6 questions is working, that you can edit or preview it without any warning or problem. In the matching question, verify that you get get 3 subquestions (cat, frog, newt) and 3 choices (mammal, insect, amphibian). Opening it for editing you should see 4 subquestions one of them with empty question and insect as answer. PART FOUR TEST RECODING OF ENTITIES Download the apostrophe.zip file attached to this issue (I don't remember where I got it but it's a user file). Try to import this file file using the "Blackboard V6+" file format. Verify no warnings are displayed during import. This should import only 1 MCQ question. Open it for editing. Both in the title and question text you should see Mendel's classic pea and looking at the html source code of the question text you should not see an entity instead of the apostrophe between Mendel and s. PART FIVE TEST IMPORT OF AN "ASSESSEMENT QTI" .ZIP ARCHIVE WITH SOME IMAGES Download the testgen_images.ZIP file attached to the present issue Try to import this file file using the "Blackboard V6+" file format. Verify no warnings are displayed during import. You should also get no notification of missing image. This file should import 1 MCQ question with name : Describe this image. Verify that both in preview and in editing the question text and each choice display a book image (the right answer is not what you may think first). Looking at the question text the image src should be a valid draftfile url ending with ppg__questions with image1127111308__f1q1g1.jpg Looking at the choice with the image that looks the same, the img src should be a valid draftfile url ending with ppg__questions with image1127111308__f1q1g3.jpg PART SIX : TEST IMPORT OF A "QUESTION POOL" .ZIP ARCHIVE WITH SOME IMAGES Download the 6.4 greeks.zip file from http://moodle.tccsa.net/tccsa2/mod/resource/view.php?id=635 1072 ko. Try to import this file file using the "Blackboard V6+" file format. Verify no warnings are displayed during import. You should also get no notification of missing image. This file should import 12 MCQ questions. One of these questions has a name that display as * but is in fact * followed by a non breaking space entity. This name is wierd but is the expected result as we don't want any image included in question's names. Preview this question, it should diplay an image with some text in the question's text (text in the image begin by atlas), verify it is working as expected, the right answer is Greeks. Open the same question for editing and look at the html source of the question text. The image src should be a valid draftfile url with ppg__examview__6-4-greeks__mc009-1.jpg as last part, alt should be just "mc009-1.jpg". Preview the question whose name start with "Ancients Greeks used myths about ..." and verify that for both correct and incorrect responses a clipart of Zeus is displayed. Open this question for editing and verify the draftfile url in combined feedback for any correct response ends with ppg__examview__6-4-greeks__mc010-1.jpg (this test is distinct from the one we did for question text because question text is a special case so we must ensure all is OK in both cases) Download the alg2_sequences_and_series_benchmark.zip file from http://moodle.org/pluginfile.php/134/mod_forum/attachment/621559/alg2_sequences_and_series_benchmark.zip . Try to import this file file using the "Blackboard V6+" file format. Verify no warnings are displayed during import. You should also get no notification of missing image. It should import 12 MCQ questions. Verify that 2 of these questions have names "imported question q8" and "imported question q12". Open these questions for editing and verify that their question text is just an img tag inside a <p> tag. This test that the code to set a default name when there is no plain text in question text is working and also the id of the question is correctly extracted from the file. Open the question "What is the fifth term of the expansion of ?" and verify that each of the 4 choices is just an image inside a <p> tag. For the answer that reads 420 x^4^, the image draftfile url should ends with ppg__examview__alg2_sec_and_series__mc003-5.jpg PART SEVEN : TEST IMPORT OF AN "UNDEPLOYED ASSESSEMENT QTI" .ZIP ARCHIVE This part is to verify that MDL-23893 is fixed (see forum http://moodle.org/mod/forum/discuss.php?d=155794 , the sample file used come from this thread). Download the file L1Group.zip (this file is issued from this forum thread but I have attached it to the present issue for commodity) attached to the present issue and try to import it using the "Blackboard V6+" format. This should import 22 MCQ questions written in some oriental language. You are not required to do anything with these questions, the test is just to verify that they are imported because previously no questions were found in the archive. PART EIGHT TEST IMPORT OF AN "ASSESSEMENT QTI" ZIP ARCHIVE WITH MULTIPLE QUESTIONS RESSOURCES FILES IN IT This part is to verify that MDL-10314 is fixed, but I will use a rather big file to also test performance of the code. Be sure your Moodle server settings allow for enough memory and that the max execution time of php scripts is set to > 4 minutes before attempting this test. First try to download the file as it was uploaded by user : download David_0538753099_249380.zip file uploaded 27/Jan/11 8:59 AM 479 kB from the present issue. This big archive weight more than 10 Mo when unzipped. Try to import this file using the using the "Blackboard V6+" file format. You should get an error message : Error importing question Error while parsing the IMS manifest document Cannot read import file (or file is empty) This is the error message displayed to user but as you have debugging activated you should see that the problem is "Input is not proper UTF-8". This file contains invalid UTF-8 characters. Download the David_0538753099_249380_corrected.zip file. This is exactly the same archive but I have corrected the invalid UTF-8 characters problem. Try to import it using the "Blackboard V6+" import format. You should get 1950 questions if your server is able to swallow it. This test that now the import format is able to find all resource files in the archive. Previously only 145 questions were imported. Thanks a lot for all that testing. I will be more than happy if all is working as expected.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
    • Rank:
      474

      Description

      I have recently upgraded to 2.0 and then 2.0+ (this weeks build). Ever since I upgraded I have not be able to import my quizzes from Examview -> Blackboard using the Blackboard 5 feature. The error that occurs is "Error writing to database". I am not concerned with the images at this time, just the quiz. I can work with the images later like have done in the past (1.9+).

      1. adub_question Import error.txt
        4 kB
        Adub Adub
      2. fmm4k00001.dat
        81 kB
        Wen Hao Chuang
      3. res00000.dat
        21 kB
        Scott Ellis
      4. RTF Format questions.rtf
        74 kB
        Daryle Niedermayer
      5. RTF Format questions.rtf
        74 kB
        Daryle Niedermayer
      6. smurf.xml
        16 kB
        Eloy Lafuente (stronk7)
      1. quiz_import_error.png
        16 kB

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Example files exported from Examview in Blackboard format. See http://moodle.org/mod/forum/discuss.php?d=163754#p717835

          Show
          Tim Hunt added a comment - Example files exported from Examview in Blackboard format. See http://moodle.org/mod/forum/discuss.php?d=163754#p717835
          Hide
          Scott Ellis added a comment -

          The error seems to be happening as soon as the import function encounters a reference to an image. If it is just multiple choice questions with no images it imports those questions fine. This seems to happening in Lesson module also.

          Show
          Scott Ellis added a comment - The error seems to be happening as soon as the import function encounters a reference to an image. If it is just multiple choice questions with no images it imports those questions fine. This seems to happening in Lesson module also.
          Hide
          Wen Hao Chuang added a comment -

          I have also encountered similar problem recently, and I'm going to upload two files (I believe they are both BlackBoard V6+ .zip files). The error messages that I got was:

          "questioni/cannotfindquestionfile

          More information about this error"

          By the way, even with the "standard" theme, this error message would show up with red background and "white" and "blue" fonts, which is NOT really readable (see attached screenshot). This should be a quick fix (should I propose and submit a fix real quick?), thanks!

          Show
          Wen Hao Chuang added a comment - I have also encountered similar problem recently, and I'm going to upload two files (I believe they are both BlackBoard V6+ .zip files). The error messages that I got was: "questioni/cannotfindquestionfile More information about this error" By the way, even with the "standard" theme, this error message would show up with red background and "white" and "blue" fonts, which is NOT really readable (see attached screenshot). This should be a quick fix (should I propose and submit a fix real quick?), thanks!
          Hide
          Wen Hao Chuang added a comment -

          Sample Blackboard V6+ quiz "test bank" that can NOT be imported correctly

          Show
          Wen Hao Chuang added a comment - Sample Blackboard V6+ quiz "test bank" that can NOT be imported correctly
          Hide
          Wen Hao Chuang added a comment -

          Quiz import error message (and the weird colors)

          Show
          Wen Hao Chuang added a comment - Quiz import error message (and the weird colors)
          Hide
          Wen Hao Chuang added a comment -

          The test bank .dat file that also got:

          "Error writing to database
          More information about this error"

          Error message... It seems to do the parsing correctly though...

          Show
          Wen Hao Chuang added a comment - The test bank .dat file that also got: "Error writing to database More information about this error" Error message... It seems to do the parsing correctly though...
          Hide
          Chad Frerichs added a comment -

          My teachers are complaining of the same issue, and have been since our upgrade to 2.0. It has led to much frustration.

          We have tried files both containing images and those not containing images with the same failure message "Error writing to database"

          Show
          Chad Frerichs added a comment - My teachers are complaining of the same issue, and have been since our upgrade to 2.0. It has led to much frustration. We have tried files both containing images and those not containing images with the same failure message "Error writing to database"
          Hide
          Rob added a comment -

          I was asked to explain how the 1.9 plugin is used in production.

          Teachers export a set of questions from Examview in "Blackboard 6/7" format. Teachers see a new inport format option, "Examview Blackboard 6.x. This is added by the 1.9 plugin (/lang/en_utf8/quiz.php needs editing to have the string display correctly). Teachers import the zip file, and the system parses the questions. The imported images are placed in a site files subdirectory. Questions maintain links to that site files directory without the need to relink. I imagine this make a 2.0 version even more complicated, as the legacy site and course files need to be enabled in Moodle 2. I imagine we would want to avoid placing images there.

          Show
          Rob added a comment - I was asked to explain how the 1.9 plugin is used in production. Teachers export a set of questions from Examview in "Blackboard 6/7" format. Teachers see a new inport format option, "Examview Blackboard 6.x. This is added by the 1.9 plugin (/lang/en_utf8/quiz.php needs editing to have the string display correctly). Teachers import the zip file, and the system parses the questions. The imported images are placed in a site files subdirectory. Questions maintain links to that site files directory without the need to relink. I imagine this make a 2.0 version even more complicated, as the legacy site and course files need to be enabled in Moodle 2. I imagine we would want to avoid placing images there.
          Hide
          janet abdoulaye added a comment -

          I have not been able to upload to Moodle's quiz module since the 2.0 upgrade. This is a major problem.

          Show
          janet abdoulaye added a comment - I have not been able to upload to Moodle's quiz module since the 2.0 upgrade. This is a major problem.
          Hide
          Adub Adub added a comment -

          Files necessary for bug report.

          Show
          Adub Adub added a comment - Files necessary for bug report.
          Hide
          Adub Adub added a comment -

          I am having a similar issue, but it appears that I have more debugging code available.

          Essentially, I attempt to load a quiz exported from Blackboard 8. I upload it using the Question Bank->Import, and selecting Blackboard V6+. I then select the zip file exported from Blackboard and receive the following debug code:

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450
          Matching question types are not handled because the quiz question type 'Rendered Matching' does not exist in this installation of Moodle
          Omitted Question: adflu

          Importing 4 questions from file

          1. anwer the question

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/type/essay/questiontype.php on line 50

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/type/essay/questiontype.php on line 51

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/type/essay/questiontype.php on line 53

          Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/type/questiontype.php on line 1685

          Warning: mysqli::real_escape_string() expects parameter 1 to be string, array given in /usr/local/moodle/two_oh/lib/dml/mysqli_native_moodle_database.php on line 676

          Error writing to database

          More information about this error
          Debug info: Incorrect decimal value: '' for column 'fraction' at row 1
          UPDATE mdl_question_answers SET question = ?,answer = ?,feedback = ?,feedbackformat = ?,answerformat = ?,fraction = ? WHERE id=?
          [array (
          0 => 182,
          1 => '',
          2 => '',
          3 => 0,
          4 => 0,
          5 =>
          array (
          0 => 1,
          ),
          6 => 578,
          )]
          Stack trace:

          • line 394 of /lib/dml/moodle_database.php: dml_write_exception thrown
          • line 980 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
          • line 1012 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->update_record_raw()
          • line 59 of /question/type/essay/questiontype.php: call to mysqli_native_moodle_database->update_record()
          • line 368 of /question/format.php: call to question_essay_qtype->save_question_options()
          • line 111 of /question/import.php: call to qformat_default->importprocess()

          I have uploaded this log and the offending blackboard zip to this ticket under the files named adub_*.(zip|txt).

          Show
          Adub Adub added a comment - I am having a similar issue, but it appears that I have more debugging code available. Essentially, I attempt to load a quiz exported from Blackboard 8. I upload it using the Question Bank->Import, and selecting Blackboard V6+. I then select the zip file exported from Blackboard and receive the following debug code: Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/format/blackboard_six/format.php on line 450 Matching question types are not handled because the quiz question type 'Rendered Matching' does not exist in this installation of Moodle Omitted Question: adflu Importing 4 questions from file 1. anwer the question Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/type/essay/questiontype.php on line 50 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/type/essay/questiontype.php on line 51 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/type/essay/questiontype.php on line 53 Notice: Uninitialized string offset: 0 in /usr/local/moodle/two_oh/question/type/questiontype.php on line 1685 Warning: mysqli::real_escape_string() expects parameter 1 to be string, array given in /usr/local/moodle/two_oh/lib/dml/mysqli_native_moodle_database.php on line 676 Error writing to database More information about this error Debug info: Incorrect decimal value: '' for column 'fraction' at row 1 UPDATE mdl_question_answers SET question = ?,answer = ?,feedback = ?,feedbackformat = ?,answerformat = ?,fraction = ? WHERE id=? [array ( 0 => 182, 1 => '', 2 => '', 3 => 0, 4 => 0, 5 => array ( 0 => 1, ), 6 => 578, )] Stack trace: line 394 of /lib/dml/moodle_database.php: dml_write_exception thrown line 980 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end() line 1012 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->update_record_raw() line 59 of /question/type/essay/questiontype.php: call to mysqli_native_moodle_database->update_record() line 368 of /question/format.php: call to question_essay_qtype->save_question_options() line 111 of /question/import.php: call to qformat_default->importprocess() I have uploaded this log and the offending blackboard zip to this ticket under the files named adub_*.(zip|txt).
          Hide
          darian leduc added a comment - - edited

          SAME ISSUE AS OTHERS have

          "questioni/cannotfindquestionfile

          As far as files for testing and debugging, I shall assume that the ones that other submutted are enough. IF not please email me and I will propvide a veritable cornucopia of test exports from examview (v 6.x only)

          I thought that upgrading to 2.0 would help in the import of qeustions situatuion. It did not. Now I cannot even get any questions in - with or without images.

          Show
          darian leduc added a comment - - edited SAME ISSUE AS OTHERS have "questioni/cannotfindquestionfile As far as files for testing and debugging, I shall assume that the ones that other submutted are enough. IF not please email me and I will propvide a veritable cornucopia of test exports from examview (v 6.x only) I thought that upgrading to 2.0 would help in the import of qeustions situatuion. It did not. Now I cannot even get any questions in - with or without images.
          Hide
          Michael Blake added a comment -

          Please give this issue priority; reported as a problem on MP sites.

          Show
          Michael Blake added a comment - Please give this issue priority; reported as a problem on MP sites.
          Hide
          Tim Hunt added a comment -

          Mike, if you want to prioritise this, it will have to be assigned to someone other than me. I won't have time to look at this for the foreseeable future.

          I have explained, several time in different places, what needs to be done to fix this.

          Show
          Tim Hunt added a comment - Mike, if you want to prioritise this, it will have to be assigned to someone other than me. I won't have time to look at this for the foreseeable future. I have explained, several time in different places, what needs to be done to fix this.
          Hide
          Tim Hunt added a comment -

          Assigning to Moodle.com because they seem to think it is a priority, and I don't have time (or any knowledge of blackboard file formats) to enable me to work on it.

          Show
          Tim Hunt added a comment - Assigning to Moodle.com because they seem to think it is a priority, and I don't have time (or any knowledge of blackboard file formats) to enable me to work on it.
          Hide
          Dan Poltawski added a comment -

          Hello, I don't wish to cross the boundaries of what is acceptable but this tracker issue seems the best place to communicate this:

          If anyone has funding resources to support the development of this feature at LUNS we have resource in order to carry this out. We've worked quite closely with Tim on some other projects and would be willing to ensure it gets integrated into core. This could even be funded by multiple bodies. Please email me if this is of interest.

          Show
          Dan Poltawski added a comment - Hello, I don't wish to cross the boundaries of what is acceptable but this tracker issue seems the best place to communicate this: If anyone has funding resources to support the development of this feature at LUNS we have resource in order to carry this out. We've worked quite closely with Tim on some other projects and would be willing to ensure it gets integrated into core. This could even be funded by multiple bodies. Please email me if this is of interest.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Dan,
          Have you seen the work that I have done and made available in the forums here : http://moodle.org/mod/forum/discuss.php?d=117876 (see the file posted by me on July 19th 2011 and named examview_bb6plus.zip you have also another version posted by Gerry Jenkins on July 22th 2011 named examview_blackboard_6.zip)
          It import with no problem all examview questions saved as blackboard 6 that I have tried.
          It doesn't import general files exported by blackbord, only Examview ones. So there is still place for work and funding
          But maybe you can use it as a start if you work on this ?
          Please note that I don't use Examview or Blackboard, I have ony done this as a service to others users. As I understand it a lot of teachers in USA and elsewhere have a lot of examview files they want to import in Moodle . Here in France Examview is nearly totally unknown.

          Show
          Jean-Michel Vedrine added a comment - Hello Dan, Have you seen the work that I have done and made available in the forums here : http://moodle.org/mod/forum/discuss.php?d=117876 (see the file posted by me on July 19th 2011 and named examview_bb6plus.zip you have also another version posted by Gerry Jenkins on July 22th 2011 named examview_blackboard_6.zip) It import with no problem all examview questions saved as blackboard 6 that I have tried. It doesn't import general files exported by blackbord, only Examview ones. So there is still place for work and funding But maybe you can use it as a start if you work on this ? Please note that I don't use Examview or Blackboard, I have ony done this as a service to others users. As I understand it a lot of teachers in USA and elsewhere have a lot of examview files they want to import in Moodle . Here in France Examview is nearly totally unknown.
          Hide
          Jean-Michel Vedrine added a comment -

          replacement for the blackboard_six format in Moodle 2.1 (and 2.2)

          Show
          Jean-Michel Vedrine added a comment - replacement for the blackboard_six format in Moodle 2.1 (and 2.2)
          Hide
          Jean-Michel Vedrine added a comment -

          replacement blackboard_six and nex examview_bbsixplus import format for Moodle 2.1

          Show
          Jean-Michel Vedrine added a comment - replacement blackboard_six and nex examview_bbsixplus import format for Moodle 2.1
          Hide
          Jean-Michel Vedrine added a comment -

          I added 2 import format for Moodle 2.1 (and I think soon to be released 2.2)

          • blackboard_six is a replacement for the blackboard_six format included in Moodle
          • examview_bbsixplus is a new import format
            With this 2 formats you can import all examples files posted in this issue and also all that I found in the forums (not sure I catched them all !) including images and graphics.
            The only exceptions I found are
          • xml files xxxxx.dat need to be packaged in a zip file to be imported because import format in Moodle 2.1 can only accept 1 type of files, so these formats only accept zipfiles
          • in adub_complex_test.zip there is a malformed matching question wich of course is not course not imported
          Show
          Jean-Michel Vedrine added a comment - I added 2 import format for Moodle 2.1 (and I think soon to be released 2.2) blackboard_six is a replacement for the blackboard_six format included in Moodle examview_bbsixplus is a new import format With this 2 formats you can import all examples files posted in this issue and also all that I found in the forums (not sure I catched them all !) including images and graphics. The only exceptions I found are xml files xxxxx.dat need to be packaged in a zip file to be imported because import format in Moodle 2.1 can only accept 1 type of files, so these formats only accept zipfiles in adub_complex_test.zip there is a malformed matching question wich of course is not course not imported
          Hide
          Tim Hunt added a comment -

          Thank you very much for working on this Jean-Michel.

          Given that I know almost nothing about these format, it would be really great if anyone can help with the following tasks that are required to get this into Moodle.

          1. Covert what Jean-Michel has uploaded into a branch in a git repository.

          2. Code-review the new code.

          3. Test the code with more examples.

          I can probably do 1. and 2.

          3. is where I really need help. Thanks.

          Show
          Tim Hunt added a comment - Thank you very much for working on this Jean-Michel. Given that I know almost nothing about these format, it would be really great if anyone can help with the following tasks that are required to get this into Moodle. 1. Covert what Jean-Michel has uploaded into a branch in a git repository. 2. Code-review the new code. 3. Test the code with more examples. I can probably do 1. and 2. 3. is where I really need help. Thanks.
          Hide
          Jean-Michel Vedrine added a comment -

          I forgot to add something :
          As I said these 2 format import all images and graphics included in the zip files (and recode urls in questions texts, answers and feedbacks)
          There are various tutorials on the web (including video ones !) and in the Moodle forums that explain how to edit the xml files generated by ExamView and testGen so that images import in Moodle works. Whith this 2 formats YOU MUST NOT DO THAT, just import the files as they are exported by Examview and testGen. If you edit the xml files manually, import of images will fail.

          Show
          Jean-Michel Vedrine added a comment - I forgot to add something : As I said these 2 format import all images and graphics included in the zip files (and recode urls in questions texts, answers and feedbacks) There are various tutorials on the web (including video ones !) and in the Moodle forums that explain how to edit the xml files generated by ExamView and testGen so that images import in Moodle works. Whith this 2 formats YOU MUST NOT DO THAT, just import the files as they are exported by Examview and testGen. If you edit the xml files manually, import of images will fail.
          Hide
          Jean-Michel Vedrine added a comment -

          I quite agree with Tim,
          Unfortunately I asked for examples files in the Moodle forums but received few responses (thank a lot to all people who send files). This is how I ended installing TestGen to do some tests.
          I have sucessfully imported nearly all files I was able to find. But when you are not the question's author it's very difficult and time consuming to verify that everything is OK in the imported questions. So even if all these files produced no errors during import I am not quite sure the import is 100% working. Also it is very difficult to do a unit testing because these imports format need to adjust to the files generated by Examview and TestGen and I guess none of these programs produce real blackboard format files, surely they are not really fully conformant to the blackboard standard !!

          Show
          Jean-Michel Vedrine added a comment - I quite agree with Tim, Unfortunately I asked for examples files in the Moodle forums but received few responses (thank a lot to all people who send files). This is how I ended installing TestGen to do some tests. I have sucessfully imported nearly all files I was able to find. But when you are not the question's author it's very difficult and time consuming to verify that everything is OK in the imported questions. So even if all these files produced no errors during import I am not quite sure the import is 100% working. Also it is very difficult to do a unit testing because these imports format need to adjust to the files generated by Examview and TestGen and I guess none of these programs produce real blackboard format files, surely they are not really fully conformant to the blackboard standard !!
          Hide
          Tim Hunt added a comment -

          1. Done.

          2. I fixed up a lot of whitespace errors while adding this to git.

          There are still a huge number of Moodle coding style violations in the code (mostly dating from the original code). It would be really good to fix them. May I recommend https://github.com/timhunt/moodle-local_codechecker/

          Also, there is absolutely no need to have functions outside the import class. These should be converted to be methods of the class.

          I will review in more detail once those points have been addressed.

          Show
          Tim Hunt added a comment - 1. Done. 2. I fixed up a lot of whitespace errors while adding this to git. There are still a huge number of Moodle coding style violations in the code (mostly dating from the original code). It would be really good to fix them. May I recommend https://github.com/timhunt/moodle-local_codechecker/ Also, there is absolutely no need to have functions outside the import class. These should be converted to be methods of the class. I will review in more detail once those points have been addressed.
          Hide
          Tim Hunt added a comment -

          Another thing. What I did for GIFT was made an example file that contained a wide range of possible questions, that could be used to test the importer in future. See https://github.com/moodle/moodle/tree/master/question/format/gift/simpletest/fixtures.

          Please can you make a file like that for these new formats. Actually in addition to what I have done, I would suggest adding a README.txt file to that folder, to explain what questions are in the file, to help the person using it for testing. Thanks.

          Show
          Tim Hunt added a comment - Another thing. What I did for GIFT was made an example file that contained a wide range of possible questions, that could be used to test the importer in future. See https://github.com/moodle/moodle/tree/master/question/format/gift/simpletest/fixtures . Please can you make a file like that for these new formats. Actually in addition to what I have done, I would suggest adding a README.txt file to that folder, to explain what questions are in the file, to help the person using it for testing. Thanks.
          Hide
          Petr Škoda added a comment -

          do I see reverting of $CFG->tempdir there? If yes, please, do not do that.

          Show
          Petr Škoda added a comment - do I see reverting of $CFG->tempdir there? If yes, please, do not do that.
          Hide
          Jean-Michel Vedrine added a comment -

          2. Oh yes, I used codechecker for one of the format but forgot to clean the other. I wil start from your git branch and do further cleaning.

          3. The use of a Directory Iterator wa a late idea when I realized that the current code in Moodle was not working and that zipfiles and unzipped files were actually accumulating in the temp folder and never deleted (I wonder how much files some Moodle websites should have there !!). I will put it in the class and do the same thing for blackboard_six format.

          4. The idea to do an example file is quite good. No problem to make a TestGen file as I now have TestGen installed. I will look to find a way to also do an ExamView file and write some README.txt

          5. Grrr I had searched the tracker for "blackboard" and "examview" to find related issues but I just discovered MDL-23893 and realized I should have also searched for "bb6" !! So the comment "assuming that the information is in res0001.dat after looking at 6 examples this was always the case" is wrong. I need to modify the manifest.xml parser to fix this issue too.

          Maybe it would be a good idea to create a META for all issues related to blackboard import or at least link these issues ?

          Show
          Jean-Michel Vedrine added a comment - 2. Oh yes, I used codechecker for one of the format but forgot to clean the other. I wil start from your git branch and do further cleaning. 3. The use of a Directory Iterator wa a late idea when I realized that the current code in Moodle was not working and that zipfiles and unzipped files were actually accumulating in the temp folder and never deleted (I wonder how much files some Moodle websites should have there !!). I will put it in the class and do the same thing for blackboard_six format. 4. The idea to do an example file is quite good. No problem to make a TestGen file as I now have TestGen installed. I will look to find a way to also do an ExamView file and write some README.txt 5. Grrr I had searched the tracker for "blackboard" and "examview" to find related issues but I just discovered MDL-23893 and realized I should have also searched for "bb6" !! So the comment "assuming that the information is in res0001.dat after looking at 6 examples this was always the case" is wrong. I need to modify the manifest.xml parser to fix this issue too. Maybe it would be a good idea to create a META for all issues related to blackboard import or at least link these issues ?
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Petr, can you explain your comment ? I don't think I have modified anything in the way the code worked to unzip files except I delete files after import and the original code was not.
          But if you explain me what I should do i promise I will do it the right way

          Show
          Jean-Michel Vedrine added a comment - Hello Petr, can you explain your comment ? I don't think I have modified anything in the way the code worked to unzip files except I delete files after import and the original code was not. But if you explain me what I should do i promise I will do it the right way
          Hide
          Jean-Michel Vedrine added a comment -

          Oh yes I see the problem : I started from my Moodle 2.1 copy of question/format/blackboard_six/format.php that used $CFG->dataroot."/temp" but apparently things changed since and I should use $CFG->tempdir
          I will change it.

          Show
          Jean-Michel Vedrine added a comment - Oh yes I see the problem : I started from my Moodle 2.1 copy of question/format/blackboard_six/format.php that used $CFG->dataroot."/temp" but apparently things changed since and I should use $CFG->tempdir I will change it.
          Hide
          Jean-Michel Vedrine added a comment -

          Tim, because of this change I think it is best to make a master branch first with $CFG->tempdir and change it when it will be backported to Moodle 2.1 to $CFG->dataroot."/temp" but of course this will only happend once all problems are fixed .

          Show
          Jean-Michel Vedrine added a comment - Tim, because of this change I think it is best to make a master branch first with $CFG->tempdir and change it when it will be backported to Moodle 2.1 to $CFG->dataroot."/temp" but of course this will only happend once all problems are fixed .
          Hide
          Petr Škoda added a comment -

          sorry for the confusion and thanks for working on this.

          Show
          Petr Škoda added a comment - sorry for the confusion and thanks for working on this.
          Hide
          Tim Hunt added a comment -

          Jean-Michel, Please remind me about the $CFG->dataroot."/temp" when the time comes.

          I really think that at least one of the 40 people who have voted for this should make the test files for you. You are already contributing enough.

          It is up to you how you handle the various bugs. You could link them all to this one, or create a META. Really, whatever is most convenient for you.

          Show
          Tim Hunt added a comment - Jean-Michel, Please remind me about the $CFG->dataroot."/temp" when the time comes. I really think that at least one of the 40 people who have voted for this should make the test files for you. You are already contributing enough. It is up to you how you handle the various bugs. You could link them all to this one, or create a META. Really, whatever is most convenient for you.
          Hide
          Daryle Niedermayer added a comment -

          I'm fairly new to Moodle and the bug tracking system for this platform but I'm very interested in this issue and I teach IT and System Design at a College level. I'm hoping I can supply some test data for you.

          Attached are a number of export files generated by ExamView 6.2.1. All contain a combination of question types: true-false, multiple choice, matching, completion, and short answer. None contain any graphics or other multi-media files.

          They have been exported to:
          Blackboard 6.0/7.0 version
          Blackboard 7.1+ version
          WebCT CE 4-6/Vista 3-4 version
          Rich Text Format version

          The Blackboard formatted files were exported using settings:
          Formatting: HTML without default font
          Feedback: Use ExamView Rationale

          The WebCT version was exported using:
          Category Name: "Use question bank title"
          Formatting: HTML without default fonts

          The files should be attached to this message/ticket (provided I can figure out how to do that).

          I hope this helps. Let me know if there is anything more I can do.

          Show
          Daryle Niedermayer added a comment - I'm fairly new to Moodle and the bug tracking system for this platform but I'm very interested in this issue and I teach IT and System Design at a College level. I'm hoping I can supply some test data for you. Attached are a number of export files generated by ExamView 6.2.1. All contain a combination of question types: true-false, multiple choice, matching, completion, and short answer. None contain any graphics or other multi-media files. They have been exported to: Blackboard 6.0/7.0 version Blackboard 7.1+ version WebCT CE 4-6/Vista 3-4 version Rich Text Format version The Blackboard formatted files were exported using settings: Formatting: HTML without default font Feedback: Use ExamView Rationale The WebCT version was exported using: Category Name: "Use question bank title" Formatting: HTML without default fonts The files should be attached to this message/ticket (provided I can figure out how to do that). I hope this helps. Let me know if there is anything more I can do.
          Hide
          Daryle Niedermayer added a comment -

          To be attached to my comment/posting of November 25, 2011.

          Show
          Daryle Niedermayer added a comment - To be attached to my comment/posting of November 25, 2011.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Daryle,
          Thanks a lot for your examples files. I need this is exactly what we need : the exact same questions in all differents formats sso that we can write tests to verify all questions are imported correctly in all formats each time we change the code.
          I have been busy reading all trackers issues with the words"blackboard", "bb6", "examview", and even "bb" !!
          Here are my current plans :
          My main objective is to solve
          MDL-9408 : Make Blackboard Quiz import formats into one
          This will resolve all issues where import fails just because the user don't select the right import format.
          For this we need to correctly parse the imsmanifest.xml file's content (in fact in the current code, it is not parsed), and for each ressource file found, read it's type (assessment/x-bb-pool, assessment/x-bb-qti-test, assessment/x-bb-qti-pool)
          Then readquestions method will call the appropriate subclass to correctly decode the questions.
          But I think I will also solve in the process the following issues :
          MDL-10317 : Streamline blackboard import
          This issue is about importing images in the same process, so is already fixed by my current code.
          MDL-23893 BB6 question import fails when quiz not deployed in BB.
          The problem is that current code always import questions from the res00001.dat ressource file.
          By correctly parsing the manifest we will find all ressource files that contain questionq and not only res00001.dat.
          This will also solve MDL-10314 that has been marqued as duplicate (part of MDL-9408) : import all questions and not only questions in the first .dat file.
          MDL-10313 : Import of BB6 test/quizzes doesn't bring in point values.
          I don't know is this is fixed or not in the current code. Should only be a matter of correctly reading the correct value in the xml tree.
          MDL-16479 : Importing matching questions using Blackboard import process does not handle multiple selections or distractors
          As core match questions is not able to handle blackboard matching questions because answers can contain html tags and images, I use contrib ddmatch question type.
          I am sure it corrects at least part of this issue but I need to see if all cases are correctly handled.

          Show
          Jean-Michel Vedrine added a comment - Hello Daryle, Thanks a lot for your examples files. I need this is exactly what we need : the exact same questions in all differents formats sso that we can write tests to verify all questions are imported correctly in all formats each time we change the code. I have been busy reading all trackers issues with the words"blackboard", "bb6", "examview", and even "bb" !! Here are my current plans : My main objective is to solve MDL-9408 : Make Blackboard Quiz import formats into one This will resolve all issues where import fails just because the user don't select the right import format. For this we need to correctly parse the imsmanifest.xml file's content (in fact in the current code, it is not parsed), and for each ressource file found, read it's type (assessment/x-bb-pool, assessment/x-bb-qti-test, assessment/x-bb-qti-pool) Then readquestions method will call the appropriate subclass to correctly decode the questions. But I think I will also solve in the process the following issues : MDL-10317 : Streamline blackboard import This issue is about importing images in the same process, so is already fixed by my current code. MDL-23893 BB6 question import fails when quiz not deployed in BB. The problem is that current code always import questions from the res00001.dat ressource file. By correctly parsing the manifest we will find all ressource files that contain questionq and not only res00001.dat. This will also solve MDL-10314 that has been marqued as duplicate (part of MDL-9408 ) : import all questions and not only questions in the first .dat file. MDL-10313 : Import of BB6 test/quizzes doesn't bring in point values. I don't know is this is fixed or not in the current code. Should only be a matter of correctly reading the correct value in the xml tree. MDL-16479 : Importing matching questions using Blackboard import process does not handle multiple selections or distractors As core match questions is not able to handle blackboard matching questions because answers can contain html tags and images, I use contrib ddmatch question type. I am sure it corrects at least part of this issue but I need to see if all cases are correctly handled.
          Hide
          Pierre Pichet added a comment - - edited

          Hi Jean-Michel,
          "As core match questions is not able to handle blackboard matching questions because answers can contain html tags and images,"
          If I remember well when we migrate from WebCT to Moodle, the match question on moodle is not as fully developped on Moodle as on WebCT. The answers can contain images as the choices display is not restricted to a select element but to full multichoice display ( not exactly ).
          I think that this (ddmatch ?) should be integrated as core question .

          Show
          Pierre Pichet added a comment - - edited Hi Jean-Michel, "As core match questions is not able to handle blackboard matching questions because answers can contain html tags and images," If I remember well when we migrate from WebCT to Moodle, the match question on moodle is not as fully developped on Moodle as on WebCT. The answers can contain images as the choices display is not restricted to a select element but to full multichoice display ( not exactly ). I think that this (ddmatch ?) should be integrated as core question .
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Pierre,
          In fact I was wrong about MDL-16479, this issue was reporting a bug in the import of matching questions : the format code was unable to import a question if 2 subquestions had the same subanswer or if a subanswer had an empty subquestion (distractor) but Moodle core match question type is perfectly able to handle these situations (ddmatch too of course).

          Show
          Jean-Michel Vedrine added a comment - Hello Pierre, In fact I was wrong about MDL-16479 , this issue was reporting a bug in the import of matching questions : the format code was unable to import a question if 2 subquestions had the same subanswer or if a subanswer had an empty subquestion (distractor) but Moodle core match question type is perfectly able to handle these situations (ddmatch too of course).
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          I really don't understand why there is a method :
          function save_question_options($question) {
          in the blackboard_six format and what it could be used for ?
          I was thinking that maybe it would be used by lesson when importing questions but this doesn't seems to be the case because

          • lesson has it's own different lesson_save_question_options method
          • lesson can't use the blackboard_six plugin at all, as there is no realfilename var defined in lesson qformat class (would be easy to solve but there is no maintainer for the lesson plugin and I want to stay concentrated on the question bank plugins)
            So I am really mystified (but there are so many strange things in the qformats code ! )
          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I really don't understand why there is a method : function save_question_options($question) { in the blackboard_six format and what it could be used for ? I was thinking that maybe it would be used by lesson when importing questions but this doesn't seems to be the case because lesson has it's own different lesson_save_question_options method lesson can't use the blackboard_six plugin at all, as there is no realfilename var defined in lesson qformat class (would be easy to solve but there is no maintainer for the lesson plugin and I want to stay concentrated on the question bank plugins) So I am really mystified (but there are so many strange things in the qformats code ! )
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Anyway, I will create an issue in the tracker for the lesson problem so that maybe one day it could be solved !

          Show
          Jean-Michel Vedrine added a comment - - edited Anyway, I will create an issue in the tracker for the lesson problem so that maybe one day it could be solved !
          Hide
          Tim Hunt added a comment -

          I agree. Create a separate issue for the lesson bug, and carry on.

          Show
          Tim Hunt added a comment - I agree. Create a separate issue for the lesson bug, and carry on.
          Hide
          Tim Hunt added a comment -

          The save_question_options has been there from the start: https://github.com/moodle/moodle/commit/d1c974813032984dd852ea3f4586f04d8eecb817, but I don't think it has ever been used. Please delete it.

          Show
          Tim Hunt added a comment - The save_question_options has been there from the start: https://github.com/moodle/moodle/commit/d1c974813032984dd852ea3f4586f04d8eecb817 , but I don't think it has ever been used. Please delete it.
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks for having exhumed this 6 years old file ! Diff with current state don't show a tremendous activity

          Show
          Jean-Michel Vedrine added a comment - Thanks for having exhumed this 6 years old file ! Diff with current state don't show a tremendous activity
          Hide
          Pierre Pichet added a comment -

          6 years ago is about the same time "thepurpleblob" modify the code for "Moving import/export to new question area." .

          Show
          Pierre Pichet added a comment - 6 years ago is about the same time "thepurpleblob" modify the code for "Moving import/export to new question area." .
          Hide
          Jean-Michel Vedrine added a comment -

          In fact development has started some time ago, but as I have commited a first bunch of changes to Github, I think it's time to say that development has started !

          Show
          Jean-Michel Vedrine added a comment - In fact development has started some time ago, but as I have commited a first bunch of changes to Github, I think it's time to say that development has started !
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Basic changes and phpunit tests. The format is mostly working and the tests are OK, but some more work is required. So I don't think this is ready for peer-review but I wanted this to be on github so that it can't be lost.
          The boring work of converting all paths to use getpath is only half done.
          The 6 basic questions that I use for the phpunit testing takes more than 1000 lines of xml.
          Comments welcome.

          Show
          Jean-Michel Vedrine added a comment - - edited Basic changes and phpunit tests. The format is mostly working and the tests are OK, but some more work is required. So I don't think this is ready for peer-review but I wanted this to be on github so that it can't be lost. The boring work of converting all paths to use getpath is only half done. The 6 basic questions that I use for the phpunit testing takes more than 1000 lines of xml. Comments welcome.
          Hide
          Tim Hunt added a comment -

          Good idea to get the code onto github, even if it is not ready yet. Let me know when it is ready for peer review.

          Show
          Tim Hunt added a comment - Good idea to get the code onto github, even if it is not ready yet. Let me know when it is ready for peer review.
          Hide
          Jean-Michel Vedrine added a comment -

          Summary of the curent problems to solve before this is ready :

          • Now that cleaninput function is used in several formats it should be moved to question/format.php to avoid code duplication
          • the get_filecontent function and the place where it is used (in process_block function to parse "FILE_BLOCK") is a BIG problem : how to deal with files associated to a question ? The problem is quite different if it's an image file, a multimedia file, a java applet, ...)
            This problem is now very complex in Moodle 2.x. What makes it even more complex is the fact that I don't have samples of files !! Maybe we can leave this for now and ignore FILE_BLOCK as not many files seems to include anything here ?
          • What to do with all commented code ? The codechecker don't like it but maybe it would be usefull for the future to leave it somewhere.
          • need to finsh to convert all paths to get_path. Boring but easy.
          • if a .zip file of the wrong type is uploaded the user is correctly warned that he should try to import this file with another format but if a .dat file of the wrong type is uploaded the error message is the generic one "There are no questions in the import file". Here also we should test if the data seems to be of the wrong type (testing for a field known to exist in the other type like <POOL>) and warn the user to use another format to import the file.
          Show
          Jean-Michel Vedrine added a comment - Summary of the curent problems to solve before this is ready : Now that cleaninput function is used in several formats it should be moved to question/format.php to avoid code duplication the get_filecontent function and the place where it is used (in process_block function to parse "FILE_BLOCK") is a BIG problem : how to deal with files associated to a question ? The problem is quite different if it's an image file, a multimedia file, a java applet, ...) This problem is now very complex in Moodle 2.x. What makes it even more complex is the fact that I don't have samples of files !! Maybe we can leave this for now and ignore FILE_BLOCK as not many files seems to include anything here ? What to do with all commented code ? The codechecker don't like it but maybe it would be usefull for the future to leave it somewhere. need to finsh to convert all paths to get_path. Boring but easy. if a .zip file of the wrong type is uploaded the user is correctly warned that he should try to import this file with another format but if a .dat file of the wrong type is uploaded the error message is the generic one "There are no questions in the import file". Here also we should test if the data seems to be of the wrong type (testing for a field known to exist in the other type like <POOL>) and warn the user to use another format to import the file.
          Hide
          Tim Hunt added a comment -

          Commented code will always be available in git history if anyone really wants it. Don't worry about deleting it.

          One day, I would like to change how import works. The nice way would be:

          1. First screen, you just upload a file/
          2. Moodle analyses the file and automatically works out what format it is. If Moodle cannot work out exactly, then there is a screen where the user can choose from a short list of possible choices.
          3. There is then a screen that shows the user what questions are in the file (categories and questions). Here, the user can choose to import some or all of the questions, and where in the question bank they get added. This screen would also allow the user to set options that are not set in the file. (E.g. multichoice shuffle option when importing GIFT.)
          4. Once the user is happy, and confirms, then the import is really done.

          At the moment this is just an idle dream. I won't have time to work on it, but it is a much nicer UI.

          Show
          Tim Hunt added a comment - Commented code will always be available in git history if anyone really wants it. Don't worry about deleting it. One day, I would like to change how import works. The nice way would be: First screen, you just upload a file/ Moodle analyses the file and automatically works out what format it is. If Moodle cannot work out exactly, then there is a screen where the user can choose from a short list of possible choices. There is then a screen that shows the user what questions are in the file (categories and questions). Here, the user can choose to import some or all of the questions, and where in the question bank they get added. This screen would also allow the user to set options that are not set in the file. (E.g. multichoice shuffle option when importing GIFT.) Once the user is happy, and confirms, then the import is really done. At the moment this is just an idle dream. I won't have time to work on it, but it is a much nicer UI.
          Hide
          Jean-Michel Vedrine added a comment -

          Very good idea and the ability to set options will be good too.
          You need to allow each format to be able to add it's own options to this screen. For instance a format importing non Moodle files could ask the user how he wants wrong choices to be graded for multichoice multi questions, but this options makes no sense for Moodle file format where a fraction is defined for each choice.

          Show
          Jean-Michel Vedrine added a comment - Very good idea and the ability to set options will be good too. You need to allow each format to be able to add it's own options to this screen. For instance a format importing non Moodle files could ask the user how he wants wrong choices to be graded for multichoice multi questions, but this options makes no sense for Moodle file format where a fraction is defined for each choice.
          Hide
          Tim Hunt added a comment -

          Right. The list of options to display also depends on which questions are in the file. No point asking that question if there are no multichoice multi questions.

          Show
          Tim Hunt added a comment - Right. The list of options to display also depends on which questions are in the file. No point asking that question if there are no multichoice multi questions.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          I pushed a rather big commit to github.
          The blackboard_six format is now able to handle absolutely all types of Blackboard files (without requiring the user to know of what type is the file he wants to import) :

          • .dat files that were previously imported by the "Blackboard" format
          • .dat files that were previously imported by the "Blackboard V6+" format
          • .zip files that were previously imported by the "Blackboard V6+" format
          • .zip files that were previously imported by Gerry Jenkins or my own code but were impossible to import with formats in core.

          That of course means that the "Blackboard" format is no more necessary and that "MDL-34846 Extend the Blackboard plugin to import zip files with images" has no object now because the new format is able to handle this too.
          I also modified the parsing of the imsmanifest so that when they are several .dat files in the zip file, all are parsed and all questions are imported (I tested the David_0538753099_249380.zip file from january 2011 attached to this issue and it imported 1950 questions from 13 different .dat files)
          This is interesting because MoodleXMLBuilder that many people use since the formats in core were broken was not able to handle this case.
          Of course the code is not beautifull because it's in fact a mix of old blackboard and blackboard_six codes but it works !
          I really think it's a step forward. I am quite tempted to call this "A format to rule them all"

          Show
          Jean-Michel Vedrine added a comment - - edited I pushed a rather big commit to github. The blackboard_six format is now able to handle absolutely all types of Blackboard files (without requiring the user to know of what type is the file he wants to import) : .dat files that were previously imported by the "Blackboard" format .dat files that were previously imported by the "Blackboard V6+" format .zip files that were previously imported by the "Blackboard V6+" format .zip files that were previously imported by Gerry Jenkins or my own code but were impossible to import with formats in core. That of course means that the "Blackboard" format is no more necessary and that " MDL-34846 Extend the Blackboard plugin to import zip files with images" has no object now because the new format is able to handle this too. I also modified the parsing of the imsmanifest so that when they are several .dat files in the zip file, all are parsed and all questions are imported (I tested the David_0538753099_249380.zip file from january 2011 attached to this issue and it imported 1950 questions from 13 different .dat files) This is interesting because MoodleXMLBuilder that many people use since the formats in core were broken was not able to handle this case. Of course the code is not beautifull because it's in fact a mix of old blackboard and blackboard_six codes but it works ! I really think it's a step forward. I am quite tempted to call this "A format to rule them all"
          Hide
          Jean-Michel Vedrine added a comment -

          The code also show that the actual import workflow is not very good because I was not able to write a beautifull function to find the type of the uploaded file : in readata I can find if it's a zip file or a dat file and if it's a zip file I can read the IMSmanifest to find the questions file(s) type. But if it's a dat file I can't yet determine the type because for that I need to xmlize the content wich will only be done later in readquestions.
          This show that in your above scheme, API need to be carefully planned.

          Show
          Jean-Michel Vedrine added a comment - The code also show that the actual import workflow is not very good because I was not able to write a beautifull function to find the type of the uploaded file : in readata I can find if it's a zip file or a dat file and if it's a zip file I can read the IMSmanifest to find the questions file(s) type. But if it's a dat file I can't yet determine the type because for that I need to xmlize the content wich will only be done later in readquestions. This show that in your above scheme, API need to be carefully planned.
          Hide
          Tim Hunt added a comment -

          To save me from having to think, what does this mean for the other changes we have sent to integration for next week?

          Show
          Tim Hunt added a comment - To save me from having to think, what does this mean for the other changes we have sent to integration for next week?
          Hide
          Jean-Michel Vedrine added a comment - - edited

          In fact they are now all integrated in this format :

          • all the code to parse the questions that was in question/format/blackboard/format.php is now integrated (with changes : for instance most of the functions had "_pool" added to their names to avoid clash with functions with similar names that were already there) in question/format/blackboard_six/format.php
          • all the phpunit tests that were in question/format/blackboard/tests/blackboardformat_test.php are now (with some minor changes) in question/format/blackboard_six/tests/blackboardformatpool_test.php along with the tests for the other format in question/format/blackboard_six/tests/blackboardformatqti_test.php

          I don't know if you will agree but my proposal is

          • let the files currently in integration be tested, integrated and be part of Moodle. If any issue is found during this proccess I will correct the blackboard format and report the same changes in the WIP for blackboard_six
          • I continue the work on the blackboard_six format and when ready, I submit it to you for peer-review. When you are OK, I write testing instructions that will be a lot more complex because we will need to test each type of file.
            We then submit it for integration.
            Then we can deprecate the blackboard format (I don't know if it will be possible to deprecate something in stable branches so maybe we will only be able to deprecate it in 2.4 ?).

          Other possibily
          We ask the changes for the blackboard format to be removed from the integration process and wait that the blackboard_six format is ready to send it to integration. This will let us the problem of the (still broken in that case) blackboard format be deprecated or not in 2.2 and 2.3.

          If I am not clear or if you want more informations don't hesitate to ask.

          In fact the progress of my today's work were not quite expected, because I had the problem of importing several dat files and of finding a way to determine the file's type in my head for some time without finding a solution.
          So this is why I finised the work on the blackboard format and asked you if it could be integrated.
          Then sudently this morning I wrote the missing parts of the code and in less than an hour I had a format able to import all my example files.

          Show
          Jean-Michel Vedrine added a comment - - edited In fact they are now all integrated in this format : all the code to parse the questions that was in question/format/blackboard/format.php is now integrated (with changes : for instance most of the functions had "_pool" added to their names to avoid clash with functions with similar names that were already there) in question/format/blackboard_six/format.php all the phpunit tests that were in question/format/blackboard/tests/blackboardformat_test.php are now (with some minor changes) in question/format/blackboard_six/tests/blackboardformatpool_test.php along with the tests for the other format in question/format/blackboard_six/tests/blackboardformatqti_test.php I don't know if you will agree but my proposal is let the files currently in integration be tested, integrated and be part of Moodle. If any issue is found during this proccess I will correct the blackboard format and report the same changes in the WIP for blackboard_six I continue the work on the blackboard_six format and when ready, I submit it to you for peer-review. When you are OK, I write testing instructions that will be a lot more complex because we will need to test each type of file. We then submit it for integration. Then we can deprecate the blackboard format (I don't know if it will be possible to deprecate something in stable branches so maybe we will only be able to deprecate it in 2.4 ?). Other possibily We ask the changes for the blackboard format to be removed from the integration process and wait that the blackboard_six format is ready to send it to integration. This will let us the problem of the (still broken in that case) blackboard format be deprecated or not in 2.2 and 2.3. If I am not clear or if you want more informations don't hesitate to ask. In fact the progress of my today's work were not quite expected, because I had the problem of importing several dat files and of finding a way to determine the file's type in my head for some time without finding a solution. So this is why I finised the work on the blackboard format and asked you if it could be integrated. Then sudently this morning I wrote the missing parts of the code and in less than an hour I had a format able to import all my example files.
          Hide
          Tim Hunt added a comment -

          I agree with your proposal. We have some code that works much better than the current broken code. So let us get that integrated into all stable branches next week.

          Then we can afford to work on the big clean-up until it is right, and, once we see it, perhaps we will think that it has to be Moodle 2.4 only.

          Show
          Tim Hunt added a comment - I agree with your proposal. We have some code that works much better than the current broken code. So let us get that integrated into all stable branches next week. Then we can afford to work on the big clean-up until it is right, and, once we see it, perhaps we will think that it has to be Moodle 2.4 only.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Hello Tim
          To avoid code duplication, would you agree to move:
          protected function add_blank_combined_feedback
          to the qformat_default class ?
          this function is currently used in 3 formats :

          • blackboard_six (the future import-all blackboard format)
          • examview (the format for the old examview v4 format)
          • gift

          and possibly other (I think Pierre may need it when he will fix webct format)
          If yes, is this just possible for Moodle 2.4 or can it be backported ?
          Should I open a separate issue ?

          Show
          Jean-Michel Vedrine added a comment - - edited Hello Tim To avoid code duplication, would you agree to move: protected function add_blank_combined_feedback to the qformat_default class ? this function is currently used in 3 formats : blackboard_six (the future import-all blackboard format) examview (the format for the old examview v4 format) gift and possibly other (I think Pierre may need it when he will fix webct format) If yes, is this just possible for Moodle 2.4 or can it be backported ? Should I open a separate issue ?
          Hide
          Tim Hunt added a comment -

          Please move function add_blank_combined_feedback -> qformat_default. I am happy for this to be back-ported. It will not break anything, I think. I think it is OK to do it as part of this bug. We don't need a separate one.

          Show
          Tim Hunt added a comment - Please move function add_blank_combined_feedback -> qformat_default. I am happy for this to be back-ported. It will not break anything, I think. I think it is OK to do it as part of this bug. We don't need a separate one.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Hello Tim,
          I know you are very busy but I think I need you peer review this code.
          I am at a point I don't see clearly what I could improve and what other changes are needed. Of course there are surely tons of things that could be improved ...
          The phpunit tests are OK, and I am able to import any "Blackboard" file attached to any tracker's issue related to "Blackboard", "TestGen", Examview, "bb6". For the few ones that the code fails to import this is because they are badly broken. I have also tested import of all files submited in the quiz forum, and all seems OK.
          To try to save you some time while reviewing this, I have written a guide to the changes I did to the question/format/blackboard_six/format.php file (massive changes are in that file, and changes to other files are smalls so I don't think you need help for them )

          Lines 36 - 818

          This is the qformat_blackboard_qti class used for parsing Blackboard files known as "QTI ASSESSMENT"
          This parser is the one that was before my work in the same file: question/format/blackboard_six/format.php so it's interesting to compare both files before and after modifications. Unfortunately comparison is obfuscated by the many changes I did to replace all paths by calls to getpath.
          But in fact apart the use of getpath there are not so many changes.

          Changes in the readquestions function

          The only change is that this function now expect a string rather than an array so don't implode it at the begining, but the foreach loop and the case are unchanged.

          Changes in the create_raw_question function

          No real changes apart the use of getpath

          Changes in the process_block function

          There is a big change : if a "FILE_BLOCK" with a file material in it is encountered, I don't know what to do with that file. Previously the file was copied to the course data folder and later a messsage was printed on screen to warn the user that a file was copied. I only output a notification about the file's presence, after all it's a zipe file and the user can extract the file if he wants.
          If somebody is able to send me a Blackboard archive with a file that could be handled (for instance a multimedia file) we can extend the code in the future

          And for all the other functions in that class, i don't think I made significant changes other than the many replacements of paths by call to getpath.
          I don't think that despite many verifications it's possible I broke nothing because it's very easy to make a mistake in the parameters passed to gethpath. But as the phpunits tests are still OK, I hope I didn't break "too many" things

          Lines 819 - 1263

          This is the qformat_blackboard_pool class used for parsing Blackboard files known as "QUESTIONS POOL"
          It's interesting to compare this part with the question/format/blackboard/format.php file that we submitted for integration in MDL-34738 because you will see it's exactly the same class that qformat_blackboard with only minimal changes :

          • name changed for consistency
          • extends extends qformat_import_zipfile (was previously qformat_based_on_xml) so that we are able to import images
          • readquestions expect a string rather than an array so don't implode it at the begining
          • when importing questiontext and questiontextfiles, images and images urls are managed

          But as you will se questions are parsed quite identically so you have already reviewed this code in MDL-34738.

          Lines 1265 - 1422

          This is the real qformat_blackboard_six class that will put all the bricks together.
          $filetype will hold the type of file being imported (1= QTI ASSESSMENT, 2 = POOL) maybe I should have choosen a string rather than an integer or defined CONST ?

          get_filecontent function

          This function is used to get the content of each questions ressource file. Maybe (surely) there is already something like this in Moodle that I should have used ? But I was unable to find it.

          readdata function process the file
          • if it's a .dat file, looking in the content for known mandatory tags the type is found and the content is returned as unique lement of an array
          • if it's a zip file, it's unzipped in e temporary folder, the imsmanifest is parsed to find all questions ressource files and also find the type of file been imported.
          • the content of each question ressource file is returned as element of an array.
          readquestions function parse the questions

          It's job is really simple : it just instanciate the right qformat class based on the $filetype value
          It set the filebase of this class so that images path are corrects.
          loop on all the element of the array passed by readdata calling the readquestions function of the importer qformat.

          As you know, my knowledge of OOP is no so good so there is surely a smarter way of doing that. But I choose this way because it permitted me to reuse the parsers done by Scott Elliott and Michael Penney nearly untouched.
          By the way, as this file is now a mix of 2 old formats, is it possible to put 2 @copyright lines like I did at the begining of the file ?

          Good review, and thanks a lot for your advices and help.

          Show
          Jean-Michel Vedrine added a comment - - edited Hello Tim, I know you are very busy but I think I need you peer review this code. I am at a point I don't see clearly what I could improve and what other changes are needed. Of course there are surely tons of things that could be improved ... The phpunit tests are OK, and I am able to import any "Blackboard" file attached to any tracker's issue related to "Blackboard", "TestGen", Examview, "bb6". For the few ones that the code fails to import this is because they are badly broken. I have also tested import of all files submited in the quiz forum, and all seems OK. To try to save you some time while reviewing this, I have written a guide to the changes I did to the question/format/blackboard_six/format.php file (massive changes are in that file, and changes to other files are smalls so I don't think you need help for them ) Lines 36 - 818 This is the qformat_blackboard_qti class used for parsing Blackboard files known as "QTI ASSESSMENT" This parser is the one that was before my work in the same file: question/format/blackboard_six/format.php so it's interesting to compare both files before and after modifications. Unfortunately comparison is obfuscated by the many changes I did to replace all paths by calls to getpath. But in fact apart the use of getpath there are not so many changes. Changes in the readquestions function The only change is that this function now expect a string rather than an array so don't implode it at the begining, but the foreach loop and the case are unchanged. Changes in the create_raw_question function No real changes apart the use of getpath Changes in the process_block function There is a big change : if a "FILE_BLOCK" with a file material in it is encountered, I don't know what to do with that file. Previously the file was copied to the course data folder and later a messsage was printed on screen to warn the user that a file was copied. I only output a notification about the file's presence, after all it's a zipe file and the user can extract the file if he wants. If somebody is able to send me a Blackboard archive with a file that could be handled (for instance a multimedia file) we can extend the code in the future And for all the other functions in that class, i don't think I made significant changes other than the many replacements of paths by call to getpath. I don't think that despite many verifications it's possible I broke nothing because it's very easy to make a mistake in the parameters passed to gethpath. But as the phpunits tests are still OK, I hope I didn't break "too many" things Lines 819 - 1263 This is the qformat_blackboard_pool class used for parsing Blackboard files known as "QUESTIONS POOL" It's interesting to compare this part with the question/format/blackboard/format.php file that we submitted for integration in MDL-34738 because you will see it's exactly the same class that qformat_blackboard with only minimal changes : name changed for consistency extends extends qformat_import_zipfile (was previously qformat_based_on_xml) so that we are able to import images readquestions expect a string rather than an array so don't implode it at the begining when importing questiontext and questiontextfiles, images and images urls are managed But as you will se questions are parsed quite identically so you have already reviewed this code in MDL-34738 . Lines 1265 - 1422 This is the real qformat_blackboard_six class that will put all the bricks together. $filetype will hold the type of file being imported (1= QTI ASSESSMENT, 2 = POOL) maybe I should have choosen a string rather than an integer or defined CONST ? get_filecontent function This function is used to get the content of each questions ressource file. Maybe (surely) there is already something like this in Moodle that I should have used ? But I was unable to find it. readdata function process the file if it's a .dat file, looking in the content for known mandatory tags the type is found and the content is returned as unique lement of an array if it's a zip file, it's unzipped in e temporary folder, the imsmanifest is parsed to find all questions ressource files and also find the type of file been imported. the content of each question ressource file is returned as element of an array. readquestions function parse the questions It's job is really simple : it just instanciate the right qformat class based on the $filetype value It set the filebase of this class so that images path are corrects. loop on all the element of the array passed by readdata calling the readquestions function of the importer qformat. As you know, my knowledge of OOP is no so good so there is surely a smarter way of doing that. But I choose this way because it permitted me to reuse the parsers done by Scott Elliott and Michael Penney nearly untouched. By the way, as this file is now a mix of 2 old formats, is it possible to put 2 @copyright lines like I did at the begining of the file ? Good review, and thanks a lot for your advices and help.
          Hide
          Jean-Michel Vedrine added a comment -

          I will begin to write the testing instructions but due to the number of differents files the import format is now able to handle the test suite will be rather long.

          Show
          Jean-Michel Vedrine added a comment - I will begin to write the testing instructions but due to the number of differents files the import format is now able to handle the test suite will be rather long.
          Hide
          Tim Hunt added a comment -

          I am not going to look tonight. I will try to look tomorrow. I saw your post in the forum where you said how long you have left to work on this. With luck we can get this submitted for integration in time for next week.

          Show
          Tim Hunt added a comment - I am not going to look tonight. I will try to look tomorrow. I saw your post in the forum where you said how long you have left to work on this. With luck we can get this submitted for integration in time for next week.
          Hide
          Jean-Michel Vedrine added a comment -

          Well as long as most of the work is done when courses start, I will manage to find some time to do the required changes even after that date. I do want this to get into Moodle ! Thank for your efforts to review this. I really appreciate because I know you have a lot of other things to look after.

          Show
          Jean-Michel Vedrine added a comment - Well as long as most of the work is done when courses start, I will manage to find some time to do the required changes even after that date. I do want this to get into Moodle ! Thank for your efforts to review this. I really appreciate because I know you have a lot of other things to look after.
          Hide
          Tim Hunt added a comment -

          Here are an initial set of comments. I am afraid it is rather long:

          1. $string['imagenotfound'] = 'import image file at path {$a} does not exist';

          Better English would probably be:

          $string['imagenotfound'] = 'Image file with path {$a} was not found in the import.';

          2. I guess some of the changes in question/format.php, like class qformat_based_on_xml extends qformat_default {, will stop being changes after this week's integaration.

          3. I think, at least at the moment, that class qformat_import_zipfile extends qformat_based_on_xml is specific to blackboard format, not really generic, so I think you should move it.

          4. Actually, I think you should separate out all the classes in question/format/blackboard_six/format.php into separate files.

          5. You don't need to make your own check_dir_exists function. Use the standard Moodle function make_temp_directory instead. You can probably even get rid of check_and_create_import_dir.

          6. Similarly, you don't need remove_directory, just the fulldelete function.

          7. GLOBAL $OUTPUT; - global should be lower case. But actually, methods like this should not be using $OUTPUT at all. Throw an exception, or call $this->error(). I think the same comment applies to all the other uses of $OUTPUT in this file. E.g. create_raw_question, process_block, ...

          8. Don't get too hung-up about @copyright tags. There should really only be one, for the person who created the file. The real information about who authored each line is available in git history.

          9. If you are passing an object like $block, then you don't need the &. For example public function process_block($cur_block, &$block) should not have the &.

          10. Many methods are missing PHPdoc comments.

          11. The coding style says variable names should not contain _. There are lots of variables that break that rule. I guess at some point you will run codechecker.

          12. Typically I prefer protected to private. The main difference is that protected is more flexible if someone wants to make a subclass. However, that might just be my preference.

          13. If qformat_blackboard_pool is virtually the same as qformat_blackboard, can't you subclass qformat_blackboard to avoid duplicated code? Or possibly do it the othe way around. Make qformat_blackboard use qformat_blackboard_pool in the same way that qformat_blackboard_six does.

          14. Within one plugin, like qformat_blackboard_six, all class names should start qformat_blackboard_six_...

          15. Using a field like $this->filetype is almost certainly the wrong way to do this. I think this should just be a local variable inside readdata that you pass to any other methods that need it.

          16. new statements like $importer = new qformat_blackboard_qti; should always have the (), so it should be $importer = new qformat_blackboard_qti();

          17. As Eloy commented on the other bug, better if you can load the file from fixtures, rather than having it inline in the unit tests. (I know I need to fix Moodle XML import/export tests to work like that.)

          18. About handling files. There are two ways you can do this in question import.

          The simpler way, which Moodle XML import uses, is to load the contents of the file into the $q->questiontext['files'] array. Look at the data structure that qformat_xml::import_files returns. The problem with this approach is that we need to load the contents of all the files into memory at the same time, so typically the process runs out of memeory.

          The better way to do this is to do what the question editing form does. Put the files into a draft file ares, and put the draftitemid in $q->questiontext['itemid'];

          Something like

          protected function store_file_for_text_field(&$text, $tempdir, $filepathinsidetempdir, $filename) {
              global $USER;
              $fs = get_file_storage();
              if (empty($text['itemid'])) {
                  $text['itemid'] = file_get_unused_draft_itemid();
              }
              $filerecord = array(
                  'contextid' => context_user::instance($USER->id)->id,
                  'component' => 'user',
                  'filearea'  => 'draft',
                  'itemid'    => $text['itemid'],
                  'filepath'  => $filepathinsidetempdir,
                  'filename'  => $filename,
              );
              $fs->create_file_from_pathname($filerecord, $tempdir . '/' . $filepathinsidetempdir . '/' . $filename);
          }
          

          then

          $this->store_file_for_text_field($q->correctfeedback, ...);
          

          I have not tested or debugged any of that code. These array('text' => ..., 'format' => ..., 'files' => ...), or array('text' => ..., 'format' => ..., 'itemid' => ...) structured acutally get saved by the question_type::import_or_save_files method in question/type/questiontypebase.php.

          Despite all those comments, I think the code is generally very good. It just needs to be improved further before it can be considered done.

          Show
          Tim Hunt added a comment - Here are an initial set of comments. I am afraid it is rather long: 1. $string ['imagenotfound'] = 'import image file at path {$a} does not exist'; Better English would probably be: $string ['imagenotfound'] = 'Image file with path {$a} was not found in the import.'; 2. I guess some of the changes in question/format.php, like class qformat_based_on_xml extends qformat_default {, will stop being changes after this week's integaration. 3. I think, at least at the moment, that class qformat_import_zipfile extends qformat_based_on_xml is specific to blackboard format, not really generic, so I think you should move it. 4. Actually, I think you should separate out all the classes in question/format/blackboard_six/format.php into separate files. 5. You don't need to make your own check_dir_exists function. Use the standard Moodle function make_temp_directory instead. You can probably even get rid of check_and_create_import_dir. 6. Similarly, you don't need remove_directory, just the fulldelete function. 7. GLOBAL $OUTPUT; - global should be lower case. But actually, methods like this should not be using $OUTPUT at all. Throw an exception, or call $this->error(). I think the same comment applies to all the other uses of $OUTPUT in this file. E.g. create_raw_question, process_block, ... 8. Don't get too hung-up about @copyright tags. There should really only be one, for the person who created the file. The real information about who authored each line is available in git history. 9. If you are passing an object like $block, then you don't need the &. For example public function process_block($cur_block, &$block) should not have the &. 10. Many methods are missing PHPdoc comments. 11. The coding style says variable names should not contain _. There are lots of variables that break that rule. I guess at some point you will run codechecker. 12. Typically I prefer protected to private. The main difference is that protected is more flexible if someone wants to make a subclass. However, that might just be my preference. 13. If qformat_blackboard_pool is virtually the same as qformat_blackboard, can't you subclass qformat_blackboard to avoid duplicated code? Or possibly do it the othe way around. Make qformat_blackboard use qformat_blackboard_pool in the same way that qformat_blackboard_six does. 14. Within one plugin, like qformat_blackboard_six, all class names should start qformat_blackboard_six_... 15. Using a field like $this->filetype is almost certainly the wrong way to do this. I think this should just be a local variable inside readdata that you pass to any other methods that need it. 16. new statements like $importer = new qformat_blackboard_qti; should always have the (), so it should be $importer = new qformat_blackboard_qti(); 17. As Eloy commented on the other bug, better if you can load the file from fixtures, rather than having it inline in the unit tests. (I know I need to fix Moodle XML import/export tests to work like that.) 18. About handling files. There are two ways you can do this in question import. The simpler way, which Moodle XML import uses, is to load the contents of the file into the $q->questiontext ['files'] array. Look at the data structure that qformat_xml::import_files returns. The problem with this approach is that we need to load the contents of all the files into memory at the same time, so typically the process runs out of memeory. The better way to do this is to do what the question editing form does. Put the files into a draft file ares, and put the draftitemid in $q->questiontext ['itemid'] ; Something like protected function store_file_for_text_field(&$text, $tempdir, $filepathinsidetempdir, $filename) { global $USER; $fs = get_file_storage(); if (empty($text['itemid'])) { $text['itemid'] = file_get_unused_draft_itemid(); } $filerecord = array( 'contextid' => context_user::instance($USER->id)->id, 'component' => 'user', 'filearea' => 'draft', 'itemid' => $text['itemid'], 'filepath' => $filepathinsidetempdir, 'filename' => $filename, ); $fs->create_file_from_pathname($filerecord, $tempdir . '/' . $filepathinsidetempdir . '/' . $filename); } then $ this ->store_file_for_text_field($q->correctfeedback, ...); I have not tested or debugged any of that code. These array('text' => ..., 'format' => ..., 'files' => ...), or array('text' => ..., 'format' => ..., 'itemid' => ...) structured acutally get saved by the question_type::import_or_save_files method in question/type/questiontypebase.php. Despite all those comments, I think the code is generally very good. It just needs to be improved further before it can be considered done.
          Hide
          Tim Hunt added a comment -

          Finishing peer-review for now. I am sure we will need some more iterations.

          Show
          Tim Hunt added a comment - Finishing peer-review for now. I am sure we will need some more iterations.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim
          Yes, this was only the first peer-review but thank a lot for all of this.
          I really apreciate all the time you put in this issue.
          I will start making the changes but something keep my eye : you say "I guess at some point you will run codechecker" but the problem is I already did that and I got no error or warning!
          This is not the first time that your codechecker seems a lot more picky than mine. Maybe I am not using the right version or I didn't configured it as I should ? I was thinking I had downloaded it from an url you gave me, but maybe I have mixed my versions of the codechecker somewhere ? Can you point me to a solution because I really need a codechecker that list all the things I need to improve in the code.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim Yes, this was only the first peer-review but thank a lot for all of this. I really apreciate all the time you put in this issue. I will start making the changes but something keep my eye : you say "I guess at some point you will run codechecker" but the problem is I already did that and I got no error or warning! This is not the first time that your codechecker seems a lot more picky than mine. Maybe I am not using the right version or I didn't configured it as I should ? I was thinking I had downloaded it from an url you gave me, but maybe I have mixed my versions of the codechecker somewhere ? Can you point me to a solution because I really need a codechecker that list all the things I need to improve in the code.
          Hide
          Tim Hunt added a comment -

          The official version of codechecker is https://github.com/moodlehq/moodle-local_codechecker. It may not check for the _ in variable names rule.

          Show
          Tim Hunt added a comment - The official version of codechecker is https://github.com/moodlehq/moodle-local_codechecker . It may not check for the _ in variable names rule.
          Hide
          Jean-Michel Vedrine added a comment -

          That was the version of the codechecker I use so most probably variable name rule is not checked.
          Here are the only remarks I have trouble to understand :

          13. Well I was thinking that once this would be in core, all the blackboard format could be deprecated (It will be no more needed because blackboard_six will be able to handle all imports) so this would suppress all code duplication . But until that I can make qformat_blackboard use qformat_blackboard_six_pool (new name according to point #14 in your review) it will be very easy.

          15. I use filetype to pass a value from readdata to readquestions because I don't master the arguments of these 2 functions defined for all qformat classes, but maybe I don't understand what you mean ?

          18. I really like your "better way". I need to do that because memory needs for bigs archives is high due to the use of xmlize. I will write and test some code and ask for your help if I don't manage to get it working.

          Show
          Jean-Michel Vedrine added a comment - That was the version of the codechecker I use so most probably variable name rule is not checked. Here are the only remarks I have trouble to understand : 13. Well I was thinking that once this would be in core, all the blackboard format could be deprecated (It will be no more needed because blackboard_six will be able to handle all imports) so this would suppress all code duplication . But until that I can make qformat_blackboard use qformat_blackboard_six_pool (new name according to point #14 in your review) it will be very easy. 15. I use filetype to pass a value from readdata to readquestions because I don't master the arguments of these 2 functions defined for all qformat classes, but maybe I don't understand what you mean ? 18. I really like your "better way". I need to do that because memory needs for bigs archives is high due to the use of xmlize. I will write and test some code and ask for your help if I don't manage to get it working.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          I tried to exploit your #18 remark but I think I have a problem I don't manage to solve : my filepathinsidetempdir usually have several levels of subdirectories (in general 2 or 3) but if I remember well question file areas don't support subdirs, and in questiontypebase $fileoptions['subdirs'] is set to false. But maybe I am mixing several things because the subdirs idea is very unclear to me !

          Show
          Jean-Michel Vedrine added a comment - - edited I tried to exploit your #18 remark but I think I have a problem I don't manage to solve : my filepathinsidetempdir usually have several levels of subdirectories (in general 2 or 3) but if I remember well question file areas don't support subdirs, and in questiontypebase $fileoptions ['subdirs'] is set to false. But maybe I am mixing several things because the subdirs idea is very unclear to me !
          Hide
          Tim Hunt added a comment -

          13. True, we could just wait for the other formats to be deprecated. Actually, we should probably aim to remove the other formats in 2.4, but let us wait until this issue is integrated, and do that in a separate issue.

          15. If you want a function with different arguments, just give it a different name, There is no need to use the readquestions function name if it does not help your format.

          18. Oh yes, the old problem about paths and no paths. I am not sure that was an entirely good feature of the file API. Is it possible to change the path during the import? I suppose it is difficult (e.g. dir1/image.png and dir2/image.png. But, what about replacing / with __, or some hack like that?

          Show
          Tim Hunt added a comment - 13. True, we could just wait for the other formats to be deprecated. Actually, we should probably aim to remove the other formats in 2.4, but let us wait until this issue is integrated, and do that in a separate issue. 15. If you want a function with different arguments, just give it a different name, There is no need to use the readquestions function name if it does not help your format. 18. Oh yes, the old problem about paths and no paths. I am not sure that was an entirely good feature of the file API. Is it possible to change the path during the import? I suppose it is difficult (e.g. dir1/image.png and dir2/image.png. But, what about replacing / with __, or some hack like that?
          Hide
          Jean-Michel Vedrine added a comment -

          15. Your idea is surely good, but I am affraid I don't follow you. How would I deal with the code in importproccess :

                  if (! $lines = $this->readdata($this->filename)) {
                      echo $OUTPUT->notification(get_string('cannotread', 'question'));
                      return false;
                  }
          
                  if (!$questions = $this->readquestions($lines)) {   // Extract all the questions
                      echo $OUTPUT->notification(get_string('noquestionsinfile', 'question'));
                      return false;
                  }
          

          if I don't use readquestions ?

          18. Yes maybe I could do something like :

          $filename = clean_param(str_replace('/', '__', $filepathinsidetempdir . '__' . $filename), PARAM_FILE);
          

          And just put '/' in the filepath of the filerecord. Is that what you mean ?

          Show
          Jean-Michel Vedrine added a comment - 15. Your idea is surely good, but I am affraid I don't follow you. How would I deal with the code in importproccess : if (! $lines = $ this ->readdata($ this ->filename)) { echo $OUTPUT->notification(get_string('cannotread', 'question')); return false ; } if (!$questions = $ this ->readquestions($lines)) { // Extract all the questions echo $OUTPUT->notification(get_string('noquestionsinfile', 'question')); return false ; } if I don't use readquestions ? 18. Yes maybe I could do something like : $filename = clean_param(str_replace('/', '__', $filepathinsidetempdir . '__' . $filename), PARAM_FILE); And just put '/' in the filepath of the filerecord. Is that what you mean ?
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          This trick to change the filename during import seems to work quite well, I think that the chance of a collision between images from differents subdirs are null, and I am able to import images in nearly all fields but I have some problems with the very special case of questiontext and generalfeedback (see importprocess in question/format.php lines 412 424). Any idea ? Overwritting importprocess seems a very bad idea. Maybe add a test on questiontext[itemid'] ?

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, This trick to change the filename during import seems to work quite well, I think that the chance of a collision between images from differents subdirs are null, and I am able to import images in nearly all fields but I have some problems with the very special case of questiontext and generalfeedback (see importprocess in question/format.php lines 412 424). Any idea ? Overwritting importprocess seems a very bad idea. Maybe add a test on questiontext [itemid'] ?
          Hide
          Jean-Michel Vedrine added a comment - - edited

          I think importproccess needs some refactoring : the $question variable can't be used to get an id from the database as questiontext and generalfeedback are arrays and we need this id to call file_save_draft_area_files if itemid is set.
          questiontypebase save_question function don't have this problem as there is both $form and $question.
          Maybe we could make importprocess use save_question or is there a big reason I don't see that prevent doing so ?
          That's a scary change to do anyway !

          Show
          Jean-Michel Vedrine added a comment - - edited I think importproccess needs some refactoring : the $question variable can't be used to get an id from the database as questiontext and generalfeedback are arrays and we need this id to call file_save_draft_area_files if itemid is set. questiontypebase save_question function don't have this problem as there is both $form and $question. Maybe we could make importprocess use save_question or is there a big reason I don't see that prevent doing so ? That's a scary change to do anyway !
          Hide
          Tim Hunt added a comment -

          18. Yes, that is what I meant. You will also have to change the <img src="..."> (or whatever) in the HTML that points to it.

          15. Oh dear. I just looked at importproccess again to remind myself how it works, and you are right, that is horrible. I can now see why you used $this->filetype. It is the least bad way to solve this at the moment. In future we really need to sort out the import format code, but I don't think we should try to do that now. (It should probably be done as part of http://tracker.moodle.org/browse/MDL-25492?focusedCommentId=173607&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-173607.)

          I don't know why questiontext and generalfeedback are done in that inconsistent way. It is a pain. (Acutally, I do know. It is because converting the quiz/question code to the files API was one of the last things done before Moodle 2.0 could be called finished, and it was done in a rush.) I suppose we will need a small change to importprocess to cope with having $q->questiontext as a three-element array. If you look at question_type::save_question, you will see that it can cope with either form. Why doesn't qformat_default call question_type::save_question? That is another one where I have no idea!

          Show
          Tim Hunt added a comment - 18. Yes, that is what I meant. You will also have to change the <img src="..."> (or whatever) in the HTML that points to it. 15. Oh dear. I just looked at importproccess again to remind myself how it works, and you are right, that is horrible. I can now see why you used $this->filetype. It is the least bad way to solve this at the moment. In future we really need to sort out the import format code, but I don't think we should try to do that now. (It should probably be done as part of http://tracker.moodle.org/browse/MDL-25492?focusedCommentId=173607&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-173607 .) I don't know why questiontext and generalfeedback are done in that inconsistent way. It is a pain. (Acutally, I do know. It is because converting the quiz/question code to the files API was one of the last things done before Moodle 2.0 could be called finished, and it was done in a rush.) I suppose we will need a small change to importprocess to cope with having $q->questiontext as a three-element array. If you look at question_type::save_question, you will see that it can cope with either form. Why doesn't qformat_default call question_type::save_question? That is another one where I have no idea!
          Hide
          Jean-Michel Vedrine added a comment -

          18. Yes it tooks me some time (and some debugging in the filestorage lib) before realizing that in my yesterday code images were saved correctly, but the img tags in the html were wrong. I managed to make it work a few hours ago this morning. The only bit left in the image processing is the questiontext/general feedback thing all the other fields are working.
          I am unable to do any profiling but the code seems faster when importing an archive with a lot of big images. Maybe this is just an impression ?

          Show
          Jean-Michel Vedrine added a comment - 18. Yes it tooks me some time (and some debugging in the filestorage lib) before realizing that in my yesterday code images were saved correctly, but the img tags in the html were wrong. I managed to make it work a few hours ago this morning. The only bit left in the image processing is the questiontext/general feedback thing all the other fields are working. I am unable to do any profiling but the code seems faster when importing an archive with a lot of big images. Maybe this is just an impression ?
          Hide
          Jean-Michel Vedrine added a comment -

          I make it works but my code is awfull !! There is surely a way to improve this :

          
                      $fileoptions = array(
                              'subdirs' => false,
                              'maxfiles' => -1,
                              'maxbytes' => 0,
                          );
                      if (!empty($question->questiontext['itemid'])) {
                          // importing images from draftfile
                          $questiontext = $question->questiontext;
                          $question->questiontext = $questiontext['text'];
                      }
                      if (!empty($question->generalfeedback['itemid'])) {
                          $generalfeedback = $question->generalfeedback;
                          $question->generalfeedback = $generalfeedback['text'];
                      }
          
                      $question->id = $DB->insert_record('question', $question);
          
                      if (!empty($questiontext['itemid'])) {
                          $question->questiontext = file_save_draft_area_files($questiontext['itemid'],
                                  $this->importcontext->id, 'question', 'questiontext', $question->id,
                                  $fileoptions, $question->questiontext);
                      } else if (isset($question->questiontextfiles)) {
                          foreach ($question->questiontextfiles as $file) {
                              question_bank::get_qtype($question->qtype)->import_file(
                                      $this->importcontext, 'question', 'questiontext', $question->id, $file);
                          }
                      }
                      if (!empty($generalfeedback['itemid'])) {
                          $question->generalfeedback = file_save_draft_area_files($generalfeedback['itemid'],
                                  $this->importcontext->id, 'question', 'generalfeedback', $question->id,
                                  $fileoptions, $question->generalfeedback);
                      } else if (isset($question->generalfeedbackfiles)) {
                          foreach ($question->generalfeedbackfiles as $file) {
                              question_bank::get_qtype($question->qtype)->import_file(
                                      $this->importcontext, 'question', 'generalfeedback', $question->id, $file);
                          }
                      }
                      $DB->update_record('question', $question);
          
          Show
          Jean-Michel Vedrine added a comment - I make it works but my code is awfull !! There is surely a way to improve this : $fileoptions = array( 'subdirs' => false , 'maxfiles' => -1, 'maxbytes' => 0, ); if (!empty($question->questiontext['itemid'])) { // importing images from draftfile $questiontext = $question->questiontext; $question->questiontext = $questiontext['text']; } if (!empty($question->generalfeedback['itemid'])) { $generalfeedback = $question->generalfeedback; $question->generalfeedback = $generalfeedback['text']; } $question->id = $DB->insert_record('question', $question); if (!empty($questiontext['itemid'])) { $question->questiontext = file_save_draft_area_files($questiontext['itemid'], $ this ->importcontext->id, 'question', 'questiontext', $question->id, $fileoptions, $question->questiontext); } else if (isset($question->questiontextfiles)) { foreach ($question->questiontextfiles as $file) { question_bank::get_qtype($question->qtype)->import_file( $ this ->importcontext, 'question', 'questiontext', $question->id, $file); } } if (!empty($generalfeedback['itemid'])) { $question->generalfeedback = file_save_draft_area_files($generalfeedback['itemid'], $ this ->importcontext->id, 'question', 'generalfeedback', $question->id, $fileoptions, $question->generalfeedback); } else if (isset($question->generalfeedbackfiles)) { foreach ($question->generalfeedbackfiles as $file) { question_bank::get_qtype($question->qtype)->import_file( $ this ->importcontext, 'question', 'generalfeedback', $question->id, $file); } } $DB->update_record('question', $question);
          Hide
          Tim Hunt added a comment -

          I cannot think of an easy way to improve that. We may just have to live with it.

          Show
          Tim Hunt added a comment - I cannot think of an easy way to improve that. We may just have to live with it.
          Hide
          Jean-Michel Vedrine added a comment -

          Results of the first peer review :

          1. Done
          2. To be done by rebasing after the weekly release if all goes well
          3. Done
          4. Done
          5. Done
          6. Done
          7. Done
          8. Don't really know what to do : all olds files have changed names and the one with an old name (question/format/blackboard_six/format.php) is entirely changed.
          9. Well spotted ! I forgot process_block but I think all the other uses of passing by reference are for arrays. So done, unless you find another one wich is quite possible !
          10. Oulala ! But for most of the functions in the parser I don't really know what they do apart they "parse something in the XML structure". Well I imagine I can give the args and return type in the phpdoc bloc as a minimum ?
          11. Done except some objects' properties names have upercase and contains underscore (see for instance $quest->QUESTION_BLOCK or $quest->RESPONSE_BLOCK) this is because of the line $question->{$block->type} = $block; in create_raw_question.
            I am somewhat hesitant to change it because I fail to clearly see if it can break something or not. But if this is mandatory, I will change it.
          12. I need to note this advice somewhere because I am quite sure it is usefull. Done.
          13. See our discussion above
          14. Done
          15. Left as is. See our discussion above
          16. Done
          17. I think it was Dan, not Eloy but it was a good comment, so I did it at the same time I changed tests for the issues send to integration
          18. Done but IMHO need some more work, see my code above

          I will commit my changes to github.

          Show
          Jean-Michel Vedrine added a comment - Results of the first peer review : Done To be done by rebasing after the weekly release if all goes well Done Done Done Done Done Don't really know what to do : all olds files have changed names and the one with an old name (question/format/blackboard_six/format.php) is entirely changed. Well spotted ! I forgot process_block but I think all the other uses of passing by reference are for arrays. So done, unless you find another one wich is quite possible ! Oulala ! But for most of the functions in the parser I don't really know what they do apart they "parse something in the XML structure". Well I imagine I can give the args and return type in the phpdoc bloc as a minimum ? Done except some objects' properties names have upercase and contains underscore (see for instance $quest->QUESTION_BLOCK or $quest->RESPONSE_BLOCK) this is because of the line $question->{$block->type} = $block; in create_raw_question. I am somewhat hesitant to change it because I fail to clearly see if it can break something or not. But if this is mandatory, I will change it. I need to note this advice somewhere because I am quite sure it is usefull. Done. See our discussion above Done Left as is. See our discussion above Done I think it was Dan, not Eloy but it was a good comment, so I did it at the same time I changed tests for the issues send to integration Done but IMHO need some more work, see my code above I will commit my changes to github.
          Hide
          Jean-Michel Vedrine added a comment -

          Are you sure we can't improve this ? It looks like spaghettis !!

          Show
          Jean-Michel Vedrine added a comment - Are you sure we can't improve this ? It looks like spaghettis !!
          Hide
          Tim Hunt added a comment -

          10. Well, you probably know more about this code that anyone else right now, so anything you can write down to help people in future would be good, but better to write nothing than to write something misleading.

          11. OK, I think leave it.

          18. Well the best way to improve it would be to change all this code to use $qtype->save_question(). However, that is another big change, and I think we should not try to do that at the same time as all these other changes.

          Show
          Tim Hunt added a comment - 10. Well, you probably know more about this code that anyone else right now, so anything you can write down to help people in future would be good, but better to write nothing than to write something misleading. 11. OK, I think leave it. 18. Well the best way to improve it would be to change all this code to use $qtype->save_question(). However, that is another big change, and I think we should not try to do that at the same time as all these other changes.
          Hide
          Jean-Michel Vedrine added a comment -

          I pushed my changes to github so maybe it's time for a 2nd peer-review ?
          But while writting this message I just realized that phpunit tests must be broken because we are no more expecting the 'text', 'format', 'files' elements in arrays !! I need to look at this

          Show
          Jean-Michel Vedrine added a comment - I pushed my changes to github so maybe it's time for a 2nd peer-review ? But while writting this message I just realized that phpunit tests must be broken because we are no more expecting the 'text', 'format', 'files' elements in arrays !! I need to look at this
          Hide
          Jean-Michel Vedrine added a comment -

          phpunit tests corrected to match the new method for importing images.

          Show
          Jean-Michel Vedrine added a comment - phpunit tests corrected to match the new method for importing images.
          Hide
          Jean-Michel Vedrine added a comment -

          Tomorrow I will work on phpdoc blocs and testing instructions.
          I was about to squash all commits but maybe it's better for now that you can look at the latest commits separately, so I will wait your approval to do so.

          Show
          Jean-Michel Vedrine added a comment - Tomorrow I will work on phpdoc blocs and testing instructions. I was about to squash all commits but maybe it's better for now that you can look at the latest commits separately, so I will wait your approval to do so.
          Hide
          Jean-Michel Vedrine added a comment -

          Some files to be used in test

          Show
          Jean-Michel Vedrine added a comment - Some files to be used in test
          Hide
          Jean-Michel Vedrine added a comment -

          More files to be used in testing

          Show
          Jean-Michel Vedrine added a comment - More files to be used in testing
          Hide
          Lawrence N added a comment -

          Hi Jean-Michel,

          When I implemented your change and tried to import a Blackboard quiz, I can see part of the code base above our header.

          We are testing on 2.2.3+

          http://ourmoodle.site.com/question/import.php

          This is what I see ...

          mview question importer. * * @package qformat * @subpackage examview * @copyright 2005 Howard Miller * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later / defined('MOODLE_INTERNAL') || die(); require_once($CFG->libdir . '/xmlize.php'); /* * Examview question importer. * * @copyright 2005 Howard Miller * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class qformat_examview extends qformat_default { public $qtypes = array( 'tf' => TRUEFALSE, 'mc' => MULTICHOICE, 'yn' => TRUEFALSE, 'co' => SHORTANSWER, 'ma' => MATCH, 'mtf' => 99, 'nr' => NUMERICAL, 'pr' => 99, 'es' => ESSAY, 'ca' => 99, 'ot' => 99, 'sa' => SHORTANSWER ); public $matching_questions = array(); public function provide_import()

          { return true; } public function mime_type() { return 'application/xml'; } /** * unxmlise reconstructs part of the xml data structure in order * to identify the actual data therein * @param array $xml section of the xml data structure * @return string data with evrything else removed / protected function unxmlise( $xml ) { // If it's not an array then it's probably just data. if (Unable to render embedded object: File (is_array($xml)) { $text = s($xml); } else { // Otherwise parse the array. $text = ''; foreach ($xml as $tag => $data) { // If tag is '@' then it's attributes and we don't care. if ($tag) not found.=='@') { $text = $text . $this->unxmlise( $data ); } } } // Currently we throw the tags we found. $text = strip_tags($text); return $text; } protected function text_field($text) { return array( 'text' => htmlspecialchars(trim($text), ENT_NOQUOTES), 'format' => FORMAT_HTML, 'files' => array(), ); } protected function parse_matching_groups($matching_groups) { if (empty($matching_groups)) { return; } foreach ($matching_groups as $match_group) { $newgroup = new stdClass(); $groupname = trim($match_group['@']['name']); $questiontext = $this->unxmlise($match_group'#'['text'][0]'#'); $newgroup->questiontext = trim($questiontext); $newgroup->subchoices = array(); $newgroup->subquestions = array(); $newgroup->subanswers = array(); $choices = $match_group'#'['choices']['0']'#'; foreach ($choices as $key => $value) { if (strpos(trim($key), 'choice-') !== false) { $key = strtoupper(trim(str_replace('choice-', '', $key))); $newgroup->subchoices[$key] = trim($value['0']['#']); } } $this->matching_questions[$groupname] = $newgroup; } } protected function parse_ma($qrec, $groupname) { $match_group = $this->matching_questions[$groupname]; $phrase = trim($this->unxmlise($qrec['text']['0']['#'])); $answer = trim($this->unxmlise($qrec['answer']['0']['#'])); $answer = strip_tags( $answer ); $match_group->subquestions[] = $phrase; $match_group->subanswers[] = $match_group->subchoices[$answer]; $this->matching_questions[$groupname] = $match_group; return null; } protected function process_matches(&$questions) { if (empty($this->matching_questions)) { return; } foreach ($this->matching_questions as $match_group) { $question = $this->defaultquestion(); $htmltext = s($match_group->questiontext); $question->questiontext = $htmltext; $question->questiontextformat = FORMAT_HTML; $question->questiontextfiles = array(); $question->name = shorten_text( $question->questiontext, 250 ); $question->qtype = MATCH; $question = $this->add_blank_combined_feedback($question); $question->subquestions = array(); $question->subanswers = array(); foreach ($match_group->subquestions as $key => $value) { $htmltext = s($value); $question->subquestions[] = $this->text_field($htmltext); $htmltext = s($match_group->subanswers[$key]); $question->subanswers[] = $htmltext; } $questions[] = $question; } } protected function cleanunicode($text) { return str_replace('’', "'", $text); } protected function readquestions($lines) { // Parses an array of lines into an array of questions, // where each item is a question object as defined by // readquestion(). $questions = array(); $currentquestion = array(); $text = implode($lines, ' '); $text = $this->cleanunicode($text); $xml = xmlize($text, 0); if (!empty($xml['examview']'#'['matching-group'])) { $this->parse_matching_groups($xml['examview']['#']['matching-group']); } $questionnode = $xml['examview']'#'['question']; foreach ($questionnode as $currentquestion) { if ($question = $this->readquestion($currentquestion)) { $questions[] = $question; } } $this->process_matches($questions); return $questions; } public function readquestion($qrec) { $type = trim($qrec['@']['type']); $question = $this->defaultquestion(); if (array_key_exists($type, $this->qtypes)) { $question->qtype = $this->qtypes[$type]; } else { $question->qtype = null; } $question->single = 1; // Only one answer is allowed. $htmltext = $this->unxmlise($qrec'#'['text'][0]'#'); $question->questiontext = $htmltext; $question->questiontextformat = FORMAT_HTML; $question->questiontextfiles = array(); $question->name = shorten_text( $question->questiontext, 250 ); switch ($question->qtype) { case MULTICHOICE: $question = $this->parse_mc($qrec['#'], $question); break; case MATCH: $groupname = trim($qrec['@']['group']); $question = $this->parse_ma($qrec['#'], $groupname); break; case TRUEFALSE: $question = $this->parse_tf_yn($qrec['#'], $question); break; case SHORTANSWER: $question = $this->parse_co($qrec['#'], $question); break; case ESSAY: $question = $this->parse_es($qrec['#'], $question); break; case NUMERICAL: $question = $this->parse_nr($qrec['#'], $question); break; break; default: print(" Question type ".$type." import not supported for ".$question->questiontext." "); $question = null; } return $question; } protected function parse_tf_yn($qrec, $question) { $choices = array('T' => 1, 'Y' => 1, 'F' => 0, 'N' => 0 ); $answer = trim($qrec['answer'][0]'#'); $question->answer = $choices[$answer]; $question->correctanswer = $question->answer; if ($question->answer == 1) { $question->feedbacktrue = $this->text_field('Correct'); $question->feedbackfalse = $this->text_field('Incorrect'); } else { $question->feedbacktrue = $this->text_field('Incorrect'); $question->feedbackfalse = $this->text_field('Correct'); } return $question; } protected function parse_mc($qrec, $question) { $question = $this->add_blank_combined_feedback($question); $answer = 'choice-'.strtolower(trim($qrec['answer'][0]'#')); $choices = $qrec['choices'][0]'#'; foreach ($choices as $key => $value) { if (strpos(trim($key), 'choice-') !== false) { $question->answer[$key] = $this->text_field(s($this->unxmlise($value[0]'#'))); if (strcmp($key, $answer) == 0) { $question->fraction[$key] = 1; $question->feedback[$key] = $this->text_field('Correct'); } else { $question->fraction[$key] = 0; $question->feedback[$key] = $this->text_field('Incorrect'); } } } return $question; } protected function parse_co($qrec, $question) { $question->usecase = 0; $answer = trim($this->unxmlise($qrec['answer'][0]'#')); $answer = strip_tags( $answer ); $answers = explode("\n", $answer); foreach ($answers as $key => $value) { $value = trim($value); if (strlen($value) > 0) { $question->answer[$key] = $value; $question->fraction[$key] = 1; $question->feedback[$key] = $this->text_field("Correct"); } } return $question; } protected function parse_es($qrec, $question) { $feedback = trim($this->unxmlise($qrec['answer'][0]['#'])); $question->graderinfo = $this->text_field($feedback); $question->feedback = $feedback; $question->responseformat = 'editor'; $question->responsefieldlines = 15; $question->attachments = 0; $question->fraction = 0; return $question; } protected function parse_nr($qrec, $question) { $answer = trim($this->unxmlise($qrec['answer'][0]'#')); $answer = strip_tags( $answer ); $answers = explode("\n", $answer); foreach ($answers as $key => $value) { $value = trim($value); if (is_numeric($value)) { $errormargin = 0; $question->answer[$key] = $value; $question->fraction[$key] = 1; $question->feedback[$key] = $this->text_field("Correct"); $question->tolerance[$key] = $errormargin; } } return $question; } } // End class. FT format question importer/exporter. * * @package qformat * @subpackage gift * @copyright 2003 Paul Tsuchido Shew * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ defined('MOODLE_INTERNAL') || die(); /* * The GIFT import filter was designed as an easy to use method * for teachers writing questions as a text file. It supports most * question types and the missing word format. * * Multiple Choice / Missing Word * Who's buried in Grant's tomb?{~Grant ~Jefferson =no one} * Grant is {~buried =entombed ~living} in Grant's tomb. * True-False: * Grant is buried in Grant's tomb.{FALSE} * Short-Answer. * Who's buried in Grant's tomb?{=no one =nobody} * Numerical * When was Ulysses S. Grant born?{#1822:5} * Matching * Match the following countries with their corresponding * capitals.{=Canada->Ottawa =Italy->Rome =Japan->Tokyo} * * Comment lines start with a double backslash (//). * Optional question names are enclosed in double colon(:. * Answer feedback is indicated with hash mark (#). * Percentage answer weights immediately follow the tilde (for * multiple choice) or equal sign (for short answer and numerical), * and are enclosed in percent signs (% %). See docs and examples.txt for more. * * This filter was written through the collaboration of numerous * members of the Moodle community. It was originally based on * the missingword format, which included code from Thomas Robb * and others. Paul Tsuchido Shew wrote this filter in December 2003. * * @copyright 2003 Paul Tsuchido Shew * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class qformat_gift extends qformat_default { public function provide_import() { return true; }

          public function provide_export()

          { return true; }

          public function export_file_extension()

          { return '.txt'; }

          protected function answerweightparser(&$answer)

          { $answer = substr($answer, 1); // removes initial % $end_position = strpos($answer, "%"); $answer_weight = substr($answer, 0, $end_position); // gets weight as integer $answer_weight = $answer_weight/100; // converts to percent $answer = substr($answer, $end_position+1); // removes comment from answer return $answer_weight; }

          protected function commentparser($answer, $defaultformat) { $bits = explode('#', $answer, 2); $ans = $this->parse_text_with_format(trim($bits[0]), $defaultformat); if (count($bits) > 1)

          { $feedback = $this->parse_text_with_format(trim($bits[1]), $defaultformat); }

          else

          { $feedback = array('text' => '', 'format' => $defaultformat, 'files' => array()); }

          return array($ans, $feedback); } protected function split_truefalse_comment($answer, $defaultformat) { $bits = explode('#', $answer, 3); $ans = $this->parse_text_with_format(trim($bits[0]), $defaultformat); if (count($bits) > 1)

          { $wrongfeedback = $this->parse_text_with_format(trim($bits[1]), $defaultformat); }

          else

          { $wrongfeedback = array('text' => '', 'format' => $defaultformat, 'files' => array()); }

          if (count($bits) > 2)

          { $rightfeedback = $this->parse_text_with_format(trim($bits[2]), $defaultformat); }

          else

          { $rightfeedback = array('text' => '', 'format' => $defaultformat, 'files' => array()); }

          return array($ans, $wrongfeedback, $rightfeedback); } protected function escapedchar_pre($string) { //Replaces escaped control characters with a placeholder BEFORE processing $escapedcharacters = array("
          :", "
          #", "
          =", "

          {", "\\}

          ", "
          ~", "
          n" ); //dlnsk $placeholders = array("&&058;", "&&035;", "&&061;", "&&123;", "&&125;", "&&126;", "&&010"); //dlnsk $string = str_replace("\\\\", "&&092;", $string); $string = str_replace($escapedcharacters, $placeholders, $string); $string = str_replace("&&092;", "
          ", $string); return $string; } protected function escapedchar_post($string) { //Replaces placeholders with corresponding character AFTER processing is done $placeholders = array("&&058;", "&&035;", "&&061;", "&&123;", "&&125;", "&&126;", "&&010"); //dlnsk $characters = array(":", "#", "=", "

          {", "}

          ", "~", "\n" ); //dlnsk $string = str_replace($placeholders, $characters, $string); return $string; } protected function check_answer_count($min, $answers, $text) { $countanswers = count($answers); if ($countanswers < $min)

          { $this->error(get_string('importminerror', 'qformat_gift'), $text); return false; }

          return true; } protected function parse_text_with_format($text, $defaultformat = FORMAT_MOODLE) { $result = array( 'text' => $text, 'format' => $defaultformat, 'files' => array(), ); if (strpos($text, '[') === 0) { $formatend = strpos($text, ']'); $result['format'] = $this->format_name_to_const(substr($text, 1, $formatend - 1)); if ($result['format'] == -1)

          { $result['format'] = $defaultformat; }

          else

          { $result['text'] = substr($text, $formatend + 1); }

          } $result['text'] = trim($this->escapedchar_post($result['text'])); return $result; } public function readquestion($lines) { // Given an array of lines known to define a question in this format, this function // converts it into a question object suitable for processing and insertion into Moodle. $question = $this->defaultquestion(); $comment = NULL; // define replaced by simple assignment, stop redefine notices $gift_answerweight_regex = '/^%-*([0-9]

          {1,2}

          )\.?([0-9]*)%/'; // REMOVED COMMENTED LINES and IMPLODE foreach ($lines as $key => $line) { $line = trim($line); if (substr($line, 0, 2) == '//')

          { $lines[$key] = ' '; }

          } $text = trim(implode(' ', $lines)); if ($text == '')

          { return false; } // Substitute escaped control characters with placeholders $text = $this->escapedchar_pre($text); // Look for category modifier if (preg_match('^\$CATEGORY:', $text)) { // $newcategory = $matches[1]; $newcategory = trim(substr($text, 10)); // build fake question to contain category $question->qtype = 'category'; $question->category = $newcategory; return $question; } // QUESTION NAME parser if (substr($text, 0, 2) == '::') { $text = substr($text, 2); $namefinish = strpos($text, '::'); if ($namefinish === false) { $question->name = false; // name will be assigned after processing question text below } else { $questionname = substr($text, 0, $namefinish); $question->name = trim($this->escapedchar_post($questionname)); $text = trim(substr($text, $namefinish+2)); // Remove name from text } } else { $question->name = false; } // FIND ANSWER section // no answer means its a description $answerstart = strpos($text, '{'); $answerfinish = strpos($text, '}'); $description = false; if (($answerstart === false) and ($answerfinish === false)) { $description = true; $answertext = ''; $answerlength = 0; } else if (Unable to render embedded object: File ( $question->defaultmark = 0; $question->length = 0; return $question; case ESSAY: $question->responseformat = 'editor'; $question->responsefieldlines = 15; $question->attachments = 0; $question->graderinfo = array( 'text' => '', 'format' => FORMAT_HTML, 'files' => array()); return $question; case MULTICHOICE: if (strpos($answertext,"=") === false) { $question->single = 0; // multiple answers are enabled if no single answer is 100% correct } else { $question->single = 1; // only one answer allowed (the default) } $question = $this->add_blank_combined_feedback($question); $answertext = str_replace("=", "~=", $answertext); $answers = explode("~", $answertext); if (isset($answers[0])) { $answers[0] = trim($answers[0]); } if (empty($answers[0])) { array_shift($answers); } $countanswers = count($answers); if () not found.$this->check_answer_count(2, $answers, $text)) { return false; }

          foreach ($answers as $key => $answer) { $answer = trim($answer); // determine answer weight if ($answer[0] == '=')

          { $answer_weight = 1; $answer = substr($answer, 1); }

          else if (preg_match($gift_answerweight_regex, $answer))

          { // check for properly formatted answer weight $answer_weight = $this->answerweightparser($answer); } else { //default, i.e., wrong anwer $answer_weight = 0; } list($question->answer[$key], $question->feedback[$key]) = $this->commentparser($answer, $question->questiontextformat); $question->fraction[$key] = $answer_weight; } // end foreach answer return $question; case MATCH: $question = $this->add_blank_combined_feedback($question); $answers = explode('=', $answertext); if (isset($answers[0])) { $answers[0] = trim($answers[0]); } if (empty($answers[0])) { array_shift($answers); } if (!$this->check_answer_count(2,$answers,$text)) { return false; } foreach ($answers as $key => $answer) { $answer = trim($answer); if (strpos($answer, ">") === false) { $this->error(get_string('giftmatchingformat','qformat_gift'), $answer); return false; } $marker = strpos($answer, '>'); $question->subquestions[$key] = $this->parse_text_with_format( substr($answer, 0, $marker), $question->questiontextformat); $question->subanswers[$key] = trim($this->escapedchar_post( substr($answer, $marker + 2))); } return $question; case TRUEFALSE: list($answer, $wrongfeedback, $rightfeedback) = $this->split_truefalse_comment($answertext, $question->questiontextformat); if ($answer['text'] == "T" OR $answer['text'] == "TRUE") { $question->correctanswer = 1; $question->feedbacktrue = $rightfeedback; $question->feedbackfalse = $wrongfeedback; } else { $question->correctanswer = 0; $question->feedbacktrue = $wrongfeedback; $question->feedbackfalse = $rightfeedback; } $question->penalty = 1; return $question; case SHORTANSWER: // SHORTANSWER Question $answers = explode("=", $answertext); if (isset($answers[0])) { $answers[0] = trim($answers[0]); } if (empty($answers[0])) { array_shift($answers); } if (!$this->check_answer_count(1, $answers, $text)) { return false; } foreach ($answers as $key => $answer) { $answer = trim($answer); // Answer weight if (preg_match($gift_answerweight_regex, $answer)) { // check for properly formatted answer weight $answer_weight = $this->answerweightparser($answer); }

          else

          { //default, i.e., full-credit anwer $answer_weight = 1; } list($answer, $question->feedback[$key]) = $this->commentparser( $answer, $question->questiontextformat); $question->answer[$key] = $answer['text']; $question->fraction[$key] = $answer_weight; } return $question; case NUMERICAL: // Note similarities to ShortAnswer $answertext = substr($answertext, 1); // remove leading "#" // If there is feedback for a wrong answer, store it for now. if (($pos = strpos($answertext, '~')) !== false) { $wrongfeedback = substr($answertext, $pos); $answertext = substr($answertext, 0, $pos); } else { $wrongfeedback = ''; } $answers = explode("=", $answertext); if (isset($answers[0])) { $answers[0] = trim($answers[0]); } if (empty($answers[0])) { array_shift($answers); } if (count($answers) == 0) { // invalid question $giftnonumericalanswers = get_string('giftnonumericalanswers','qformat_gift'); $this->error($giftnonumericalanswers, $text); return false; } foreach ($answers as $key => $answer) { $answer = trim($answer); // Answer weight if (preg_match($gift_answerweight_regex, $answer)) { // check for properly formatted answer weight $answer_weight = $this->answerweightparser($answer); } else { //default, i.e., full-credit anwer $answer_weight = 1; }

          list($answer, $question->feedback[$key]) = $this->commentparser( $answer, $question->questiontextformat); $question->fraction[$key] = $answer_weight; $answer = $answer['text']; //Calculate Answer and Min/Max values if (strpos($answer,"..") > 0)

          { // optional [min]..[max] format $marker = strpos($answer,".."); $max = trim(substr($answer, $marker+2)); $min = trim(substr($answer, 0, $marker)); $ans = ($max + $min)/2; $tol = $max - $ans; }

          else if (strpos($answer, ':') > 0)

          { // standard [answer]:[errormargin] format $marker = strpos($answer, ':'); $tol = trim(substr($answer, $marker+1)); $ans = trim(substr($answer, 0, $marker)); }

          else

          { // only one valid answer (zero errormargin) $tol = 0; $ans = trim($answer); }

          if (!(is_numeric($ans) || $ans = '*') || !is_numeric($tol))

          { $errornotnumbers = get_string('errornotnumbers'); $this->error($errornotnumbers, $text); return false; }

          // store results $question->answer[$key] = $ans; $question->tolerance[$key] = $tol; } if ($wrongfeedback)

          { $key += 1; $question->fraction[$key] = 0; list($notused, $question->feedback[$key]) = $this->commentparser( $wrongfeedback, $question->questiontextformat); $question->answer[$key] = '*'; $question->tolerance[$key] = ''; }

          return $question; default: $this->error(get_string('giftnovalidquestion', 'qformat_gift'), $text); return false; } } protected function repchar($text, $notused = 0) { // Escapes 'reserved' characters # = ~ {) : // Removes new lines $reserved = array( '
          ', '#', '=', '~', '

          {', '}

          ', ':', "\n", "\r"); $escaped = array('\\\\', '#','\=','~','{','}','\:', '\n', '' ); $newtext = str_replace($reserved, $escaped, $text); return $newtext; } /** * @param int $format one of the FORMAT_ constants. * @return string the corresponding name. */ protected function format_const_to_name($format) { if ($format == FORMAT_MOODLE)

          { return 'moodle'; } else if ($format == FORMAT_HTML) { return 'html'; } else if ($format == FORMAT_PLAIN) { return 'plain'; } else if ($format == FORMAT_MARKDOWN) { return 'markdown'; } else { return 'moodle'; }

          } /** * @param int $format one of the FORMAT_ constants. * @return string the corresponding name. */ protected function format_name_to_const($format) { if ($format == 'moodle')

          { return FORMAT_MOODLE; }

          else if ($format == 'html')

          { return FORMAT_HTML; }

          else if ($format == 'plain')

          { return FORMAT_PLAIN; }

          else if ($format == 'markdown')

          { return FORMAT_MARKDOWN; }

          else

          { return -1; }

          } public function write_name($name)

          { return '::' . $this->repchar($name) . '::'; }

          public function write_questiontext($text, $format, $defaultformat = FORMAT_MOODLE) { $output = ''; if ($text != '' && $format != $defaultformat)

          { $output .= '[' . $this->format_const_to_name($format) . ']'; }

          $output .= $this->repchar($text, $format); return $output; } public function writequestion($question) { global $OUTPUT; // Start with a comment $expout = "// question: $question->id name: $question->name\n"; // output depends on question type switch($question->qtype) { case 'category': // not a real question, used to insert category switch $expout .= "\$CATEGORY: $question->category\n"; break; case DESCRIPTION: $expout .= $this->write_name($question->name); $expout .= $this->write_questiontext($question->questiontext, $question->questiontextformat); break; case ESSAY: $expout .= $this->write_name($question->name); $expout .= $this->write_questiontext($question->questiontext, $question->questiontextformat); $expout .= "{}\n"; break; case TRUEFALSE: $trueanswer = $question->options->answers[$question->options->trueanswer]; $falseanswer = $question->options->answers[$question->options->falseanswer]; if ($trueanswer->fraction == 1)

          { $answertext = 'TRUE'; $rightfeedback = $this->write_questiontext($trueanswer->feedback, $trueanswer->feedbackformat, $question->questiontextformat); $wrongfeedback = $this->write_questiontext($falseanswer->feedback, $falseanswer->feedbackformat, $question->questiontextformat); }

          else

          { $answertext = 'FALSE'; $rightfeedback = $this->write_questiontext($falseanswer->feedback, $falseanswer->feedbackformat, $question->questiontextformat); $wrongfeedback = $this->write_questiontext($trueanswer->feedback, $trueanswer->feedbackformat, $question->questiontextformat); }

          $expout .= $this->write_name($question->name); $expout .= $this->write_questiontext($question->questiontext, $question->questiontextformat); $expout .= '{' . $this->repchar($answertext); if ($wrongfeedback)

          { $expout .= '#' . $wrongfeedback; }

          else if ($rightfeedback)

          { $expout .= '#'; }

          if ($rightfeedback)

          { $expout .= '#' . $rightfeedback; }

          $expout .= "}\n"; break; case MULTICHOICE: $expout .= $this->write_name($question->name); $expout .= $this->write_questiontext($question->questiontext, $question->questiontextformat); $expout .= "{\n"; foreach($question->options->answers as $answer) { if ($answer->fraction == 1)

          { $answertext = '='; }

          else if ($answer->fraction == 0)

          { $answertext = '~'; }

          else

          { $weight = $answer->fraction * 100; $answertext = '~%' . $weight . '%'; }

          $expout .= "\t" . $answertext . $this->write_questiontext($answer->answer, $answer->answerformat, $question->questiontextformat); if ($answer->feedback != '')

          { $expout .= '#' . $this->write_questiontext($answer->feedback, $answer->feedbackformat, $question->questiontextformat); }

          $expout .= "\n"; } $expout .= "}\n"; break; case SHORTANSWER: $expout .= $this->write_name($question->name); $expout .= $this->write_questiontext($question->questiontext, $question->questiontextformat); $expout .= "{\n"; foreach($question->options->answers as $answer)

          { $weight = 100 * $answer->fraction; $expout .= "\t=%" . $weight . '%' . $this->repchar($answer->answer) . '#' . $this->write_questiontext($answer->feedback, $answer->feedbackformat, $question->questiontextformat) . "\n"; }

          $expout .= "}\n"; break; case NUMERICAL: $expout .= $this->write_name($question->name); $expout .= $this->write_questiontext($question->questiontext, $question->questiontextformat); $expout .= "{#\n"; foreach ($question->options->answers as $answer) { if ($answer->answer != '' && $answer->answer != '*')

          { $weight = 100 * $answer->fraction; $expout .= "\t=%" . $weight . '%' . $answer->answer . ':' . (float)$answer->tolerance . '#' . $this->write_questiontext($answer->feedback, $answer->feedbackformat, $question->questiontextformat) . "\n"; }

          else

          { $expout .= "\t~#" . $this->write_questiontext($answer->feedback, $answer->feedbackformat, $question->questiontextformat) . "\n"; }

          } $expout .= "}\n"; break; case MATCH: $expout .= $this->write_name($question->name); $expout .= $this->write_questiontext($question->questiontext, $question->questiontextformat); $expout .= "{\n"; foreach($question->options->subquestions as $subquestion)

          { $expout .= "\t=" . $this->write_questiontext($subquestion->questiontext, $subquestion->questiontextformat, $question->questiontextformat) . ' -> ' . $this->repchar($subquestion->answertext) . "\n"; }

          $expout .= "}\n"; break; default: // Check for plugins if ($out = $this->try_exporting_using_qtypes($question->qtype, $question))

          { $expout .= $out; }

          else

          { $expout .= "Question type $question->qtype is not supported\n"; echo $OUTPUT->notification(get_string('nohandler', 'qformat_gift', question_bank::get_qtype_name($question->qtype))); }

          } // Add empty line to delimit questions $expout .= "\n"; return $expout; } }

          then shows the top of our page

          Show
          Lawrence N added a comment - Hi Jean-Michel, When I implemented your change and tried to import a Blackboard quiz, I can see part of the code base above our header. We are testing on 2.2.3+ http://ourmoodle.site.com/question/import.php This is what I see ... mview question importer. * * @package qformat * @subpackage examview * @copyright 2005 Howard Miller * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later / defined('MOODLE_INTERNAL') || die(); require_once($CFG->libdir . '/xmlize.php'); / * * Examview question importer. * * @copyright 2005 Howard Miller * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class qformat_examview extends qformat_default { public $qtypes = array( 'tf' => TRUEFALSE, 'mc' => MULTICHOICE, 'yn' => TRUEFALSE, 'co' => SHORTANSWER, 'ma' => MATCH, 'mtf' => 99, 'nr' => NUMERICAL, 'pr' => 99, 'es' => ESSAY, 'ca' => 99, 'ot' => 99, 'sa' => SHORTANSWER ); public $matching_questions = array(); public function provide_import() { return true; } public function mime_type() { return 'application/xml'; } /** * unxmlise reconstructs part of the xml data structure in order * to identify the actual data therein * @param array $xml section of the xml data structure * @return string data with evrything else removed / protected function unxmlise( $xml ) { // If it's not an array then it's probably just data. if ( Unable to render embedded object: File (is_array($xml)) { $text = s($xml); } else { // Otherwise parse the array. $text = ''; foreach ($xml as $tag => $data) { // If tag is '@' then it's attributes and we don't care. if ($tag) not found. =='@') { $text = $text . $this->unxmlise( $data ); } } } // Currently we throw the tags we found. $text = strip_tags($text); return $text; } protected function text_field($text) { return array( 'text' => htmlspecialchars(trim($text), ENT_NOQUOTES), 'format' => FORMAT_HTML, 'files' => array(), ); } protected function parse_matching_groups($matching_groups) { if (empty($matching_groups)) { return; } foreach ($matching_groups as $match_group) { $newgroup = new stdClass(); $groupname = trim($match_group ['@'] ['name'] ); $questiontext = $this->unxmlise($match_group '#' ['text'] [0] '#' ); $newgroup->questiontext = trim($questiontext); $newgroup->subchoices = array(); $newgroup->subquestions = array(); $newgroup->subanswers = array(); $choices = $match_group '#' ['choices'] ['0'] '#' ; foreach ($choices as $key => $value) { if (strpos(trim($key), 'choice-') !== false) { $key = strtoupper(trim(str_replace('choice-', '', $key))); $newgroup->subchoices[$key] = trim($value['0']['#']); } } $this->matching_questions [$groupname] = $newgroup; } } protected function parse_ma($qrec, $groupname) { $match_group = $this->matching_questions[$groupname]; $phrase = trim($this->unxmlise($qrec['text']['0']['#'])); $answer = trim($this->unxmlise($qrec['answer']['0']['#'])); $answer = strip_tags( $answer ); $match_group->subquestions[] = $phrase; $match_group->subanswers[] = $match_group->subchoices[$answer]; $this->matching_questions[$groupname] = $match_group; return null; } protected function process_matches(&$questions) { if (empty($this->matching_questions)) { return; } foreach ($this->matching_questions as $match_group) { $question = $this->defaultquestion(); $htmltext = s($match_group->questiontext); $question->questiontext = $htmltext; $question->questiontextformat = FORMAT_HTML; $question->questiontextfiles = array(); $question->name = shorten_text( $question->questiontext, 250 ); $question->qtype = MATCH; $question = $this->add_blank_combined_feedback($question); $question->subquestions = array(); $question->subanswers = array(); foreach ($match_group->subquestions as $key => $value) { $htmltext = s($value); $question->subquestions[] = $this->text_field($htmltext); $htmltext = s($match_group->subanswers[$key]); $question->subanswers[] = $htmltext; } $questions[] = $question; } } protected function cleanunicode($text) { return str_replace('’', "'", $text); } protected function readquestions($lines) { // Parses an array of lines into an array of questions, // where each item is a question object as defined by // readquestion(). $questions = array(); $currentquestion = array(); $text = implode($lines, ' '); $text = $this->cleanunicode($text); $xml = xmlize($text, 0); if (!empty($xml ['examview'] '#' ['matching-group'] )) { $this->parse_matching_groups($xml['examview']['#']['matching-group']); } $questionnode = $xml ['examview'] '#' ['question'] ; foreach ($questionnode as $currentquestion) { if ($question = $this->readquestion($currentquestion)) { $questions[] = $question; } } $this->process_matches($questions); return $questions; } public function readquestion($qrec) { $type = trim($qrec ['@'] ['type'] ); $question = $this->defaultquestion(); if (array_key_exists($type, $this->qtypes)) { $question->qtype = $this->qtypes[$type]; } else { $question->qtype = null; } $question->single = 1; // Only one answer is allowed. $htmltext = $this->unxmlise($qrec '#' ['text'] [0] '#' ); $question->questiontext = $htmltext; $question->questiontextformat = FORMAT_HTML; $question->questiontextfiles = array(); $question->name = shorten_text( $question->questiontext, 250 ); switch ($question->qtype) { case MULTICHOICE: $question = $this->parse_mc($qrec['#'], $question); break; case MATCH: $groupname = trim($qrec['@']['group']); $question = $this->parse_ma($qrec['#'], $groupname); break; case TRUEFALSE: $question = $this->parse_tf_yn($qrec['#'], $question); break; case SHORTANSWER: $question = $this->parse_co($qrec['#'], $question); break; case ESSAY: $question = $this->parse_es($qrec['#'], $question); break; case NUMERICAL: $question = $this->parse_nr($qrec['#'], $question); break; break; default: print(" Question type ".$type." import not supported for ".$question->questiontext." "); $question = null; } return $question; } protected function parse_tf_yn($qrec, $question) { $choices = array('T' => 1, 'Y' => 1, 'F' => 0, 'N' => 0 ); $answer = trim($qrec ['answer'] [0] '#' ); $question->answer = $choices [$answer] ; $question->correctanswer = $question->answer; if ($question->answer == 1) { $question->feedbacktrue = $this->text_field('Correct'); $question->feedbackfalse = $this->text_field('Incorrect'); } else { $question->feedbacktrue = $this->text_field('Incorrect'); $question->feedbackfalse = $this->text_field('Correct'); } return $question; } protected function parse_mc($qrec, $question) { $question = $this->add_blank_combined_feedback($question); $answer = 'choice-'.strtolower(trim($qrec ['answer'] [0] '#' )); $choices = $qrec ['choices'] [0] '#' ; foreach ($choices as $key => $value) { if (strpos(trim($key), 'choice-') !== false) { $question->answer [$key] = $this->text_field(s($this->unxmlise($value [0] '#' ))); if (strcmp($key, $answer) == 0) { $question->fraction[$key] = 1; $question->feedback[$key] = $this->text_field('Correct'); } else { $question->fraction[$key] = 0; $question->feedback[$key] = $this->text_field('Incorrect'); } } } return $question; } protected function parse_co($qrec, $question) { $question->usecase = 0; $answer = trim($this->unxmlise($qrec ['answer'] [0] '#' )); $answer = strip_tags( $answer ); $answers = explode("\n", $answer); foreach ($answers as $key => $value) { $value = trim($value); if (strlen($value) > 0) { $question->answer[$key] = $value; $question->fraction[$key] = 1; $question->feedback[$key] = $this->text_field("Correct"); } } return $question; } protected function parse_es($qrec, $question) { $feedback = trim($this->unxmlise($qrec['answer'][0]['#'])); $question->graderinfo = $this->text_field($feedback); $question->feedback = $feedback; $question->responseformat = 'editor'; $question->responsefieldlines = 15; $question->attachments = 0; $question->fraction = 0; return $question; } protected function parse_nr($qrec, $question) { $answer = trim($this->unxmlise($qrec ['answer'] [0] '#' )); $answer = strip_tags( $answer ); $answers = explode("\n", $answer); foreach ($answers as $key => $value) { $value = trim($value); if (is_numeric($value)) { $errormargin = 0; $question->answer[$key] = $value; $question->fraction[$key] = 1; $question->feedback[$key] = $this->text_field("Correct"); $question->tolerance[$key] = $errormargin; } } return $question; } } // End class. FT format question importer/exporter. * * @package qformat * @subpackage gift * @copyright 2003 Paul Tsuchido Shew * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ defined('MOODLE_INTERNAL') || die(); / * * The GIFT import filter was designed as an easy to use method * for teachers writing questions as a text file. It supports most * question types and the missing word format. * * Multiple Choice / Missing Word * Who's buried in Grant's tomb?{~Grant ~Jefferson =no one} * Grant is {~buried =entombed ~living} in Grant's tomb. * True-False: * Grant is buried in Grant's tomb.{FALSE} * Short-Answer. * Who's buried in Grant's tomb?{=no one =nobody} * Numerical * When was Ulysses S. Grant born?{#1822:5} * Matching * Match the following countries with their corresponding * capitals.{=Canada->Ottawa =Italy->Rome =Japan->Tokyo} * * Comment lines start with a double backslash (//). * Optional question names are enclosed in double colon(: . * Answer feedback is indicated with hash mark (#). * Percentage answer weights immediately follow the tilde (for * multiple choice) or equal sign (for short answer and numerical), * and are enclosed in percent signs (% %). See docs and examples.txt for more. * * This filter was written through the collaboration of numerous * members of the Moodle community. It was originally based on * the missingword format, which included code from Thomas Robb * and others. Paul Tsuchido Shew wrote this filter in December 2003. * * @copyright 2003 Paul Tsuchido Shew * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class qformat_gift extends qformat_default { public function provide_import() { return true; } public function provide_export() { return true; } public function export_file_extension() { return '.txt'; } protected function answerweightparser(&$answer) { $answer = substr($answer, 1); // removes initial % $end_position = strpos($answer, "%"); $answer_weight = substr($answer, 0, $end_position); // gets weight as integer $answer_weight = $answer_weight/100; // converts to percent $answer = substr($answer, $end_position+1); // removes comment from answer return $answer_weight; } protected function commentparser($answer, $defaultformat) { $bits = explode('#', $answer, 2); $ans = $this->parse_text_with_format(trim($bits [0] ), $defaultformat); if (count($bits) > 1) { $feedback = $this->parse_text_with_format(trim($bits[1]), $defaultformat); } else { $feedback = array('text' => '', 'format' => $defaultformat, 'files' => array()); } return array($ans, $feedback); } protected function split_truefalse_comment($answer, $defaultformat) { $bits = explode('#', $answer, 3); $ans = $this->parse_text_with_format(trim($bits [0] ), $defaultformat); if (count($bits) > 1) { $wrongfeedback = $this->parse_text_with_format(trim($bits[1]), $defaultformat); } else { $wrongfeedback = array('text' => '', 'format' => $defaultformat, 'files' => array()); } if (count($bits) > 2) { $rightfeedback = $this->parse_text_with_format(trim($bits[2]), $defaultformat); } else { $rightfeedback = array('text' => '', 'format' => $defaultformat, 'files' => array()); } return array($ans, $wrongfeedback, $rightfeedback); } protected function escapedchar_pre($string) { //Replaces escaped control characters with a placeholder BEFORE processing $escapedcharacters = array(" :", " #", " =", " {", "\\} ", " ~", " n" ); //dlnsk $placeholders = array("&&058;", "&&035;", "&&061;", "&&123;", "&&125;", "&&126;", "&&010"); //dlnsk $string = str_replace("\\\\", "&&092;", $string); $string = str_replace($escapedcharacters, $placeholders, $string); $string = str_replace("&&092;", " ", $string); return $string; } protected function escapedchar_post($string) { //Replaces placeholders with corresponding character AFTER processing is done $placeholders = array("&&058;", "&&035;", "&&061;", "&&123;", "&&125;", "&&126;", "&&010"); //dlnsk $characters = array(":", "#", "=", " {", "} ", "~", "\n" ); //dlnsk $string = str_replace($placeholders, $characters, $string); return $string; } protected function check_answer_count($min, $answers, $text) { $countanswers = count($answers); if ($countanswers < $min) { $this->error(get_string('importminerror', 'qformat_gift'), $text); return false; } return true; } protected function parse_text_with_format($text, $defaultformat = FORMAT_MOODLE) { $result = array( 'text' => $text, 'format' => $defaultformat, 'files' => array(), ); if (strpos($text, ' [') === 0) { $formatend = strpos($text, '] '); $result ['format'] = $this->format_name_to_const(substr($text, 1, $formatend - 1)); if ($result ['format'] == -1) { $result['format'] = $defaultformat; } else { $result['text'] = substr($text, $formatend + 1); } } $result ['text'] = trim($this->escapedchar_post($result ['text'] )); return $result; } public function readquestion($lines) { // Given an array of lines known to define a question in this format, this function // converts it into a question object suitable for processing and insertion into Moodle. $question = $this->defaultquestion(); $comment = NULL; // define replaced by simple assignment, stop redefine notices $gift_answerweight_regex = '/^%-*( [0-9] {1,2} )\.?( [0-9] *)%/'; // REMOVED COMMENTED LINES and IMPLODE foreach ($lines as $key => $line) { $line = trim($line); if (substr($line, 0, 2) == '//') { $lines[$key] = ' '; } } $text = trim(implode(' ', $lines)); if ($text == '') { return false; } // Substitute escaped control characters with placeholders $text = $this->escapedchar_pre($text); // Look for category modifier if (preg_match(' ^\$CATEGORY: ', $text)) { // $newcategory = $matches[1]; $newcategory = trim(substr($text, 10)); // build fake question to contain category $question->qtype = 'category'; $question->category = $newcategory; return $question; } // QUESTION NAME parser if (substr($text, 0, 2) == '::') { $text = substr($text, 2); $namefinish = strpos($text, '::'); if ($namefinish === false) { $question->name = false; // name will be assigned after processing question text below } else { $questionname = substr($text, 0, $namefinish); $question->name = trim($this->escapedchar_post($questionname)); $text = trim(substr($text, $namefinish+2)); // Remove name from text } } else { $question->name = false; } // FIND ANSWER section // no answer means its a description $answerstart = strpos($text, '{'); $answerfinish = strpos($text, '}'); $description = false; if (($answerstart === false) and ($answerfinish === false)) { $description = true; $answertext = ''; $answerlength = 0; } else if ( Unable to render embedded object: File ( $question->defaultmark = 0; $question->length = 0; return $question; case ESSAY: $question->responseformat = 'editor'; $question->responsefieldlines = 15; $question->attachments = 0; $question->graderinfo = array( 'text' => '', 'format' => FORMAT_HTML, 'files' => array()); return $question; case MULTICHOICE: if (strpos($answertext,"=") === false) { $question->single = 0; // multiple answers are enabled if no single answer is 100% correct } else { $question->single = 1; // only one answer allowed (the default) } $question = $this->add_blank_combined_feedback($question); $answertext = str_replace("=", "~=", $answertext); $answers = explode("~", $answertext); if (isset($answers[0])) { $answers[0] = trim($answers[0]); } if (empty($answers[0])) { array_shift($answers); } $countanswers = count($answers); if () not found. $this->check_answer_count(2, $answers, $text)) { return false; } foreach ($answers as $key => $answer) { $answer = trim($answer); // determine answer weight if ($answer [0] == '=') { $answer_weight = 1; $answer = substr($answer, 1); } else if (preg_match($gift_answerweight_regex, $answer)) { // check for properly formatted answer weight $answer_weight = $this->answerweightparser($answer); } else { //default, i.e., wrong anwer $answer_weight = 0; } list($question->answer [$key] , $question->feedback [$key] ) = $this->commentparser($answer, $question->questiontextformat); $question->fraction [$key] = $answer_weight; } // end foreach answer return $question; case MATCH: $question = $this->add_blank_combined_feedback($question); $answers = explode('=', $answertext); if (isset($answers [0] )) { $answers[0] = trim($answers[0]); } if (empty($answers [0] )) { array_shift($answers); } if (!$this->check_answer_count(2,$answers,$text)) { return false; } foreach ($answers as $key => $answer) { $answer = trim($answer); if (strpos($answer, " >") === false) { $this->error(get_string('giftmatchingformat','qformat_gift'), $answer); return false; } $marker = strpos($answer, ' >'); $question->subquestions [$key] = $this->parse_text_with_format( substr($answer, 0, $marker), $question->questiontextformat); $question->subanswers [$key] = trim($this->escapedchar_post( substr($answer, $marker + 2))); } return $question; case TRUEFALSE: list($answer, $wrongfeedback, $rightfeedback) = $this->split_truefalse_comment($answertext, $question->questiontextformat); if ($answer ['text'] == "T" OR $answer ['text'] == "TRUE") { $question->correctanswer = 1; $question->feedbacktrue = $rightfeedback; $question->feedbackfalse = $wrongfeedback; } else { $question->correctanswer = 0; $question->feedbacktrue = $wrongfeedback; $question->feedbackfalse = $rightfeedback; } $question->penalty = 1; return $question; case SHORTANSWER: // SHORTANSWER Question $answers = explode("=", $answertext); if (isset($answers [0] )) { $answers[0] = trim($answers[0]); } if (empty($answers [0] )) { array_shift($answers); } if (!$this->check_answer_count(1, $answers, $text)) { return false; } foreach ($answers as $key => $answer) { $answer = trim($answer); // Answer weight if (preg_match($gift_answerweight_regex, $answer)) { // check for properly formatted answer weight $answer_weight = $this->answerweightparser($answer); } else { //default, i.e., full-credit anwer $answer_weight = 1; } list($answer, $question->feedback [$key] ) = $this->commentparser( $answer, $question->questiontextformat); $question->answer [$key] = $answer ['text'] ; $question->fraction [$key] = $answer_weight; } return $question; case NUMERICAL: // Note similarities to ShortAnswer $answertext = substr($answertext, 1); // remove leading "#" // If there is feedback for a wrong answer, store it for now. if (($pos = strpos($answertext, '~')) !== false) { $wrongfeedback = substr($answertext, $pos); $answertext = substr($answertext, 0, $pos); } else { $wrongfeedback = ''; } $answers = explode("=", $answertext); if (isset($answers [0] )) { $answers[0] = trim($answers[0]); } if (empty($answers [0] )) { array_shift($answers); } if (count($answers) == 0) { // invalid question $giftnonumericalanswers = get_string('giftnonumericalanswers','qformat_gift'); $this->error($giftnonumericalanswers, $text); return false; } foreach ($answers as $key => $answer) { $answer = trim($answer); // Answer weight if (preg_match($gift_answerweight_regex, $answer)) { // check for properly formatted answer weight $answer_weight = $this->answerweightparser($answer); } else { //default, i.e., full-credit anwer $answer_weight = 1; } list($answer, $question->feedback [$key] ) = $this->commentparser( $answer, $question->questiontextformat); $question->fraction [$key] = $answer_weight; $answer = $answer ['text'] ; //Calculate Answer and Min/Max values if (strpos($answer,"..") > 0) { // optional [min]..[max] format $marker = strpos($answer,".."); $max = trim(substr($answer, $marker+2)); $min = trim(substr($answer, 0, $marker)); $ans = ($max + $min)/2; $tol = $max - $ans; } else if (strpos($answer, ':') > 0) { // standard [answer]:[errormargin] format $marker = strpos($answer, ':'); $tol = trim(substr($answer, $marker+1)); $ans = trim(substr($answer, 0, $marker)); } else { // only one valid answer (zero errormargin) $tol = 0; $ans = trim($answer); } if (!(is_numeric($ans) || $ans = '*') || !is_numeric($tol)) { $errornotnumbers = get_string('errornotnumbers'); $this->error($errornotnumbers, $text); return false; } // store results $question->answer [$key] = $ans; $question->tolerance [$key] = $tol; } if ($wrongfeedback) { $key += 1; $question->fraction[$key] = 0; list($notused, $question->feedback[$key]) = $this->commentparser( $wrongfeedback, $question->questiontextformat); $question->answer[$key] = '*'; $question->tolerance[$key] = ''; } return $question; default: $this->error(get_string('giftnovalidquestion', 'qformat_gift'), $text); return false; } } protected function repchar($text, $notused = 0) { // Escapes 'reserved' characters # = ~ {) : // Removes new lines $reserved = array( ' ', '#', '=', '~', ' {', '} ', ':', "\n", "\r"); $escaped = array('\\\\', '#','\=','~','{','}','\:', '\n', '' ); $newtext = str_replace($reserved, $escaped, $text); return $newtext; } /** * @param int $format one of the FORMAT_ constants. * @return string the corresponding name. */ protected function format_const_to_name($format) { if ($format == FORMAT_MOODLE) { return 'moodle'; } else if ($format == FORMAT_HTML) { return 'html'; } else if ($format == FORMAT_PLAIN) { return 'plain'; } else if ($format == FORMAT_MARKDOWN) { return 'markdown'; } else { return 'moodle'; } } /** * @param int $format one of the FORMAT_ constants. * @return string the corresponding name. */ protected function format_name_to_const($format) { if ($format == 'moodle') { return FORMAT_MOODLE; } else if ($format == 'html') { return FORMAT_HTML; } else if ($format == 'plain') { return FORMAT_PLAIN; } else if ($format == 'markdown') { return FORMAT_MARKDOWN; } else { return -1; } } public function write_name($name) { return '::' . $this->repchar($name) . '::'; } public function write_questiontext($text, $format, $defaultformat = FORMAT_MOODLE) { $output = ''; if ($text != '' && $format != $defaultformat) { $output .= '[' . $this->format_const_to_name($format) . ']'; } $output .= $this->repchar($text, $format); return $output; } public function writequestion($question) { global $OUTPUT; // Start with a comment $expout = "// question: $question->id name: $question->name\n"; // output depends on question type switch($question->qtype) { case 'category': // not a real question, used to insert category switch $expout .= "\$CATEGORY: $question->category\n"; break; case DESCRIPTION: $expout .= $this->write_name($question->name); $expout .= $this->write_questiontext($question->questiontext, $question->questiontextformat); break; case ESSAY: $expout .= $this->write_name($question->name); $expout .= $this->write_questiontext($question->questiontext, $question->questiontextformat); $expout .= "{}\n"; break; case TRUEFALSE: $trueanswer = $question->options->answers [$question->options->trueanswer] ; $falseanswer = $question->options->answers [$question->options->falseanswer] ; if ($trueanswer->fraction == 1) { $answertext = 'TRUE'; $rightfeedback = $this->write_questiontext($trueanswer->feedback, $trueanswer->feedbackformat, $question->questiontextformat); $wrongfeedback = $this->write_questiontext($falseanswer->feedback, $falseanswer->feedbackformat, $question->questiontextformat); } else { $answertext = 'FALSE'; $rightfeedback = $this->write_questiontext($falseanswer->feedback, $falseanswer->feedbackformat, $question->questiontextformat); $wrongfeedback = $this->write_questiontext($trueanswer->feedback, $trueanswer->feedbackformat, $question->questiontextformat); } $expout .= $this->write_name($question->name); $expout .= $this->write_questiontext($question->questiontext, $question->questiontextformat); $expout .= '{' . $this->repchar($answertext); if ($wrongfeedback) { $expout .= '#' . $wrongfeedback; } else if ($rightfeedback) { $expout .= '#'; } if ($rightfeedback) { $expout .= '#' . $rightfeedback; } $expout .= "}\n"; break; case MULTICHOICE: $expout .= $this->write_name($question->name); $expout .= $this->write_questiontext($question->questiontext, $question->questiontextformat); $expout .= "{\n"; foreach($question->options->answers as $answer) { if ($answer->fraction == 1) { $answertext = '='; } else if ($answer->fraction == 0) { $answertext = '~'; } else { $weight = $answer->fraction * 100; $answertext = '~%' . $weight . '%'; } $expout .= "\t" . $answertext . $this->write_questiontext($answer->answer, $answer->answerformat, $question->questiontextformat); if ($answer->feedback != '') { $expout .= '#' . $this->write_questiontext($answer->feedback, $answer->feedbackformat, $question->questiontextformat); } $expout .= "\n"; } $expout .= "}\n"; break; case SHORTANSWER: $expout .= $this->write_name($question->name); $expout .= $this->write_questiontext($question->questiontext, $question->questiontextformat); $expout .= "{\n"; foreach($question->options->answers as $answer) { $weight = 100 * $answer->fraction; $expout .= "\t=%" . $weight . '%' . $this->repchar($answer->answer) . '#' . $this->write_questiontext($answer->feedback, $answer->feedbackformat, $question->questiontextformat) . "\n"; } $expout .= "}\n"; break; case NUMERICAL: $expout .= $this->write_name($question->name); $expout .= $this->write_questiontext($question->questiontext, $question->questiontextformat); $expout .= "{#\n"; foreach ($question->options->answers as $answer) { if ($answer->answer != '' && $answer->answer != '*') { $weight = 100 * $answer->fraction; $expout .= "\t=%" . $weight . '%' . $answer->answer . ':' . (float)$answer->tolerance . '#' . $this->write_questiontext($answer->feedback, $answer->feedbackformat, $question->questiontextformat) . "\n"; } else { $expout .= "\t~#" . $this->write_questiontext($answer->feedback, $answer->feedbackformat, $question->questiontextformat) . "\n"; } } $expout .= "}\n"; break; case MATCH: $expout .= $this->write_name($question->name); $expout .= $this->write_questiontext($question->questiontext, $question->questiontextformat); $expout .= "{\n"; foreach($question->options->subquestions as $subquestion) { $expout .= "\t=" . $this->write_questiontext($subquestion->questiontext, $subquestion->questiontextformat, $question->questiontextformat) . ' -> ' . $this->repchar($subquestion->answertext) . "\n"; } $expout .= "}\n"; break; default: // Check for plugins if ($out = $this->try_exporting_using_qtypes($question->qtype, $question)) { $expout .= $out; } else { $expout .= "Question type $question->qtype is not supported\n"; echo $OUTPUT->notification(get_string('nohandler', 'qformat_gift', question_bank::get_qtype_name($question->qtype))); } } // Add empty line to delimit questions $expout .= "\n"; return $expout; } } then shows the top of our page
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Christpher,
          Most probably you have messed some files when implementing the changes. php code should not be displayed.
          Some points :

          • the Pull master diff url above is for master only. I have not yet done the branches for Moodle 2.3 (and 2.2 if people from moodle HQ agree to backport this to 2.2 but I have some doubt they will be OK ... fingers crossed)
          • It's very easy to get it wrong unless you use git and know how to cherry-pick
          • this change is dependant on other changes (for instance it use changes in question/format.php) that I commited in other issues when I fixed other import formats.
          Show
          Jean-Michel Vedrine added a comment - Hello Christpher, Most probably you have messed some files when implementing the changes. php code should not be displayed. Some points : the Pull master diff url above is for master only. I have not yet done the branches for Moodle 2.3 (and 2.2 if people from moodle HQ agree to backport this to 2.2 but I have some doubt they will be OK ... fingers crossed) It's very easy to get it wrong unless you use git and know how to cherry-pick this change is dependant on other changes (for instance it use changes in question/format.php) that I commited in other issues when I fixed other import formats.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          I really think this is ready for a 2nd peer-review now, but it will certainly not be the end of it !
          I tried to do my best for phpdoc blocks, explaining the qti import process as this is the most complex one. But far from perfect.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I really think this is ready for a 2nd peer-review now, but it will certainly not be the end of it ! I tried to do my best for phpdoc blocks, explaining the qti import process as this is the most complex one. But far from perfect.
          Hide
          Jean-Michel Vedrine added a comment -

          There are so many $this->text_field($this->cleaninput($var)) in the code that it seems a lot better to add a 2nd boolean parameter to text_field telling if the text should be cleaned or not. In fact there are very few calls where it should NOT be cleaned.
          Of course that means I will have one more time to modify question/format.php and the formats already done (examview and blackboard) !

          Show
          Jean-Michel Vedrine added a comment - There are so many $this->text_field($this->cleaninput($var)) in the code that it seems a lot better to add a 2nd boolean parameter to text_field telling if the text should be cleaned or not. In fact there are very few calls where it should NOT be cleaned. Of course that means I will have one more time to modify question/format.php and the formats already done (examview and blackboard) !
          Hide
          Tim Hunt added a comment -

          Rather than add an argument to the existing method, it is probably better to make a new method

          $this->cleaned_text_field

          or something.

          Show
          Tim Hunt added a comment - Rather than add an argument to the existing method, it is probably better to make a new method $this->cleaned_text_field or something.
          Hide
          Jean-Michel Vedrine added a comment -

          Yes this is a lot better. Done.
          I rebased after this weekly release.

          Show
          Jean-Michel Vedrine added a comment - Yes this is a lot better. Done. I rebased after this weekly release.
          Hide
          Tim Hunt added a comment -

          Brillant work again. Almost everything I am spotting here is code-checker type issues, rather than real problems. So, I think it is time to squash it down to one commit; run codechecker and the unit tests one last time, then we can submit this for integration next week.

          (Thank you for not squashing the commits before.)

          Do we backport to 2.3 or 2.2. Again, I don't see why not if this will cherry-pick, although the final call will be with the integrators.

          Specific comments follow:

          Typo blanck at https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L0R664

          The comment here: https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L0R947 could be written in better English.

          Double space https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L2R18

          https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L2R43 Personally, I would to if (is_file($file) && is_readable($file)) but that is probably just personal preference.

          I know this is existing code, not yours, but $ext = substr($this->realfilename, strpos($this->realfilename, '.'), strlen($this->realfilename)-1); is not very good. Think about a filename like file.oops.dat.

          Rather than using hard-coded 1 and 2 for the file type, please can we have class constants const FILETYPE_POOL = 2; and const FILETYPE_QTI = 1;

          The fulldelete lines here are badly indented: https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L2R149. Also https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L2R185. (Why isn't codechecker picking this up?)

          No newline at end of file: https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L3R157. Also a few of the other files. Please can you check them all.

          Extra white-space inside the brackets: https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L5R533 (also in the other similar methods).

          blanck type again: https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L5R568

          Show
          Tim Hunt added a comment - Brillant work again. Almost everything I am spotting here is code-checker type issues, rather than real problems. So, I think it is time to squash it down to one commit; run codechecker and the unit tests one last time, then we can submit this for integration next week. (Thank you for not squashing the commits before.) Do we backport to 2.3 or 2.2. Again, I don't see why not if this will cherry-pick, although the final call will be with the integrators. Specific comments follow: Typo blanck at https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L0R664 The comment here: https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L0R947 could be written in better English. Double space https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L2R18 https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L2R43 Personally, I would to if (is_file($file) && is_readable($file)) but that is probably just personal preference. I know this is existing code, not yours, but $ext = substr($this->realfilename, strpos($this->realfilename, '.'), strlen($this->realfilename)-1); is not very good. Think about a filename like file.oops.dat. Rather than using hard-coded 1 and 2 for the file type, please can we have class constants const FILETYPE_POOL = 2; and const FILETYPE_QTI = 1; The fulldelete lines here are badly indented: https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L2R149 . Also https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L2R185 . (Why isn't codechecker picking this up?) No newline at end of file: https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L3R157 . Also a few of the other files. Please can you check them all. Extra white-space inside the brackets: https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L5R533 (also in the other similar methods). blanck type again: https://github.com/jmvedrine/moodle/compare/master...MDL-25492#L5R568
          Hide
          Tim Hunt added a comment -

          To celebrate this being nearly done, I decided to review all the import/export bugs: http://tracker.moodle.org/secure/IssueNavigator.jspa?reset=true&jqlQuery=project+%3D+MDL+AND+%28summary+%7E+%22import+export%22+OR+description+%7E+%22import+export%22%29+AND+resolution+%3D+Unresolved+AND+component+%3D+Questions+ORDER+BY+key+ASC
          There were 54, but now there are only 42 left, once duplicates and things that got fixed ages ago have been weeded out.

          Show
          Tim Hunt added a comment - To celebrate this being nearly done, I decided to review all the import/export bugs: http://tracker.moodle.org/secure/IssueNavigator.jspa?reset=true&jqlQuery=project+%3D+MDL+AND+%28summary+%7E+%22import+export%22+OR+description+%7E+%22import+export%22%29+AND+resolution+%3D+Unresolved+AND+component+%3D+Questions+ORDER+BY+key+ASC There were 54, but now there are only 42 left, once duplicates and things that got fixed ages ago have been weeded out.
          Hide
          Jean-Michel Vedrine added a comment -

          Wonderfull ! Additionnaly looking at your search is very interesting for me to understand how to do advanced searches in the tracker.
          Some notes :

          • I think I already fixed MDL-26255 in this week release.
          • I have code somewhere for MDL-25465 (admin page to manage import/export formats) but I didn't wanted to show it before fixing the Blackboard plugins.
          • MDL-32464 would be easy to fix as Pierre and I have already suggested fix.
          Show
          Jean-Michel Vedrine added a comment - Wonderfull ! Additionnaly looking at your search is very interesting for me to understand how to do advanced searches in the tracker. Some notes : I think I already fixed MDL-26255 in this week release. I have code somewhere for MDL-25465 (admin page to manage import/export formats) but I didn't wanted to show it before fixing the Blackboard plugins. MDL-32464 would be easy to fix as Pierre and I have already suggested fix.
          Hide
          Jean-Michel Vedrine added a comment -

          2 small problems : I wrote

           
              /**#@+ @var integer named constants for the filetype var. */
              // Blackboard assessment qti files were always imported by the blackboard_six plugin.
              const FILETYPE_QTI = 1;
              // Blackboard question pool files were previously handled by the blackboard plugin.
              const FILETYPE_POOL = 2;
               /** @var int type of file being imported, one of the constants FILETYPE_QTI or FILETYPE_POOL. */
              public $filetype;
          

          But the codechecker don(t like the first line
          Other thing, I am unsure that the following bloc is really clear and good English

              /**
               * A lot of imported files contains unwanted entities.
               * This method try to clean up all known problems.
               * @param string str string to correct
               * @return string the corrected string
               */
          
          Show
          Jean-Michel Vedrine added a comment - 2 small problems : I wrote /**#@+ @ var integer named constants for the filetype var . */ // Blackboard assessment qti files were always imported by the blackboard_six plugin. const FILETYPE_QTI = 1; // Blackboard question pool files were previously handled by the blackboard plugin. const FILETYPE_POOL = 2; /** @ var int type of file being imported, one of the constants FILETYPE_QTI or FILETYPE_POOL. */ public $filetype; But the codechecker don(t like the first line Other thing, I am unsure that the following bloc is really clear and good English /** * A lot of imported files contains unwanted entities. * This method try to clean up all known problems. * @param string str string to correct * @ return string the corrected string */
          Hide
          Tim Hunt added a comment -

          1. Codechecker does not like that style of comment. Just label each constant separately:

              /** @var int Blackboard assessment qti files were always imported by the blackboard_six plugin. */
              const FILETYPE_QTI = 1;
              /** @var int Blackboard question pool files were previously handled by the blackboard plugin. */
              const FILETYPE_POOL = 2;
          

          2. That is pretty good. Just contains -> contain, and try -> tries.

          Show
          Tim Hunt added a comment - 1. Codechecker does not like that style of comment. Just label each constant separately: /** @ var int Blackboard assessment qti files were always imported by the blackboard_six plugin. */ const FILETYPE_QTI = 1; /** @ var int Blackboard question pool files were previously handled by the blackboard plugin. */ const FILETYPE_POOL = 2; 2. That is pretty good. Just contains -> contain, and try -> tries.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          I think I have integrated all the results of the second peer review. I hope I didn't forget something !
          phpunit says "OK" and codechecker says "Well done".
          Not yet squashed because I think it will be easier for you to review the latest changes.
          Once squashed i also need to do the other branchs.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I think I have integrated all the results of the second peer review. I hope I didn't forget something ! phpunit says "OK" and codechecker says "Well done". Not yet squashed because I think it will be easier for you to review the latest changes. Once squashed i also need to do the other branchs.
          Hide
          Tim Hunt added a comment -
          Show
          Tim Hunt added a comment - How picky can I get? https://github.com/jmvedrine/moodle/commit/4a859effb2f9197473b414c2897c0e7195d00e69#L1R37 has wrong indent. Also https://github.com/jmvedrine/moodle/commit/4a859effb2f9197473b414c2897c0e7195d00e69#L2R33 And thanks for not squashing yet. Apart from that, great!
          Hide
          Jean-Michel Vedrine added a comment -

          I am interested too in that being as good as possible !
          Why is the codechecker not reporting this ?

          Show
          Jean-Michel Vedrine added a comment - I am interested too in that being as good as possible ! Why is the codechecker not reporting this ?
          Hide
          Jean-Michel Vedrine added a comment -

          Fixed. After lunch if we don't find anything else I will squash all commits.

          Show
          Jean-Michel Vedrine added a comment - Fixed. After lunch if we don't find anything else I will squash all commits.
          Hide
          Jean-Michel Vedrine added a comment -

          Squashed !
          Tim, what is the best strategy to backport the changes to MOODLE_22_STABLE, except all the content of the "tests" subfolder ?

          Show
          Jean-Michel Vedrine added a comment - Squashed ! Tim, what is the best strategy to backport the changes to MOODLE_22_STABLE, except all the content of the "tests" subfolder ?
          Hide
          Tim Hunt added a comment -

          Yay! submitting for integration review.

          The way I normally do that sort of back-port is to do the cherry-pick, then you get a merge conflict because of the test files, which you can then remove.

          Show
          Tim Hunt added a comment - Yay! submitting for integration review. The way I normally do that sort of back-port is to do the cherry-pick, then you get a merge conflict because of the test files, which you can then remove.
          Hide
          Jean-Michel Vedrine added a comment -

          Branchs for Moodle 2.2 and 2.3 done.
          Looking at the 2.2 branch is interesting because it says 1,735 additions and 956 deletions (the number of lines without the tests is about the half of the 3,593 lines in master and 2.3).
          Some maths :
          1,735 - 956 = 779
          But we must deduce the approx 500 lines that were previously in question/format/blackboard/format.php and are now part of question/format/blackboard_six/formatpool.php so this is a much smaller change that it seems at first .

          Show
          Jean-Michel Vedrine added a comment - Branchs for Moodle 2.2 and 2.3 done. Looking at the 2.2 branch is interesting because it says 1,735 additions and 956 deletions (the number of lines without the tests is about the half of the 3,593 lines in master and 2.3). Some maths : 1,735 - 956 = 779 But we must deduce the approx 500 lines that were previously in question/format/blackboard/format.php and are now part of question/format/blackboard_six/formatpool.php so this is a much smaller change that it seems at first .
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Wow, awesome development and review process.. but does it work? (joking!)

          Unit tests also say that everything continues working, perfect.

          Just one small note, I've seen a bunch of incorrect phpdocs. It seems that you are missing the "dollar" in a lot of methods here and there. And also, there are some functions (tests mainly) missing basic phpdocs. I'm attaching one xml file showing all those minor problems.

          Said that, I think it's ok to delegate the task of fixing those phpdocs to another issue (surely 23 and master is fair enough). Or if you prefer, I can hold this to get everything correct together here.

          Finally, I feel myself a bit more concerned about backporting this to 22_STABLE, mainly the lack of tests make me struggle a bit. But Tim, it seems it's good for you, so, I'm going to add one warning to testing instructions about to test that specifically under all branches and backport to 22_STABLE. Is that ok for you?

          I'll wait some hours for your feedback, many thanks for the big effort!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Wow, awesome development and review process.. but does it work? (joking!) Unit tests also say that everything continues working, perfect. Just one small note, I've seen a bunch of incorrect phpdocs. It seems that you are missing the "dollar" in a lot of methods here and there. And also, there are some functions (tests mainly) missing basic phpdocs. I'm attaching one xml file showing all those minor problems. Said that, I think it's ok to delegate the task of fixing those phpdocs to another issue (surely 23 and master is fair enough). Or if you prefer, I can hold this to get everything correct together here. Finally, I feel myself a bit more concerned about backporting this to 22_STABLE, mainly the lack of tests make me struggle a bit. But Tim, it seems it's good for you, so, I'm going to add one warning to testing instructions about to test that specifically under all branches and backport to 22_STABLE. Is that ok for you? I'll wait some hours for your feedback, many thanks for the big effort! Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Attaching smurf.xml file containing details of some phpdoc problems.

          Show
          Eloy Lafuente (stronk7) added a comment - Attaching smurf.xml file containing details of some phpdoc problems.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Hello Eloy,
          I awake and find your comments (it's 7am here). Thanks a lot for your comments.
          Unfortunately, I am still learning to write phpdocs blocks. But I spend so many hours making the code as good as I can that I am afraid I neglected this learning. I promise I will do it !
          I will open another issue to correct phpdocs on all question formats that I have fixed in the latest weeks (this is the last one). Maybe I should also write some notes on the structure of blackboard formats somewhere so that it helps for the future ?
          If possible I would prefer this to be integrated this week because it's my last summer holidays week ! After next Monday I will resume teaching math and will have to struggle a lot more to find time for Moodle coding !
          For backporting this to 22_STABLE, I think it's very good and very needed to add to testing instructions to specifically test this on the 22_STABLE branch even if I am quite confident because those files have not seems a lot of changes for ages. I forgot to include this in instructions.
          Apart the small bloc of changes in question/format.php lines 411-448 the change is in fact very limited to the question/format/blackboard_six subfolder so the chance to break something are small.
          I think it's important to backport this to 22_STABLE : the blackboard import functionality is needed by a lot of people (see number of votes), this is broken since Moodle 2.0 release, and it causes quite a lot of messages in the quiz forum, so it would be a lot easier for me to answer those running Moodle 2.2.x to upgrade to latest 22_STABLE even if they can't upgrade to 23_STABLE now.
          But I will agree your decision.

          Show
          Jean-Michel Vedrine added a comment - - edited Hello Eloy, I awake and find your comments (it's 7am here). Thanks a lot for your comments. Unfortunately, I am still learning to write phpdocs blocks. But I spend so many hours making the code as good as I can that I am afraid I neglected this learning. I promise I will do it ! I will open another issue to correct phpdocs on all question formats that I have fixed in the latest weeks (this is the last one). Maybe I should also write some notes on the structure of blackboard formats somewhere so that it helps for the future ? If possible I would prefer this to be integrated this week because it's my last summer holidays week ! After next Monday I will resume teaching math and will have to struggle a lot more to find time for Moodle coding ! For backporting this to 22_STABLE, I think it's very good and very needed to add to testing instructions to specifically test this on the 22_STABLE branch even if I am quite confident because those files have not seems a lot of changes for ages. I forgot to include this in instructions. Apart the small bloc of changes in question/format.php lines 411-448 the change is in fact very limited to the question/format/blackboard_six subfolder so the chance to break something are small. I think it's important to backport this to 22_STABLE : the blackboard import functionality is needed by a lot of people (see number of votes), this is broken since Moodle 2.0 release, and it causes quite a lot of messages in the quiz forum, so it would be a lot easier for me to answer those running Moodle 2.2.x to upgrade to latest 22_STABLE even if they can't upgrade to 23_STABLE now. But I will agree your decision.
          Hide
          Jean-Michel Vedrine added a comment -

          I already found a (silly) mistake : while backporting to 22_STABLE I upgraded the version.php "too much" : wanting to correct a typo I also changed the version and requires ! Sorry. I will look the 23_STABLE to see if everything is OK.
          Do I push a commit to my github repo ?

          Show
          Jean-Michel Vedrine added a comment - I already found a (silly) mistake : while backporting to 22_STABLE I upgraded the version.php "too much" : wanting to correct a typo I also changed the version and requires ! Sorry. I will look the 23_STABLE to see if everything is OK. Do I push a commit to my github repo ?
          Hide
          Jean-Michel Vedrine added a comment -

          I pushed a fix on my github repo. Everything is OK on the 23_STABLE branch.

          Show
          Jean-Michel Vedrine added a comment - I pushed a fix on my github repo. Everything is OK on the 23_STABLE branch.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi Jean-Michel,

          I've amended the testing instructions to enforce them to be run against all branches and also I've created MDL-35103 as a followup about the phpdocs.

          I'm taking a final, overall, review of this and will be integrating it soon.

          Many thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi Jean-Michel, I've amended the testing instructions to enforce them to be run against all branches and also I've created MDL-35103 as a followup about the phpdocs. I'm taking a final, overall, review of this and will be integrating it soon. Many thanks and ciao
          Hide
          Jean-Michel Vedrine added a comment -

          Many, many thanks Eloy

          Show
          Jean-Michel Vedrine added a comment - Many, many thanks Eloy
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has been integrated (22, 23 & master), thanks!

          Note: I've added one extra commit both in 23 and master, bumping bb6 qformat version to 2012061701, just in case it helps in the future as reference for the before and after this patch.

          Show
          Eloy Lafuente (stronk7) added a comment - And this has been integrated (22, 23 & master), thanks! Note: I've added one extra commit both in 23 and master, bumping bb6 qformat version to 2012061701, just in case it helps in the future as reference for the before and after this patch.
          Hide
          Rajesh Taneja added a comment -

          Thanks for all the Great work, Jean.

          Everything works as described in test instructions. (Thanks for clear and elaborative test instructions)

          Show
          Rajesh Taneja added a comment - Thanks for all the Great work, Jean. Everything works as described in test instructions. (Thanks for clear and elaborative test instructions)
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks Rajesh. I really appreciate your kind words.

          Show
          Jean-Michel Vedrine added a comment - Thanks Rajesh. I really appreciate your kind words.
          Hide
          Tim Hunt added a comment -

          Oops! we screwed up: MDL-35147. Or rather the crazy way that Lesson works caught us out.

          Show
          Tim Hunt added a comment - Oops! we screwed up: MDL-35147 . Or rather the crazy way that Lesson works caught us out.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm so proud...of you, many thanks!

          http://youtu.be/n64CdfDRnZY

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao
          Hide
          Lawrence N added a comment -

          Jean-Michel,

          You are correct. One the files were not properly formatted (missed the first few lines of the page!)

          Thanks again

          Show
          Lawrence N added a comment - Jean-Michel, You are correct. One the files were not properly formatted (missed the first few lines of the page!) Thanks again
          Hide
          Justin Litalien added a comment -

          Hi folks, was able to test this out on my version of 2.2.5 and things worked perfectly. A much deserved THANK YOU goes out to Jean-Michel, Tim, and all those who helped get this squared away. My university greatly appreciates your efforts!

          Show
          Justin Litalien added a comment - Hi folks, was able to test this out on my version of 2.2.5 and things worked perfectly. A much deserved THANK YOU goes out to Jean-Michel, Tim, and all those who helped get this squared away. My university greatly appreciates your efforts!

            People

            • Votes:
              47 Vote for this issue
              Watchers:
              27 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: