Moodle
  1. Moodle
  2. MDL-34738

Blackboard question import is broken

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.10, 2.1.7, 2.2.4, 2.3.1
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      IMPORTANT NOTICE

      All tests should be conducted with developper 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.

      WHAT ARE WE TESTING ?

      Go to Question Bank -> Import.
      This issue is about fixing the "Blackboard" question import file format.
      Don't be confused, this issue is just about the "Blackboard" file format.
      Clicking on the help icon for this format format should dispay : Blackboard format enables questions saved in the Blackboard version 5 "POOL" type export format to be imported.).
      Don't use the "Blackboard V6+" it's another "Blackboard" file format wich is broken and will be fixed in another traker issue.
      If you are somewhat lost with all these "Blackboard" formats, so are all 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 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. I suggest not to try the other formats as some are still broken at this point.

      PART TWO FIRST TEST FOR EACH QUESTION TYPE

      Try to import the sample_blackboard.dat file attached to the present tracker issue (MDL-34738) using the "Blackboard" 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).
      These are the same questions that are 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.
      The matching question is interesting because this type of question was never imported correctly, even in Moodle 1.9. You should get 3 subquestions (cat, frog, newt) and 3 choices (mammal, insect, amphibian) of course insect is a distractor and frog and newt are both amphibians. These are the parts that were not working previously.
      This part of the test will test that MDL-16479 (marked as duplicate) is fixed.

      PART THREE TEST WITH A REAL FILE SUBMITTED BY USER

      Try to import the res00000.dat file attached by Matt Cambell to tracker issue MDL-16479 (it contains 40 questions and should import cleanly).
      Specially look at the question "Match the following descriptions with the appropriate bone structure." and verify it include 6 subquestions and 6 choices. Periosteum and Medullary cavity are distractors, Diaphysis and Epiphyses are used 2 times each (but I really don't know what all this means )

      PART FOUR OTHER FILE SUBMITTED BY USER

      Go to tracker issue MDL-25492 and download the fmm4k0000.dat file 81kb uploaded January 27th 2011
      Try to import this file file using the "Blackboard" file format. Verify no warnings are displayed during import.
      This file should import 80 questions of various types on something like "marketing?" You are not required to test that each question can be edited and previewed correctly but a few one would be appreciated.

      PART FIVE : NO IMAGE !!

      Go to same tracker issue MDL-25492 and download the res00000.dat file 21kb uploaded December 02nd 2010.
      This file should import 15 questions about "numbers".
      These questions include ref to images.
      Open some of these questions for editing. You should see broken images. This is the expected result.
      We can't do anything to correct this problem because the poster of the file has not included these images. So you should consider the test as passed even with these broken images.
      But you should no see any oter problem and except the broken images these questions should work without any problem.

      PART SIX : TEST IMPORT OF A FILE NOT SUITABLE FOR THIS PLUGIN

      As I said most Moodle users are completely lost between all "Blackboard" formats. So we will now try to import a file of another format.
      Try importing the res00001.dat attached to the present issue (MDL-34738) using the "Blackboard" file format.
      You should get an error message "There are no questions in the import file" wich is quite normal as this file is of another Blackboard format (it's not a "POOL" file !) so if you get this error message, please consider this test as passed.

      At this point you should be as convinced as I am that the Blackboard format is no more broken (wich of course don't means it is bug free !).
      If not, ask for more sample files

      Show
      IMPORTANT NOTICE All tests should be conducted with developper 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. WHAT ARE WE TESTING ? Go to Question Bank -> Import. This issue is about fixing the "Blackboard" question import file format. Don't be confused, this issue is just about the "Blackboard" file format. Clicking on the help icon for this format format should dispay : Blackboard format enables questions saved in the Blackboard version 5 "POOL" type export format to be imported.). Don't use the "Blackboard V6+" it's another "Blackboard" file format wich is broken and will be fixed in another traker issue. If you are somewhat lost with all these "Blackboard" formats, so are all 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 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. I suggest not to try the other formats as some are still broken at this point. PART TWO FIRST TEST FOR EACH QUESTION TYPE Try to import the sample_blackboard.dat file attached to the present tracker issue ( MDL-34738 ) using the "Blackboard" 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). These are the same questions that are 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. The matching question is interesting because this type of question was never imported correctly, even in Moodle 1.9. You should get 3 subquestions (cat, frog, newt) and 3 choices (mammal, insect, amphibian) of course insect is a distractor and frog and newt are both amphibians. These are the parts that were not working previously. This part of the test will test that MDL-16479 (marked as duplicate) is fixed. PART THREE TEST WITH A REAL FILE SUBMITTED BY USER Try to import the res00000.dat file attached by Matt Cambell to tracker issue MDL-16479 (it contains 40 questions and should import cleanly). Specially look at the question "Match the following descriptions with the appropriate bone structure." and verify it include 6 subquestions and 6 choices. Periosteum and Medullary cavity are distractors, Diaphysis and Epiphyses are used 2 times each (but I really don't know what all this means ) PART FOUR OTHER FILE SUBMITTED BY USER Go to tracker issue MDL-25492 and download the fmm4k0000.dat file 81kb uploaded January 27th 2011 Try to import this file file using the "Blackboard" file format. Verify no warnings are displayed during import. This file should import 80 questions of various types on something like "marketing?" You are not required to test that each question can be edited and previewed correctly but a few one would be appreciated. PART FIVE : NO IMAGE !! Go to same tracker issue MDL-25492 and download the res00000.dat file 21kb uploaded December 02nd 2010. This file should import 15 questions about "numbers". These questions include ref to images. Open some of these questions for editing. You should see broken images. This is the expected result. We can't do anything to correct this problem because the poster of the file has not included these images. So you should consider the test as passed even with these broken images. But you should no see any oter problem and except the broken images these questions should work without any problem. PART SIX : TEST IMPORT OF A FILE NOT SUITABLE FOR THIS PLUGIN As I said most Moodle users are completely lost between all "Blackboard" formats. So we will now try to import a file of another format. Try importing the res00001.dat attached to the present issue ( MDL-34738 ) using the "Blackboard" file format. You should get an error message "There are no questions in the import file" wich is quite normal as this file is of another Blackboard format (it's not a "POOL" file !) so if you get this error message, please consider this test as passed. At this point you should be as convinced as I am that the Blackboard format is no more broken (wich of course don't means it is bug free !). If not, ask for more sample files
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
    • Rank:
      43207

      Description

      The blackboard format in question/format/blackboard is broken since Moodle 2.0 because of the changes made in the files API.
      This issue is different from MDL-25492 wich deals with the "Blackboard V6+" format in question/format/blackboard_six, but I understand that with so many "Blackboard" formats people are completely lost .
      As the question/format/blackboard format is a lot simpler it seems logical to first tackle this one before doing the hardest one.
      While I was working on it I discovered that import of matching questions was not working in all cases, so I reworked it.
      So the proposed file includes :

      • fixes to repair the format for all question types
      • reworked import of matching questions that I think works in all cases
      • coding style fixes, the codechecker still reports 15 warnings about too long lines that I didn't bother to fix (in fact I don't really know the rules to break lines !)
        I have included phpunit tests of truefalse, multichoice single and multi, and matching imports. The test of matching import was really needed to see if my new code was working and if I didn't broke something, but while I was writing tests I also included questions types for wich I had sample files (truefalse and multichoice single and multi). I started from real examples files and changed the questions to standard ones.
        Unfortunately I don't have sample of essay and fill in the blank questions in blackboard form, so I was not able to include them in tests. Of course I can do some "reverseengineering" of the format's code and create fake examples but that don't seems to me a very good proof that the code is working .
        I also included the xml file used for the tests in question/format/blackboard/test/fixtures/ so that for instance it is possible to use this file to test import in Moodle 1.9 and see that the matching question "Classify the animals." is not imported correctly in Moodle 1.9, contrary to my reworked version.

        Issue Links

          Activity

          Hide
          Jean-Michel Vedrine added a comment -

          sorry to not provide the fix as a gitub link but I am still reading chapter one of Pro Git !!

          Show
          Jean-Michel Vedrine added a comment - sorry to not provide the fix as a gitub link but I am still reading chapter one of Pro Git !!
          Hide
          Jean-Michel Vedrine added a comment -

          hello Tim,
          If you can have a look at my phpunit tests and tell me if I am doing it right (or what I am doing wrong )it would helps me as I am quite a beginer at unit testing.

          Show
          Jean-Michel Vedrine added a comment - hello Tim, If you can have a look at my phpunit tests and tell me if I am doing it right (or what I am doing wrong )it would helps me as I am quite a beginer at unit testing.
          Hide
          Jean-Michel Vedrine added a comment -

          Further remarks :
          1) I think it would be really better to use the same getpath function that the xml format already use for all imports formats dealing with an xmldata array. But I am a bit worried to duplicate the code of getpath in each format. Could this function be moved from question/format/xml/format.php to qformat_default class ?
          2) I don't think
          $question->fraction[$j] = floor(100000/$correctanswercount)/100000; // Strange behavior if we have more than 5 decimal places.
          is still needed. I don't see why a simple $question->fraction[$j] = 1/$correctanswercount; would not work ? What is your thinking ?
          3) I am really worried to give you some work that could be avoided if I was able to submit this (and more important subsequents versions when you will request some changes or I will discover some bugs) as a github patch. So I will read one more time http://docs.moodle.org/dev/Git_for_developers and I will try again to prepare a github patch, so don't do it yourself now, wait to see if I manage to do it myself.

          Show
          Jean-Michel Vedrine added a comment - Further remarks : 1) I think it would be really better to use the same getpath function that the xml format already use for all imports formats dealing with an xmldata array. But I am a bit worried to duplicate the code of getpath in each format. Could this function be moved from question/format/xml/format.php to qformat_default class ? 2) I don't think $question->fraction [$j] = floor(100000/$correctanswercount)/100000; // Strange behavior if we have more than 5 decimal places. is still needed. I don't see why a simple $question->fraction [$j] = 1/$correctanswercount; would not work ? What is your thinking ? 3) I am really worried to give you some work that could be avoided if I was able to submit this (and more important subsequents versions when you will request some changes or I will discover some bugs) as a github patch. So I will read one more time http://docs.moodle.org/dev/Git_for_developers and I will try again to prepare a github patch, so don't do it yourself now, wait to see if I manage to do it myself.
          Hide
          Jean-Michel Vedrine added a comment -

          Well even with my good will, I have some problems to do a github branch.
          I "think" I did it until the "Preparing a patch" paragraph of the Git for developpers doc from MoodleDocs
          My first problem is that when I add a new file (question/format/blackboard/tests/blackboardformat_test.php for instance) I get a warning :
          warning: LF will be replaced by CRLF in question/format/blackboard/tests/blackboardformat_test.php.
          The file will have its original line endings in your working directory.
          Is this the signal of a future problem with lines ending ?
          Also it seems that I managed to commit my changes to my local repo because if I do git status, I get

          1. On branch MDL-34738
          2. Your branch is ahead of 'origin/master' by 2 commits.
            #
            nothing to commit (working directory clean)

          But alas, doing git push do nothing, I get Everything up-to-date
          Sorry to seem so dumb !

          Show
          Jean-Michel Vedrine added a comment - Well even with my good will, I have some problems to do a github branch. I "think" I did it until the "Preparing a patch" paragraph of the Git for developpers doc from MoodleDocs My first problem is that when I add a new file (question/format/blackboard/tests/blackboardformat_test.php for instance) I get a warning : warning: LF will be replaced by CRLF in question/format/blackboard/tests/blackboardformat_test.php. The file will have its original line endings in your working directory. Is this the signal of a future problem with lines ending ? Also it seems that I managed to commit my changes to my local repo because if I do git status, I get On branch MDL-34738 Your branch is ahead of 'origin/master' by 2 commits. # nothing to commit (working directory clean) But alas, doing git push do nothing, I get Everything up-to-date Sorry to seem so dumb !
          Hide
          Jean-Michel Vedrine added a comment -

          I know the blackboard format is rather old code but what lines like :
          line 192: $question->answer[$j] = $question->answer[$j];
          line 202: $question->feedback[$j] = $question->feedback[$j];
          and other similar lines supposed to do ? Unless there is some very black magic involved they do nothing IMHO ! Maybe they are leftover of previous changes in the code ?
          In a next commit I will remove a lot of code duplication because the code do the exact same things at the begining of each question type, so rather create a common method.

          Show
          Jean-Michel Vedrine added a comment - I know the blackboard format is rather old code but what lines like : line 192: $question->answer [$j] = $question->answer [$j] ; line 202: $question->feedback [$j] = $question->feedback [$j] ; and other similar lines supposed to do ? Unless there is some very black magic involved they do nothing IMHO ! Maybe they are leftover of previous changes in the code ? In a next commit I will remove a lot of code duplication because the code do the exact same things at the begining of each question type, so rather create a common method.
          Hide
          Tim Hunt added a comment -

          Thank you very much for working on this. Sorry not to reply sooner, I was away for the weekend.

          You probably need to do "git push MDL-34738".

          About the line-ending thing. Are you are using msysgit? If so, then one page of the installer has a setting about line-endings. You must choose 'Checkout as-is, commit as-is'. Strangely, the installer says that option is not recommended, but actually it is the only sane option.

          Did I give you this link before? http://tjhunt.blogspot.co.uk/2012/03/fixing-bug-in-moodle-core-mechanics.html It may help.

          1) It is not really good design to move getpath to qformat_default. That method is only relevant for XML-based question formats. The good design would be to make a new class

          class question_format_based_on_xml extends qformat_default {
              function getpath() {
                  // ...
              }
          }
          

          and then do

          class qformat_xml extends question_format_based_on_xml {
              // ...
          }
          
          class qformat_blackboard extends question_format_based_on_xml {
              // ...
          }
          

          2) you are right that line of code is crazy. 1/$correctanswercount is all that is required.

          3) You have just saved me many hours of work that I really did not want to do by sorting out this code. Don't worry if you need my help with git. Of course, it is better for me to spend the time helping you learn git, that to just do it for you.

          I will look at the unit tests now.

          Show
          Tim Hunt added a comment - Thank you very much for working on this. Sorry not to reply sooner, I was away for the weekend. You probably need to do "git push MDL-34738 ". About the line-ending thing. Are you are using msysgit? If so, then one page of the installer has a setting about line-endings. You must choose 'Checkout as-is, commit as-is'. Strangely, the installer says that option is not recommended, but actually it is the only sane option. Did I give you this link before? http://tjhunt.blogspot.co.uk/2012/03/fixing-bug-in-moodle-core-mechanics.html It may help. 1) It is not really good design to move getpath to qformat_default. That method is only relevant for XML-based question formats. The good design would be to make a new class class question_format_based_on_xml extends qformat_default { function getpath() { // ... } } and then do class qformat_xml extends question_format_based_on_xml { // ... } class qformat_blackboard extends question_format_based_on_xml { // ... } 2) you are right that line of code is crazy. 1/$correctanswercount is all that is required. 3) You have just saved me many hours of work that I really did not want to do by sorting out this code. Don't worry if you need my help with git. Of course, it is better for me to spend the time helping you learn git, that to just do it for you. I will look at the unit tests now.
          Hide
          Tim Hunt added a comment -

          One other suggestion, in whichever editor you use, can you get it to make whitespace characters visible?

          In Eclipse, do Preferences -> General -> Editors -> Text editor -> Show whitespace characters.

          In Notepad++, it is View -> Show Symbol -> Show White Space and Tab.

          In the sample XML, you have a mix of spaces and tabs, and I think it should all be spaces. (So, search and replace \t -> four spaces.)

          The unit tests are good, in that they work the same way as the Moodle XML tests. I have never been very happy with the qformat_xml tests. They are ugly, verbose code, building up the $expectedq objects, but sadly I have never been able to think of a better way to do it.

          Oh, and the question text for the true-false question is wrong. 42 is the ultimate answer to life, the universe and everything

          Show
          Tim Hunt added a comment - One other suggestion, in whichever editor you use, can you get it to make whitespace characters visible? In Eclipse, do Preferences -> General -> Editors -> Text editor -> Show whitespace characters. In Notepad++, it is View -> Show Symbol -> Show White Space and Tab. In the sample XML, you have a mix of spaces and tabs, and I think it should all be spaces. (So, search and replace \t -> four spaces.) The unit tests are good, in that they work the same way as the Moodle XML tests. I have never been very happy with the qformat_xml tests. They are ugly, verbose code, building up the $expectedq objects, but sadly I have never been able to think of a better way to do it. Oh, and the question text for the true-false question is wrong. 42 is the ultimate answer to life, the universe and everything
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          Thanks a lot for all these infos.
          For the true-false question, isn't this the way it is supposed to be : the text of the question is "42 is the Absolute Answer to everything." but if you answer True you won't earn any point and you will get the feedback "42 is the Ultimate Answer." but if you answer false you will get "You gave the right answer.". Maybe I have mixed it up the feedbacks, I will check. But I think correctanswer is correctly set to 0.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, Thanks a lot for all these infos. For the true-false question, isn't this the way it is supposed to be : the text of the question is "42 is the Absolute Answer to everything." but if you answer True you won't earn any point and you will get the feedback "42 is the Ultimate Answer." but if you answer false you will get "You gave the right answer.". Maybe I have mixed it up the feedbacks, I will check. But I think correctanswer is correctly set to 0.
          Hide
          Jean-Michel Vedrine added a comment -

          Sorry for the mix of spaces and tabs in the sample XML file. This is because I used a real blackboard file as a starting point (I only "adapted" the questions ). I was thinking I had converted all tabs to spaces and suppressed all spaces at end of lines (I use notepad++ and there are commands for these 2 operations). But you are right better to make whitespaces characters visibles.

          Show
          Jean-Michel Vedrine added a comment - Sorry for the mix of spaces and tabs in the sample XML file. This is because I used a real blackboard file as a starting point (I only "adapted" the questions ). I was thinking I had converted all tabs to spaces and suppressed all spaces at end of lines (I use notepad++ and there are commands for these 2 operations). But you are right better to make whitespaces characters visibles.
          Hide
          Tim Hunt added a comment -

          Sorry, I did not real the t/f question carefully enough. You are right.

          Show
          Tim Hunt added a comment - Sorry, I did not real the t/f question carefully enough. You are right.
          Hide
          Jean-Michel Vedrine added a comment -

          OK, I will create a new class and then extend it.
          What would be the good place (and name) for the file containing the "question_format_based_on_xml" class so that many imports can use it easily ?
          In fact I need 2 base class :
          the first one for formats dealing with xml that can be used by blackboard, blackboard_six and examview and possibly others.
          and the second one extending the first for formats dealing with xml files and images packed in a zip file like blackboard_six and the future version of examview able to import images.

          Show
          Jean-Michel Vedrine added a comment - OK, I will create a new class and then extend it. What would be the good place (and name) for the file containing the "question_format_based_on_xml" class so that many imports can use it easily ? In fact I need 2 base class : the first one for formats dealing with xml that can be used by blackboard, blackboard_six and examview and possibly others. and the second one extending the first for formats dealing with xml files and images packed in a zip file like blackboard_six and the future version of examview able to import images.
          Hide
          Tim Hunt added a comment -

          I think put the class(es) in question/format.php, after qformat_default.

          Show
          Tim Hunt added a comment - I think put the class(es) in question/format.php, after qformat_default.
          Hide
          Jean-Michel Vedrine added a comment -

          I think I managed to do a git branch.
          I added a phpunit test for each of the 6 imported question types.
          Feel free to comment, even if there are a ton of things to improve I am very happy about my git progress !
          Your blog post was very helpfull, and the latest post about standards is very interesting too . I quite agee with you about qti !!

          Show
          Jean-Michel Vedrine added a comment - I think I managed to do a git branch. I added a phpunit test for each of the 6 imported question types. Feel free to comment, even if there are a ton of things to improve I am very happy about my git progress ! Your blog post was very helpfull, and the latest post about standards is very interesting too . I quite agee with you about qti !!
          Hide
          Jean-Michel Vedrine added a comment -

          Using Marina's phpdoc checker, I see that there are many phpdoc blocks to add or improve. I will try to do it, but I never really looked at phpdoc so I need to learn first.

          Show
          Jean-Michel Vedrine added a comment - Using Marina's phpdoc checker, I see that there are many phpdoc blocks to add or improve. I will try to do it, but I never really looked at phpdoc so I need to learn first.
          Hide
          Jean-Michel Vedrine added a comment -

          Sometimes I feel really stupid !
          It takes me several days working on this format (question/format/blackboard) to realize it's in fact the very same syntax for all questions that the newer "Examview (Blackboard 6 +) that I have submitted in MDL-25492 under the name "examview_bbsixplus".
          The difference is that examview_bbsixplus can import a zip file but once the imsmanifest is parsed, the code to parse the question is exactly the same except that examview_bbsixplus can import images included in the zip file.
          So it makes sense to only have a single format able to accept both .dat and .zip files exactly like the blackboard_six format do.
          So I am left with only 2 formats to import absolutely all blackboard files and I need to find a way to conbine them into an unique format.

          Show
          Jean-Michel Vedrine added a comment - Sometimes I feel really stupid ! It takes me several days working on this format (question/format/blackboard) to realize it's in fact the very same syntax for all questions that the newer "Examview (Blackboard 6 +) that I have submitted in MDL-25492 under the name "examview_bbsixplus". The difference is that examview_bbsixplus can import a zip file but once the imsmanifest is parsed, the code to parse the question is exactly the same except that examview_bbsixplus can import images included in the zip file. So it makes sense to only have a single format able to accept both .dat and .zip files exactly like the blackboard_six format do. So I am left with only 2 formats to import absolutely all blackboard files and I need to find a way to conbine them into an unique format.
          Hide
          Tim Hunt added a comment -

          Git branch looks good, although I did not review it in great detail.

          Combining all these various import formats would be good. I will leave you to tell me when you think there is a chunk of work ready to submit for integration.

          Show
          Tim Hunt added a comment - Git branch looks good, although I did not review it in great detail. Combining all these various import formats would be good. I will leave you to tell me when you think there is a chunk of work ready to submit for integration.
          Hide
          Jean-Michel Vedrine added a comment -

          Most questions coming from systems external to Moodle don't provide a question name.
          Currently as you noted in another tracker's issue the situation is a true mess : each format has it's own way to create a name.
          Can we agree on a standard method to get a question name
          I was thinking of something like
          if question->name not empty left as is
          else
          remove tags and shorten question->questiontext (because I think question name must be plain text but maybe I am wrong ?)
          if doing this we end up with an empty string (this can be the case for instance if questiontext is just an img tag), use a default string (possibly including a question id if the format provide any ?)
          such a method could be included in question/format and used elsewhere.
          Or any better method you can think of.

          Show
          Jean-Michel Vedrine added a comment - Most questions coming from systems external to Moodle don't provide a question name. Currently as you noted in another tracker's issue the situation is a true mess : each format has it's own way to create a name. Can we agree on a standard method to get a question name I was thinking of something like if question->name not empty left as is else remove tags and shorten question->questiontext (because I think question name must be plain text but maybe I am wrong ?) if doing this we end up with an empty string (this can be the case for instance if questiontext is just an img tag), use a default string (possibly including a question id if the format provide any ?) such a method could be included in question/format and used elsewhere. Or any better method you can think of.
          Hide
          Tim Hunt added a comment -

          That sounds like a good approach. There should be a method in qformat_default.

          However, it may be better not to try to do too many things at the same time. For this issue, just get qformat_blackboard working, with the current method for creating question name.

          Then, once that is done, create a separate issue for using the same default question name algorithm in all formats.

          Show
          Tim Hunt added a comment - That sounds like a good approach. There should be a method in qformat_default. However, it may be better not to try to do too many things at the same time. For this issue, just get qformat_blackboard working, with the current method for creating question name. Then, once that is done, create a separate issue for using the same default question name algorithm in all formats.
          Hide
          Jean-Michel Vedrine added a comment -

          I quite agree,
          For this isue I will introduce the new class in question/format after qformat default with the getpath method and use it in blackboard qformat.
          I will verify that phpunit tests are still OK (tis is where it's interesting to have done them before, this will permit me to verify that I don't break questions parsing in qformat blackboard when using getpath )
          When this is done I will ask you for peer review, make changes according to your suggestions and correct the problems you will found.
          Once you are satisfied we will submit this for integration.
          Additionnaly to the phpunit tests, I think I should provide some testing instructions ?
          Once this is integrated, tested and closed I will create separates tracker issues for all other improvments I saw while working on this.

          Show
          Jean-Michel Vedrine added a comment - I quite agree, For this isue I will introduce the new class in question/format after qformat default with the getpath method and use it in blackboard qformat. I will verify that phpunit tests are still OK (tis is where it's interesting to have done them before, this will permit me to verify that I don't break questions parsing in qformat blackboard when using getpath ) When this is done I will ask you for peer review, make changes according to your suggestions and correct the problems you will found. Once you are satisfied we will submit this for integration. Additionnaly to the phpunit tests, I think I should provide some testing instructions ? Once this is integrated, tested and closed I will create separates tracker issues for all other improvments I saw while working on this.
          Hide
          Tim Hunt added a comment -

          Yes. That sounds like a good plan.

          Every issue submitted for integration requires testing instructions, and I think you should write them. I will review the testing instructions at the same time as I review the patch.

          Show
          Tim Hunt added a comment - Yes. That sounds like a good plan. Every issue submitted for integration requires testing instructions, and I think you should write them. I will review the testing instructions at the same time as I review the patch.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          I have made a commit to introduce the qformat_based_on_xml class in question/format.php and use it in all the code of the blackboard plugin.
          I double checked all the paths and additionnaly the phpunit tests are still OK.
          I will wait your comments (I think most probably you will object to at least one of my ideas, I let you discover it ...)
          You can look at this commit alone here https://github.com/jmvedrine/moodle/commit/422c193302c723dd1fc24fdee2470c0634914350
          For now I only commited to head, I will cherry-pick to 2.2 and 2.3 once I will have fixed all your review findings and you are satisfied.
          I will work on writting testing instructions.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I have made a commit to introduce the qformat_based_on_xml class in question/format.php and use it in all the code of the blackboard plugin. I double checked all the paths and additionnaly the phpunit tests are still OK. I will wait your comments (I think most probably you will object to at least one of my ideas, I let you discover it ...) You can look at this commit alone here https://github.com/jmvedrine/moodle/commit/422c193302c723dd1fc24fdee2470c0634914350 For now I only commited to head, I will cherry-pick to 2.2 and 2.3 once I will have fixed all your review findings and you are satisfied. I will work on writting testing instructions.
          Hide
          Jean-Michel Vedrine added a comment -

          I don't know if this is what is right for controlling the workflow of this issue and asking for your peer review Tim ?

          Show
          Jean-Michel Vedrine added a comment - I don't know if this is what is right for controlling the workflow of this issue and asking for your peer review Tim ?
          Hide
          Tim Hunt added a comment -

          Requesting peer review is exactly right.

          Show
          Tim Hunt added a comment - Requesting peer review is exactly right.
          Hide
          Tim Hunt added a comment -

          This is looking very good. I just added a few minor code-review comments using the github comment interface. I hope that works OK.

          Show
          Tim Hunt added a comment - This is looking very good. I just added a few minor code-review comments using the github comment interface. I hope that works OK.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim thanks a lot for the comments. I was unsure if I could change qformat_xml without been invited to, but if you are OK, I will surely do it !
          I spotted another mistake : line 469 in question/format/blackboard/format.php :
          $fiber = array_search($choice_id, $mappings);
          should not be here (I wrongly used array_search instead of array_keys with a needle, as I search all mappings with this choice_id)
          but as the right line is just after, there was not adverse effect, and I didn't saw it was still here).
          I was thinking you would not like the use of ddmatch to import matching questions (but the import still work if ddmatch is not installed only all tags are suppressed in subanswers) but I am very pleased if you don't object.
          The problems with the XML are due to the fact I edited it to add tests on the feedbacks where re-reading some forums threads I realized what some users said :
          There is never differents feedbacks for differents answers and there is only a correct and an incorrect feedback at most for each question.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim thanks a lot for the comments. I was unsure if I could change qformat_xml without been invited to, but if you are OK, I will surely do it ! I spotted another mistake : line 469 in question/format/blackboard/format.php : $fiber = array_search($choice_id, $mappings); should not be here (I wrongly used array_search instead of array_keys with a needle, as I search all mappings with this choice_id) but as the right line is just after, there was not adverse effect, and I didn't saw it was still here). I was thinking you would not like the use of ddmatch to import matching questions (but the import still work if ddmatch is not installed only all tags are suppressed in subanswers) but I am very pleased if you don't object. The problems with the XML are due to the fact I edited it to add tests on the feedbacks where re-reading some forums threads I realized what some users said : There is never differents feedbacks for differents answers and there is only a correct and an incorrect feedback at most for each question.
          Hide
          Jean-Michel Vedrine added a comment -

          I suddently realized that making qformat_xml using qformat_based_on_xml will break lesson question import!! Or am I wrong? Do you see a solution ?

          Show
          Jean-Michel Vedrine added a comment - I suddently realized that making qformat_xml using qformat_based_on_xml will break lesson question import!! Or am I wrong? Do you see a solution ?
          Hide
          Tim Hunt added a comment -

          To be honest, I just did not see the ddmatch bit. Oops! I had better look at this more closely. However, the way you have done if (installed) is neat. I am inclined to leave it, even though it is not good practice.

          I do wonder if you need a html_to_text call in the match case?

          You are right that if you change qformat_xml it will break lesson. Stupid lesson module. Someone really needs to convert it to use the qbank / qengine. I guess we have to leave it for now.

          I am afraid that I am away tomorrow and over the weekend, so I probably won't be able to review this again until next week.

          Show
          Tim Hunt added a comment - To be honest, I just did not see the ddmatch bit. Oops! I had better look at this more closely. However, the way you have done if (installed) is neat. I am inclined to leave it, even though it is not good practice. I do wonder if you need a html_to_text call in the match case? You are right that if you change qformat_xml it will break lesson. Stupid lesson module. Someone really needs to convert it to use the qbank / qengine. I guess we have to leave it for now. I am afraid that I am away tomorrow and over the weekend, so I probably won't be able to review this again until next week.
          Hide
          Jean-Michel Vedrine added a comment -

          Have a nice week-end. Yes html_to_text would be a safe way to avoid all problems when ddmatch is not installed. I also need to learn how to rebase my git branchs.

          Show
          Jean-Michel Vedrine added a comment - Have a nice week-end. Yes html_to_text would be a safe way to avoid all problems when ddmatch is not installed. I also need to learn how to rebase my git branchs.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          I am attaching the res00001.dat fot the tester of this issue. WARNING this file CAN'T be imported with the "Blackboard" file format. It is here just for testing what happend when an user try to import a file of the wrong type. This file will in fact be imported by the "Blackboard V6+" format when MDL-25492 will be solved.

          Show
          Jean-Michel Vedrine added a comment - - edited I am attaching the res00001.dat fot the tester of this issue. WARNING this file CAN'T be imported with the "Blackboard" file format. It is here just for testing what happend when an user try to import a file of the wrong type. This file will in fact be imported by the "Blackboard V6+" format when MDL-25492 will be solved.
          Hide
          Jean-Michel Vedrine added a comment -

          I studied the lib code and it seems that trim(html_to_text($some_html, 0)) is near perfect (the only improvment I see would be to replace newlines and tabs by spaces).

          Show
          Jean-Michel Vedrine added a comment - I studied the lib code and it seems that trim(html_to_text($some_html, 0)) is near perfect (the only improvment I see would be to replace newlines and tabs by spaces).
          Hide
          Tim Hunt added a comment -

          I will try to peer review this tomorrow.

          Show
          Tim Hunt added a comment - I will try to peer review this tomorrow.
          Hide
          Jean-Michel Vedrine added a comment -

          Some things to change :

          • I had a look at the textlib class and entities_to_utf8 is a lot better than what I currently use to clean texts inputs. I will change to use it
          • the default value I put in some of the call to getpath need to be verified to avoid producing invalid argument errors in subsequent code.
          • the code :
            if ($ddmatch_is_installed) { $choicetext = $this->text_field($this->cleaninput($this->getpath($choice, array('#', 'TEXT', 0, '#'), '', true))); }

            else

            { $choicetext = trim(strip_tags($this->getpath($choice, array('#', 'TEXT', 0, '#'), '', true))); }

          if ($choicetext != '') { // Only import non empty subanswers.
          need to be reworked because if ddmatch is installed $choicetext is an array so I can't compare it with empty string.

          Show
          Jean-Michel Vedrine added a comment - Some things to change : I had a look at the textlib class and entities_to_utf8 is a lot better than what I currently use to clean texts inputs. I will change to use it the default value I put in some of the call to getpath need to be verified to avoid producing invalid argument errors in subsequent code. the code : if ($ddmatch_is_installed) { $choicetext = $this->text_field($this->cleaninput($this->getpath($choice, array('#', 'TEXT', 0, '#'), '', true))); } else { $choicetext = trim(strip_tags($this->getpath($choice, array('#', 'TEXT', 0, '#'), '', true))); } if ($choicetext != '') { // Only import non empty subanswers. need to be reworked because if ddmatch is installed $choicetext is an array so I can't compare it with empty string.
          Hide
          Tim Hunt added a comment -

          I have added some more comments inline in github. I hate to think how many emails to you that triggered. One other point that did not fit as an inline comment:

          1. There should be exactly one blank line between function definitions in the code.

          Almost all my comments were trivial coding style points like that. There were only a few significant ones. Hopefully you can get this finished this week, so we can submit it for integration in time for next week's cycle.

          Show
          Tim Hunt added a comment - I have added some more comments inline in github. I hate to think how many emails to you that triggered. One other point that did not fit as an inline comment: There should be exactly one blank line between function definitions in the code. Almost all my comments were trivial coding style points like that. There were only a few significant ones. Hopefully you can get this finished this week, so we can submit it for integration in time for next week's cycle.
          Hide
          Tim Hunt added a comment -

          As usual, I forgot to click the buttons!

          Show
          Tim Hunt added a comment - As usual, I forgot to click the buttons!
          Hide
          Tim Hunt added a comment -

          One other thing, the testing instructions are a bit of a mass of text, and quite hard to read. Making numbered steps can be a good idea. MDL-33041 is a good example.

          Show
          Tim Hunt added a comment - One other thing, the testing instructions are a bit of a mass of text, and quite hard to read. Making numbered steps can be a good idea. MDL-33041 is a good example.
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks a lot for reviewing this and for your comments. Github mails are not a problem. I will make the modifications and commit to github ASAP.
          The question about what fraction to allow for wrong choices in a multichoice multi is something that bothered me too : the way it was (and it still is in my code) done is stupid : if a student check all choices he surely get maxmark for this question !! I wonder if teachers using this import format realized that ? Sudents surely promptly realized it but I doubt they alerted their teacher
          I will try to improve my testing instructions but I certainly can't compete with Sam Marshall who surely win the gold medail for testing instructions (I am always admirative).

          Show
          Jean-Michel Vedrine added a comment - Thanks a lot for reviewing this and for your comments. Github mails are not a problem. I will make the modifications and commit to github ASAP. The question about what fraction to allow for wrong choices in a multichoice multi is something that bothered me too : the way it was (and it still is in my code) done is stupid : if a student check all choices he surely get maxmark for this question !! I wonder if teachers using this import format realized that ? Sudents surely promptly realized it but I doubt they alerted their teacher I will try to improve my testing instructions but I certainly can't compete with Sam Marshall who surely win the gold medail for testing instructions (I am always admirative).
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim, I have rebased and done 2 commits (well the 2nd one was just to fix what I forgot in the first !)
          I have integrated what you found in your review and also :

          • the use of textlib to convert numerical entities to utf-8 letting other entities untouched.
          • changed a lot of default parameters in getpath calls from false to array() when the result was used in foreach loops so to suppress invalid parameter errors (this can only happend on broken XML files of course but I think we should avoid that).
            Putting in peer review again. I will work on the testing instructions.
          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I have rebased and done 2 commits (well the 2nd one was just to fix what I forgot in the first !) I have integrated what you found in your review and also : the use of textlib to convert numerical entities to utf-8 letting other entities untouched. changed a lot of default parameters in getpath calls from false to array() when the result was used in foreach loops so to suppress invalid parameter errors (this can only happend on broken XML files of course but I think we should avoid that). Putting in peer review again. I will work on the testing instructions.
          Hide
          Jean-Michel Vedrine added a comment -

          File sample with 6 questions one of each type

          Show
          Jean-Michel Vedrine added a comment - File sample with 6 questions one of each type
          Hide
          Tim Hunt added a comment -

          I think we are nearly there. I just spotted two whitespace errors.

          • No space before the comma in "—" => "-" ,
          • No space after ( in $str = textlib::entities_to_utf8( $str, false);

          Are you using codechecker? https://github.com/moodlehq/moodle-local_codechecker

          The other thing to do here is to squash this down to a single commit. I don't think the history of the code-review is interesting. Are you familiar with using "git rebase -i" to change 4 commits into one commit?

          Show
          Tim Hunt added a comment - I think we are nearly there. I just spotted two whitespace errors. No space before the comma in "—" => "-" , No space after ( in $str = textlib::entities_to_utf8( $str, false); Are you using codechecker? https://github.com/moodlehq/moodle-local_codechecker The other thing to do here is to squash this down to a single commit. I don't think the history of the code-review is interesting. Are you familiar with using "git rebase -i" to change 4 commits into one commit?
          Hide
          Jean-Michel Vedrine added a comment -

          Yes I do use the codechecker but I unfortunately made some "adjustments" and after that I re-run phpunit tests, but forgot to re-run codechecker !
          I never used git rebase -i but I will study the help.

          Show
          Jean-Michel Vedrine added a comment - Yes I do use the codechecker but I unfortunately made some "adjustments" and after that I re-run phpunit tests, but forgot to re-run codechecker ! I never used git rebase -i but I will study the help.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          Could you look if I correctly squashed all commits, and if testing instructions look OK to you ?

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, Could you look if I correctly squashed all commits, and if testing instructions look OK to you ?
          Hide
          Tim Hunt added a comment -

          The commit looks fine, except for the commit comment. You need to edit that (git commit --amend) and clean it up.

          Show
          Tim Hunt added a comment - The commit looks fine, except for the commit comment. You need to edit that (git commit --amend) and clean it up.
          Hide
          Jean-Michel Vedrine added a comment -

          Done (I think). What about the testing instructions (they certainly can't rivalise with Sam Marshall's ones)

          Show
          Jean-Michel Vedrine added a comment - Done (I think). What about the testing instructions (they certainly can't rivalise with Sam Marshall's ones)
          Hide
          Jean-Michel Vedrine added a comment -

          If you think that master branch is OK is it time to update 2.2 and 2.3 branchs too ?

          Show
          Jean-Michel Vedrine added a comment - If you think that master branch is OK is it time to update 2.2 and 2.3 branchs too ?
          Hide
          Tim Hunt added a comment -

          To INTEGRATORS: This may look like a big scary change, so you may be wondering if it should go on the stable branches. The answer is that it should, because currently this import format is totally broken, so the code we currently have on the stable branches is mostly useless and should be replaced.

          Jean-Michel, everything looks great now, including the testing instructions, so I am submitting this for integration. You may still need to re-base the branches once, after this week's weekly releases are done.

          Show
          Tim Hunt added a comment - To INTEGRATORS: This may look like a big scary change, so you may be wondering if it should go on the stable branches. The answer is that it should, because currently this import format is totally broken, so the code we currently have on the stable branches is mostly useless and should be replaced. Jean-Michel, everything looks great now, including the testing instructions, so I am submitting this for integration. You may still need to re-base the branches once, after this week's weekly releases are done.
          Hide
          Tim Hunt added a comment -

          It looks like you have already done the stable branches.

          Show
          Tim Hunt added a comment - It looks like you have already done the stable branches.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim, I don't think the stable branchs are up to date as I have done them on my first commit days ago when I started (maybe it wasn't a good strategy ?) so I need to update them. What is the easiest method to do so ? Maybe to delete these old branchs and cherry pick ?

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I don't think the stable branchs are up to date as I have done them on my first commit days ago when I started (maybe it wasn't a good strategy ?) so I need to update them. What is the easiest method to do so ? Maybe to delete these old branchs and cherry pick ?
          Hide
          Tim Hunt added a comment -

          Oops. I did not look closely enough. Yes, delete old branch and cherry-pick is probably the best way.

          Show
          Tim Hunt added a comment - Oops. I did not look closely enough. Yes, delete old branch and cherry-pick is probably the best way.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          i think stable branchs are ok now, but it's better you verify. Cherry picking to 2.3 was easy, cherry picking to 2.2 was a little hardest as git mentionned a conflict but looking at the file the editing looked easy so I hope I got it right.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, i think stable branchs are ok now, but it's better you verify. Cherry picking to 2.3 was easy, cherry picking to 2.2 was a little hardest as git mentionned a conflict but looking at the file the editing looked easy so I hope I got it right.
          Hide
          Jean-Michel Vedrine added a comment -

          As I was not 100% sure I properly resolved the conflict in the 2.2 branch, I just tested an import with the 2.2 branch and all is OK.

          Show
          Jean-Michel Vedrine added a comment - As I was not 100% sure I properly resolved the conflict in the 2.2 branch, I just tested an import with the 2.2 branch and all is OK.
          Hide
          Tim Hunt added a comment -

          Another test you can do on the 22 branch is

          git diff MDL-34738_22 MDL-34738_23 question/format/blackboard/format.php

          Show
          Tim Hunt added a comment - Another test you can do on the 22 branch is git diff MDL-34738 _22 MDL-34738 _23 question/format/blackboard/format.php
          Hide
          Jean-Michel Vedrine added a comment -

          But those 2 branchs are in 2 differents local repo moodle_22 and moodle_23.
          In fact I did something pretty similar (I think) using winmerge (a windows tool I am fimiliar with) to compare these 2 files and they are the same.
          Additionnaly really using the MOODLE_22_STABLE branch to import something make me feel better.
          You must understand I am somewhat nervous to get something so big integrated as my very first try of using git.
          Your advices on git during these last days have been unvaluable to make me understand how things work.

          Show
          Jean-Michel Vedrine added a comment - But those 2 branchs are in 2 differents local repo moodle_22 and moodle_23. In fact I did something pretty similar (I think) using winmerge (a windows tool I am fimiliar with) to compare these 2 files and they are the same. Additionnaly really using the MOODLE_22_STABLE branch to import something make me feel better. You must understand I am somewhat nervous to get something so big integrated as my very first try of using git. Your advices on git during these last days have been unvaluable to make me understand how things work.
          Hide
          Tim Hunt added a comment -

          You can get the two branches into the same repository, and compare them, using

          git fetch origin
          git diff origin/MDL-34738_22 origin/MDL-34738_23 question/format/blackboard/format.php

          Show
          Tim Hunt added a comment - You can get the two branches into the same repository, and compare them, using git fetch origin git diff origin/ MDL-34738 _22 origin/ MDL-34738 _23 question/format/blackboard/format.php
          Hide
          Jean-Michel Vedrine added a comment -

          Sorry if this seems like a stupid question but as this is "Waiting for integration review", now that the weekly release is out, when should I rebase ?

          Show
          Jean-Michel Vedrine added a comment - Sorry if this seems like a stupid question but as this is "Waiting for integration review", now that the weekly release is out, when should I rebase ?
          Hide
          Tim Hunt added a comment -

          You can rebase any time, and you should rebase before Monday morning New Zealand time.

          Normally you get a message reminding you to do that, but they did not send it this week.

          Show
          Tim Hunt added a comment - You can rebase any time, and you should rebase before Monday morning New Zealand time. Normally you get a message reminding you to do that, but they did not send it this week.
          Hide
          Jean-Michel Vedrine added a comment -

          I rebased all branchs. I hope I did it right !

          Show
          Jean-Michel Vedrine added a comment - I rebased all branchs. I hope I did it right !
          Hide
          Tim Hunt added a comment -

          That looks good.

          Show
          Tim Hunt added a comment - That looks good.
          Hide
          Dan Poltawski added a comment -

          Hi Jean-Michel,

          This is looking good, but there are two problems i've spotted quickly:

          1. Moodle 2.2 doesn't have phpunit, but it looks like you've included the phpunit tests on the patch for that. I don't think its necessary to backport the tests to simpletest, but I don't think we should be included the test related files on 2.2.
          2. Is there a reason why make_test_xml doesn't load the 'test fixture'? I've not looked too deeply (yet) but it looks roughtly similar and would make the test file easier to read.
          Show
          Dan Poltawski added a comment - Hi Jean-Michel, This is looking good, but there are two problems i've spotted quickly: Moodle 2.2 doesn't have phpunit, but it looks like you've included the phpunit tests on the patch for that. I don't think its necessary to backport the tests to simpletest, but I don't think we should be included the test related files on 2.2. Is there a reason why make_test_xml doesn't load the 'test fixture'? I've not looked too deeply (yet) but it looks roughtly similar and would make the test file easier to read.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Dan, Thanks a lot for the review.

          1. You are right, tests should not be included in 2.2 stable branch, only in 2.3 and master. Should I make a commit or will you correct the problem ?
          2. I don't really know. Both should be totally identical. Tim has included the xml in it's tests for other formats, so I did it the same way. I can change that if you think it's better.
            If so, what code should I use to load the file (if you can point me to another test that load a code that would really help me) I only know the php file_get_contents function but maybe there is a more "moodleish" way to do it.
          Show
          Jean-Michel Vedrine added a comment - Hello Dan, Thanks a lot for the review. You are right, tests should not be included in 2.2 stable branch, only in 2.3 and master. Should I make a commit or will you correct the problem ? I don't really know. Both should be totally identical. Tim has included the xml in it's tests for other formats, so I did it the same way. I can change that if you think it's better. If so, what code should I use to load the file (if you can point me to another test that load a code that would really help me) I only know the php file_get_contents function but maybe there is a more "moodleish" way to do it.
          Hide
          Jean-Michel Vedrine added a comment -

          Forget about my second question. I looked at other tests and they seems to all use file_get_contents, so I will do that.

          Show
          Jean-Michel Vedrine added a comment - Forget about my second question. I looked at other tests and they seems to all use file_get_contents, so I will do that.
          Hide
          Dan Poltawski added a comment -

          Great - thanks Jean-Michel, if you can sort out the problem in the 2.2 branch that would be best.

          thanks

          Show
          Dan Poltawski added a comment - Great - thanks Jean-Michel, if you can sort out the problem in the 2.2 branch that would be best. thanks
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Dan,

          1. phpunit tests removed from MOODLE_22_STABLE branch
          2. phpunit tests now load the sample file from fixtures sub dir both in master and MOODLE_23_STABLE.
            Note that contrary to my beleif there was a tiny difference between the .dat file content and the xml in tests, so I modified the sample .dat file so that tests are still OK. The difference is not significant.
          Show
          Jean-Michel Vedrine added a comment - Hello Dan, phpunit tests removed from MOODLE_22_STABLE branch phpunit tests now load the sample file from fixtures sub dir both in master and MOODLE_23_STABLE. Note that contrary to my beleif there was a tiny difference between the .dat file content and the xml in tests, so I modified the sample .dat file so that tests are still OK. The difference is not significant.
          Hide
          Dan Poltawski added a comment -

          Thanks guys, i've integrated this now to 22, 23 and master.

          On the master/23 branch I added a commit incrementing the plugin version number, since this is quite distinct change so seems worth having a new version.

          On the 22 branch I did the same, but also squashed down your removal of the tests commit, since that is probably best not being in the 22 history.

          thanks!

          Show
          Dan Poltawski added a comment - Thanks guys, i've integrated this now to 22, 23 and master. On the master/23 branch I added a commit incrementing the plugin version number, since this is quite distinct change so seems worth having a new version. On the 22 branch I did the same, but also squashed down your removal of the tests commit, since that is probably best not being in the 22 history. thanks!
          Hide
          David Monllaó added a comment -

          Sorry, it failed on master, in the 2nd part of the testing instructions, trying to load the sample_blackboard.dat file. The questions are imported well on 2.2

          I've attached a screenshot

          Show
          David Monllaó added a comment - Sorry, it failed on master, in the 2nd part of the testing instructions, trying to load the sample_blackboard.dat file. The questions are imported well on 2.2 I've attached a screenshot
          Hide
          Jean-Michel Vedrine added a comment -

          Hello David,
          Thanks a lot for the screenshot.
          But I really don't understand what is happening on master because I can't reproduce your problem on my local git repo with my commits on top of last week master.
          Additionnaly the error displayed don't make sense to me : why a .dat file would be considered as an xml one ? The code of the function used by the plugin to tell what filetypes he is able to import is very short (4-5 lines) and i don't see any error here.
          Tim, I need your help on this because I am lost.
          Could something other integrated this week have broken things or could the commit integrated be different form my git repo ?

          Show
          Jean-Michel Vedrine added a comment - Hello David, Thanks a lot for the screenshot. But I really don't understand what is happening on master because I can't reproduce your problem on my local git repo with my commits on top of last week master. Additionnaly the error displayed don't make sense to me : why a .dat file would be considered as an xml one ? The code of the function used by the plugin to tell what filetypes he is able to import is very short (4-5 lines) and i don't see any error here. Tim, I need your help on this because I am lost. Could something other integrated this week have broken things or could the commit integrated be different form my git repo ?
          Hide
          Tim Hunt added a comment -

          Jean-Michel, you can get the code with everything integrated by doing:

          git remote add integration git://git.moodle.org/integration.git
          git fetch integration
          git checkout integration/master

          (or git merge integration/master for the last command.)

          Show
          Tim Hunt added a comment - Jean-Michel, you can get the code with everything integrated by doing: git remote add integration git://git.moodle.org/integration.git git fetch integration git checkout integration/master (or git merge integration/master for the last command.)
          Hide
          Tim Hunt added a comment -

          This error does seem to be qformat-specific. Start looking at validate_uploaded_file in question/import_form.php.

          So, the problem is that $file->get_mimetype() returns text/plain for a .dat file, but mimeinfo('type', '.dat') returns document/unknown.

          Show
          Tim Hunt added a comment - This error does seem to be qformat-specific. Start looking at validate_uploaded_file in question/import_form.php. So, the problem is that $file->get_mimetype() returns text/plain for a .dat file, but mimeinfo('type', '.dat') returns document/unknown.
          Hide
          Jean-Michel Vedrine added a comment -

          I begin to understand the problem, but note completely !

          When the can_import_file function was introduced some code was added in the blackboard_six import format :

              public function can_import_file($file) {
                  $mimetypes = array(
                      mimeinfo('type', '.dat'),
                      mimeinfo('type', '.zip')
                  );
                  return in_array($file->get_mimetype(), $mimetypes);
              }
          

          So when I begin to repair the blackboard format, to make it accept .dat files I was thinking that mimeinfo was working for .dat files, and I used the code :

          public function mime_type() {
              return mimeinfo('type', '.dat');
          }
          

          But in fact looking in the filelib library code, the mime type of .dat files is no known so mimetype('type', '.dat') don't return something meaningfull. On my system with last week master it returns : document/unknown
          Now if we look at the code in question/import.php

          public function can_import_file($file) {
              return ($file->get_mimetype() == $this->mime_type());
          }
          

          and the code in question/import_form.php

          if (!$qformat->can_import_file($file)) {
              $a = new stdClass();
              $a->actualtype = $file->get_mimetype();
              $a->expectedtype = $qformat->mime_type();
              $errors['newfile'] = get_string('importwrongfiletype', 'question', $a);
          }
          

          the message on Divid's screenshot appear les mysterious :
          The type of file you selected (application/xml) does not match the type expected by this import format (document/unknown).
          the last part "the type expected by this import format (document/unknown" is logical becaus e this is the value returned by qformat_blackboard mime_type function.
          But what I don't understand is why $file->get_mimetype would return application system on some server and document/unknown on some other.

          Anyway the root of the problem is that .dat files are not correctly managed by Moodle mime system.

          Show
          Jean-Michel Vedrine added a comment - I begin to understand the problem, but note completely ! When the can_import_file function was introduced some code was added in the blackboard_six import format : public function can_import_file($file) { $mimetypes = array( mimeinfo('type', '.dat'), mimeinfo('type', '.zip') ); return in_array($file->get_mimetype(), $mimetypes); } So when I begin to repair the blackboard format, to make it accept .dat files I was thinking that mimeinfo was working for .dat files, and I used the code : public function mime_type() { return mimeinfo('type', '.dat'); } But in fact looking in the filelib library code, the mime type of .dat files is no known so mimetype('type', '.dat') don't return something meaningfull. On my system with last week master it returns : document/unknown Now if we look at the code in question/import.php public function can_import_file($file) { return ($file->get_mimetype() == $ this ->mime_type()); } and the code in question/import_form.php if (!$qformat->can_import_file($file)) { $a = new stdClass(); $a->actualtype = $file->get_mimetype(); $a->expectedtype = $qformat->mime_type(); $errors['newfile'] = get_string('importwrongfiletype', 'question', $a); } the message on Divid's screenshot appear les mysterious : The type of file you selected (application/xml) does not match the type expected by this import format (document/unknown). the last part "the type expected by this import format (document/unknown" is logical becaus e this is the value returned by qformat_blackboard mime_type function. But what I don't understand is why $file->get_mimetype would return application system on some server and document/unknown on some other. Anyway the root of the problem is that .dat files are not correctly managed by Moodle mime system.
          Hide
          Tim Hunt added a comment -

          The problem is this bit of code in lib/filestorage/file_storage.php:

                  $type = mimeinfo('type', $filename);
                  if ($type === 'document/unknown' && class_exists('finfo') && file_exists($pathname)) {
                      $finfo = new finfo(FILEINFO_MIME_TYPE);
                      $type = mimeinfo_from_type('type', $finfo->file($pathname));
                  }
          

          Introduced by MDL-33082 and MDL-33144.

          Show
          Tim Hunt added a comment - The problem is this bit of code in lib/filestorage/file_storage.php: $type = mimeinfo('type', $filename); if ($type === 'document/unknown' && class_exists('finfo') && file_exists($pathname)) { $finfo = new finfo(FILEINFO_MIME_TYPE); $type = mimeinfo_from_type('type', $finfo->file($pathname)); } Introduced by MDL-33082 and MDL-33144 .
          Hide
          Tim Hunt added a comment -

          Odd. That code was written last May! It has not changed recently.

          Show
          Tim Hunt added a comment - Odd. That code was written last May! It has not changed recently.
          Hide
          Tim Hunt added a comment -

          Perhaps you do not have the finfo PHP extension Jean-Michel?

          Show
          Tim Hunt added a comment - Perhaps you do not have the finfo PHP extension Jean-Michel?
          Hide
          Tim Hunt added a comment -

          At them moment, the best idea I have for fixing the problem is to override can_import_file in qformat_blackboard like this:

              public function can_import_file($file) {
                  return parent::can_import_file($file) || mimeinfo('type', $file->get_filename()) == $this->mime_type();
              }
          

          Probably with an added comment explaining why this hack is necessary.

          Show
          Tim Hunt added a comment - At them moment, the best idea I have for fixing the problem is to override can_import_file in qformat_blackboard like this: public function can_import_file($file) { return parent::can_import_file($file) || mimeinfo('type', $file->get_filename()) == $ this ->mime_type(); } Probably with an added comment explaining why this hack is necessary.
          Hide
          Jean-Michel Vedrine added a comment -

          Good guess Tim,
          finfo PHP extension is not activated on my test server. I will activate it to avoid that kind of problem in the future.
          Is there somewhere a list of "recommended extensions" different from the "required extensions" list in Moodle requirements ?

          Anyway it seems wierd that $file->get_mimetype and mimeinfo don't match for .dat files

          This is an unrelated problem Tim but it seems to me that the bit of code :

          if (!$qformat->can_import_file($file)) {
              $a = new stdClass();
              $a->actualtype = $file->get_mimetype();
              $a->expectedtype = $qformat->mime_type();
              $errors['newfile'] = get_string('importwrongfiletype', 'question', $a);
          }
          

          Seams to means that a format who overwrite can_import_file and don't provide export must also redefine mimetype so that a meaningfull message is displayed for a nom importable file bacause the default method use export_file_extension.
          So I must do that for blackboard_six.

          Show
          Jean-Michel Vedrine added a comment - Good guess Tim, finfo PHP extension is not activated on my test server. I will activate it to avoid that kind of problem in the future. Is there somewhere a list of "recommended extensions" different from the "required extensions" list in Moodle requirements ? Anyway it seems wierd that $file->get_mimetype and mimeinfo don't match for .dat files This is an unrelated problem Tim but it seems to me that the bit of code : if (!$qformat->can_import_file($file)) { $a = new stdClass(); $a->actualtype = $file->get_mimetype(); $a->expectedtype = $qformat->mime_type(); $errors['newfile'] = get_string('importwrongfiletype', 'question', $a); } Seams to means that a format who overwrite can_import_file and don't provide export must also redefine mimetype so that a meaningfull message is displayed for a nom importable file bacause the default method use export_file_extension. So I must do that for blackboard_six.
          Hide
          Jean-Michel Vedrine added a comment -

          Results of some tests :

          Before Tim's code change

          • if finfo is not activated, upload of a .dat file display no message and the import is OK. this is the expected result.
          • if finfo is not activated, upload of a non importable file (.zip file) fails with the error message :
            The type of the file you selected (application/zip) does not match the type expected by this import format (document/unknown).
            This is the expected result.
          • if finfo is activated upload of a .dat file fails with the error message :
            The type of the file you selected (application/xml) does not match the type expected by this import format (document/unknown).
            So this confirm David's testing, and is a wrong result.

          With Tim's code change

          I get exactly the same results weither finfo is activated or not

          • upload of a .dat file display no message and the import is OK. This is the expected result.
          • upload of a non importable file (.zip file) result in the message :
            The type of the file you selected (application/zip) does not match the type expected by this import format (document/unknown).
            This is the expected result

          So for me Tim's change is working in all cases (finfo activated or not).
          The only improvable thing would be the last bit on the message (document/unknown) wich is somewhat misleading but I don't see how.

          So now, Gentlemen, as this is my first issue failing during testing (Champagne !) how do we do to commit the fix ?

          Show
          Jean-Michel Vedrine added a comment - Results of some tests : Before Tim's code change if finfo is not activated, upload of a .dat file display no message and the import is OK. this is the expected result . if finfo is not activated, upload of a non importable file (.zip file) fails with the error message : The type of the file you selected (application/zip) does not match the type expected by this import format (document/unknown). This is the expected result . if finfo is activated upload of a .dat file fails with the error message : The type of the file you selected (application/xml) does not match the type expected by this import format (document/unknown). So this confirm David's testing, and is a wrong result . With Tim's code change I get exactly the same results weither finfo is activated or not upload of a .dat file display no message and the import is OK. This is the expected result . upload of a non importable file (.zip file) result in the message : The type of the file you selected (application/zip) does not match the type expected by this import format (document/unknown). This is the expected result So for me Tim's change is working in all cases (finfo activated or not). The only improvable thing would be the last bit on the message (document/unknown) wich is somewhat misleading but I don't see how. So now, Gentlemen, as this is my first issue failing during testing (Champagne !) how do we do to commit the fix ?
          Hide
          Tim Hunt added a comment -

          1. Make a new branch on top of integration/master - for example

          git checkout -b MDL-34738_fixup integration/master

          2. Make a commit on that branch to fix the problem.

          3. Push the branch to github, and comment here telling integrators what to do.

          Show
          Tim Hunt added a comment - 1. Make a new branch on top of integration/master - for example git checkout -b MDL-34738 _fixup integration/master 2. Make a commit on that branch to fix the problem. 3. Push the branch to github, and comment here telling integrators what to do.
          Hide
          Jean-Michel Vedrine added a comment -

          I have some troube to correctly create a link {@link ) in the phpdoc bloc to refer to file_storage mimetype function.

          Show
          Jean-Michel Vedrine added a comment - I have some troube to correctly create a link {@link ) in the phpdoc bloc to refer to file_storage mimetype function.
          Hide
          Tim Hunt added a comment -

          I think it is just

          {@link file_storage::mimetype()}
          Show
          Tim Hunt added a comment - I think it is just {@link file_storage::mimetype()}
          Hide
          Jean-Michel Vedrine added a comment -

          Here is my try

              /**
               * Check if the given file is capable of being imported by this plugin.
               * As {@link file_storage::mimetype()} now uses finfo PHP extension if available,
               * the value returned by $file->get_mimetype for a .dat file is not the same on all servers.
               * So if the parent method fails we must use mimeinfo on the filename.
               * @param stored_file $file the file to check
               * @return bool whether this plugin can import the file
               */
          
          Show
          Jean-Michel Vedrine added a comment - Here is my try /** * Check if the given file is capable of being imported by this plugin. * As {@link file_storage::mimetype()} now uses finfo PHP extension if available, * the value returned by $file->get_mimetype for a .dat file is not the same on all servers. * So if the parent method fails we must use mimeinfo on the filename. * @param stored_file $file the file to check * @ return bool whether this plugin can import the file */
          Hide
          Jean-Michel Vedrine added a comment -

          To integrators,
          I made a MDL-34738_fixup branch on top of integration/master with Tim's fix for the problem found during testing.
          You should really look carefully at what I did, as this is brand new for me so it's quite possible I didn't got it right !
          commit is at :
          https://github.com/jmvedrine/moodle/commit/7263fd6c53db77da7b947a83b75b07994f512215

          Show
          Jean-Michel Vedrine added a comment - To integrators, I made a MDL-34738 _fixup branch on top of integration/master with Tim's fix for the problem found during testing. You should really look carefully at what I did, as this is brand new for me so it's quite possible I didn't got it right ! commit is at : https://github.com/jmvedrine/moodle/commit/7263fd6c53db77da7b947a83b75b07994f512215
          Hide
          Tim Hunt added a comment -

          That looks good to me. +1

          Show
          Tim Hunt added a comment - That looks good to me. +1
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks Tim,
          Is there anthing else to do (I don't seems to be able to do something on the issue's workflow) or is it the integrator's role to change the issue's status ?

          Show
          Jean-Michel Vedrine added a comment - Thanks Tim, Is there anthing else to do (I don't seems to be able to do something on the issue's workflow) or is it the integrator's role to change the issue's status ?
          Hide
          Tim Hunt added a comment -

          The integrators will be looking at http://tracker.moodle.org/secure/Dashboard.jspa?selectPageId=11350, particularly the pie chart bottom middle. Therefore, the know this bug needs attention.

          Show
          Tim Hunt added a comment - The integrators will be looking at http://tracker.moodle.org/secure/Dashboard.jspa?selectPageId=11350 , particularly the pie chart bottom middle. Therefore, the know this bug needs attention.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          New commit integrated (23 & master), many thanks!

          Sending this again to testing...

          Show
          Eloy Lafuente (stronk7) added a comment - New commit integrated (23 & master), many thanks! Sending this again to testing...
          Hide
          David Monllaó added a comment -

          Congratulations! It passes. All the steps tested both in master and 22

          Show
          David Monllaó added a comment - Congratulations! It passes. All the steps tested both in master and 22
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks David. This was somewhat an epic story for a beginer like me !

          Show
          Jean-Michel Vedrine added a comment - Thanks David. This was somewhat an epic story for a beginer like me !
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YEAR!*

          CAF*, TOT!*

          • Your effort amazingly resulted. (unbelievable :-P)
          • Closing as fixed.
          • Tons of thanks.
          Show
          Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.
          Hide
          Anthony Borrow added a comment -

          Was any testing done using MSSQL? I have a zip file that imports on 2.3 in a LAMP environment but it is returning a dmlwriteexception on a Windows server and I am suspecting that they are using MSSQL (I've not confirmed that). If I find an issue I will create another issue but was curious if any testing was done with other databases. Peace - Anthony

          Show
          Anthony Borrow added a comment - Was any testing done using MSSQL? I have a zip file that imports on 2.3 in a LAMP environment but it is returning a dmlwriteexception on a Windows server and I am suspecting that they are using MSSQL (I've not confirmed that). If I find an issue I will create another issue but was curious if any testing was done with other databases. Peace - Anthony
          Hide
          David Monllaó added a comment -

          Hi Anthony,

          It was tested using postgres DBs

          Regards

          Show
          David Monllaó added a comment - Hi Anthony, It was tested using postgres DBs Regards
          Hide
          Anthony Borrow added a comment -

          Thanks David - I'll work to get more information from debugging and follow up with a separate issue. Peace - Anthony

          Show
          Anthony Borrow added a comment - Thanks David - I'll work to get more information from debugging and follow up with a separate issue. Peace - Anthony
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Anthony,
          I was unable to test on MSSQL because I have no MSSQL server available. I tested on MySql and David tested on Postgres (but I do also have a server with Postgres). Looking at the code I see absolutely no strange query. It's only using the same ones that any other import format. Could the problem be related to unzipping of file copy ?
          If you can provide debugging informations I will be glad to help if I can.

          Show
          Jean-Michel Vedrine added a comment - Hello Anthony, I was unable to test on MSSQL because I have no MSSQL server available. I tested on MySql and David tested on Postgres (but I do also have a server with Postgres). Looking at the code I see absolutely no strange query. It's only using the same ones that any other import format. Could the problem be related to unzipping of file copy ? If you can provide debugging informations I will be glad to help if I can.
          Hide
          Tim Hunt added a comment -

          Question import uses the standard functions to save the question definition, and editing questions works on any DB. So, if it is a MSSQL problem, it must be a weird one. If you can reproduce, please create a new issue with

          • Full developer debug error message.
          • Example import file attached that can be used to reproduce the problem.
            Thanks.
          Show
          Tim Hunt added a comment - Question import uses the standard functions to save the question definition, and editing questions works on any DB. So, if it is a MSSQL problem, it must be a weird one. If you can reproduce, please create a new issue with Full developer debug error message. Example import file attached that can be used to reproduce the problem. Thanks.
          Hide
          Anthony Borrow added a comment -

          Well I was getting mixed reports from the users. At first I was told that it was a dmlwriteexception error; however, later the site admin sent me screenshot that revealed it was showing the error cannotfindquestionfile on line 223 of /question/format/blackboard_six/format.php which indicated to me files from Moodle 2.1 but the environment.php showed they were running 2.3. Supposedly they had downloaded from a zip file. In any case, I had them update the code and the issue disappeared. It was very strange but it seems there is no issue with MSSQL. Peace - Anthony

          Show
          Anthony Borrow added a comment - Well I was getting mixed reports from the users. At first I was told that it was a dmlwriteexception error; however, later the site admin sent me screenshot that revealed it was showing the error cannotfindquestionfile on line 223 of /question/format/blackboard_six/format.php which indicated to me files from Moodle 2.1 but the environment.php showed they were running 2.3. Supposedly they had downloaded from a zip file. In any case, I had them update the code and the issue disappeared. It was very strange but it seems there is no issue with MSSQL. Peace - Anthony

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: