Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-26511

Adding images in Cloze vertical and horizontal display multiplechoices subquestions

    Details

    • Type: Improvement
    • Status: Reopened
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 2.0.3
    • Fix Version/s: None
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      Mainly import the some questions from https://tracker.moodle.org/secure/attachment/32973/questions-T-26511_with_or_no_images-20130604-2349.xml which put specific images in the question text, answers for multi and feedback for all question types and the same questions without images.

      Check the behaviour of set_subquestions_elements_itemid($question, '@@PLUGINFILE@@');
      and looking at the file table records that the setting of image links are only created if there is an image in the given element. This also verify that the import is OK.

      Then edit copy the questions. test the decode verify display, save the question. This will test the filtering process
      set_subquestions_elements_itemid($question, 'draftfile.php');

      Create a copy of the question to another category.

      Move the questions to another category and context.

      Create a quiz, test OK.

      Show
      Mainly import the some questions from https://tracker.moodle.org/secure/attachment/32973/questions-T-26511_with_or_no_images-20130604-2349.xml which put specific images in the question text, answers for multi and feedback for all question types and the same questions without images. Check the behaviour of set_subquestions_elements_itemid($question, '@@PLUGINFILE@@'); and looking at the file table records that the setting of image links are only created if there is an image in the given element. This also verify that the import is OK. Then edit copy the questions. test the decode verify display, save the question. This will test the filtering process set_subquestions_elements_itemid($question, 'draftfile.php'); Create a copy of the question to another category. Move the questions to another category and context. Create a quiz, test OK.
    • Affected Branches:
      MOODLE_20_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      This is a follow up of adding HTML to multiplechoice question MDL-24594.
      In 1.9 users can put image in Cloze multiplechoice answers so we should implement this in 2.0..
      The HTML editor is already there for the questiontext that is used to generate the subquestions answers.
      This will need a very specific code given that the question images of the main cloze question will be used answers of the child multiplechoice questions.
      This could not be implement in the select element classic multichoice.

      The coding will be more complex and necessitates a distinct project.

        Gliffy Diagrams

        1. cloze-with-image.xml
          0.5 kB
          UMass Amherst Instructional Media Lab
        2. questions-T100-import1.9mix-20110314-1202.xml
          52 kB
          Pierre Pichet
        3. questions-T1-export 26511 selection-20130424-2301.xml
          477 kB
          Pierre Pichet
        4. questions-T-26511_with_or_no_images-20130604-2349.xml
          93 kB
          Pierre Pichet
        5. questions-T-26511-Default for Mis.xml
          46 kB
          Pierre Pichet
        1. all_images.jpg
          202 kB
        2. Menu_in-line.jpg
          249 kB
        3. screenshot-1.jpg
          75 kB
        4. screenshot-2.jpg
          132 kB
        5. screenshot-3.jpg
          68 kB

          Issue Links

            Activity

            Hide
            ppichet Pierre Pichet added a comment - - edited

            Tim
            The problem is the following.
            If I create a Cloze question with one image in one multiple choice option and put in the question text.
            The database will contain
            question id 1, question text
            <p> </p>
            <p>Cette question comporte du texte dans lequel on a intégré une première <img title="rr" src="http://www.moodle.uqam.ca/coursv2/file.php/197/irose.jpg" border="0" alt="rr" hspace="0" vspace="0" width="80" height="60" /> sous-question à choix multiples

            {#1}

            ;</p>

            then a second child question 2(parent id 1 ) questiontext (in the actual code the child question is not considered as HTML)

            {1:MULTICHOICE_V:<img style="margin: 0px; border: 0pt none;" title="rr" src="http://132.208.141.198/moodlehg/draftfile.php/13/user/draft/708801269/IORANGE.jpg" alt="rr" width="80" height="60" />#Feedback pour cette mauvaise réponse~Une autre mauvaise réponse#Feedback pour cette autre mauvaise réponse~=Bonne réponse#Feedback pour la bonne réponse~%50%Réponse qui vaut la moitié des points#Feedback pour la question qui vaut la moitié des points}

            And finally an answer of child question 2 will contain (unconverted)

            <img style="margin: 0px; border: 0pt none;" title="rr" src="http://132.208.141.198/moodlehg/draftfile.php/13/user/draft/708801269/IORANGE.jpg" alt="rr" width="80" height="60" />

            If I implement the actual code 1 IORANGE.jpg reference in question_files table will be in 'question' 'questiontext' and a second one will be in 'question' 'answertext' with the answer id.

            The two in the same context.

            Here I understand that I don't understand sufficiently this hash structure to know if there will be only 1 copy (user/draft) or 3 copies (user/draft, question/questiontext and one question/answertext) of IORANGE.jpg in the moodledata.

            Show
            ppichet Pierre Pichet added a comment - - edited Tim The problem is the following. If I create a Cloze question with one image in one multiple choice option and put in the question text. The database will contain question id 1, question text <p> </p> <p>Cette question comporte du texte dans lequel on a intégré une première <img title="rr" src="http://www.moodle.uqam.ca/coursv2/file.php/197/irose.jpg" border="0" alt="rr" hspace="0" vspace="0" width="80" height="60" /> sous-question à choix multiples {#1} ;</p> then a second child question 2(parent id 1 ) questiontext (in the actual code the child question is not considered as HTML) {1:MULTICHOICE_V:<img style="margin: 0px; border: 0pt none;" title="rr" src="http://132.208.141.198/moodlehg/draftfile.php/13/user/draft/708801269/IORANGE.jpg" alt="rr" width="80" height="60" />#Feedback pour cette mauvaise réponse~Une autre mauvaise réponse#Feedback pour cette autre mauvaise réponse~=Bonne réponse#Feedback pour la bonne réponse~%50%Réponse qui vaut la moitié des points#Feedback pour la question qui vaut la moitié des points} And finally an answer of child question 2 will contain (unconverted) <img style="margin: 0px; border: 0pt none;" title="rr" src="http://132.208.141.198/moodlehg/draftfile.php/13/user/draft/708801269/IORANGE.jpg" alt="rr" width="80" height="60" /> If I implement the actual code 1 IORANGE.jpg reference in question_files table will be in 'question' 'questiontext' and a second one will be in 'question' 'answertext' with the answer id. The two in the same context. Here I understand that I don't understand sufficiently this hash structure to know if there will be only 1 copy (user/draft) or 3 copies (user/draft, question/questiontext and one question/answertext) of IORANGE.jpg in the moodledata.
            Hide
            ppichet Pierre Pichet added a comment -

            Some time later...
            I understand that there is only one copy of the file of a given hash name whatever name is used to identify it in the code (i.e. the name given when the file (i.e. ivert.jpg ) was include using the API file interface.

            If the hash is '6155eeb0b04b95444b876f53dd9611c786dba061'
            it will be stored as moodledata/filedir/61/55/6155eeb0b04b95444b876f53dd9611c786dba061

            the records files data for this can be something like

            25, '6155eeb0b04b95444b876f53dd9611c786dba061', '62260892500cce17ca1a35ff866926ad63d169bc', 21, 'question', 'questiontext', 16, '/', 'ivert.jpg', , 8818, 'image/jpeg', 0, '', '', '', 1297870972, 1297870972, 0

            35, '6155eeb0b04b95444b876f53dd9611c786dba061', '7e4243b4f42fd487c068b3b1943c0ad839d97b2a', 13, 'user', 'draft', 23834229, '/', 'iivert', 2, 8818, 'document/unknown', 0, '', 'Admin User', 'allrightsreserved', 1297913037, 1297913037, 0

            41, '6155eeb0b04b95444b876f53dd9611c786dba061', 'd0935e55d39d0a371cbc9fab9349b569843549e8', 37, 'question', 'answerfeedback', 62, '/', 'iivert', 2, 8818, 'document/unknown', 0, '', 'Admin User', 'allrightsreserved', 1297913037, 1297913037, 0

            So let's proceed although my comment on MDL-24594 on 'answertext' and answer['text'] remained before going further.

            Show
            ppichet Pierre Pichet added a comment - Some time later... I understand that there is only one copy of the file of a given hash name whatever name is used to identify it in the code (i.e. the name given when the file (i.e. ivert.jpg ) was include using the API file interface. If the hash is '6155eeb0b04b95444b876f53dd9611c786dba061' it will be stored as moodledata/filedir/61/55/6155eeb0b04b95444b876f53dd9611c786dba061 the records files data for this can be something like 25, '6155eeb0b04b95444b876f53dd9611c786dba061', '62260892500cce17ca1a35ff866926ad63d169bc', 21, 'question', 'questiontext', 16, '/', 'ivert.jpg', , 8818, 'image/jpeg', 0, '', '', '', 1297870972, 1297870972, 0 35, '6155eeb0b04b95444b876f53dd9611c786dba061', '7e4243b4f42fd487c068b3b1943c0ad839d97b2a', 13, 'user', 'draft', 23834229, '/', 'iivert', 2, 8818, 'document/unknown', 0, '', 'Admin User', 'allrightsreserved', 1297913037, 1297913037, 0 41, '6155eeb0b04b95444b876f53dd9611c786dba061', 'd0935e55d39d0a371cbc9fab9349b569843549e8', 37, 'question', 'answerfeedback', 62, '/', 'iivert', 2, 8818, 'document/unknown', 0, '', 'Admin User', 'allrightsreserved', 1297913037, 1297913037, 0 So let's proceed although my comment on MDL-24594 on 'answertext' and answer ['text'] remained before going further.
            Hide
            timhunt Tim Hunt added a comment -

            Hmm. Yes. That is tricky. Not sure of the best solution at the moment.

            Is it possible to just keep all the files in the questiontext file area for the multianswer question? That might be the simplest thing, because it corresponds most closely with what happens on the editing form.

            Show
            timhunt Tim Hunt added a comment - Hmm. Yes. That is tricky. Not sure of the best solution at the moment. Is it possible to just keep all the files in the questiontext file area for the multianswer question? That might be the simplest thing, because it corresponds most closely with what happens on the editing form.
            Hide
            ppichet Pierre Pichet added a comment -

            "all the files in the questiontext file area"
            This was my solution before reading your comments.
            This will also simplify the import-export process.
            and the print_question_display that produced the answer display is the multianswer/questiontype.php function so we can rebuild the answers with the correct settings.

            So back to work after some exams "manual" grading.

            Show
            ppichet Pierre Pichet added a comment - "all the files in the questiontext file area" This was my solution before reading your comments. This will also simplify the import-export process. and the print_question_display that produced the answer display is the multianswer/questiontype.php function so we can rebuild the answers with the correct settings. So back to work after some exams "manual" grading.
            Hide
            ppichet Pierre Pichet added a comment -

            Finaaly I try to use a more standard method i.e. using the multiplechoice/questiontype.php to handle the coding of much of the coding with the exceptiob of the print and display.
            I have a partial succes i.e. the images a saved correclty but their display is only partial.
            This seems to be conncted to the
            $a->text = quiz_rewrite_question_urls($mcanswer->answer, 'pluginfile.php', $context->id, 'question', 'answertext',array( $state->attempt ,$state->question ), $mcanswer->id);
            and more specifically to the array( $state->attempt ,$state->question ), parameters which I need to explore the code before going further.

            This bug seems like as exam on the new file API

            Show
            ppichet Pierre Pichet added a comment - Finaaly I try to use a more standard method i.e. using the multiplechoice/questiontype.php to handle the coding of much of the coding with the exceptiob of the print and display. I have a partial succes i.e. the images a saved correclty but their display is only partial. This seems to be conncted to the $a->text = quiz_rewrite_question_urls($mcanswer->answer, 'pluginfile.php', $context->id, 'question', 'answertext',array( $state->attempt ,$state->question ), $mcanswer->id); and more specifically to the array( $state->attempt ,$state->question ), parameters which I need to explore the code before going further. This bug seems like as exam on the new file API
            Hide
            ppichet Pierre Pichet added a comment -

            The answers and feedback image show correctly.
            I add a not so restrictive condition to file access.
            function check_file_access($question, $state, $options, $contextid, $component,
            $filearea, $args) {
            if ($component == 'question' && $filearea == 'answerfeedback')

            { return $options->feedback ; }


            else if ($component == 'question' && $filearea == 'answertext')

            { return true ;// array_key_exists($itemid,$question->options->questions); }

            else

            { return parent::check_file_access($question, $state, $options, $contextid, $component, $filearea, $args); }

            }
            As I cannot easily use the answer->id, at least I could change it to test the question->id like questiontext.

            Show
            ppichet Pierre Pichet added a comment - The answers and feedback image show correctly. I add a not so restrictive condition to file access. function check_file_access($question, $state, $options, $contextid, $component, $filearea, $args) { if ($component == 'question' && $filearea == 'answerfeedback') { return $options->feedback ; } else if ($component == 'question' && $filearea == 'answertext') { return true ;// array_key_exists($itemid,$question->options->questions); } else { return parent::check_file_access($question, $state, $options, $contextid, $component, $filearea, $args); } } As I cannot easily use the answer->id, at least I could change it to test the question->id like questiontext.
            Hide
            ppichet Pierre Pichet added a comment -

            Tim,
            Things are going nicely.
            However I have a problem when I move the questions in another context as the questionlib
            question_move_questions_to_category although it moves the question files do not move the subquestion files even if it changes the category of the subquestions.
            I need to add the following code to transfer correctly the images of the sub multichoice.

            function question_move_questions_to_category($questionids, $newcategoryid) {
                global $DB, $QTYPES;
             
                $newcontextid = $DB->get_field('question_categories', 'contextid',
                        array('id' => $newcategoryid));
                list($questionidcondition, $params) = $DB->get_in_or_equal($questionids);
                $questions = $DB->get_records_sql("
                        SELECT q.id, q.qtype, qc.contextid
                          FROM {question} q
                          JOIN {question_categories} qc ON q.category = qc.id
                         WHERE  q.id $questionidcondition ", $params);
                foreach ($questions as $question) {
                    if ($newcontextid != $question->contextid) {
                        $QTYPES[$question->qtype]->move_files($question->id,
                                $question->contextid, $newcontextid);        
                        $subquestions = $DB->get_records_sql("
                        SELECT q.id, q.qtype, qc.contextid
                              FROM {question} q
                              JOIN {question_categories} qc ON q.category = qc.id
                             WHERE  q.parent $questionidcondition ", $params);         
                        foreach ($subquestions as $question) {            
                        if ($newcontextid != $question->contextid) {
                            $QTYPES[$question->qtype]->move_files($question->id,
                                    $question->contextid, $newcontextid);
                        }
                    }        
                }
             
                // Move the questions themselves.
                $DB->set_field_select('question', 'category', $newcategoryid, "id $questionidcondition", $params);
             
                // Move any subquestions belonging to them.
                $DB->set_field_select('question', 'category', $newcategoryid, "parent $questionidcondition", $params);
             
                // TODO Deal with datasets.
             
                return true;

            Any comment on the code will be appreciate.
            This remind me that I have also an open bug on datasets

            Show
            ppichet Pierre Pichet added a comment - Tim, Things are going nicely. However I have a problem when I move the questions in another context as the questionlib question_move_questions_to_category although it moves the question files do not move the subquestion files even if it changes the category of the subquestions. I need to add the following code to transfer correctly the images of the sub multichoice. function question_move_questions_to_category($questionids, $newcategoryid) { global $DB, $QTYPES;   $newcontextid = $DB->get_field('question_categories', 'contextid', array('id' => $newcategoryid)); list($questionidcondition, $params) = $DB->get_in_or_equal($questionids); $questions = $DB->get_records_sql(" SELECT q.id, q.qtype, qc.contextid FROM {question} q JOIN {question_categories} qc ON q.category = qc.id WHERE q.id $questionidcondition ", $params); foreach ($questions as $question) { if ($newcontextid != $question->contextid) { $QTYPES[$question->qtype]->move_files($question->id, $question->contextid, $newcontextid); $subquestions = $DB->get_records_sql(" SELECT q.id, q.qtype, qc.contextid FROM {question} q JOIN {question_categories} qc ON q.category = qc.id WHERE q.parent $questionidcondition ", $params); foreach ($subquestions as $question) { if ($newcontextid != $question->contextid) { $QTYPES[$question->qtype]->move_files($question->id, $question->contextid, $newcontextid); } } }   // Move the questions themselves. $DB->set_field_select('question', 'category', $newcategoryid, "id $questionidcondition", $params);   // Move any subquestions belonging to them. $DB->set_field_select('question', 'category', $newcategoryid, "parent $questionidcondition", $params);   // TODO Deal with datasets.   return true; Any comment on the code will be appreciate. This remind me that I have also an open bug on datasets
            Hide
            timhunt Tim Hunt added a comment -

            I don't think that code is right. I think that instead we should either:

            1. Put code like that inside the qtype_multianswer::move_files method; or

            2. Change the first query in the function so that it does

            $questions = $DB->get_records_sql("
                        SELECT q.id, q.qtype, qc.contextid
                          FROM {question} q
                          JOIN {question_categories} qc ON q.category = qc.id
                         WHERE  q.id $questionidcondition OR q.parent $questionidcondition", array_merge($params, $params));

            and then you don't need to change the foreach loop at all

            Actually, 2. is right, because it is consistent with the two $DB->set_field_select calls that come afterwards.

            Show
            timhunt Tim Hunt added a comment - I don't think that code is right. I think that instead we should either: 1. Put code like that inside the qtype_multianswer::move_files method; or 2. Change the first query in the function so that it does $questions = $DB->get_records_sql( " SELECT q.id, q.qtype, qc.contextid FROM {question} q JOIN {question_categories} qc ON q.category = qc.id WHERE q.id $questionidcondition OR q.parent $questionidcondition" , array_merge($params, $params)); and then you don't need to change the foreach loop at all Actually, 2. is right, because it is consistent with the two $DB->set_field_select calls that come afterwards.
            Hide
            ppichet Pierre Pichet added a comment -

            " because it is consistent with the two $DB->set_field_select calls that come afterwards."

            Solution 1 to put the code inside the questiontype seems to me more in-line with the plug-in philosophy that a question data should not be done "externally" unless we are sure that this does not broke anything unless we set restrictions on questiontype features which will be difficult before finishing the new engine code.

            Solution 2 setting of category id means that questions should be completely independent of the category setting which is not a "written constraint".

            Personnaly I prefer solution 1 which could mean to also change the way we modified the category setting by a call to a questiontype function.

            Using option 1 or 2 will modify the way I will build the move dataset code.

            Show
            ppichet Pierre Pichet added a comment - " because it is consistent with the two $DB->set_field_select calls that come afterwards." Solution 1 to put the code inside the questiontype seems to me more in-line with the plug-in philosophy that a question data should not be done "externally" unless we are sure that this does not broke anything unless we set restrictions on questiontype features which will be difficult before finishing the new engine code. Solution 2 setting of category id means that questions should be completely independent of the category setting which is not a "written constraint". Personnaly I prefer solution 1 which could mean to also change the way we modified the category setting by a call to a questiontype function. Using option 1 or 2 will modify the way I will build the move dataset code.
            Hide
            ppichet Pierre Pichet added a comment -

            Test moving with your suggestion, works correctly (as usual ).

            Backup and restore seems working also.

            So need code cleaning, other testing and time to do it, So early next week ...

            Show
            ppichet Pierre Pichet added a comment - Test moving with your suggestion, works correctly (as usual ). Backup and restore seems working also. So need code cleaning, other testing and time to do it, So early next week ...
            Hide
            ppichet Pierre Pichet added a comment -

            My more complex database code was the less complex code to apply and to test at 11H30 PM

            Show
            ppichet Pierre Pichet added a comment - My more complex database code was the less complex code to apply and to test at 11H30 PM
            Hide
            ppichet Pierre Pichet added a comment -

            It won't be early this week
            I have succeeded in XML import export BUT discover that the files table was full on non-used files.

            I am modifying the decode-saving process of the main question and subquestions to include at the rigth place the file saving for subquestion, answers and feedback of the images that are all in the main questiontext.
            I have a first solution for xml import answertext where files already exists (i.e.['files'] is not empty that should work for editing where ['itemid'] is not empty..

            Show
            ppichet Pierre Pichet added a comment - It won't be early this week I have succeeded in XML import export BUT discover that the files table was full on non-used files. I am modifying the decode-saving process of the main question and subquestions to include at the rigth place the file saving for subquestion, answers and feedback of the images that are all in the main questiontext. I have a first solution for xml import answertext where files already exists (i.e. ['files'] is not empty that should work for editing where ['itemid'] is not empty..
            Hide
            ppichet Pierre Pichet added a comment -

            The code flow is that the satus of either HTML with files should be set by the embedded_cloze_qtype->qtype_multianswer_extract_question() function which return the parameters (like answer) either as an array [text][forma][itemid] or as a non array answer.

            The embedded_cloze_qtype->save_question and embedded_cloze_qtype->save_question_options or the format->import applies to each components of the subquestions the right data [itemid] or [files] before saving them.
            In format import this gives something like this as files already exist

                    foreach($qo->options->questions as $wrapped) {
                        foreach($wrapped->answer as $key => $answer) {
                            if(is_array($answer)){
                                foreach($qo->questiontextfiles as $file ){
                                    if(preg_match('/(@@PLUGINFILE@@\/'.$file->name.')/',$answer['text'])){
                                        $wrapped->answer[$key]['files'][]=$file ;
                                }
                            }
                        }
                    }

            This should be the same in embedded_cloze_qtype->save_question_options...
            More news later

            Show
            ppichet Pierre Pichet added a comment - The code flow is that the satus of either HTML with files should be set by the embedded_cloze_qtype->qtype_multianswer_extract_question() function which return the parameters (like answer) either as an array [text] [forma] [itemid] or as a non array answer. The embedded_cloze_qtype->save_question and embedded_cloze_qtype->save_question_options or the format->import applies to each components of the subquestions the right data [itemid] or [files] before saving them. In format import this gives something like this as files already exist foreach($qo->options->questions as $wrapped) { foreach($wrapped->answer as $key => $answer) { if(is_array($answer)){ foreach($qo->questiontextfiles as $file ){ if(preg_match('/(@@PLUGINFILE@@\/'.$file->name.')/',$answer['text'])){ $wrapped->answer[$key]['files'][]=$file ; } } } } This should be the same in embedded_cloze_qtype->save_question_options... More news later
            Hide
            ppichet Pierre Pichet added a comment -

            Tim,
            The questiontype->move_files() move all files in one context to another one.
            What I need is a function to move some of the files already in question questiontext to the specific context where they will be used.
            This is not really moving i.e. no delete in main multianswer question question text but create a new link in for example question answertext or question feedbacktext.

            I could retrieve the files form the question questiontext area and apply them to each answer element that used them as I do on import but is there a more simple way.

            In file storage lib the function seems to always transfer all the files from one context to the other.

            I understand that in this case as we will need somewhere in the multianswer all the files in the multianswer draft area that were all moved when saving in the quuestion questiontext, there should be no delete when "virtually" moving the files in one or more contexts.

            Show
            ppichet Pierre Pichet added a comment - Tim, The questiontype->move_files() move all files in one context to another one. What I need is a function to move some of the files already in question questiontext to the specific context where they will be used. This is not really moving i.e. no delete in main multianswer question question text but create a new link in for example question answertext or question feedbacktext. I could retrieve the files form the question questiontext area and apply them to each answer element that used them as I do on import but is there a more simple way. In file storage lib the function seems to always transfer all the files from one context to the other. I understand that in this case as we will need somewhere in the multianswer all the files in the multianswer draft area that were all moved when saving in the quuestion questiontext, there should be no delete when "virtually" moving the files in one or more contexts.
            Hide
            ppichet Pierre Pichet added a comment -

            Sorry if the precedent comment is not as clear as it should be

            Show
            ppichet Pierre Pichet added a comment - Sorry if the precedent comment is not as clear as it should be
            Hide
            ppichet Pierre Pichet added a comment -

            Or I use the actual movefiles function as this is just supplementary records in "files" database table.

            Show
            ppichet Pierre Pichet added a comment - Or I use the actual movefiles function as this is just supplementary records in "files" database table.
            Hide
            ppichet Pierre Pichet added a comment -

            TIm
            I have a code solution that can create using the edit_multianswer_form or xml format, a multianswer question where multiple choice vertical or horizontal answers can include images.
            The answerfeedback can use image for all question types although the image cannot be shown in the actual pop-up window (this is a specific bug ).
            The actual code repeat all the files in each area (question questiontext, question answertext and question feedbacktext) if there are images in the coresponding answer or feedback.
            This create more records in the files table than necessary.
            However when the question is deleted, all those records are deleted.

            Si I can
            1. finish testing and clean the code of all print_r etc and commit as it is now.
            2. solve the file handling then commit but inactivate the images in feedback for question types other than MCV or MCH
            3. solve the file handling and the feedback pop-up and and commit a "completely" finish solution
            4. or ...

            Show
            ppichet Pierre Pichet added a comment - TIm I have a code solution that can create using the edit_multianswer_form or xml format, a multianswer question where multiple choice vertical or horizontal answers can include images. The answerfeedback can use image for all question types although the image cannot be shown in the actual pop-up window (this is a specific bug ). The actual code repeat all the files in each area (question questiontext, question answertext and question feedbacktext) if there are images in the coresponding answer or feedback. This create more records in the files table than necessary. However when the question is deleted, all those records are deleted. Si I can 1. finish testing and clean the code of all print_r etc and commit as it is now. 2. solve the file handling then commit but inactivate the images in feedback for question types other than MCV or MCH 3. solve the file handling and the feedback pop-up and and commit a "completely" finish solution 4. or ...
            Hide
            ppichet Pierre Pichet added a comment -

            AS this is an improvement and not a bug, I will take the time necessary to handle moving individual file from one context to another (not real moving in the new file API) so to limit the number of records in files table.
            More news later

            Show
            ppichet Pierre Pichet added a comment - AS this is an improvement and not a bug, I will take the time necessary to handle moving individual file from one context to another (not real moving in the new file API) so to limit the number of records in files table. More news later
            Hide
            ppichet Pierre Pichet added a comment -

            Tim,
            I think that the more useable way to do this is to use the images in the draft_area item for questiontext as I have done in the different areas related to either question answertext and question feedbacktext.
            This will give us more records in th files table but the code will be safe...
            This is more or less the solution that is suggested in filelib.php

            function file_save_draft_area_files($draftitemid, $contextid, $component, $filearea, $itemid, array $options=null, $text=null, $forcehttps=false) {

            ...
            line 714

            // note: do not purge the draft area - we clean up areas later in cron,
            // the reason is that user might press submit twice and they would loose the files,
            // also sometimes we might want to use hacks that save files into two different areas

            So I clean the code, do more tests and prepare the commit

            Show
            ppichet Pierre Pichet added a comment - Tim, I think that the more useable way to do this is to use the images in the draft_area item for questiontext as I have done in the different areas related to either question answertext and question feedbacktext. This will give us more records in th files table but the code will be safe... This is more or less the solution that is suggested in filelib.php function file_save_draft_area_files($draftitemid, $contextid, $component, $filearea, $itemid, array $options=null, $text=null, $forcehttps=false) { ... line 714 // note: do not purge the draft area - we clean up areas later in cron, // the reason is that user might press submit twice and they would loose the files, // also sometimes we might want to use hacks that save files into two different areas So I clean the code, do more tests and prepare the commit
            Hide
            timhunt Tim Hunt added a comment -

            That sounds like a reasonable plan. I look forwards to reviewing the code.

            Show
            timhunt Tim Hunt added a comment - That sounds like a reasonable plan. I look forwards to reviewing the code.
            Hide
            ppichet Pierre Pichet added a comment -

            The images can also been shown on pop-up feedback for all question types (i.e multiplechoice select, shortaswer and numerical) as in 1.9.

            Show
            ppichet Pierre Pichet added a comment - The images can also been shown on pop-up feedback for all question types (i.e multiplechoice select, shortaswer and numerical) as in 1.9.
            Hide
            ppichet Pierre Pichet added a comment -

            I should be able to put everything on commits before the weekend.

            Show
            ppichet Pierre Pichet added a comment - I should be able to put everything on commits before the weekend.
            Hide
            timhunt Tim Hunt added a comment -

            That would be great. I can't promise to review it this weekend, or indeed before next Wednesday, but I will try to do it shortly after that.

            Show
            timhunt Tim Hunt added a comment - That would be great. I can't promise to review it this weekend, or indeed before next Wednesday, but I will try to do it shortly after that.
            Hide
            ppichet Pierre Pichet added a comment -

            OK let say that I postpone my delay to before next Wednesday so I can do more testing.

            Show
            ppichet Pierre Pichet added a comment - OK let say that I postpone my delay to before next Wednesday so I can do more testing.
            Hide
            ppichet Pierre Pichet added a comment -

            The commit
            https://github.com/ppichet/moodle/commit/a6deba0ebb4ff168a4ea737bf37af118a42f462a

            seems to contain everything for images in Cloze vertical and horizontal dispaly as in feedback of all the question types multiplechoice variants, shortanswer and numerical.

            I have test by creating a question using all these types.
            Editing the question as a new one.
            Moving the questions in another context
            Exporting the questions in XML format
            Importing them in XML format (incidently the files from 1,9 do not stored the images with the exception of the question image.)
            Previewing, Putting the questions in a quiz.
            Doing attempt as a student.
            Making a backup of the course mostly the quiz and attampts.
            Doing a restore to another course
            Testing that the attempt data was good

            In all these tests the images follow
            Testing that deleting the question, delete all the files records related to this question as the question and answers related records.

            So I let you continue ...

            Show
            ppichet Pierre Pichet added a comment - The commit https://github.com/ppichet/moodle/commit/a6deba0ebb4ff168a4ea737bf37af118a42f462a seems to contain everything for images in Cloze vertical and horizontal dispaly as in feedback of all the question types multiplechoice variants, shortanswer and numerical. I have test by creating a question using all these types. Editing the question as a new one. Moving the questions in another context Exporting the questions in XML format Importing them in XML format (incidently the files from 1,9 do not stored the images with the exception of the question image.) Previewing, Putting the questions in a quiz. Doing attempt as a student. Making a backup of the course mostly the quiz and attampts. Doing a restore to another course Testing that the attempt data was good In all these tests the images follow Testing that deleting the question, delete all the files records related to this question as the question and answers related records. So I let you continue ...
            Hide
            ppichet Pierre Pichet added a comment -

            xml file for export import tests

            Show
            ppichet Pierre Pichet added a comment - xml file for export import tests
            Hide
            ppichet Pierre Pichet added a comment -

            The main question used to test

            Show
            ppichet Pierre Pichet added a comment - The main question used to test
            Hide
            instructoit UMass Amherst Instructional Media Lab added a comment -

            Is there any further progress on this "improvement"? We agree with Respondus that this feels more like a bug than an improvement: we have several Cloze questions with images in them, but we are unable to export them or backup the questions because the images get dropped from the Moodle XML file format. This means that once we add an image to a Cloze question, we cannot subsequently move that question from one course to another course – backing up and restoring the course also does not work because the images disappear in the restored course.

            I am attaching a Moodle XML file containing the code for a simple Cloze question with an inlined image. The image data is missing from the XML file, so when you import this XML file into another course the image is broken.

            Show
            instructoit UMass Amherst Instructional Media Lab added a comment - Is there any further progress on this "improvement"? We agree with Respondus that this feels more like a bug than an improvement: we have several Cloze questions with images in them, but we are unable to export them or backup the questions because the images get dropped from the Moodle XML file format. This means that once we add an image to a Cloze question, we cannot subsequently move that question from one course to another course – backing up and restoring the course also does not work because the images disappear in the restored course. I am attaching a Moodle XML file containing the code for a simple Cloze question with an inlined image. The image data is missing from the XML file, so when you import this XML file into another course the image is broken.
            Hide
            instructoit UMass Amherst Instructional Media Lab added a comment -

            Cloze q with image, but image data is missing from the exported Moodle XML file.

            Show
            instructoit UMass Amherst Instructional Media Lab added a comment - Cloze q with image, but image data is missing from the exported Moodle XML file.
            Hide
            ppichet Pierre Pichet added a comment -

            Tim,
            I understand that you have a quite heavy case load for the following days .

            Next week I am back on Moodle and UQAM autumn courses and I will review my march proposal as code have changed even recently for multianswer and also in the import, backup and restore process.
            Hopefully this sould be available in the first days of september.

            Show
            ppichet Pierre Pichet added a comment - Tim, I understand that you have a quite heavy case load for the following days . Next week I am back on Moodle and UQAM autumn courses and I will review my march proposal as code have changed even recently for multianswer and also in the import, backup and restore process. Hopefully this sould be available in the first days of september.
            Hide
            timhunt Tim Hunt added a comment -

            OK. I am on holiday 2nd to 9th September, so this will probably have to wait until I get back.

            Show
            timhunt Tim Hunt added a comment - OK. I am on holiday 2nd to 9th September, so this will probably have to wait until I get back.
            Hide
            timhunt Tim Hunt added a comment -

            This bug is marked 'Waiting for peer review, but I cannot immediately see what I am meant to be reviewing.

            Show
            timhunt Tim Hunt added a comment - This bug is marked 'Waiting for peer review, but I cannot immediately see what I am meant to be reviewing.
            Hide
            instructoit UMass Amherst Instructional Media Lab added a comment -

            Hi Tim, Pierre,

            Is there any progress on this issue? We are about to migrate from 2.0.5 to 2.1.2 and we see that this is still a bug in 2.1.2. As we mention above, we have several faculty clients who have quizzes on 2.0.5 that have images in their cloze question types, and this issue makes it impossible for them to move their quiz questions from one course to another course.

            Thanks! Take care,
            hari
            Instructional Media Lab
            UMass Amherst

            Show
            instructoit UMass Amherst Instructional Media Lab added a comment - Hi Tim, Pierre, Is there any progress on this issue? We are about to migrate from 2.0.5 to 2.1.2 and we see that this is still a bug in 2.1.2. As we mention above, we have several faculty clients who have quizzes on 2.0.5 that have images in their cloze question types, and this issue makes it impossible for them to move their quiz questions from one course to another course. Thanks! Take care, hari Instructional Media Lab UMass Amherst
            Hide
            ppichet Pierre Pichet added a comment -

            "We are about to migrate"
            What are your time schedule so that we could hopefully synchronize

            The solution
            https://github.com/ppichet/moodle/commit/a6deba0ebb4ff168a4ea737bf37af118a42f462a
            was working in march but needs to be merged with modifications and new testings as the master has changed since.

            Show
            ppichet Pierre Pichet added a comment - "We are about to migrate" What are your time schedule so that we could hopefully synchronize The solution https://github.com/ppichet/moodle/commit/a6deba0ebb4ff168a4ea737bf37af118a42f462a was working in march but needs to be merged with modifications and new testings as the master has changed since.
            Hide
            ppichet Pierre Pichet added a comment -

            Tim,
            Given the modifications related to the new engine, I should somehow apply the march code to 2,0 which imply the XML format then do the merge with the actual master.
            The images are already saved correctly in the questiontext.
            The problem is to save them again as part of the subquestions when decoding the question text.
            I will set two simultaneous master and 2,0 moodle site to be sure that the 2,0 code is optimized for merging in master.

            Show
            ppichet Pierre Pichet added a comment - Tim, Given the modifications related to the new engine, I should somehow apply the march code to 2,0 which imply the XML format then do the merge with the actual master. The images are already saved correctly in the questiontext. The problem is to save them again as part of the subquestions when decoding the question text. I will set two simultaneous master and 2,0 moodle site to be sure that the 2,0 code is optimized for merging in master.
            Hide
            instructoit UMass Amherst Instructional Media Lab added a comment -

            Hi Pierre,

            As for a timeline, we are currently evaluating 2.1.2 and deciding whether to move from 2.0.5 to 2.1 in January or over the summer of 2012. We have several courses on 2.0.5 that have images in their questions – some couple hundred of these questions are Embedded Answer (Cloze) type questions with images in the questions. These questions are currently breaking our ability to export/import quizzes from course to course, or even exporting/importing entire courses because the images get lost in transition.

            Is it possible for this fix to get into 2.1.3 and then backported to 2.1.2 and/or 2.0.5?

            Thanks Pierre and Tim! Take care,
            hari
            Instructional Media Lab
            UMass Amherst

            Show
            instructoit UMass Amherst Instructional Media Lab added a comment - Hi Pierre, As for a timeline, we are currently evaluating 2.1.2 and deciding whether to move from 2.0.5 to 2.1 in January or over the summer of 2012. We have several courses on 2.0.5 that have images in their questions – some couple hundred of these questions are Embedded Answer (Cloze) type questions with images in the questions. These questions are currently breaking our ability to export/import quizzes from course to course, or even exporting/importing entire courses because the images get lost in transition. Is it possible for this fix to get into 2.1.3 and then backported to 2.1.2 and/or 2.0.5? Thanks Pierre and Tim! Take care, hari Instructional Media Lab UMass Amherst
            Hide
            instructoit UMass Amherst Instructional Media Lab added a comment -

            Also Pierre, we tried to get your proposed solution from the github repository but there were way too many changes in that source code to integrate into our installation, especially since we already had several other patches in place for an Oracle-related bug that got fixed in 2.0.5.

            Show
            instructoit UMass Amherst Instructional Media Lab added a comment - Also Pierre, we tried to get your proposed solution from the github repository but there were way too many changes in that source code to integrate into our installation, especially since we already had several other patches in place for an Oracle-related bug that got fixed in 2.0.5.
            Hide
            ppichet Pierre Pichet added a comment -

            "there were way too many changes in that source code"
            This why I wrote "to be sure that the 2,0 code is optimized for merging in master"

            I cannot promise anything in January , however I have only one course in winter so this will be solved hopefully before spring.

            We discussed yesterday of our ( Université du Québec à Montréal) moodle comity of moving from 1,9 to at least 2,2 and we concluded that we will do it when everything will have been really tested given the complexity of the new question engine and the file api .

            "really tested" has a more precise meaning "OU tested"

            Given all the local modifications our calendar could be January 2013 for some courses and a general deployment in autumn 2013.

            Show
            ppichet Pierre Pichet added a comment - "there were way too many changes in that source code" This why I wrote "to be sure that the 2,0 code is optimized for merging in master" I cannot promise anything in January , however I have only one course in winter so this will be solved hopefully before spring. We discussed yesterday of our ( Université du Québec à Montréal) moodle comity of moving from 1,9 to at least 2,2 and we concluded that we will do it when everything will have been really tested given the complexity of the new question engine and the file api . "really tested" has a more precise meaning "OU tested" Given all the local modifications our calendar could be January 2013 for some courses and a general deployment in autumn 2013.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            Just a comment that I have started to merge with actual master.
            first tests are successfull.
            1. Images can stored in main question text , multichoice answers and feedbacks.
            2. questions preview correctly
            3. questions can be moved to other categories
            4. backup and restore quiz from courses tested OK
            5. next: xml import export

            Show
            ppichet Pierre Pichet added a comment - - edited Just a comment that I have started to merge with actual master. first tests are successfull. 1. Images can stored in main question text , multichoice answers and feedbacks. 2. questions preview correctly 3. questions can be moved to other categories 4. backup and restore quiz from courses tested OK 5. next: xml import export
            Hide
            timhunt Tim Hunt added a comment -

            Thanks for the update. Sounds like it is going well. Do you think you will have time to peer-review MDL-30322?

            Show
            timhunt Tim Hunt added a comment - Thanks for the update. Sounds like it is going well. Do you think you will have time to peer-review MDL-30322 ?
            Hide
            ppichet Pierre Pichet added a comment -

            Import-export seems OK on up-to-date 2,2.
            More tests from 1,9 to 2,2 and between different installations are needed.

            Show
            ppichet Pierre Pichet added a comment - Import-export seems OK on up-to-date 2,2. More tests from 1,9 to 2,2 and between different installations are needed.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            Tim,
            working on details.
            From 2,0 the elements (i.e. questiontiontext, feedback ) are defined as an array and the [format] has been defined as FORMAT_HTML.
            each wrapped questiontext is a fraction of the main questiontext so I proposed to set as

                    $wrapped->generalfeedback['text'] = '';
                    $wrapped->generalfeedback['format'] = FORMAT_HTML;
                    $wrapped->generalfeedback['itemid'] = '';
                    $wrapped->questiontext['text'] = $answerregs[0];
                    $wrapped->questiontext['format'] = $question->questiontext['format'];
                    $wrapped->questiontext['itemid'] = $question->questiontext['itemid'];

            Using FORMAT_HTML or the questiontext['format'] are equivalent for 2,0 as the html editor is always set.
            But this could be different on xml import ($qo->questiontextformat).

            Show
            ppichet Pierre Pichet added a comment - - edited Tim, working on details. From 2,0 the elements (i.e. questiontiontext, feedback ) are defined as an array and the [format] has been defined as FORMAT_HTML. each wrapped questiontext is a fraction of the main questiontext so I proposed to set as $wrapped->generalfeedback['text'] = ''; $wrapped->generalfeedback['format'] = FORMAT_HTML; $wrapped->generalfeedback['itemid'] = ''; $wrapped->questiontext['text'] = $answerregs[0]; $wrapped->questiontext['format'] = $question->questiontext['format']; $wrapped->questiontext['itemid'] = $question->questiontext['itemid']; Using FORMAT_HTML or the questiontext ['format'] are equivalent for 2,0 as the html editor is always set. But this could be different on xml import ($qo->questiontextformat).
            Hide
            ppichet Pierre Pichet added a comment -

            I have push a new version rebase today at

            https://github.com/ppichet/moodle/tree/MDL-26511-2

            Seems to work correctly although there are so many options to test.

            Doing the testing, I noticed some minor features with numerical tolerance that can be set and applied to each individual answers since at least 1,9 although in the older versions, the first tolerance was applied to each answers.

            I will identify precisely and set the bug on return from a 2 weeks holiday in Dominican Republic ( I wish you could do something similar ) starting next sunday.

            My teaching load in winter will be lower so I will be able to increase my Moodle one.

            Show
            ppichet Pierre Pichet added a comment - I have push a new version rebase today at https://github.com/ppichet/moodle/tree/MDL-26511-2 Seems to work correctly although there are so many options to test. Doing the testing, I noticed some minor features with numerical tolerance that can be set and applied to each individual answers since at least 1,9 although in the older versions, the first tolerance was applied to each answers. I will identify precisely and set the bug on return from a 2 weeks holiday in Dominican Republic ( I wish you could do something similar ) starting next sunday. My teaching load in winter will be lower so I will be able to increase my Moodle one.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            Bad news.
            The import code from 1,9 is not working properly because the 1,9 export does not work for
            images references in the question text.

            Having completed the import-export for 2,0, I just tested if it works with 1,9 export.

            As usual it was somehow too late in the night ;( and I did not realize that I was using the same Firefox to do the 2 jobs so I had access to the files in the 1,9 versions...

            So although the 1,9 cloze code allow you to insert images in all parts of the subquestions, this work only if you stay in the same course or can access it when logged.

            We should then consider the MDL-26511 is specific to 2,0 and does not solve the 1,9 cloze handling of images.

            I don't know how the migration code handle this.

            Show
            ppichet Pierre Pichet added a comment - - edited Bad news. The import code from 1,9 is not working properly because the 1,9 export does not work for images references in the question text. Having completed the import-export for 2,0, I just tested if it works with 1,9 export. As usual it was somehow too late in the night ;( and I did not realize that I was using the same Firefox to do the 2 jobs so I had access to the files in the 1,9 versions... So although the 1,9 cloze code allow you to insert images in all parts of the subquestions, this work only if you stay in the same course or can access it when logged. We should then consider the MDL-26511 is specific to 2,0 and does not solve the 1,9 cloze handling of images. I don't know how the migration code handle this.
            Hide
            timhunt Tim Hunt added a comment -

            A few quick comments:

            1. Please try to avoid whitespace changes like https://github.com/ppichet/moodle/commit/214ca33590e9e394d7d461923db4897fde0807f7#L0L584 - it makes it harder to see what has really changed.

            2. I assume question/type/multianswer/lib1.php should not exist, and that you will clean it up eventually.

            3. Instead of making this change: https://github.com/ppichet/moodle/commit/214ca33590e9e394d7d461923db4897fde0807f7#L4L208 would it be better to change the multianswer qtype to correctly initialise everything that the numerical qtype needs?

            Show
            timhunt Tim Hunt added a comment - A few quick comments: 1. Please try to avoid whitespace changes like https://github.com/ppichet/moodle/commit/214ca33590e9e394d7d461923db4897fde0807f7#L0L584 - it makes it harder to see what has really changed. 2. I assume question/type/multianswer/lib1.php should not exist, and that you will clean it up eventually. 3. Instead of making this change: https://github.com/ppichet/moodle/commit/214ca33590e9e394d7d461923db4897fde0807f7#L4L208 would it be better to change the multianswer qtype to correctly initialise everything that the numerical qtype needs?
            Hide
            ppichet Pierre Pichet added a comment - - edited

            1.Please try to avoid whitespace changes.
            I just use the code checker recommandations ...after adding your proposal.
            So next time, code changes first and adding here a note on code checker...

            2. This was set to ignore and I did not noticed that I badly select the file when doing the commit.

            3.Instead of making this change:..would it be better to change the multianswer qtype to correctly initialise everything that the numerical qtype needs?

            In principle you are right. However this means reworking somehow the big preg of multianswer and I did not have the time to do it correctly as we should also rework the handling of tolerance.
            Adding this supplementary test about a possible empty array is not a bad test by itself and it removes the notice send by the debugger that I encounter because of a specific more than one answer in the numerical subquestion that I use to test for the MDL-26511.

            In all cases, there are other minor problems with numerical on which I will have to work in january.

            I just did a force update to include the correct lib file.
            I expect that this was the thing to do...

            Show
            ppichet Pierre Pichet added a comment - - edited 1.Please try to avoid whitespace changes. I just use the code checker recommandations ...after adding your proposal. So next time, code changes first and adding here a note on code checker... 2. This was set to ignore and I did not noticed that I badly select the file when doing the commit. 3.Instead of making this change:..would it be better to change the multianswer qtype to correctly initialise everything that the numerical qtype needs? In principle you are right. However this means reworking somehow the big preg of multianswer and I did not have the time to do it correctly as we should also rework the handling of tolerance. Adding this supplementary test about a possible empty array is not a bad test by itself and it removes the notice send by the debugger that I encounter because of a specific more than one answer in the numerical subquestion that I use to test for the MDL-26511 . In all cases, there are other minor problems with numerical on which I will have to work in january. I just did a force update to include the correct lib file. I expect that this was the thing to do...
            Hide
            ppichet Pierre Pichet added a comment -

            The differences between this last version MDL-26511-2 and the MDL-26511 are not very important.
            It should be not too difficult to create the versions for the moodle 2,x versions where you want
            to apply this.
            However as there is no urgency, I could do in january the merging once this has been tested and put on master.

            Show
            ppichet Pierre Pichet added a comment - The differences between this last version MDL-26511 -2 and the MDL-26511 are not very important. It should be not too difficult to create the versions for the moodle 2,x versions where you want to apply this. However as there is no urgency, I could do in january the merging once this has been tested and put on master.
            Hide
            timhunt Tim Hunt added a comment -

            Removing the patch label, since there is not a current patch that needs peer review.

            Show
            timhunt Tim Hunt added a comment - Removing the patch label, since there is not a current patch that needs peer review.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Pierre,
            Just to warn you that MDL-37313 Moodle XML import format should use draft file areas, not arrays (currently waiting for integration) will have some consequences on this issue and will need that you rework part of your code.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Pierre, Just to warn you that MDL-37313 Moodle XML import format should use draft file areas, not arrays (currently waiting for integration) will have some consequences on this issue and will need that you rework part of your code.
            Hide
            ppichet Pierre Pichet added a comment -

            Thanks for the comment and on your work on moodle.
            This was a first draft when things were not correctly settled in the new file handling which is not the best thing in 2,0...

            We had a quite difficult situation in Québec universities the last semesters but things are back to a more normal state.
            So I should be able to solve this bug among other ones.

            Show
            ppichet Pierre Pichet added a comment - Thanks for the comment and on your work on moodle. This was a first draft when things were not correctly settled in the new file handling which is not the best thing in 2,0... We had a quite difficult situation in Québec universities the last semesters but things are back to a more normal state. So I should be able to solve this bug among other ones.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment - - edited

            Unfortunately I think that after the recent changes to the way Moodle XML qformat is handling files Pierre's code need important changes to work again
            But I wonder if we could return to using Tim's idea : "Is it possible to just keep all the files in the questiontext file area for the multianswer question? That might be the simplest thing, because it corresponds most closely with what happens on the editing form."
            Not only does this corresponds more closely to what happens on the editing form but I think this also simplify other places in the code.

            • In the renderer.php file all the renderers (including those of the subq) would use the same file area and itemid when calling format_text
            • during xml import/export it also match more closely what happen because all files are after the questiontext

            Of course maybe there are problems that I don't see, but it's worth exploring this idea.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - - edited Unfortunately I think that after the recent changes to the way Moodle XML qformat is handling files Pierre's code need important changes to work again But I wonder if we could return to using Tim's idea : "Is it possible to just keep all the files in the questiontext file area for the multianswer question? That might be the simplest thing, because it corresponds most closely with what happens on the editing form." Not only does this corresponds more closely to what happens on the editing form but I think this also simplify other places in the code. In the renderer.php file all the renderers (including those of the subq) would use the same file area and itemid when calling format_text during xml import/export it also match more closely what happen because all files are after the questiontext Of course maybe there are problems that I don't see, but it's worth exploring this idea.
            Hide
            ppichet Pierre Pichet added a comment -

            The solution I had last year was working before they modified all the way to handle files.
            Your excellent work on file import allow to settle in many ways how to handlefiles.
            My last year proposal have the advantage to put the images where the subquestions expect they are.
            This should be the main cloze code objective if we want to not duplicate all the subquestion code in cloze.

            My main spring semester courses are finished (exam today).
            I have things to fix in calculateds first and I see elsewhere that among other things your are exploring
            Open university new questiontype that could render obsolete Cloze.

            Given coding proficiency and your knowledge about file handling among other things, you should be be able to handle this more faster than me.
            So feel free to assign this to you.

            Show
            ppichet Pierre Pichet added a comment - The solution I had last year was working before they modified all the way to handle files. Your excellent work on file import allow to settle in many ways how to handlefiles. My last year proposal have the advantage to put the images where the subquestions expect they are. This should be the main cloze code objective if we want to not duplicate all the subquestion code in cloze. My main spring semester courses are finished (exam today). I have things to fix in calculateds first and I see elsewhere that among other things your are exploring Open university new questiontype that could render obsolete Cloze. Given coding proficiency and your knowledge about file handling among other things, you should be be able to handle this more faster than me. So feel free to assign this to you.
            Hide
            ppichet Pierre Pichet added a comment -

            We should not modify the question code plug-in philosophy because the file handling is badly designed but patch the
            file hanfling so that is allows plug-in.
            File handling remains by large the Moodle 2.0 bad case.

            Show
            ppichet Pierre Pichet added a comment - We should not modify the question code plug-in philosophy because the file handling is badly designed but patch the file hanfling so that is allows plug-in. File handling remains by large the Moodle 2.0 bad case.
            Hide
            ppichet Pierre Pichet added a comment -

            However I will try your patch on MDL-38880 and see how it can be used to solve the MDL-26511 case.

            Show
            ppichet Pierre Pichet added a comment - However I will try your patch on MDL-38880 and see how it can be used to solve the MDL-26511 case.
            Hide
            ppichet Pierre Pichet added a comment -

            Good news.
            Updating the old MDL-26511 to actual master is working quite well i.e. images can be used in multichoice answer and feeback.
            some trimmings like correct answer remain.
            So I am coming up on this bug.

            As a test I will insert MDL-38880 patch.

            Show
            ppichet Pierre Pichet added a comment - Good news. Updating the old MDL-26511 to actual master is working quite well i.e. images can be used in multichoice answer and feeback. some trimmings like correct answer remain. So I am coming up on this bug. As a test I will insert MDL-38880 patch.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Pierre,
            Looking at your code https://github.com/ppichet/moodle/commit/9200042144fd0e60d4f5ffdfa68fc4835f1affbc (I don't know if this is your latest version)
            I think it's easy to adapt your code to MDL-38880:

            • suppress all you changes to question/format/xml/format.php because they are no more necessary
            • in question/type/multianswer/questiontype.php just change 2 lines in the qtype_multianswer_extract_question function so that itemid is set for answers and answers feedback

            change line

            $wrapped->feedback["$answerindex"]['itemid'] = '';

            to

            $wrapped->feedback["$answerindex"]['itemid'] = $text['itemid'];

            and lines

            $wrapped->answer["$answerindex"] = array(
                                         'text' => $wrapped->answer["$answerindex"],
                                         'format' => FORMAT_HTML,
                                         'itemid' => '');

            to

            $wrapped->answer["$answerindex"] = array(
                                         'text' => $wrapped->answer["$answerindex"],
                                         'format' => FORMAT_HTML,
                                         'itemid' => $text['itemid']);
             

            that way during import and also during question creation/edition draftfiles are considered as both part of the main question and of the subquestions (of course that will lead to some records duplication in mdl_file table but that doesn't waste too much space as the images data are no duplicated).

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Pierre, Looking at your code https://github.com/ppichet/moodle/commit/9200042144fd0e60d4f5ffdfa68fc4835f1affbc (I don't know if this is your latest version) I think it's easy to adapt your code to MDL-38880 : suppress all you changes to question/format/xml/format.php because they are no more necessary in question/type/multianswer/questiontype.php just change 2 lines in the qtype_multianswer_extract_question function so that itemid is set for answers and answers feedback change line $wrapped->feedback["$answerindex"]['itemid'] = ''; to $wrapped->feedback["$answerindex"]['itemid'] = $text['itemid']; and lines $wrapped->answer["$answerindex"] = array( 'text' => $wrapped->answer["$answerindex"], 'format' => FORMAT_HTML, 'itemid' => ''); to $wrapped->answer["$answerindex"] = array( 'text' => $wrapped->answer["$answerindex"], 'format' => FORMAT_HTML, 'itemid' => $text['itemid']);   that way during import and also during question creation/edition draftfiles are considered as both part of the main question and of the subquestions (of course that will lead to some records duplication in mdl_file table but that doesn't waste too much space as the images data are no duplicated).
            Hide
            ppichet Pierre Pichet added a comment -

            Thanks for your suggestions.
            I have done just a manual addition of your code in rebuilding manually the MDL-26511 as I want to review the last year issue.
            MDL-38880 will be put on master before I will have completed my work on MDL-26511.
            On rebase things will set correctly.
            The setting of itemid is done when saving the question after the extraction.
            https://github.com/ppichet/moodle/commit/9200042144fd0e60d4f5ffdfa68fc4835f1affbc#L3R173
            I need to review this because we could allow images on more questiontypes feedback but not answers (short, numerical)

            Show
            ppichet Pierre Pichet added a comment - Thanks for your suggestions. I have done just a manual addition of your code in rebuilding manually the MDL-26511 as I want to review the last year issue. MDL-38880 will be put on master before I will have completed my work on MDL-26511 . On rebase things will set correctly. The setting of itemid is done when saving the question after the extraction. https://github.com/ppichet/moodle/commit/9200042144fd0e60d4f5ffdfa68fc4835f1affbc#L3R173 I need to review this because we could allow images on more questiontypes feedback but not answers (short, numerical)
            Hide
            jmvedrine Jean-Michel Vedrine added a comment - - edited

            Hello Pierre,
            Thanks for the link.
            I was unsure that setting itemid when saving the question would works both for edition and import but you are right it seems to work quite well with the added benefit that you can choose to add it only if the answer text contains "draftfile.php".
            Your link remind me that I forgot to say I don't understand why you have a check_file_access function in your questiontype.php, surely this fonction belongs to the qtype_multianswer_question class so it should be in question.php isn't it ?
            But multianser code is complex so maybe there is something I don't understand.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - - edited Hello Pierre, Thanks for the link. I was unsure that setting itemid when saving the question would works both for edition and import but you are right it seems to work quite well with the added benefit that you can choose to add it only if the answer text contains "draftfile.php". Your link remind me that I forgot to say I don't understand why you have a check_file_access function in your questiontype.php, surely this fonction belongs to the qtype_multianswer_question class so it should be in question.php isn't it ? But multianser code is complex so maybe there is something I don't understand.
            Hide
            ppichet Pierre Pichet added a comment -

            My actual proposal give the following result:
            Let's take a simple shortanswer in which we want to put an image in the feedback (images are allowed in the feedback)
            The initial questiontext is
            The capital of France is

            {1:SHORTANSWER:%100%Paris#Congratulations!...image.gif}

            The main (parent) questiontext will become
            The capital of France is

            {#1}
            then the subquestion questiontext is {1:SHORTANSWER:%100%Paris#Congratulations!<img src="@@PLUGINFILE@@image.gif" alt:"image".../>}
            the answer feedback is
            Congratulations!<img src="@@PLUGINFILE@@image.gif" alt:"image".../

            In the data base files there are 8 references to image.gif
            2 filearea draft . and image.gif
            2 questiontext itemid 133 (i.e. main question id)
            2 questiontext itemid 134 subquestion id
            2 answerfeedback itemid 158 answerid

            When the question is reedit the {#1}

            in the question text is replaced by the sub questiontext

            When the question is used the link is done through the answerfeedback filearea data.

            The proposal will then be extended to all subquestions (short, numerical, multiplechoices) answer feedback
            and to multiplechoice_V and multiplechoice_H .

            The validation will have to check that the users don't put images in short, numerical, multiplechoiceselect answers.

            The image rendering in the feedbacks is not solved for all cases actually.

            Show
            ppichet Pierre Pichet added a comment - My actual proposal give the following result: Let's take a simple shortanswer in which we want to put an image in the feedback (images are allowed in the feedback) The initial questiontext is The capital of France is {1:SHORTANSWER:%100%Paris#Congratulations!...image.gif} The main (parent) questiontext will become The capital of France is {#1} then the subquestion questiontext is {1:SHORTANSWER:%100%Paris#Congratulations!<img src="@@PLUGINFILE@@image.gif" alt:"image".../>} the answer feedback is Congratulations!<img src="@@PLUGINFILE@@image.gif" alt:"image".../ In the data base files there are 8 references to image.gif 2 filearea draft . and image.gif 2 questiontext itemid 133 (i.e. main question id) 2 questiontext itemid 134 subquestion id 2 answerfeedback itemid 158 answerid When the question is reedit the {#1} in the question text is replaced by the sub questiontext When the question is used the link is done through the answerfeedback filearea data. The proposal will then be extended to all subquestions (short, numerical, multiplechoices) answer feedback and to multiplechoice_V and multiplechoice_H . The validation will have to check that the users don't put images in short, numerical, multiplechoiceselect answers. The image rendering in the feedbacks is not solved for all cases actually.
            Hide
            ppichet Pierre Pichet added a comment -

            Tim,
            I solve the image rendering problem in feedback.
            However when adding images the 'blue field' containing the question is not sufficient.
            Where (or what) is the parameter that control the question space (length) display that is allowed by (quiz or preview)
            renderer to the question ?

            Show
            ppichet Pierre Pichet added a comment - Tim, I solve the image rendering problem in feedback. However when adding images the 'blue field' containing the question is not sufficient. Where (or what) is the parameter that control the question space (length) display that is allowed by (quiz or preview) renderer to the question ?
            Hide
            timhunt Tim Hunt added a comment -

            When I try to import the sample file, questions-T100-import1.9mix-20110314-1202.xml, I get an error:

            Field 'version' doesn't have a default value
            INSERT INTO question (qtype,questiontext,generalfeedback,defaultmark,name,questiontextformat,generalfeedbackformat,penalty,category,stamp,createdby,timecreated,modifiedby,timemodified) VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?,?)

            I don't have time to work out why yet.

            Sorry, I will try to look at this later.

            Show
            timhunt Tim Hunt added a comment - When I try to import the sample file, questions-T100-import1.9mix-20110314-1202.xml, I get an error: Field 'version' doesn't have a default value INSERT INTO question (qtype,questiontext,generalfeedback,defaultmark,name,questiontextformat,generalfeedbackformat,penalty,category,stamp,createdby,timecreated,modifiedby,timemodified) VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?,?) I don't have time to work out why yet. Sorry, I will try to look at this later.
            Hide
            ppichet Pierre Pichet added a comment -

            Sorry my github is not updated to my actual experimental version.
            I will put images that illustrate the problem and update the github.

            Show
            ppichet Pierre Pichet added a comment - Sorry my github is not updated to my actual experimental version. I will put images that illustrate the problem and update the github.
            Hide
            ppichet Pierre Pichet added a comment -

            The problem is related to the HTML editor which include <pre> and <p> when a file is included

            A simple question without image will look as

            <pre>Los Angeles: {1:MULTICHOICE_V:=California2#OK~Arizona#Wrong}<br /><br /></pre>

            with an image its is changed to

            <pre>Los Angeles: {1:MULTICHOICE_V:=California
            </pre>
            <p><img src="http://132.208.141.198/moodle_w/draftfile.php/5/user/draft/271545410/xeo4%20%281%29.gif" alt="" width="30" height="30" /></p>
            <pre>#OK~Arizona#Wrong}</pre>

            which scramble all the display as the db answer is

            California</pre>
            <p><img src="@@PLUGINFILE@@/xeo4%20%281%29.gif" alt="" width="30" height="30" /></p>
            <pre>

            When doing it with careful handling I succeeded to a valid and correct display.

            So I must add some additional validate steps of the answer content.

            Show
            ppichet Pierre Pichet added a comment - The problem is related to the HTML editor which include <pre> and <p> when a file is included A simple question without image will look as <pre>Los Angeles: {1:MULTICHOICE_V:=California2#OK~Arizona#Wrong}<br /><br /></pre> with an image its is changed to <pre>Los Angeles: {1:MULTICHOICE_V:=California </pre> <p><img src="http://132.208.141.198/moodle_w/draftfile.php/5/user/draft/271545410/xeo4%20%281%29.gif" alt="" width="30" height="30" /></p> <pre>#OK~Arizona#Wrong}</pre> which scramble all the display as the db answer is California</pre> <p><img src="@@PLUGINFILE@@/xeo4%20%281%29.gif" alt="" width="30" height="30" /></p> <pre> When doing it with careful handling I succeeded to a valid and correct display. So I must add some additional validate steps of the answer content.
            Hide
            ppichet Pierre Pichet added a comment -

            A valid one showing image in the answer and its feedback

            Show
            ppichet Pierre Pichet added a comment - A valid one showing image in the answer and its feedback
            Hide
            timhunt Tim Hunt added a comment -

            Right. That is a problem with the HTML editor. In the qtype_multichoice, I did CSS like https://github.com/moodle/moodle/blob/master/question/type/multichoice/styles.css#L5 to try to fix this, but I do not recommend it. It caused bugs like MDL-35343 and MDL-37845.

            We need a better solution.

            Show
            timhunt Tim Hunt added a comment - Right. That is a problem with the HTML editor. In the qtype_multichoice, I did CSS like https://github.com/moodle/moodle/blob/master/question/type/multichoice/styles.css#L5 to try to fix this, but I do not recommend it. It caused bugs like MDL-35343 and MDL-37845 . We need a better solution.
            Hide
            ppichet Pierre Pichet added a comment -

            The multichoice editor does not generate any <pre> html which could be a protection against hacking ?
            The edit_question_form.php set the editor options in the _construct to

                    $this->editoroptions = array('subdirs' => 1, 'maxfiles' => EDITOR_UNLIMITED_FILES,
                            'context' => $this->context, 'collapsed' => 1);

            In the edit_multichoice_form.php there is no cosntruct and on answerfield

            $repeated[] = $mform->createElement('editor', 'answer',
            $label, array('rows' => 1), $this->editoroptions);

            in qtype_multianswer_edit_form extends question_edit_form {
            _cosntruct there is no reference to modifying the editoroptions

            However the 2 questiontexts do not behave the same way ...

            Show
            ppichet Pierre Pichet added a comment - The multichoice editor does not generate any <pre> html which could be a protection against hacking ? The edit_question_form.php set the editor options in the _construct to $this->editoroptions = array('subdirs' => 1, 'maxfiles' => EDITOR_UNLIMITED_FILES, 'context' => $this->context, 'collapsed' => 1); In the edit_multichoice_form.php there is no cosntruct and on answerfield $repeated[] = $mform->createElement('editor', 'answer', $label, array('rows' => 1), $this->editoroptions); in qtype_multianswer_edit_form extends question_edit_form { _cosntruct there is no reference to modifying the editoroptions However the 2 questiontexts do not behave the same way ...
            Hide
            ppichet Pierre Pichet added a comment - - edited

            GOOD NEWS "Cannot reproduce" effect.

            When I edit a question from fresh text everything was OK.
            I did the prevous using a cut and paste from the Cloze docs so the copy contained the <pre>.
            As far as I remember, I tried to remove the <pre> but perhaps I was to tired.

            Anyway, perhaps we should test that there is no unbalanced <pre> </pre> in the question text of all question types as this could led to bad format in preview or quiz.

            In the Cloze this is more critical as the <pre> could be in the answer and the </pre> in the feedback ...

            I think we could add a rule in the docs to not put any HTML like <pre> inside the { } defining a question.

            The file handler or the editor seems to put automatically the image inside a <p> </p>.

            When polishing the code we should choose how to handle this so that the display is OK as default i.e. without <p> </p>.

            Show
            ppichet Pierre Pichet added a comment - - edited GOOD NEWS "Cannot reproduce" effect. When I edit a question from fresh text everything was OK. I did the prevous using a cut and paste from the Cloze docs so the copy contained the <pre>. As far as I remember, I tried to remove the <pre> but perhaps I was to tired. Anyway, perhaps we should test that there is no unbalanced <pre> </pre> in the question text of all question types as this could led to bad format in preview or quiz. In the Cloze this is more critical as the <pre> could be in the answer and the </pre> in the feedback ... I think we could add a rule in the docs to not put any HTML like <pre> inside the { } defining a question. The file handler or the editor seems to put automatically the image inside a <p> </p>. When polishing the code we should choose how to handle this so that the display is OK as default i.e. without <p> </p>.
            Hide
            timhunt Tim Hunt added a comment -

            Right. So the question is, how much validation should we try to do, and should we work on it using this issue, or create a new issue?

            Show
            timhunt Tim Hunt added a comment - Right. So the question is, how much validation should we try to do, and should we work on it using this issue, or create a new issue?
            Hide
            ppichet Pierre Pichet added a comment -

            I think that this validation can be included in this issue as there is already a quite complete validation in the actual code.

            I will look at the editor, there could be a possibility to control the data received from the file handler so that the
            <p> </p> is not generated automatically when inserting a file.

            I should have settle more things by next monday.

            Show
            ppichet Pierre Pichet added a comment - I think that this validation can be included in this issue as there is already a quite complete validation in the actual code. I will look at the editor, there could be a possibility to control the data received from the file handler so that the <p> </p> is not generated automatically when inserting a file. I should have settle more things by next monday.
            Hide
            ppichet Pierre Pichet added a comment -

            Tim, Jean-Pierre,

            Just to note some difficulties on working on the file handling.
            I can edit, import and export with files in questiontexts, answers (when allowed i.e. multichoice_v and multichoice_h) and answers feedback.

            Everything seems OK until I look at the database files table.

            If for an answer feedback even empty the itemid is set to the questiontext[itemid] then the database files will contain additional records for each of the images that are related to the questiontext[itemid].

            So I need to check that that the fields contain effectively an image before setting the [itemid] to something else than empty.

            I am working on this as the images are not described in the same way when coming from edit_multianswer_form.php than from import format.

            Is there a way to limit the database files links to the files actually contained in a given element (i.e. answerfeedback[nn]) ?

            Show
            ppichet Pierre Pichet added a comment - Tim, Jean-Pierre, Just to note some difficulties on working on the file handling. I can edit, import and export with files in questiontexts, answers (when allowed i.e. multichoice_v and multichoice_h) and answers feedback. Everything seems OK until I look at the database files table. If for an answer feedback even empty the itemid is set to the questiontext [itemid] then the database files will contain additional records for each of the images that are related to the questiontext [itemid] . So I need to check that that the fields contain effectively an image before setting the [itemid] to something else than empty. I am working on this as the images are not described in the same way when coming from edit_multianswer_form.php than from import format. Is there a way to limit the database files links to the files actually contained in a given element (i.e. answerfeedback [nn] ) ?
            Hide
            timhunt Tim Hunt added a comment -

            It is not possible to link the the files acutally used by a given element. To understand why it is impossible, imagine that part of the input embeds a flash applet that loads the name of an image file from an XML file, all using relative URLs.

            I think the only option is to duplicate all the files into all the file areas. Because of the way the file pool works, only one copy of each file will be stored on disc.

            Show
            timhunt Tim Hunt added a comment - It is not possible to link the the files acutally used by a given element. To understand why it is impossible, imagine that part of the input embeds a flash applet that loads the name of an image file from an XML file, all using relative URLs. I think the only option is to duplicate all the files into all the file areas. Because of the way the file pool works, only one copy of each file will be stored on disc.
            Hide
            ppichet Pierre Pichet added a comment -

            I understand the principle.
            However there seems to be no mechanism inside the file handling to control if there is really a reference to a file in the element where the [itemid] is set to a value.
            To experiment I use the
            http://docs.moodle.org/24/en/Embedded_Answers_(Cloze)_question_type#Example_2
            containing many subquestions.
            I just put 1 image in the main question and nowhere else
            I save the question and there were 60 new records in the mdl_files related to this image..

            I then use the code with the additional control on using questiontext[itemid]
            https://github.com/ppichet/moodle/blob/a38fa7cb7baca7ff148e060544b4b196942b9b4e/question/type/multianswer/questiontype.php#L175
            When saving the question as new, only 2 records were added to the mdl_files and this reflects the reality.

            What should we do ?

            P.S. The filtering should be different i.e. @@PLUGINFILE@@ for format.php

            Show
            ppichet Pierre Pichet added a comment - I understand the principle. However there seems to be no mechanism inside the file handling to control if there is really a reference to a file in the element where the [itemid] is set to a value. To experiment I use the http://docs.moodle.org/24/en/Embedded_Answers_(Cloze)_question_type#Example_2 containing many subquestions. I just put 1 image in the main question and nowhere else I save the question and there were 60 new records in the mdl_files related to this image.. I then use the code with the additional control on using questiontext [itemid] https://github.com/ppichet/moodle/blob/a38fa7cb7baca7ff148e060544b4b196942b9b4e/question/type/multianswer/questiontype.php#L175 When saving the question as new, only 2 records were added to the mdl_files and this reflects the reality. What should we do ? P.S. The filtering should be different i.e. @@PLUGINFILE@@ for format.php
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Pierre,
            I don't see the problem to create 60 records in mdl_files table as there is only 1 file for all of them in the filedir structure. The place wasted is minimal.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Pierre, I don't see the problem to create 60 records in mdl_files table as there is only 1 file for all of them in the filedir structure. The place wasted is minimal.
            Hide
            timhunt Tim Hunt added a comment -

            Pierre, there is no way to sort out which files are used in which place. You could have a Cloze question like:

            {1:SHORTANSWER:=Berlin#<a target="_blank" href="@PLUGINFILE@/popup1.html">Click here</a>}

            is the capital of Germany <a target="_blank" href="@PLUGINFILE@/popup2.html">Click here</a>.

            And where popup1.html and popup2.html both refer (using a relative URL) to another file called styles.css or germanymap.png.

            There is no way for Moodle to unscramble that. You just have to duplicate the rows in the files table.

            Show
            timhunt Tim Hunt added a comment - Pierre, there is no way to sort out which files are used in which place. You could have a Cloze question like: {1:SHORTANSWER:=Berlin#<a target="_blank" href="@PLUGINFILE@/popup1.html">Click here</a>} is the capital of Germany <a target="_blank" href="@PLUGINFILE@/popup2.html">Click here</a>. And where popup1.html and popup2.html both refer (using a relative URL) to another file called styles.css or germanymap.png. There is no way for Moodle to unscramble that. You just have to duplicate the rows in the files table.
            Hide
            ppichet Pierre Pichet added a comment -

            Perhaps I am just an old-timer but I am not at ease that a table does not reflect the reality.

            This becomes a table that just tell you that perhaps a given element even an empty one (I just test it), could reference to a given file.

            However I don't know exactly how the cleaning of this table is done http://docs.moodle.org/dev/File_API_internals
            does not give clear indications of the exact process as when using the texteditor to change to a new file in a given question field there is no deleting.

            Show
            ppichet Pierre Pichet added a comment - Perhaps I am just an old-timer but I am not at ease that a table does not reflect the reality. This becomes a table that just tell you that perhaps a given element even an empty one (I just test it), could reference to a given file. However I don't know exactly how the cleaning of this table is done http://docs.moodle.org/dev/File_API_internals does not give clear indications of the exact process as when using the texteditor to change to a new file in a given question field there is no deleting.
            Hide
            ppichet Pierre Pichet added a comment -

            Tim,
            My objective is just to NOT set the param[itemid]

            1. if the param[text] is empty

            2. does not contain the @PLUGINFILE@ that is, I think, a clear signal that there is a link to a file when coming from some process as retrieving data from the database or from import format.

            3. When retrieving from the edit_multianswer_form "/(draftfile.php)/" is part of the file info that can be used to identify that there is valid file.

            So which of these tests should I put in my proposal ?

            Show
            ppichet Pierre Pichet added a comment - Tim, My objective is just to NOT set the param [itemid] 1. if the param [text] is empty 2. does not contain the @PLUGINFILE@ that is, I think, a clear signal that there is a link to a file when coming from some process as retrieving data from the database or from import format. 3. When retrieving from the edit_multianswer_form "/(draftfile.php)/" is part of the file info that can be used to identify that there is valid file. So which of these tests should I put in my proposal ?
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I think you are trying to care for something that is not important at all because you will still have all files attached to a subquestion when it contains a single @PLUGINFILE@ or draftfile.php !
            So it just complicate the code (and making it slower and more prone to break) without solving anything.
            When I say it's not important it is because that is just a bunch of lines in a table. When the question is deleted (or when an answer is deleted during question saving after editing) the filearea is cleaned and if it's the last reference to that binary file in the file pool the file is marked to be purged a few days later.
            On the contrary as you note "when using the texteditor to change to a new file in a given question field there is no deleting" and this is a very serious issue, but this is quite another story. There is already an issue in the tracker for that MDL-28019, and despite being one of the most voted tracker's issues, I seems no progress on it for years (this worry me because it's like a time bomb: big files like videos can accumulate in a filearea and suddenly backup/restore breaks without people being able to do anything).
            So IMHO, don't try to do any test about images in subquestions fields and just set itemid unconditionally.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I think you are trying to care for something that is not important at all because you will still have all files attached to a subquestion when it contains a single @PLUGINFILE@ or draftfile.php ! So it just complicate the code (and making it slower and more prone to break) without solving anything. When I say it's not important it is because that is just a bunch of lines in a table. When the question is deleted (or when an answer is deleted during question saving after editing) the filearea is cleaned and if it's the last reference to that binary file in the file pool the file is marked to be purged a few days later. On the contrary as you note "when using the texteditor to change to a new file in a given question field there is no deleting" and this is a very serious issue, but this is quite another story. There is already an issue in the tracker for that MDL-28019 , and despite being one of the most voted tracker's issues, I seems no progress on it for years (this worry me because it's like a time bomb: big files like videos can accumulate in a filearea and suddenly backup/restore breaks without people being able to do anything). So IMHO, don't try to do any test about images in subquestions fields and just set itemid unconditionally.
            Hide
            ppichet Pierre Pichet added a comment -

            Thanks Jean-Michel for your comment and the MDL-28019 reference.

            I think that I should at least test #1 for empty which in any case this could not "complicate the code (and making it slower and more prone to break) ".

            This test is done in the qtype_multianswer_extract_question() function.

            Show
            ppichet Pierre Pichet added a comment - Thanks Jean-Michel for your comment and the MDL-28019 reference. I think that I should at least test #1 for empty which in any case this could not "complicate the code (and making it slower and more prone to break) ". This test is done in the qtype_multianswer_extract_question() function.
            Hide
            timhunt Tim Hunt added a comment -

            Ah, testing for bits of text with no @pluginfile@ is a good idea!

            Pierre is right to be uneasy about this. It is not great, but we are constrained by:
            1. Editing everything in a single HTML editor - that suggests we should only have one file area.
            2. Wanting to re-use the existing code for rendering and serving files. That requires separate file areas.

            We are looking for the best compromise between the two, which I think we are close to.

            Show
            timhunt Tim Hunt added a comment - Ah, testing for bits of text with no @pluginfile@ is a good idea! Pierre is right to be uneasy about this. It is not great, but we are constrained by: 1. Editing everything in a single HTML editor - that suggests we should only have one file area. 2. Wanting to re-use the existing code for rendering and serving files. That requires separate file areas. We are looking for the best compromise between the two, which I think we are close to.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            I just push on github my last version on which I was working this week.
            I can create, export moodle xml, backup, restore, move from category to another.
            To do the testing I propose to create some questions that illustrate various aspects and export them as moodle xml.
            I will create a quiz with those question and do a backup of the quiz and one for a simple course.

            This should be done by monday.

            We are near the 2,5 release which can overload the testers, this is a new feature and there are some real bugs to solve.

            Should I put this aside and come back to MDL-31680 ?

            Show
            ppichet Pierre Pichet added a comment - - edited I just push on github my last version on which I was working this week. I can create, export moodle xml, backup, restore, move from category to another. To do the testing I propose to create some questions that illustrate various aspects and export them as moodle xml. I will create a quiz with those question and do a backup of the quiz and one for a simple course. This should be done by monday. We are near the 2,5 release which can overload the testers, this is a new feature and there are some real bugs to solve. Should I put this aside and come back to MDL-31680 ?
            Hide
            ppichet Pierre Pichet added a comment -

            I just push a new version with a specific function to set the itemid that is also used in xml format import.

            Show
            ppichet Pierre Pichet added a comment - I just push a new version with a specific function to set the itemid that is also used in xml format import.
            Hide
            ppichet Pierre Pichet added a comment -

            a simple example

            Show
            ppichet Pierre Pichet added a comment - a simple example
            Hide
            ppichet Pierre Pichet added a comment -

            I just learn that you are " on holiday today and tomorrow ".
            This will give me time to set a proposal for the testing procedure .

            Show
            ppichet Pierre Pichet added a comment - I just learn that you are " on holiday today and tomorrow ". This will give me time to set a proposal for the testing procedure .
            Hide
            ppichet Pierre Pichet added a comment -

            From the discusion around MDL-39490, I understand that "Can we add this to master just after 2.5 branch is cut?" is yes

            Show
            ppichet Pierre Pichet added a comment - From the discusion around MDL-39490 , I understand that "Can we add this to master just after 2.5 branch is cut?" is yes
            Hide
            ppichet Pierre Pichet added a comment - - edited

            Testing was doing well until I test the validation in edit_multianswer_form.

            I add some supplmentary tests as we allowed image so HTML in multiple choice answers although the drop down menu (select element) does not allow HTML.

            The case of a single < could be interpreted in various ways because of the different places where the HTML possible elements are checked including the editor itself.

            I compared what happens in regular multichoice and in multichoice created in multianswer.

            In simple cases the HTML editor is doing a good job.
            For a regular multiplechoice answer we could find in the database

             
            <p>if x <sup>2 </sup>&lt; 4ac <img src="@@PLUGINFILE@@/so42m.gif" alt="so4" width="40" height="40" /> </p> 
            

            In other cases when editing a multichoice in edit_multianswer_form the conversion was not done, so the database contains

            if x<sup>2</sup> < 0 
            

            What could be the best function to use to neutralize the single < in all cases?

            However some results are not the same that I had before rebase.
            I need to check again the validation of the select element multichoice.

            Anyway, have you more general comments on the code proposal i.e. the new function created to handle the itemid ?

            Show
            ppichet Pierre Pichet added a comment - - edited Testing was doing well until I test the validation in edit_multianswer_form. I add some supplmentary tests as we allowed image so HTML in multiple choice answers although the drop down menu (select element) does not allow HTML. The case of a single < could be interpreted in various ways because of the different places where the HTML possible elements are checked including the editor itself. I compared what happens in regular multichoice and in multichoice created in multianswer. In simple cases the HTML editor is doing a good job. For a regular multiplechoice answer we could find in the database <p>if x <sup>2 </sup>&lt; 4ac <img src="@@PLUGINFILE@@/so42m.gif" alt="so4" width="40" height="40" /> </p> In other cases when editing a multichoice in edit_multianswer_form the conversion was not done, so the database contains if x<sup>2</sup> < 0 What could be the best function to use to neutralize the single < in all cases? However some results are not the same that I had before rebase. I need to check again the validation of the select element multichoice. Anyway, have you more general comments on the code proposal i.e. the new function created to handle the itemid ?
            Hide
            ppichet Pierre Pichet added a comment -

            I will drop the lines related to testing if multiplechoice answers could have an empty display in the select element.
            Looking through the renderer code, the choices

             
                    foreach ($subq->get_order($qa) as $value => $ansid) {
                        $ans = $subq->answers[$ansid];
                        $choices[$value] = $subq->format_text($ans->answer, $ans->answerformat,
                                $qa, 'question', 'answer', $ansid);
                        if ($subq->is_choice_selected($response, $value)) {
                            $matchinganswer = $ans;
                        }
                    }

            the choices contain all the info to display correctly the HTML (i.e. images).
            all the HTML rendering is done by

            $select = html_writer::select($choices, $qa->get_qt_field_name($fieldname),
            $response, array('' => ''), $inputattributes);
            which I cannot simulate in the edit_multianswer_form unless I add the resulting select in the
            form.

            Even if the select elements appear empty, there is no system error.

            However we could had a warning in edit_multianswer_form that the user should verify the
            select element rendering by doing a question preview.

            Show
            ppichet Pierre Pichet added a comment - I will drop the lines related to testing if multiplechoice answers could have an empty display in the select element. Looking through the renderer code, the choices foreach ($subq->get_order($qa) as $value => $ansid) { $ans = $subq->answers[$ansid]; $choices[$value] = $subq->format_text($ans->answer, $ans->answerformat, $qa, 'question', 'answer', $ansid); if ($subq->is_choice_selected($response, $value)) { $matchinganswer = $ans; } } the choices contain all the info to display correctly the HTML (i.e. images). all the HTML rendering is done by $select = html_writer::select($choices, $qa->get_qt_field_name($fieldname), $response, array('' => ''), $inputattributes); which I cannot simulate in the edit_multianswer_form unless I add the resulting select in the form. Even if the select elements appear empty, there is no system error. However we could had a warning in edit_multianswer_form that the user should verify the select element rendering by doing a question preview.
            Hide
            timhunt Tim Hunt added a comment -

            You ask "have you more general comments on the code proposal?, but is there some code I am supposed to be reviewing? Sorry, I am not sure what your question is.

            Show
            timhunt Tim Hunt added a comment - You ask "have you more general comments on the code proposal?, but is there some code I am supposed to be reviewing? Sorry, I am not sure what your question is.
            Hide
            ppichet Pierre Pichet added a comment -

            Sorry, I forgot to put the reference.
            Do you agree how I handle setting the itemid if there are files in the given element?

            Apart building various questions to import to test the various feature, is there unittests that you suggest to build ?

            Show
            ppichet Pierre Pichet added a comment - Sorry, I forgot to put the reference. Do you agree how I handle setting the itemid if there are files in the given element? Apart building various questions to import to test the various feature, is there unittests that you suggest to build ?
            Hide
            timhunt Tim Hunt added a comment -

            This is looking very promising. As far as I can see, this is basically right, and there are just details that need to be sorted out. I think the way you have only set itemid if there are files in the given element is good.

            1. I don't think you should be calling $this->_form->setElementError in set_data. Surely this should be in the form validation method. (Or perhaps I don't understand this code yet?)

            2. In lib.php, you are missing one argument , array $options=array() from the qtype_multianswer_pluginfile function.

            3. With a function like set_subquestions_elements_itemid, I think it is better to use guard clauses (http://c2.com/cgi/wiki?GuardClause) rather than nesting the whole function inside a complicated if statement.

            4. You have code like

            if (preg_match('/'.$searchcriteria.'/', $wrapped->questiontext['text'])) {
                $wrapped->questiontext['itemid']=$questiontextitemid;
            } else {
                $wrapped->questiontext['itemid']='';
            }

            Is there a nice way to remove that duplication? (Basically, another method.)

            5. I think trim($question->tolerance[$key]) == '' should use ===, otherwise we will get problems with 0. (https://github.com/ppichet/moodle/compare/MDL-26511#L7R209)

            6. You have some code styles issues that code checker will tell you about when you run it.

            I don't think it is easy to create unit tests for this. I think just write good manual testing instructions here.

            Show
            timhunt Tim Hunt added a comment - This is looking very promising. As far as I can see, this is basically right, and there are just details that need to be sorted out. I think the way you have only set itemid if there are files in the given element is good. 1. I don't think you should be calling $this->_form->setElementError in set_data. Surely this should be in the form validation method. (Or perhaps I don't understand this code yet?) 2. In lib.php, you are missing one argument , array $options=array() from the qtype_multianswer_pluginfile function. 3. With a function like set_subquestions_elements_itemid, I think it is better to use guard clauses ( http://c2.com/cgi/wiki?GuardClause ) rather than nesting the whole function inside a complicated if statement. 4. You have code like if (preg_match('/'.$searchcriteria.'/', $wrapped->questiontext['text'])) { $wrapped->questiontext['itemid']=$questiontextitemid; } else { $wrapped->questiontext['itemid']=''; } Is there a nice way to remove that duplication? (Basically, another method.) 5. I think trim($question->tolerance [$key] ) == '' should use ===, otherwise we will get problems with 0. ( https://github.com/ppichet/moodle/compare/MDL-26511#L7R209 ) 6. You have some code styles issues that code checker will tell you about when you run it. I don't think it is easy to create unit tests for this. I think just write good manual testing instructions here.
            Hide
            timhunt Tim Hunt added a comment -

            As usual, I forgot to click the buttons.

            Show
            timhunt Tim Hunt added a comment - As usual, I forgot to click the buttons.
            Hide
            ppichet Pierre Pichet added a comment -

            Thanks for the comments.
            1.We are not in the validation process but in setting the default values usually when doing a submit on "decode and verify the question text" . So this is the way used in the quite non standard edit_multianwer_form .

            2. Options Thanks, I copy from a to old file...

            3. I thought that the complex if will be sufficient to exclude bad cases.

            i.e. there are subquestions, the questiontext-itemid exists and is not null 
             if ($question->options->questions != '' && isset($question->questiontext['itemid']) 
                       && $question->questiontext['itemid'] != '' && $searchcriteria != '')

            if it does not satify the criteria, the question is returned without any modifications.
            I could add a isset ($question->options->questions)

            4. The objective is to minimize the use of itemid if there is no image in the subquestions questiontext.
            Each time I set the itemid to an element there is a creation of links to this element and all the files included in the total question.
            Or I don't understand your point.
            Notice that this is used by two process (editing a question or import) .
            In the preceeding process that create this structure, the itemid could have been set to questiontext id automatically.

            5. This is a special case that I encounter when testing. The qtype_multianswer_extract_question() does not handle well * in numerical format (which is a rare case...) so the tolerance was not set. I do think that == '' will distinguih from == 0 which is not the case of the empty() function.
            But if you think === is better.

            6. I think that there is even a print_r left

            I will remove the validation of the multiplechoice in-line to detect empty rendering of HTML code.
            However today I added a select element that will display the choices as they will appear in "real life".
            The user will be able to check before previewing the question.
            This will be available on the next github update.

            Show
            ppichet Pierre Pichet added a comment - Thanks for the comments. 1.We are not in the validation process but in setting the default values usually when doing a submit on "decode and verify the question text" . So this is the way used in the quite non standard edit_multianwer_form . 2. Options Thanks, I copy from a to old file... 3. I thought that the complex if will be sufficient to exclude bad cases. i.e. there are subquestions, the questiontext-itemid exists and is not null if ($question->options->questions != '' && isset($question->questiontext['itemid']) && $question->questiontext['itemid'] != '' && $searchcriteria != '') if it does not satify the criteria, the question is returned without any modifications. I could add a isset ($question->options->questions) 4. The objective is to minimize the use of itemid if there is no image in the subquestions questiontext. Each time I set the itemid to an element there is a creation of links to this element and all the files included in the total question. Or I don't understand your point. Notice that this is used by two process (editing a question or import) . In the preceeding process that create this structure, the itemid could have been set to questiontext id automatically. 5. This is a special case that I encounter when testing. The qtype_multianswer_extract_question() does not handle well * in numerical format (which is a rare case...) so the tolerance was not set. I do think that == '' will distinguih from == 0 which is not the case of the empty() function. But if you think === is better. 6. I think that there is even a print_r left I will remove the validation of the multiplechoice in-line to detect empty rendering of HTML code. However today I added a select element that will display the choices as they will appear in "real life". The user will be able to check before previewing the question. This will be available on the next github update.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            About item 3
            About the complex if sequence, this is a special case were the return is always the $question object.
            I could wrote them differently to clarify the code.
            I should perhaps add isset tests ( answer) in the code flow for incomplete questions structure.
            However the preceeding function define default elements like answer feedback.
            I will loook for minimal additional tests.

            Show
            ppichet Pierre Pichet added a comment - - edited About item 3 About the complex if sequence, this is a special case were the return is always the $question object. I could wrote them differently to clarify the code. I should perhaps add isset tests ( answer) in the code flow for incomplete questions structure. However the preceeding function define default elements like answer feedback. I will loook for minimal additional tests.
            Hide
            ppichet Pierre Pichet added a comment -

            Illustration of the menu select when Decode and verify a regular multichoice.
            note the image in the feedback of one answer.

            Show
            ppichet Pierre Pichet added a comment - Illustration of the menu select when Decode and verify a regular multichoice. note the image in the feedback of one answer.
            Hide
            ppichet Pierre Pichet added a comment -

            An example of images in answers or feedback

            Show
            ppichet Pierre Pichet added a comment - An example of images in answers or feedback
            Hide
            ppichet Pierre Pichet added a comment -

            The master version has been rebase to actual master.
            I am waiting for this week version before rebasing if you think that this is ready for testing.

            Show
            ppichet Pierre Pichet added a comment - The master version has been rebase to actual master. I am waiting for this week version before rebasing if you think that this is ready for testing.
            Hide
            ppichet Pierre Pichet added a comment -

            Tim,

            Closing for holidays June 15 - Aug 15 as I will not have access to nothing more than an Ipad in this period.

            This weekend I will rebase to this week moodle versions.

            To which version(s) should I rebase if you think that this is ready for testing and integrate.

            If something goes wrong, I could possibly correct it in the next week.

            Show
            ppichet Pierre Pichet added a comment - Tim, Closing for holidays June 15 - Aug 15 as I will not have access to nothing more than an Ipad in this period. This weekend I will rebase to this week moodle versions. To which version(s) should I rebase if you think that this is ready for testing and integrate. If something goes wrong, I could possibly correct it in the next week.
            Hide
            ppichet Pierre Pichet added a comment -

            In case you did not notice...

            Show
            ppichet Pierre Pichet added a comment - In case you did not notice...
            Hide
            timhunt Tim Hunt added a comment -

            Sorry for the delay. I have now reviewed this, and I think the change is good, although it is a big complex change, and I am not yet 100% sure.

            What still needs to be done?

            1. We need testing instructions.

            2. It would be really nice to have unit tests for some of the complex bits of logic in the new code, but I guess that is not easy, so we can live without them.

            3. There are some minor coding style things, which are probably things that code-checker does not catch. The main one is that I would put a space either side of =. Again, I could live with the patch the way it is.

            Show
            timhunt Tim Hunt added a comment - Sorry for the delay. I have now reviewed this, and I think the change is good, although it is a big complex change, and I am not yet 100% sure. What still needs to be done? 1. We need testing instructions. 2. It would be really nice to have unit tests for some of the complex bits of logic in the new code, but I guess that is not easy, so we can live without them. 3. There are some minor coding style things, which are probably things that code-checker does not catch. The main one is that I would put a space either side of =. Again, I could live with the patch the way it is.
            Hide
            ppichet Pierre Pichet added a comment -

            I will put the testing instructions tonight.

            Mainly import the some questions from a given file (I will specify) among them there is one which put specific images in the question text, answers for multi and feedback for all question types and the same questions without images
            set_subquestions_elements_itemid($question, '@@PLUGINFILE@@');
            and looking at the file database records that the setting of image links are only created if there is an image in the given element.

            This also verify that the import is OK.

            Then edit copy the questions , test the decode verify display, save the question.

            This will test the filtering process

            set_subquestions_elements_itemid($question, 'draftfile.php');

            Create a copy of the question to another category.

            Move the questions to another category and context.

            Create a quiz, test OK.
            Do backup and restore process either to the same course or another course of a quiz ( the questions will be included)

            I will verify the code more closely

            Hopefully everything should be ready for tomorrow.

            Show
            ppichet Pierre Pichet added a comment - I will put the testing instructions tonight. Mainly import the some questions from a given file (I will specify) among them there is one which put specific images in the question text, answers for multi and feedback for all question types and the same questions without images set_subquestions_elements_itemid($question, '@@PLUGINFILE@@'); and looking at the file database records that the setting of image links are only created if there is an image in the given element. This also verify that the import is OK. Then edit copy the questions , test the decode verify display, save the question. This will test the filtering process set_subquestions_elements_itemid($question, 'draftfile.php'); Create a copy of the question to another category. Move the questions to another category and context. Create a quiz, test OK. Do backup and restore process either to the same course or another course of a quiz ( the questions will be included) I will verify the code more closely Hopefully everything should be ready for tomorrow.
            Hide
            ppichet Pierre Pichet added a comment -

            preview of a question with images in answers and feedback

            Show
            ppichet Pierre Pichet added a comment - preview of a question with images in answers and feedback
            Hide
            ppichet Pierre Pichet added a comment -

            Rebase master, correct =, add examples and instructions.

            Show
            ppichet Pierre Pichet added a comment - Rebase master, correct =, add examples and instructions.
            Hide
            timhunt Tim Hunt added a comment -

            Thanks Pierre. Submitting for integration review now.

            Show
            timhunt Tim Hunt added a comment - Thanks Pierre. Submitting for integration review now.
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated to master, thanks Pierre. I edited your commit to ensure you added yourself as copyright holder of the file you created rather than dongsheng

            Show
            poltawski Dan Poltawski added a comment - Integrated to master, thanks Pierre. I edited your commit to ensure you added yourself as copyright holder of the file you created rather than dongsheng
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Pierre,

            This looks to be breaking unit tests

            There was 1 error:

            1) qformat_xml_test::test_import_multianswer
            Undefined index: itemid

            /Users/danp/git/integration/question/type/multianswer/questiontype.php:362
            /Users/danp/git/integration/question/format/xml/format.php:456
            /Users/danp/git/integration/question/format/xml/tests/xmlformat_test.php:1308
            /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76

            To re-run:
            vendor/bin/phpunit qformat_xml_test question/format/xml/tests/xmlformat_test.php

            FAILURES!
            Tests: 1845, Assertions: 32077, Errors: 1, Skipped: 5.

            Show
            poltawski Dan Poltawski added a comment - Hi Pierre, This looks to be breaking unit tests There was 1 error: 1) qformat_xml_test::test_import_multianswer Undefined index: itemid /Users/danp/git/integration/question/type/multianswer/questiontype.php:362 /Users/danp/git/integration/question/format/xml/format.php:456 /Users/danp/git/integration/question/format/xml/tests/xmlformat_test.php:1308 /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76 To re-run: vendor/bin/phpunit qformat_xml_test question/format/xml/tests/xmlformat_test.php FAILURES! Tests: 1845, Assertions: 32077, Errors: 1, Skipped: 5.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment - - edited

            Hello Dan and Pierre,
            There is a problem in the code when question text doesn't contain any embedded image, because in qformat_xml in the import_multianswer function lines 454-455

            $questiontext = $this->import_text_with_files($question,
                            array('#', 'questiontext', 0));
            

            will only create a $questiontext array with 'text' and 'format' elements but no 'itemid' element (this is how it works, and should not be changed because we will need to change other places too that rely on not having itemid when there is no draftfile)
            So in multianswer questiontype you should check for that case in the qtype_multianswer_extract_question function and also the comment

                // Variable $text is an array [text][format][itemid].
                // If the main question contains a file then its [itemid]
                // will not be empty.
                // The other subquestions question , answers feedback and
                // multiple choice vertical and horizontal answers should be declared
                // array and should have [itemid] set to [text][itemid].
            

            is wrong because this is not how it works: the array is only [text][format] when there is no draftfile so rather than saying "will not be empty" it should be "will not be set".
            Sorry for not having spotted this problem when I looked at the code (and I should have run the phpunit tests too !!)
            Pierre if you want to further discuss on the best way to solve this, I am available all the day because course and exam are over for this year

            Show
            jmvedrine Jean-Michel Vedrine added a comment - - edited Hello Dan and Pierre, There is a problem in the code when question text doesn't contain any embedded image, because in qformat_xml in the import_multianswer function lines 454-455 $questiontext = $this->import_text_with_files($question, array('#', 'questiontext', 0)); will only create a $questiontext array with 'text' and 'format' elements but no 'itemid' element (this is how it works, and should not be changed because we will need to change other places too that rely on not having itemid when there is no draftfile) So in multianswer questiontype you should check for that case in the qtype_multianswer_extract_question function and also the comment // Variable $text is an array [text][format][itemid]. // If the main question contains a file then its [itemid] // will not be empty. // The other subquestions question , answers feedback and // multiple choice vertical and horizontal answers should be declared // array and should have [itemid] set to [text][itemid]. is wrong because this is not how it works: the array is only [text] [format] when there is no draftfile so rather than saying "will not be empty" it should be "will not be set". Sorry for not having spotted this problem when I looked at the code (and I should have run the phpunit tests too !!) Pierre if you want to further discuss on the best way to solve this, I am available all the day because course and exam are over for this year
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I think that simply testing if (isset($question->questiontext['itemid'])) should correct the problem

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I think that simply testing if (isset($question->questiontext ['itemid'] )) should correct the problem
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Jean-Michel for investigating.

            Piere, if you could confirm it would great.

            Show
            poltawski Dan Poltawski added a comment - Thanks Jean-Michel for investigating. Piere, if you could confirm it would great.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Well re-thinking to it now I am not sure this was a good decision from me when implementing using draftfiles in question import. Maybe it would have been best to have a constant structure and create an itemid in all cases in import_files_as_draft function ? After all there is an itemid form each editor in the question creation/edition form even if it doesn't contains any fie !
            But unfortunately I think we have to live with that now.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Well re-thinking to it now I am not sure this was a good decision from me when implementing using draftfiles in question import. Maybe it would have been best to have a constant structure and create an itemid in all cases in import_files_as_draft function ? After all there is an itemid form each editor in the question creation/edition form even if it doesn't contains any fie ! But unfortunately I think we have to live with that now.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            As a first comment before breakfast, I think that the import process should create the
            $question->questiontext['itemid'] even an empty one as long as its creates the structure [text][format].
            why "But unfortunately I think we have to live with that now." ?
            I let others ( Dan , Tim) decide where the test should be put.

            Sorry, I did run the tests more than once in the coding process but as evidence not in the last version.

            Show
            ppichet Pierre Pichet added a comment - - edited As a first comment before breakfast, I think that the import process should create the $question->questiontext ['itemid'] even an empty one as long as its creates the structure [text] [format] . why "But unfortunately I think we have to live with that now." ? I let others ( Dan , Tim) decide where the test should be put. Sorry, I did run the tests more than once in the coding process but as evidence not in the last version.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment - - edited

            Unfortunately I don't think changing import_files_as_draft is so easy because of the implications in question/format.php and in all the tests that would have to be modified.
            Also we would have to do a lot of tests to be sure this don't create any regression elsewhere.
            So in my opinion there is a choice between 2 very different options

            Quick fix

            Only modify qtype_multianswer_extract_question in question/type/multianswer/questiontype.php
            My current thinking is that adding some tests

            if (isset($question->questiontext['itemid']))
            

            before using it would be enough to fix the problem but you know this code better than me so you should say if you agree or not with this way of fixing the problem.
            This can be done and tested in a reasonable time so should be doable this week.

            Complicated fix

            Modify all the code so that itemid is created in all cases. I think that this need other changes elsewhere but I will make some tests to verify that this is true.
            Modifiying such a central function used to import all text fields during xml import is IMHO not doable during integration (and without Tim approval ! Don't forget he is on holidays)
            So if we choose this way, I think we need to revert this change for current integration and do more work until we are certain all is working (including in the lesson module because lesson is also using this code in an awful way !) and that there is no regression.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - - edited Unfortunately I don't think changing import_files_as_draft is so easy because of the implications in question/format.php and in all the tests that would have to be modified. Also we would have to do a lot of tests to be sure this don't create any regression elsewhere. So in my opinion there is a choice between 2 very different options Quick fix Only modify qtype_multianswer_extract_question in question/type/multianswer/questiontype.php My current thinking is that adding some tests if (isset($question->questiontext['itemid'])) before using it would be enough to fix the problem but you know this code better than me so you should say if you agree or not with this way of fixing the problem. This can be done and tested in a reasonable time so should be doable this week. Complicated fix Modify all the code so that itemid is created in all cases. I think that this need other changes elsewhere but I will make some tests to verify that this is true. Modifiying such a central function used to import all text fields during xml import is IMHO not doable during integration (and without Tim approval ! Don't forget he is on holidays) So if we choose this way, I think we need to revert this change for current integration and do more work until we are certain all is working (including in the lesson module because lesson is also using this code in an awful way !) and that there is no regression.
            Hide
            ppichet Pierre Pichet added a comment -

            The time scale is not important as we only put this on master.
            I will be on holidays without access to more than an Ipad from june 15 to august 15.
            So waiting appears the best.

            Show
            ppichet Pierre Pichet added a comment - The time scale is not important as we only put this on master. I will be on holidays without access to more than an Ipad from june 15 to august 15. So waiting appears the best.
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Everyone,

            I'm not confident that we're clear on the best way forward here and we need to get the unit tets passing.

            In the absence of Tim, i've decided its best to revert it so we can consider the complete solution.

            Thanks

            Show
            poltawski Dan Poltawski added a comment - Hi Everyone, I'm not confident that we're clear on the best way forward here and we need to get the unit tets passing. In the absence of Tim, i've decided its best to revert it so we can consider the complete solution. Thanks
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Thanks Dan,
            I think this is the right thing to do.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Thanks Dan, I think this is the right thing to do.
            Hide
            ppichet Pierre Pichet added a comment -

            I agree.

            Show
            ppichet Pierre Pichet added a comment - I agree.
            Hide
            poltawski Dan Poltawski added a comment -

            Great - thanks guys

            Show
            poltawski Dan Poltawski added a comment - Great - thanks guys
            Hide
            ppichet Pierre Pichet added a comment -

            Jean-Michel,
            Looking more closely to the code, the quick fix apppears the best way to solve the problem.
            So I put it , test that the unit test is Ok and push the result on github.

            If you think that this is Ok, you can finish the process.

            I just realize that I push it using the -f option so the lines added are included in main issue.

            Show
            ppichet Pierre Pichet added a comment - Jean-Michel, Looking more closely to the code, the quick fix apppears the best way to solve the problem. So I put it , test that the unit test is Ok and push the result on github. If you think that this is Ok, you can finish the process. I just realize that I push it using the -f option so the lines added are included in main issue.
            Hide
            timhunt Tim Hunt added a comment -

            Can you summarise what changes were made in the quick fix. That would make it easier to review. Thanks.

            Show
            timhunt Tim Hunt added a comment - Can you summarise what changes were made in the quick fix. That would make it easier to review. Thanks.
            Hide
            timhunt Tim Hunt added a comment -

            Oops! No, you can't You are on holiday.

            Show
            timhunt Tim Hunt added a comment - Oops! No, you can't You are on holiday.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment - - edited

            Hello Tim and Pierre,
            Can you summarise where we are with this issue ? I must admit I am somewhat lost, because I don't know if there is any problem left or not.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - - edited Hello Tim and Pierre, Can you summarise where we are with this issue ? I must admit I am somewhat lost, because I don't know if there is any problem left or not.
            Hide
            ppichet Pierre Pichet added a comment -

            Hi Jean-Michel,
            Good that you can come back to this as a kind of Happy New Year.
            By the way, I am beginning a 3 years 50% duty preretirement program which should let me more time for other things like home-improvment and moodle as long as my wife agrees...

            One thing that has changed is that the file handling under the moodle course files system is an obsolete process under 2,5+
            see http://docs.moodle.org/25/en/Repositories
            "(Note that you cannot upload files into a repository from Moodle.)"

            I don't understand clearly how it is working but as the initial proposal of this issue is to put files in the obsolete moodle system, I think that this issue could be more or less obsolete.
            ???

            Show
            ppichet Pierre Pichet added a comment - Hi Jean-Michel, Good that you can come back to this as a kind of Happy New Year. By the way, I am beginning a 3 years 50% duty preretirement program which should let me more time for other things like home-improvment and moodle as long as my wife agrees... One thing that has changed is that the file handling under the moodle course files system is an obsolete process under 2,5+ see http://docs.moodle.org/25/en/Repositories "(Note that you cannot upload files into a repository from Moodle.)" I don't understand clearly how it is working but as the initial proposal of this issue is to put files in the obsolete moodle system, I think that this issue could be more or less obsolete. ???
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Pierre,
            Happy new year 2014.
            I will be a happy retired teacher in June 2016 ! Unfortunately progressive pre-retirement program is no more possible here in France.
            I think you are confused because the actual state of the code for this issue in your github repo https://github.com/ppichet/moodle/compare/MDL-26511 is already using the new Moodle filesystem.
            IMHO the remaining questions are only

            • does it works in all cases ?
            • does it breaks any unit test ?
            • are the testing instructions complete enough to resubmit for integration ?
            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Pierre, Happy new year 2014. I will be a happy retired teacher in June 2016 ! Unfortunately progressive pre-retirement program is no more possible here in France. I think you are confused because the actual state of the code for this issue in your github repo https://github.com/ppichet/moodle/compare/MDL-26511 is already using the new Moodle filesystem. IMHO the remaining questions are only does it works in all cases ? does it breaks any unit test ? are the testing instructions complete enough to resubmit for integration ?
            Hide
            ppichet Pierre Pichet added a comment -

            "already using the new Moodle filesystem"
            The code already use the new Moodle file system in the general option that the files are saved with the new hash coding in the moodledata directory.
            However I am not sure that doing this, I cover all the new possibilities

            • the now obsolete course file directory
            • the new personal file directory
            • any of the external allowed file storages like Google etc.

            I need to reexplore how other questiontypes like multichoice handle files starting first by http://docs.moodle.org/dev/File_API_internals

            Show
            ppichet Pierre Pichet added a comment - "already using the new Moodle filesystem" The code already use the new Moodle file system in the general option that the files are saved with the new hash coding in the moodledata directory. However I am not sure that doing this, I cover all the new possibilities the now obsolete course file directory the new personal file directory any of the external allowed file storages like Google etc. I need to reexplore how other questiontypes like multichoice handle files starting first by http://docs.moodle.org/dev/File_API_internals
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            No,
            You don't need to look at repositories. And in fact you can't !
            All files associated with an editor instance are draft files whatever repositories they came from originally.
            If you look at other question types you will not find any code related to repositories at all.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - No, You don't need to look at repositories. And in fact you can't ! All files associated with an editor instance are draft files whatever repositories they came from originally. If you look at other question types you will not find any code related to repositories at all.
            Hide
            ppichet Pierre Pichet added a comment -

            OK, I just test on UQAM Moodle 2.5 that all files used in an activity are stored in the moodledata directory. So what remains is to clean the problems with the unit tests and generate the testing instructions once the code is updated to the last version.

            However I will received tomorrow a new computer which means some additional significant delays .

            Show
            ppichet Pierre Pichet added a comment - OK, I just test on UQAM Moodle 2.5 that all files used in an activity are stored in the moodledata directory. So what remains is to clean the problems with the unit tests and generate the testing instructions once the code is updated to the last version. However I will received tomorrow a new computer which means some additional significant delays .

              People

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

                Dates

                • Created:
                  Updated: