diff --git a/question/engine/datalib.php b/question/engine/datalib.php index 175103d06df..beca1de177b 100644 --- a/question/engine/datalib.php +++ b/question/engine/datalib.php @@ -1279,22 +1279,21 @@ class question_file_saver implements question_response_files { $string .= $file->get_filepath() . $file->get_filename() . '|' . $file->get_contenthash() . '|'; } - - if ($string) { - $hash = md5($string); - } else { - $hash = ''; - } + $hash = md5($string); if (is_null($text)) { - return $hash; + if ($string) { + return $hash; + } else { + return ''; + } } // We add the file hash so a simple string comparison will say if the // files have been changed. First strip off any existing file hash. - $text = preg_replace('/\s*\s*$/', '', $text); - $text = file_rewrite_urls_to_pluginfile($text, $draftitemid); - if ($hash) { + if ($text !== '') { + $text = preg_replace('/\s*\s*$/', '', $text); + $text = file_rewrite_urls_to_pluginfile($text, $draftitemid); $text .= ''; } return $text; @@ -1391,8 +1390,15 @@ class question_file_loader implements question_response_files { */ public function get_question_file_saver() { - // Value will be either a plain MD5 hash, or some real content, followed - // by an MD5 hash in a HTML comment. We only want the value in the latter case. + // There are three possibilities here for what $value will look like: + // 1) some HTML content followed by an MD5 hash in a HTML comment; + // 2) a plain MD5 hash; + // 3) or some real content, without any hash. + // The problem is that 3) is ambiguous in the case where a student writes + // a response that looks exactly like an MD5 hash. For attempts made now, + // we avoid case 3) by always going for case 1) or 2) (except when the + // response is blank. However, there may be case 3) data in the database + // so we need to handle it as best we can. if (preg_match('/\s*\s*$/', $this->value)) { $value = preg_replace('/\s*\s*$/', '', $this->value); @@ -1400,8 +1406,7 @@ class question_file_loader implements question_response_files { $value = null; } else { - throw new coding_exception('$value passed to question_file_loader::get_question_file_saver' . - ' was not of the expected form.'); + $value = $this->value; } list($draftid, $text) = $this->step->prepare_response_files_draft_itemid_with_text( diff --git a/question/type/essay/tests/walkthrough_test.php b/question/type/essay/tests/walkthrough_test.php index fbc7a1ad811..e64149212cd 100644 --- a/question/type/essay/tests/walkthrough_test.php +++ b/question/type/essay/tests/walkthrough_test.php @@ -348,4 +348,77 @@ class qtype_essay_walkthrough_testcase extends qbehaviour_walkthrough_test_base $this->check_current_mark(null); $this->check_step_count(1); } + + public function test_deferred_feedback_html_editor_with_files_attempt_on_last_no_files_uploaded() { + global $CFG, $USER; + + $this->resetAfterTest(true); + $this->setAdminUser(); + $usercontextid = context_user::instance($USER->id)->id; + $fs = get_file_storage(); + + // Create an essay question in the DB. + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $cat = $generator->create_question_category(); + $question = $generator->create_question('essay', 'editorfilepicker', array('category' => $cat->id)); + + // Start attempt at the question. + $q = question_bank::load_question($question->id); + $this->start_attempt_at_question($q, 'deferredfeedback', 1); + + $this->check_current_state(question_state::$todo); + $this->check_current_mark(null); + $this->check_step_count(1); + + // Process a response and check the expected result. + // First we need to get the draft item ids. + $this->render(); + if (!preg_match('/env=editor&.*?itemid=(\d+)&/', $this->currentoutput, $matches)) { + throw new coding_exception('Editor draft item id not found.'); + } + $editordraftid = $matches[1]; + if (!preg_match('/env=filemanager&action=browse&.*?itemid=(\d+)&/', $this->currentoutput, $matches)) { + throw new coding_exception('File manager draft item id not found.'); + } + $attachementsdraftid = $matches[1]; + + $this->process_submission(array( + 'answer' => 'I refuse to draw you a picture, so there!', + 'answerformat' => FORMAT_HTML, + 'answer:itemid' => $editordraftid, + 'attachments' => $attachementsdraftid)); + + $this->check_current_state(question_state::$complete); + $this->check_current_mark(null); + $this->check_step_count(2); + $this->save_quba(); + + // Now submit all and finish. + $this->finish(); + $this->check_current_state(question_state::$needsgrading); + $this->check_current_mark(null); + $this->check_step_count(3); + $this->save_quba(); + + // Now start a new attempt based on the old one. + $this->load_quba(); + $oldqa = $this->get_question_attempt(); + + $q = question_bank::load_question($question->id); + $this->quba = question_engine::make_questions_usage_by_activity('unit_test', + context_system::instance()); + $this->quba->set_preferred_behaviour('deferredfeedback'); + $this->slot = $this->quba->add_question($q, 1); + $this->quba->start_question_based_on($this->slot, $oldqa); + + $this->check_current_state(question_state::$complete); + $this->check_current_mark(null); + $this->check_step_count(1); + $this->save_quba(); + + // Check the display. + $this->load_quba(); + $this->render(); + $this->assertRegExp('/I refuse to draw you a picture, so there!/', $this->currentoutput); + } }