diff --git a/mod/assign/feedback/comments/locallib.php b/mod/assign/feedback/comments/locallib.php index bfe019c8386..6ba2770ab82 100644 --- a/mod/assign/feedback/comments/locallib.php +++ b/mod/assign/feedback/comments/locallib.php @@ -100,6 +100,29 @@ class assign_feedback_comments extends assign_feedback_plugin { return ($newvalue !== false) && ($newvalue != $commenttext); } + /** + * Has the comment feedback been modified? + * + * @param stdClass $grade The grade object. + * @param stdClass $data Data from the form submission. + * @return boolean True if the comment feedback has been modified, else false. + */ + public function is_feedback_modified(stdClass $grade, stdClass $data) { + $commenttext = ''; + if ($grade) { + $feedbackcomments = $this->get_feedback_comments($grade->id); + if ($feedbackcomments) { + $commenttext = $feedbackcomments->commenttext; + } + } + + if ($commenttext == $data->assignfeedbackcomments_editor['text']) { + return false; + } else { + return true; + } + } + /** * Override to indicate a plugin supports quickgrading. diff --git a/mod/assign/feedback/comments/tests/comments_test.php b/mod/assign/feedback/comments/tests/comments_test.php new file mode 100644 index 00000000000..8050ca42c1e --- /dev/null +++ b/mod/assign/feedback/comments/tests/comments_test.php @@ -0,0 +1,92 @@ +. + +/** + * Unit tests for assignfeedback_comments + * + * @package assignfeedback_comments + * @copyright 2016 Adrian Greeve + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/mod/assign/tests/base_test.php'); + +/** + * Unit tests for assignfeedback_comments + * + * @copyright 2016 Adrian Greeve + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class assignfeedback_comments_testcase extends mod_assign_base_testcase { + + /** + * Create an assign object and submit an online text submission. + */ + protected function create_assign_and_submit_text() { + $assign = $this->create_instance(array('assignsubmission_onlinetext_enabled' => 1, + 'assignfeedback_comments_enabled' => 1)); + + $user = $this->students[0]; + $this->setUser($user); + + // Create an online text submission. + $submission = $assign->get_user_submission($user->id, true); + + $data = new stdClass(); + $data->onlinetext_editor = array( + 'text' => '

This is some text.

', + 'format' => 1, + 'itemid' => file_get_unused_draft_itemid()); + $plugin = $assign->get_submission_plugin_by_type('onlinetext'); + $plugin->save($submission, $data); + + return $assign; + } + + /** + * Test the is_feedback_modified() method for the comments feedback. + */ + public function test_is_feedback_modified() { + $assign = $this->create_assign_and_submit_text(); + + $this->setUser($this->teachers[0]); + + // Create formdata. + $data = new stdClass(); + $data->assignfeedbackcomments_editor = array( + 'text' => '

first comment for this test

', + 'format' => 1 + ); + $grade = $assign->get_user_grade($this->students[0]->id, true); + + // This is the first time that we are submitting feedback, so it is modified. + $plugin = $assign->get_feedback_plugin_by_type('comments'); + $this->assertTrue($plugin->is_feedback_modified($grade, $data)); + // Save the feedback. + $plugin->save($grade, $data); + // Try again with the same data. + $this->assertFalse($plugin->is_feedback_modified($grade, $data)); + // Change the data. + $data->assignfeedbackcomments_editor = array( + 'text' => '

Altered comment for this test

', + 'format' => 1 + ); + $this->assertTrue($plugin->is_feedback_modified($grade, $data)); + } +} diff --git a/mod/assign/feedback/editpdf/locallib.php b/mod/assign/feedback/editpdf/locallib.php index 0bde7548b8e..57e0e71fee9 100644 --- a/mod/assign/feedback/editpdf/locallib.php +++ b/mod/assign/feedback/editpdf/locallib.php @@ -185,6 +185,53 @@ class assign_feedback_editpdf extends assign_feedback_plugin { } } + /** + * Check to see if the grade feedback for the pdf has been modified. + * + * @param stdClass $grade Grade object. + * @param stdClass $data Data from the form submission (not used). + * @return boolean True if the pdf has been modified, else false. + */ + public function is_feedback_modified(stdClass $grade, stdClass $data) { + global $USER; + $pagenumbercount = document_services::page_number_for_attempt($this->assignment, $grade->userid, $grade->attemptnumber); + for ($i = 0; $i < $pagenumbercount; $i++) { + // Select all annotations. + $draftannotations = page_editor::get_annotations($grade->id, $i, true); + $nondraftannotations = page_editor::get_annotations($grade->id, $i, false); + // Check to see if the count is the same. + if (count($draftannotations) != count($nondraftannotations)) { + // The count is different so we have a modification. + return true; + } else { + // Have a closer look and see if the draft files match the non draft files. + foreach ($nondraftannotations as $index => $ndannotation) { + foreach ($ndannotation as $key => $value) { + if ($key != 'id' && $value != $draftannotations[$index]->$key) { + return true; + } + } + } + } + // Select all comments. + $draftcomments = page_editor::get_comments($grade->id, $i, true); + $nondraftcomments = page_editor::get_comments($grade->id, $i, false); + if (count($draftcomments) != count($nondraftcomments)) { + return true; + } else { + // Go for a closer inspection. + foreach ($nondraftcomments as $index => $ndcomment) { + foreach ($ndcomment as $key => $value) { + if ($key != 'id' && $value != $draftcomments[$index]->$key) { + return true; + } + } + } + } + } + return false; + } + /** * Generate the pdf. * diff --git a/mod/assign/feedback/editpdf/tests/editpdf_test.php b/mod/assign/feedback/editpdf/tests/editpdf_test.php index 6afe93ffa8b..3a0ad840d2b 100644 --- a/mod/assign/feedback/editpdf/tests/editpdf_test.php +++ b/mod/assign/feedback/editpdf/tests/editpdf_test.php @@ -300,4 +300,124 @@ class assignfeedback_editpdf_testcase extends mod_assign_base_testcase { $this->assertEmpty($file3); } + + /** + * Test that modifying the annotated pdf form return true when modified + * and false when not modified. + */ + public function test_is_feedback_modified() { + global $DB; + $assign = $this->create_assign_and_submit_pdf(); + $this->setUser($this->teachers[0]); + + $grade = $assign->get_user_grade($this->students[0]->id, true); + + $notempty = page_editor::has_annotations_or_comments($grade->id, false); + $this->assertFalse($notempty); + + $comment = new comment(); + + $comment->rawtext = 'Comment text'; + $comment->width = 100; + $comment->x = 100; + $comment->y = 100; + $comment->colour = 'red'; + + page_editor::set_comments($grade->id, 0, array($comment)); + + $annotations = array(); + + $annotation = new annotation(); + $annotation->path = ''; + $annotation->x = 100; + $annotation->y = 100; + $annotation->endx = 200; + $annotation->endy = 200; + $annotation->type = 'line'; + $annotation->colour = 'red'; + array_push($annotations, $annotation); + + page_editor::set_annotations($grade->id, 0, $annotations); + + $plugin = $assign->get_feedback_plugin_by_type('editpdf'); + $data = new stdClass(); + $data->editpdf_source_userid = $this->students[0]->id; + $this->assertTrue($plugin->is_feedback_modified($grade, $data)); + $plugin->save($grade, $data); + + $annotation = new annotation(); + $annotation->gradeid = $grade->id; + $annotation->pageno = 0; + $annotation->path = ''; + $annotation->x = 100; + $annotation->y = 100; + $annotation->endx = 200; + $annotation->endy = 200; + $annotation->type = 'rectangle'; + $annotation->colour = 'yellow'; + + page_editor::add_annotation($annotation); + + // Add a comment as well. + $comment = new comment(); + $comment->gradeid = $grade->id; + $comment->pageno = 0; + $comment->rawtext = 'Second Comment text'; + $comment->width = 100; + $comment->x = 100; + $comment->y = 100; + $comment->colour = 'red'; + page_editor::add_comment($comment); + + $this->assertTrue($plugin->is_feedback_modified($grade, $data)); + $plugin->save($grade, $data); + + // We should have two annotations. + $this->assertCount(2, page_editor::get_annotations($grade->id, 0, false)); + // And two comments. + $this->assertCount(2, page_editor::get_comments($grade->id, 0, false)); + + // Add one annotation and delete another. + $annotation = new annotation(); + $annotation->gradeid = $grade->id; + $annotation->pageno = 0; + $annotation->path = '100,100:105,105:110,100'; + $annotation->x = 100; + $annotation->y = 100; + $annotation->endx = 110; + $annotation->endy = 105; + $annotation->type = 'pen'; + $annotation->colour = 'black'; + page_editor::add_annotation($annotation); + + $annotations = page_editor::get_annotations($grade->id, 0, true); + page_editor::remove_annotation($annotations[1]->id); + $this->assertTrue($plugin->is_feedback_modified($grade, $data)); + $plugin->save($grade, $data); + + // We should have two annotations. + $this->assertCount(2, page_editor::get_annotations($grade->id, 0, false)); + // And two comments. + $this->assertCount(2, page_editor::get_comments($grade->id, 0, false)); + + // Add a comment and then remove it. Should not be considered as modified. + $comment = new comment(); + $comment->gradeid = $grade->id; + $comment->pageno = 0; + $comment->rawtext = 'Third Comment text'; + $comment->width = 400; + $comment->x = 57; + $comment->y = 205; + $comment->colour = 'black'; + $comment->id = page_editor::add_comment($comment); + + // We should now have three comments. + $this->assertCount(3, page_editor::get_comments($grade->id, 0, true)); + // Now delete the newest record. + page_editor::remove_comment($comment->id); + // Back to two comments. + $this->assertCount(2, page_editor::get_comments($grade->id, 0, true)); + // No modification. + $this->assertFalse($plugin->is_feedback_modified($grade, $data)); + } } diff --git a/mod/assign/feedback/file/locallib.php b/mod/assign/feedback/file/locallib.php index c4d95bc1eba..27d1b344e29 100644 --- a/mod/assign/feedback/file/locallib.php +++ b/mod/assign/feedback/file/locallib.php @@ -77,6 +77,70 @@ class assign_feedback_file extends assign_feedback_plugin { return $fileoptions; } + /** + * Has the feedback file been modified? + * + * @param stdClass $grade Grade object. + * @param stdClass $data Form data. + * @return boolean True if the file area has been modified, else false. + */ + public function is_feedback_modified(stdClass $grade, stdClass $data) { + global $USER; + + $filekey = null; + $draftareainfo = null; + foreach ($data as $key => $value) { + if (strpos($key, 'files_') === 0) { + $filekey = $key; + } + } + if (isset($filekey)) { + $draftareainfo = file_get_draft_area_info($data->$filekey); + $filecount = $this->count_files($grade->id, ASSIGNFEEDBACK_FILE_FILEAREA); + if ($filecount != $draftareainfo['filecount']) { + return true; + } else { + // We need to check that the files in the draft area are the same as in the file area. + $usercontext = context_user::instance($USER->id); + $fs = get_file_storage(); + $draftfiles = $fs->get_area_files($usercontext->id, 'user', 'draft', $data->$filekey, 'id', true); + $files = $fs->get_area_files($this->assignment->get_context()->id, + 'assignfeedback_file', + ASSIGNFEEDBACK_FILE_FILEAREA, + $grade->id, + 'id', + false); + foreach ($files as $key => $file) { + // Flag for recording if we have a matching file. + $matchflag = false; + foreach ($draftfiles as $draftkey => $draftfile) { + if (!$file->is_directory()) { + // File name is the same, but it could be a different file with the same name. + if ($draftfile->get_filename() == $file->get_filename()) { + // If the file name is the same but the content hash is different, or + // The file path for the file has changed, then we have a modification. + if ($draftfile->get_contenthash() != $file->get_contenthash() || + $draftfile->get_filepath() != $file->get_filepath()) { + return true; + } + // These files match. Check the next file. + $matchflag = true; + // We have a match on the file name so we can move to the next file and not + // proceed through the other draftfiles. + break; + } + } + } + // If the file does not match then there has been a modification. + if (!$matchflag) { + return true; + } + } + } + } + return false; + } + /** * Copy all the files from one file area to another. * diff --git a/mod/assign/feedback/file/tests/file_test.php b/mod/assign/feedback/file/tests/file_test.php new file mode 100644 index 00000000000..6063dc1dad9 --- /dev/null +++ b/mod/assign/feedback/file/tests/file_test.php @@ -0,0 +1,189 @@ +. + +/** + * Unit tests for assignfeedback_file + * + * @package assignfeedback_file + * @copyright 2016 Adrian Greeve + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/mod/assign/tests/base_test.php'); + +/** + * Unit tests for assignfeedback_file + * + * @copyright 2016 Adrian Greeve + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class assignfeedback_file_testcase extends mod_assign_base_testcase { + + /** + * Create an assign object and submit an online text submission. + */ + protected function create_assign_and_submit_text() { + $assign = $this->create_instance(array('assignsubmission_onlinetext_enabled' => 1, + 'assignfeedback_comments_enabled' => 1)); + + $user = $this->students[0]; + $this->setUser($user); + + // Create an online text submission. + $submission = $assign->get_user_submission($user->id, true); + + $data = new stdClass(); + $data->onlinetext_editor = array( + 'text' => '

This is some text.

', + 'format' => 1, + 'itemid' => file_get_unused_draft_itemid()); + $plugin = $assign->get_submission_plugin_by_type('onlinetext'); + $plugin->save($submission, $data); + + return $assign; + } + + /** + * Test the is_feedback_modified() method for the file feedback. + */ + public function test_is_feedback_modified() { + $assign = $this->create_assign_and_submit_text(); + + $this->setUser($this->teachers[0]); + + $fs = get_file_storage(); + $context = context_user::instance($this->teachers[0]->id); + $draftitemid = file_get_unused_draft_itemid(); + file_prepare_draft_area($draftitemid, $context->id, 'assignfeedback_file', 'feedback_files', 1); + + $dummy = array( + 'contextid' => $context->id, + 'component' => 'user', + 'filearea' => 'draft', + 'itemid' => $draftitemid, + 'filepath' => '/', + 'filename' => 'feedback1.txt' + ); + + $file = $fs->create_file_from_string($dummy, 'This is the first feedback file'); + + // Create formdata. + $data = new stdClass(); + $data->{'files_' . $this->students[0]->id . '_filemanager'} = $draftitemid; + + $grade = $assign->get_user_grade($this->students[0]->id, true); + + // This is the first time that we are submitting feedback, so it is modified. + $plugin = $assign->get_feedback_plugin_by_type('file'); + $this->assertTrue($plugin->is_feedback_modified($grade, $data)); + // Save the feedback. + $plugin->save($grade, $data); + // Try again with the same data. + $draftitemid = file_get_unused_draft_itemid(); + file_prepare_draft_area($draftitemid, $context->id, 'assignfeedback_file', 'feedback_files', 1); + + $dummy['itemid'] = $draftitemid; + + $file = $fs->create_file_from_string($dummy, 'This is the first feedback file'); + + // Create formdata. + $data = new stdClass(); + $data->{'files_' . $this->students[0]->id . '_filemanager'} = $draftitemid; + + $this->assertFalse($plugin->is_feedback_modified($grade, $data)); + + // Same name for the file but different content. + $draftitemid = file_get_unused_draft_itemid(); + file_prepare_draft_area($draftitemid, $context->id, 'assignfeedback_file', 'feedback_files', 1); + + $dummy['itemid'] = $draftitemid; + + $file = $fs->create_file_from_string($dummy, 'This is different feedback'); + + // Create formdata. + $data = new stdClass(); + $data->{'files_' . $this->students[0]->id . '_filemanager'} = $draftitemid; + + $this->assertTrue($plugin->is_feedback_modified($grade, $data)); + $plugin->save($grade, $data); + + // Add another file. + $draftitemid = file_get_unused_draft_itemid(); + file_prepare_draft_area($draftitemid, $context->id, 'assignfeedback_file', 'feedback_files', 1); + + $dummy['itemid'] = $draftitemid; + + $file = $fs->create_file_from_string($dummy, 'This is different feedback'); + $dummy['filename'] = 'feedback2.txt'; + $file = $fs->create_file_from_string($dummy, 'A second feedback file'); + + // Create formdata. + $data = new stdClass(); + $data->{'files_' . $this->students[0]->id . '_filemanager'} = $draftitemid; + + $this->assertTrue($plugin->is_feedback_modified($grade, $data)); + $plugin->save($grade, $data); + + // Deleting a file. + $draftitemid = file_get_unused_draft_itemid(); + file_prepare_draft_area($draftitemid, $context->id, 'assignfeedback_file', 'feedback_files', 1); + + $dummy['itemid'] = $draftitemid; + + $file = $fs->create_file_from_string($dummy, 'This is different feedback'); + + // Create formdata. + $data = new stdClass(); + $data->{'files_' . $this->students[0]->id . '_filemanager'} = $draftitemid; + + $this->assertTrue($plugin->is_feedback_modified($grade, $data)); + $plugin->save($grade, $data); + + // The file was moved to a folder. + $draftitemid = file_get_unused_draft_itemid(); + file_prepare_draft_area($draftitemid, $context->id, 'assignfeedback_file', 'feedback_files', 1); + + $dummy['itemid'] = $draftitemid; + $dummy['filepath'] = '/testdir/'; + + $file = $fs->create_file_from_string($dummy, 'This is different feedback'); + + // Create formdata. + $data = new stdClass(); + $data->{'files_' . $this->students[0]->id . '_filemanager'} = $draftitemid; + + $this->assertTrue($plugin->is_feedback_modified($grade, $data)); + $plugin->save($grade, $data); + + // No modification to the file in the folder. + $draftitemid = file_get_unused_draft_itemid(); + file_prepare_draft_area($draftitemid, $context->id, 'assignfeedback_file', 'feedback_files', 1); + + $dummy['itemid'] = $draftitemid; + $dummy['filepath'] = '/testdir/'; + + $file = $fs->create_file_from_string($dummy, 'This is different feedback'); + + // Create formdata. + $data = new stdClass(); + $data->{'files_' . $this->students[0]->id . '_filemanager'} = $draftitemid; + + $this->assertFalse($plugin->is_feedback_modified($grade, $data)); + } +} diff --git a/mod/assign/feedbackplugin.php b/mod/assign/feedbackplugin.php index 6d793f47a00..8398a49ae44 100644 --- a/mod/assign/feedbackplugin.php +++ b/mod/assign/feedbackplugin.php @@ -112,6 +112,19 @@ abstract class assign_feedback_plugin extends assign_plugin { return false; } + /** + * Has the plugin form element been modified in the current submission? + * + * @param stdClass $grade The grade. + * @param stdClass $data Form data from the feedback form. + * @return boolean - True if the form element has been modified. + */ + public function is_feedback_modified(stdClass $grade, stdClass $data) { + debugging('This plugin has not overwritten the is_feedback_modified() method. Please add this method to your plugin', + DEBUG_DEVELOPER); + return true; + } + /** * Save quickgrading changes. * diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index 5d44ce84494..aa6d88792c6 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -6838,12 +6838,19 @@ class assign { $adminconfig = $this->get_admin_config(); $gradebookplugin = $adminconfig->feedback_plugin_for_gradebook; + $feedbackmodified = false; + // Call save in plugins. foreach ($this->feedbackplugins as $plugin) { if ($plugin->is_enabled() && $plugin->is_visible()) { - if (!$plugin->save($grade, $formdata)) { - $result = false; - print_error($plugin->get_error()); + $gradingmodified = $plugin->is_feedback_modified($grade, $formdata); + if ($gradingmodified) { + if (!$plugin->save($grade, $formdata)) { + $result = false; + print_error($plugin->get_error()); + } + // If $feedbackmodified is true, keep it true. + $feedbackmodified = $feedbackmodified || $gradingmodified; } if (('assignfeedback_' . $plugin->get_type()) == $gradebookplugin) { // This is the feedback plugin chose to push comments to the gradebook. @@ -6852,10 +6859,12 @@ class assign { } } } + // We do not want to update the timemodified if no grade was added. if (!empty($formdata->addattempt) || ($originalgrade !== null && $originalgrade != -1) || - ($grade->grade !== null && $grade->grade != -1)) { + ($grade->grade !== null && $grade->grade != -1) || + $feedbackmodified) { $this->update_grade($grade, !empty($formdata->addattempt)); } // Note the default if not provided for this option is true (e.g. webservices). diff --git a/mod/assign/tests/locallib_test.php b/mod/assign/tests/locallib_test.php index 547ac86a876..f80ec785694 100644 --- a/mod/assign/tests/locallib_test.php +++ b/mod/assign/tests/locallib_test.php @@ -781,7 +781,7 @@ class mod_assign_locallib_testcase extends mod_assign_base_testcase { // Wait 1 second so the submission and grade do not have the same timemodified. sleep(1); // Simulate adding a grade. - $this->setUser($this->teachers[0]); + $this->setUser($this->editingteachers[0]); $data = new stdClass(); $data->grade = '50.0'; $assign1->testable_apply_grade_to_user($data, $this->extrastudents[3]->id, 0); @@ -898,7 +898,7 @@ class mod_assign_locallib_testcase extends mod_assign_base_testcase { $plugin->save($submission, $data); // Simulate adding a grade. - $this->setUser($this->teachers[0]); + $this->setUser($this->editingteachers[0]); $data = new stdClass(); $data->grade = '50.0'; $assign->testable_apply_grade_to_user($data, $this->extrastudents[3]->id, 0); diff --git a/mod/assign/upgrade.txt b/mod/assign/upgrade.txt index d8d0282c4d7..7cadc8d1694 100644 --- a/mod/assign/upgrade.txt +++ b/mod/assign/upgrade.txt @@ -1,5 +1,9 @@ This files describes API changes in the assign code. +=== 3.1 === +* The feedback plugins now need to implement the is_feedback_modified() method. The default is to return true + for backwards compatibiltiy. + === 3.0 === * assign_submission_status renderable now requires $usergroups in its constructor