From 18e1450b4b0a2f50aabf7f335d417d4d5923c766 Mon Sep 17 00:00:00 2001 From: Andrew Hancox Date: Fri, 5 Jan 2018 18:40:54 +0000 Subject: [PATCH 1/3] MDL-41090 questions: Support for files in behaviour response --- question/engine/questionattempt.php | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/question/engine/questionattempt.php b/question/engine/questionattempt.php index d295170e3d3..1f5ea9618b5 100644 --- a/question/engine/questionattempt.php +++ b/question/engine/questionattempt.php @@ -1107,8 +1107,10 @@ class question_attempt { return null; } - return new question_file_saver($draftitemid, 'question', 'response_' . - str_replace($this->get_field_prefix(), '', $name), $text); + $filearea = str_replace($this->get_field_prefix(), '', $name); + $filearea = str_replace('-', 'bf_', $filearea); + $filearea = 'response_' . $filearea; + return new question_file_saver($draftitemid, 'question', $filearea, $text); } /** @@ -1164,6 +1166,8 @@ class question_attempt { $this->behaviour->get_expected_data(), $postdata, '-'); $expected = $this->behaviour->get_expected_qt_data(); + $this->check_qt_var_name_restrictions($expected); + if ($expected === self::USE_RAW_DATA) { $submitteddata += $this->get_all_submitted_qt_vars($postdata); } else { @@ -1172,6 +1176,23 @@ class question_attempt { return $submitteddata; } + /** + * Ensure that no reserved prefixes are being used by installed + * question types. + * @param array $expected An array of question type variables + */ + protected function check_qt_var_name_restrictions($expected) { + global $CFG; + + if ($CFG->debugdeveloper) { + foreach ($expected as $key => $value) { + if (strpos($key, 'bf_') !== false) { + debugging('The bf_ prefix is reserved and cannot be used by question types', DEBUG_DEVELOPER); + } + } + } + } + /** * Get a set of response data for this question attempt that would get the * best possible mark. If it is not possible to compute a correct From fd2ce923cfa0d28e1ff823be8af7779457bcd1eb Mon Sep 17 00:00:00 2001 From: Andrew Hancox Date: Fri, 5 Jan 2018 18:43:27 +0000 Subject: [PATCH 2/3] MDL-41090 questions: Move editor and filepicker options into engine --- question/engine/lib.php | 59 ++++++++++++++++++++++++++++++++ question/type/essay/renderer.php | 35 ++++++++----------- 2 files changed, 74 insertions(+), 20 deletions(-) diff --git a/question/engine/lib.php b/question/engine/lib.php index 1889378e87d..b65ce4b9c24 100644 --- a/question/engine/lib.php +++ b/question/engine/lib.php @@ -954,6 +954,65 @@ abstract class question_utils { $text = str_replace('@@PLUGINFILE@@/', 'http://example.com/', $text); return html_to_text(format_text($text, $format, $options), 0, false); } + + /** + * Get the options required to configure the filepicker for one of the editor + * toolbar buttons. + * @param mixed $acceptedtypes array of types of '*'. + * @param int $draftitemid the draft area item id. + * @param object $context the context. + * @return object the required options. + */ + protected static function specific_filepicker_options($acceptedtypes, $draftitemid, $context) { + $filepickeroptions = new stdClass(); + $filepickeroptions->accepted_types = $acceptedtypes; + $filepickeroptions->return_types = FILE_INTERNAL | FILE_EXTERNAL; + $filepickeroptions->context = $context; + $filepickeroptions->env = 'filepicker'; + + $options = initialise_filepicker($filepickeroptions); + $options->context = $context; + $options->client_id = uniqid(); + $options->env = 'editor'; + $options->itemid = $draftitemid; + + return $options; + } + + /** + * Get filepicker options for question related text areas. + * @param object $context the context. + * @param int $draftitemid the draft area item id. + * @return array An array of options + */ + public static function get_filepicker_options($context, $draftitemid) { + return [ + 'image' => self::specific_filepicker_options(['image'], $draftitemid, $context), + 'media' => self::specific_filepicker_options(['video', 'audio'], $draftitemid, $context), + 'link' => self::specific_filepicker_options('*', $draftitemid, $context), + ]; + } + + /** + * Get editor options for question related text areas. + * @param object $context the context. + * @return array An array of options + */ + public static function get_editor_options($context) { + global $CFG; + + $editoroptions = [ + 'subdirs' => 0, + 'context' => $context, + 'maxfiles' => EDITOR_UNLIMITED_FILES, + 'maxbytes' => $CFG->maxbytes, + 'noclean' => 0, + 'trusttext' => 0, + 'autosave' => false + ]; + + return $editoroptions; + } } diff --git a/question/type/essay/renderer.php b/question/type/essay/renderer.php index a6352ef2580..99f4075f1df 100644 --- a/question/type/essay/renderer.php +++ b/question/type/essay/renderer.php @@ -358,28 +358,27 @@ class qtype_essay_format_editorfilepicker_renderer extends qtype_essay_format_ed $name, $context->id, $step->get_qt_var($name)); } + /** + * Get editor options for question response text area. + * @param object $context the context the attempt belongs to. + * @return array options for the editor. + */ protected function get_editor_options($context) { - // Disable the text-editor autosave because quiz has it's own auto save function. - return array( - 'subdirs' => 0, - 'maxbytes' => 0, - 'maxfiles' => -1, - 'context' => $context, - 'noclean' => 0, - 'trusttext'=> 0, - 'autosave' => false - ); + return question_utils::get_editor_options($context); } /** * Get the options required to configure the filepicker for one of the editor * toolbar buttons. + * @deprecated since 3.5 * @param mixed $acceptedtypes array of types of '*'. * @param int $draftitemid the draft area item id. * @param object $context the context. * @return object the required options. */ protected function specific_filepicker_options($acceptedtypes, $draftitemid, $context) { + debugging('specific_filepicker_options() is deprecated, use get_filepicker_options instead.', DEBUG_DEVELOPER); + $filepickeroptions = new stdClass(); $filepickeroptions->accepted_types = $acceptedtypes; $filepickeroptions->return_types = FILE_INTERNAL | FILE_EXTERNAL; @@ -395,17 +394,13 @@ class qtype_essay_format_editorfilepicker_renderer extends qtype_essay_format_ed return $options; } + /** + * @param object $context the context the attempt belongs to. + * @param int $draftitemid draft item id. + * @return array filepicker options for the editor. + */ protected function get_filepicker_options($context, $draftitemid) { - global $CFG; - - return array( - 'image' => $this->specific_filepicker_options(array('image'), - $draftitemid, $context), - 'media' => $this->specific_filepicker_options(array('video', 'audio'), - $draftitemid, $context), - 'link' => $this->specific_filepicker_options('*', - $draftitemid, $context), - ); + return question_utils::get_filepicker_options($context, $draftitemid); } protected function filepicker_html($inputname, $draftitemid) { From f3d9872aa32072585311ef69012fadb1a35100d1 Mon Sep 17 00:00:00 2001 From: Andrew Hancox Date: Fri, 5 Jan 2018 18:44:07 +0000 Subject: [PATCH 3/3] MDL-41090 questions: Allow embedding files in response comments --- .../behat/manually_mark_question.feature | 42 +++++++++++++++--- mod/quiz/tests/fixtures/moodle_logo.jpg | Bin 0 -> 898 bytes question/behaviour/behaviourbase.php | 27 ++++++++--- question/behaviour/rendererbase.php | 28 ++++++++++-- question/engine/questionattempt.php | 9 ++-- 5 files changed, 88 insertions(+), 18 deletions(-) create mode 100644 mod/quiz/tests/fixtures/moodle_logo.jpg diff --git a/mod/quiz/tests/behat/manually_mark_question.feature b/mod/quiz/tests/behat/manually_mark_question.feature index 72c2c394552..f465f191427 100644 --- a/mod/quiz/tests/behat/manually_mark_question.feature +++ b/mod/quiz/tests/behat/manually_mark_question.feature @@ -36,15 +36,15 @@ Feature: Teachers can override the grade for any question And I press "Submit all and finish" And I click on "Submit all and finish" "button" in the "Confirmation" "dialogue" And I log out - And I log in as "teacher1" + + @javascript @_switch_window @_bug_phantomjs + Scenario: Validating the marking of an essay question attempt. + When I log in as "teacher1" And I am on "Course 1" course homepage And I follow "Quiz 1" And I follow "Attempts: 1" And I follow "Review attempt" - - @javascript @_switch_window @_bug_phantomjs - Scenario: Validating the marking of an essay question attempt. - When I follow "Make comment or override mark" + And I follow "Make comment or override mark" And I switch to "commentquestion" window And I set the field "Mark" to "25" And I press "Save" @@ -57,3 +57,35 @@ Feature: Teachers can override the grade for any question And I should see "Complete" in the "Manually graded 10 with comment: " "table_row" # This time is same as time the window is open. So wait for it to close before proceeding. And I wait "2" seconds + + @javascript @_switch_window @_file_upload @_bug_phantomjs + Scenario: Comment on a response to an essay question attempt. + When I log in as "teacher1" + And I follow "Manage private files" + And I upload "mod/quiz/tests/fixtures/moodle_logo.jpg" file to "Files" filemanager + And I click on "Save changes" "button" + And I am on "Course 1" course homepage + And I follow "Quiz 1" + And I follow "Attempts: 1" + And I follow "Review attempt" + And I follow "Make comment or override mark" + And I switch to "commentquestion" window + And I set the field "Comment" to "Administrator's comment" + # Atto needs focus to add image, select empty p tag to do so. + And I select the text in the "Comment" Atto editor + And I click on "Image" "button" in the "[data-fieldtype=editor]" "css_element" + And I click on "Browse repositories..." "button" + And I click on "Private files" "link" in the ".fp-repo-area" "css_element" + And I click on "moodle_logo.jpg" "link" + And I click on "Select this file" "button" + And I set the field "Describe this image for someone who cannot see it" to "It's the logo" + And I click on "Save image" "button" + # Editor is not inserting the html for the image correctly + # when running under behat so line below manually inserts it. + And I set the field "Comment" to "\"It's" + And I press "Save" and switch to main window + And I switch to the main window + And I should see "It's the logo" in the "3" "table_row" + And "//*[contains(@class, 'comment')]//img[contains(@src, 'moodle_logo.jpg')]" "xpath_element" should exist + # This time is same as time the window is open. So wait for it to close before proceeding. + And I wait "2" seconds diff --git a/mod/quiz/tests/fixtures/moodle_logo.jpg b/mod/quiz/tests/fixtures/moodle_logo.jpg new file mode 100644 index 0000000000000000000000000000000000000000..f2d536558a51c67c997da07be6df237d326c140f GIT binary patch literal 898 zcmex=qN!tIWMXEdV`>7VO-xMxA7BvVU@%}XU}jWeU=n0x7G(T? zgh3kUKqe+;1_WSdU}I)wWZ?ixnhG#5GBY!>u(5MOqM`FR!ZI`&g^_=uyCeo=>-wB^T)~^_r^DHF5RU*WV5#-_re*;$bi9 zvqI5!u}%G*jIy^?(~EcRT7CCK$;%(!aa`5rA3lgYHaPzMui=~wp(i3cL_C#?yCjbD zZM^Mnb=b!x>7&`Z%Uw}(YF&MIG&meQ#wfAMtFT?>zGPTWuwI4PR3Y1g{h1rA^dr-k zFlM^Hl6+u!ec>9-EeEz5{MJ)huq3C!eEH^-f}_$Af2Gb$)h#bss-tROaoLM``rPt& z#!sC)u2ihzUFS8a!R_s{4Yzk22pRMgpZ=P)HcYMg?9(Nk?^CyG-7L&AzqRa~cDhus z(M`VNg?ii8ae1Cty!0jKwbu#rcCN0-yY2K-xc8pvv6(w}XI*{SB2pU{adjust_display_options($options); + + if ($component == 'question' && $filearea == 'response_bf_comment') { + foreach ($this->qa->get_step_iterator() as $attemptstep) { + if ($attemptstep->get_id() == $args[0]) { + return true; + } + } + + return false; + } + return $this->question->check_file_access($this->qa, $options, $component, $filearea, $args, $forcedownload); } @@ -202,7 +213,7 @@ abstract class question_behaviour { return array(); } - $vars = array('comment' => PARAM_RAW, 'commentformat' => PARAM_INT); + $vars = array('comment' => question_attempt::PARAM_RAW_FILES, 'commentformat' => PARAM_INT); if ($this->qa->get_max_mark()) { $vars['mark'] = PARAM_RAW_TRIMMED; $vars['maxmark'] = PARAM_FLOAT; @@ -507,15 +518,20 @@ abstract class question_behaviour { * @param $comment the comment text to format. If omitted, * $this->qa->get_manual_comment() is used. * @param $commentformat the format of the comment, one of the FORMAT_... constants. + * @param $context the quiz context. * @return string the comment, ready to be output. */ - public function format_comment($comment = null, $commentformat = null) { + public function format_comment($comment = null, $commentformat = null, $context = null) { $formatoptions = new stdClass(); $formatoptions->noclean = true; $formatoptions->para = false; if (is_null($comment)) { - list($comment, $commentformat) = $this->qa->get_manual_comment(); + list($comment, $commentformat, $commentstep) = $this->qa->get_manual_comment(); + } + + if ($context !== null) { + $comment = $this->qa->rewrite_response_pluginfile_urls($comment, $context->id, 'bf_comment', $commentstep); } return format_text($comment, $commentformat, $formatoptions); @@ -528,8 +544,9 @@ abstract class question_behaviour { protected function summarise_manual_comment($step) { $a = new stdClass(); if ($step->has_behaviour_var('comment')) { - $a->comment = shorten_text(html_to_text($this->format_comment( - $step->get_behaviour_var('comment')), 0, false), 200); + list($comment, $commentformat, $commentstep) = $this->qa->get_manual_comment(); + $comment = question_utils::to_plain_text($comment, $commentformat); + $a->comment = shorten_text($comment, 200); } else { $a->comment = ''; } diff --git a/question/behaviour/rendererbase.php b/question/behaviour/rendererbase.php index b47488726e5..c791866eb4e 100644 --- a/question/behaviour/rendererbase.php +++ b/question/behaviour/rendererbase.php @@ -69,9 +69,14 @@ abstract class qbehaviour_renderer extends plugin_renderer_base { } public function manual_comment_fields(question_attempt $qa, question_display_options $options) { + global $CFG; + + require_once($CFG->dirroot.'/lib/filelib.php'); + require_once($CFG->dirroot.'/repository/lib.php'); + $inputname = $qa->get_behaviour_field_name('comment'); $id = $inputname . '_id'; - list($commenttext, $commentformat) = $qa->get_current_manual_comment(); + list($commenttext, $commentformat, $commentstep) = $qa->get_current_manual_comment(); $editor = editors_get_preferred_editor($commentformat); $strformats = format_text_menu(); @@ -80,12 +85,27 @@ abstract class qbehaviour_renderer extends plugin_renderer_base { $formats[$fid] = $strformats[$fid]; } + $draftitemareainputname = $qa->get_behaviour_field_name('comment:itemid'); + $draftitemid = optional_param($draftitemareainputname, false, PARAM_INT); + + if (!$draftitemid && $commentstep === null) { + $commenttext = ''; + $draftitemid = file_get_unused_draft_itemid(); + } else if (!$draftitemid) { + list($draftitemid, $commenttext) = $commentstep->prepare_response_files_draft_itemid_with_text( + 'bf_comment', $options->context->id, $commenttext); + } + $editor->set_text($commenttext); - $editor->use_editor($id, array('context' => $options->context)); + $editor->use_editor($id, question_utils::get_editor_options($options->context), + question_utils::get_filepicker_options($options->context, $draftitemid)); $commenteditor = html_writer::tag('div', html_writer::tag('textarea', s($commenttext), array('id' => $id, 'name' => $inputname, 'rows' => 10, 'cols' => 60))); + $attributes = ['type' => 'hidden', 'name' => $draftitemareainputname, 'value' => $draftitemid]; + $commenteditor .= html_writer::empty_tag('input', $attributes); + $editorformat = ''; if (count($formats) == 1) { reset($formats); @@ -105,7 +125,7 @@ abstract class qbehaviour_renderer extends plugin_renderer_base { $comment = html_writer::tag('div', html_writer::tag('div', html_writer::tag('label', get_string('comment', 'question'), array('for' => $id)), array('class' => 'fitemtitle')) . - html_writer::tag('div', $commenteditor, array('class' => 'felement fhtmleditor')), + html_writer::tag('div', $commenteditor, array('class' => 'felement fhtmleditor', 'data-fieldtype' => "editor")), array('class' => 'fitem')); $comment .= $editorformat; @@ -168,7 +188,7 @@ abstract class qbehaviour_renderer extends plugin_renderer_base { public function manual_comment_view(question_attempt $qa, question_display_options $options) { $output = ''; if ($qa->has_manual_comment()) { - $output .= get_string('commentx', 'question', $qa->get_behaviour()->format_comment()); + $output .= get_string('commentx', 'question', $qa->get_behaviour()->format_comment(null, null, $options->context)); } if ($options->manualcommentlink) { $url = new moodle_url($options->manualcommentlink, array('slot' => $qa->get_slot())); diff --git a/question/engine/questionattempt.php b/question/engine/questionattempt.php index 1f5ea9618b5..5473535b7c6 100644 --- a/question/engine/questionattempt.php +++ b/question/engine/questionattempt.php @@ -1391,16 +1391,17 @@ class question_attempt { /** * @return array(string, int) the most recent manual comment that was added - * to this question, and the FORMAT_... it is. + * to this question, the FORMAT_... it is and the step itself. */ public function get_manual_comment() { foreach ($this->get_reverse_step_iterator() as $step) { if ($step->has_behaviour_var('comment')) { return array($step->get_behaviour_var('comment'), - $step->get_behaviour_var('commentformat')); + $step->get_behaviour_var('commentformat'), + $step); } } - return array(null, null); + return array(null, null, null); } /** @@ -1420,7 +1421,7 @@ class question_attempt { if ($commentformat === null) { $commentformat = FORMAT_HTML; } - return array($comment, $commentformat); + return array($comment, $commentformat, null); } }