From 6a9063c588318cbc2cb28fd8beb35cf7200999e6 Mon Sep 17 00:00:00 2001 From: Thong Bui Date: Mon, 19 Apr 2021 11:28:08 +0700 Subject: [PATCH] MDL-71030 Quiz: Add 'who' made changes in question response history --- mod/quiz/attemptlib.php | 9 +++ mod/quiz/locallib.php | 1 + mod/quiz/review.php | 1 + mod/quiz/reviewquestion.php | 1 + .../behat/attempt_redo_questions.feature | 48 +++++++++---- question/engine/lib.php | 10 ++- question/engine/questionattemptstep.php | 34 ++++++++++ question/engine/questionusage.php | 29 ++++++++ question/engine/renderer.php | 28 +++++--- .../engine/tests/questionattemptstep_test.php | 47 +++++++++++++ .../tests/questionusagebyactivity_test.php | 32 +++++++++ question/engine/tests/walkthrough_test.php | 68 +++++++++++++++++++ 12 files changed, 287 insertions(+), 21 deletions(-) diff --git a/mod/quiz/attemptlib.php b/mod/quiz/attemptlib.php index 51fe4c030e6..d20cf0d07f9 100644 --- a/mod/quiz/attemptlib.php +++ b/mod/quiz/attemptlib.php @@ -719,6 +719,15 @@ class quiz_attempt { $this->number_questions(); } + /** + * Preload all attempt step users to show in Response history. + * + * @throws dml_exception + */ + public function preload_all_attempt_step_users(): void { + $this->quba->preload_all_step_users(); + } + /** * Let each slot know which section it is part of. */ diff --git a/mod/quiz/locallib.php b/mod/quiz/locallib.php index 46298e4e243..c78df8220ca 100644 --- a/mod/quiz/locallib.php +++ b/mod/quiz/locallib.php @@ -1528,6 +1528,7 @@ function quiz_get_review_options($quiz, $attempt, $context) { $options->rightanswer = question_display_options::VISIBLE; $options->overallfeedback = question_display_options::VISIBLE; $options->history = question_display_options::VISIBLE; + $options->userinfoinhistory = $attempt->userid; } diff --git a/mod/quiz/review.php b/mod/quiz/review.php index 6773b91081c..1c1d42fde15 100644 --- a/mod/quiz/review.php +++ b/mod/quiz/review.php @@ -44,6 +44,7 @@ if ($page !== 0) { $PAGE->set_url($url); $attemptobj = quiz_create_attempt_handling_errors($attemptid, $cmid); +$attemptobj->preload_all_attempt_step_users(); $page = $attemptobj->force_page_number_into_range($page); // Now we can validate the params better, re-genrate the page URL. diff --git a/mod/quiz/reviewquestion.php b/mod/quiz/reviewquestion.php index 91313890428..f49306b9774 100644 --- a/mod/quiz/reviewquestion.php +++ b/mod/quiz/reviewquestion.php @@ -41,6 +41,7 @@ if (!is_null($seq)) { $PAGE->set_url($currenturl); $attemptobj = quiz_create_attempt_handling_errors($attemptid, $cmid); +$attemptobj->preload_all_attempt_step_users(); // Check login. require_login($attemptobj->get_course(), false, $attemptobj->get_cm()); diff --git a/mod/quiz/tests/behat/attempt_redo_questions.feature b/mod/quiz/tests/behat/attempt_redo_questions.feature index 2e63a893f35..ffc0697e82e 100644 --- a/mod/quiz/tests/behat/attempt_redo_questions.feature +++ b/mod/quiz/tests/behat/attempt_redo_questions.feature @@ -13,19 +13,19 @@ Feature: Allow students to redo questions in a practice quiz, without starting a | fullname | shortname | category | | Course 1 | C1 | 0 | And the following "course enrolments" exist: - | user | course | role | - | student | C1 | student | - | teacher | C1 | teacher | + | user | course | role | + | student | C1 | student | + | teacher | C1 | teacher | And the following "question categories" exist: | contextlevel | reference | name | | Course | C1 | Test questions | And the following "questions" exist: - | questioncategory | qtype | name | questiontext | - | Test questions | truefalse | TF1 | First question | - | Test questions | truefalse | TF2 | Second question | + | questioncategory | qtype | name | questiontext | + | Test questions | truefalse | TF1 | First question | + | Test questions | truefalse | TF2 | Second question | And the following "activities" exist: - | activity | name | intro | course | idnumber | preferredbehaviour | canredoquestions | - | quiz | Quiz 1 | Quiz 1 description | C1 | quiz1 | immediatefeedback | 1 | + | activity | name | intro | course | idnumber | preferredbehaviour | canredoquestions | + | quiz | Quiz 1 | Quiz 1 description | C1 | quiz1 | immediatefeedback | 1 | And quiz "Quiz 1" contains the following questions: | question | page | maxmark | | TF1 | 1 | 2 | @@ -106,11 +106,11 @@ Feature: Allow students to redo questions in a practice quiz, without starting a @javascript Scenario: Redoing questions should work with random questions as well Given the following "questions" exist: - | questioncategory | qtype | name | questiontext | - | Test questions | random | Random (Test questions) | 0 | + | questioncategory | qtype | name | questiontext | + | Test questions | random | Random (Test questions) | 0 | And the following "activities" exist: - | activity | name | intro | course | idnumber | preferredbehaviour | canredoquestions | - | quiz | Quiz 2 | Quiz 2 description | C1 | quiz2 | immediatefeedback | 1 | + | activity | name | intro | course | idnumber | preferredbehaviour | canredoquestions | + | quiz | Quiz 2 | Quiz 2 description | C1 | quiz2 | immediatefeedback | 1 | And quiz "Quiz 2" contains the following questions: | question | page | | Random (Test questions) | 1 | @@ -125,3 +125,27 @@ Feature: Allow students to redo questions in a practice quiz, without starting a And I press "Try another question like this one" Then I should see "Second question" And "Check" "button" should exist + + Scenario: Teachers reviewing can see author of action in review attempt + Given the following "questions" exist: + | questioncategory | qtype | name | questiontext | answer 1 | grade | + | Test questions | shortanswer | SA1 | Who is author of Harry Potter? | J.K.Rowling | 100% | + And the following "activities" exist: + | activity | name | intro | course | idnumber | + | quiz | Quiz 2 | Quiz 2 description | C1 | quiz2 | + And quiz "Quiz 2" contains the following questions: + | question | page | + | SA1 | 1 | + And user "student" has attempted "Quiz 2" with responses: + | slot | response | + | 1 | J.K.Rowling | + And I am on the "Quiz 2" "mod_quiz > View" page logged in as "teacher" + And I navigate to "Results > Manual grading" in current page administration + And I follow "Also show questions that have been graded automatically" + When I click on "update grades" "link" in the "SA1" "table_row" + Then I set the field "Comment" to "I have adjusted your mark to 1.0" + And I set the field "Mark" to "1.0" + And I press "Save and go to next page" + And I follow "Results" + And I follow "Review attempt" + And I should see "Teacher One" in the "I have adjusted your mark to 1.0" "table_row" diff --git a/question/engine/lib.php b/question/engine/lib.php index b1f21d93af3..76d7707fd3a 100644 --- a/question/engine/lib.php +++ b/question/engine/lib.php @@ -476,7 +476,10 @@ abstract class question_engine { * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class question_display_options { - /**#@+ @var integer named constants for the values that most of the options take. */ + /**#@+ + * @var integer named constants for the values that most of the options take. + */ + const SHOW_ALL = -1; const HIDDEN = 0; const VISIBLE = 1; const EDITABLE = 2; @@ -631,6 +634,11 @@ class question_display_options { */ public $context; + /** + * @var int The option to show the action author in the response history. + */ + public $userinfoinhistory = self::HIDDEN; + /** * Set all the feedback-related fields {@link $feedback}, {@link generalfeedback}, * {@link rightanswer} and {@link manualcomment} to diff --git a/question/engine/questionattemptstep.php b/question/engine/questionattemptstep.php index 3781806915a..d16c88b0ef1 100644 --- a/question/engine/questionattemptstep.php +++ b/question/engine/questionattemptstep.php @@ -94,6 +94,9 @@ class question_attempt_step { /** @var array name => array of {@link stored_file}s. Caches the contents of file areas. */ private $files = array(); + /** @var stdClass User information. */ + private $user = null; + /** * You should not need to call this constructor in your own code. Steps are * normally created by {@link question_attempt} methods like @@ -172,6 +175,37 @@ class question_attempt_step { return $this->userid; } + /** + * Update full user information for step. + * + * @param stdClass $user Full user object. + * @throws coding_exception + */ + public function add_full_user_object(stdClass $user): void { + if ($user->id != $this->userid) { + throw new coding_exception('Wrong user passed to add_full_user_object'); + } + $this->user = $user; + } + + /** + * Return the full user object. + * + * @return stdClass Get full user object. + */ + public function get_user(): stdClass { + return $this->user; + } + + /** + * Get full name of user who did action. + * + * @return string full name of user. + */ + public function get_user_fullname(): string { + return fullname($this->user); + } + /** @return int the timestamp when this step was created. */ public function get_timecreated() { return $this->timecreated; diff --git a/question/engine/questionusage.php b/question/engine/questionusage.php index 7991c91dd58..9c5effb24fd 100644 --- a/question/engine/questionusage.php +++ b/question/engine/questionusage.php @@ -967,6 +967,35 @@ class question_usage_by_activity { return $quba; } + + /** + * Preload users of all question attempt steps. + * + * @throws dml_exception + */ + public function preload_all_step_users(): void { + global $DB; + + // Get all user ids. + $userids = []; + foreach ($this->questionattempts as $qa) { + foreach ($qa->get_full_step_iterator() as $step) { + $userids[$step->get_user_id()] = 1; + } + } + + // Load user information. + $users = $DB->get_records_list('user', 'id', array_keys($userids), '', '*'); + // Update user information for steps. + foreach ($this->questionattempts as $qa) { + foreach ($qa->get_full_step_iterator() as $step) { + $user = $users[$step->get_user_id()]; + if (isset($user)) { + $step->add_full_user_object($user); + } + } + } + } } diff --git a/question/engine/renderer.php b/question/engine/renderer.php index 3e59ce98d0a..0a3c83655ac 100644 --- a/question/engine/renderer.php +++ b/question/engine/renderer.php @@ -471,14 +471,10 @@ class core_question_renderer extends plugin_renderer_base { $restrictedqa = new question_attempt_with_restricted_history($qa, $i, null); - $user = new stdClass(); - $user->id = $step->get_user_id(); - $row = array( - $stepno, - userdate($step->get_timecreated(), get_string('strftimedatetimeshort')), - s($qa->summarise_action($step)), - $restrictedqa->get_state_string($options->correctness), - ); + $row = [$stepno, + userdate($step->get_timecreated(), get_string('strftimedatetimeshort')), + s($qa->summarise_action($step)) . $this->action_author($step, $options), + $restrictedqa->get_state_string($options->correctness)]; if ($options->marks >= question_display_options::MARK_AND_MAX) { $row[] = $qa->format_fraction_as_mark($step->get_fraction(), $options->markdp); @@ -495,4 +491,20 @@ class core_question_renderer extends plugin_renderer_base { array('class' => 'responsehistoryheader')); } + /** + * Action author's profile link. + * + * @param question_attempt_step $step The step. + * @param question_display_options $options The display options. + * @return string The link to user's profile. + */ + protected function action_author(question_attempt_step $step, question_display_options $options): string { + if ($options->userinfoinhistory && $step->get_user_id() != $options->userinfoinhistory) { + return html_writer::link( + new moodle_url('/user/view.php', ['id' => $step->get_user_id(), 'course' => $this->page->course->id]), + $step->get_user_fullname(), ['class' => 'd-table-cell']); + } else { + return ''; + } + } } diff --git a/question/engine/tests/questionattemptstep_test.php b/question/engine/tests/questionattemptstep_test.php index b7a73ab0d46..59139df0ade 100644 --- a/question/engine/tests/questionattemptstep_test.php +++ b/question/engine/tests/questionattemptstep_test.php @@ -128,4 +128,51 @@ class question_attempt_step_test extends advanced_testcase { $this->assertEquals(array(), $step->get_behaviour_data()); } + + + /** + * Test get_user function. + */ + public function test_get_user() { + $this->resetAfterTest(true); + $student = $this->getDataGenerator()->create_user(); + + $step = new question_attempt_step(array(), 123, $student->id); + $step->add_full_user_object($student); + + $this->assertEquals($student, $step->get_user()); + } + + /** + * Test get_user_fullname function. + */ + public function test_get_user_fullname() { + $this->resetAfterTest(true); + $student = $this->getDataGenerator()->create_user(); + + $step = new question_attempt_step(array(), 123, $student->id); + $step->add_full_user_object($student); + + $this->assertEquals(fullname($student), $step->get_user_fullname()); + } + + /** + * Test add_full_user_object function. + */ + public function test_add_full_user_object() { + $this->resetAfterTest(true); + $student1 = $this->getDataGenerator()->create_user(); + $student2 = $this->getDataGenerator()->create_user(); + + $step = new question_attempt_step(array(), 123, $student1->id); + + // Add full user with the valid user. + $step->add_full_user_object($student1); + $this->assertEquals($student1, $step->get_user()); + + // Throw exception with the invalid user. + $this->expectException('coding_exception'); + $this->expectExceptionMessage('Wrong user passed to add_full_user_object'); + $step->add_full_user_object($student2); + } } diff --git a/question/engine/tests/questionusagebyactivity_test.php b/question/engine/tests/questionusagebyactivity_test.php index 2e9dbe3bba0..f69198a0299 100644 --- a/question/engine/tests/questionusagebyactivity_test.php +++ b/question/engine/tests/questionusagebyactivity_test.php @@ -158,4 +158,36 @@ class question_usage_by_activity_test extends advanced_testcase { $this->expectException('question_out_of_sequence_exception'); $quba->process_all_actions($slot, $postdata); } + + /** + * Test function preload all step users. + */ + public function test_preload_all_step_users() { + $this->resetAfterTest(); + $this->setAdminUser(); + // Set up. + $quba = question_engine::make_questions_usage_by_activity('unit_test', + context_system::instance()); + + // Create an essay question in the DB. + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $cat = $generator->create_question_category(); + $essay = $generator->create_question('essay', 'editorfilepicker', ['category' => $cat->id]); + + // Start attempt at the question. + $q = question_bank::load_question($essay->id); + $quba->set_preferred_behaviour('deferredfeedback'); + $slot = $quba->add_question($q, 10); + $quba->start_question($slot, 1); + + // Finish the attempt. + $quba->finish_all_questions(); + question_engine::save_questions_usage_by_activity($quba); + + // The user information of question attempt step should be loaded. + $quba->preload_all_step_users(); + $qa = $quba->get_attempt_iterator()->current(); + $steps = $qa->get_full_step_iterator(); + $this->assertEquals('Admin User', $steps[0]->get_user_fullname()); + } } diff --git a/question/engine/tests/walkthrough_test.php b/question/engine/tests/walkthrough_test.php index 79fd0217b5f..2b63144721e 100644 --- a/question/engine/tests/walkthrough_test.php +++ b/question/engine/tests/walkthrough_test.php @@ -60,4 +60,72 @@ class core_question_walkthrough_testcase extends qbehaviour_walkthrough_test_bas // Verify the flag has not been lost. $this->assertTrue($this->get_question_attempt()->is_flagged()); } + + /** + * Test action_author function. + */ + public function test_action_author_with_display_options_testcase() { + $this->resetAfterTest(true); + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $teacher = $this->getDataGenerator()->create_user(); + $student = $this->getDataGenerator()->create_user(); + + // Create an essay question in the DB. + $cat = $generator->create_question_category(); + $essay = $generator->create_question('essay', 'editorfilepicker', ['category' => $cat->id]); + + // Start attempt at the question. + $q = question_bank::load_question($essay->id); + + // Student attempt the question. + $this->setUser($student); + $this->start_attempt_at_question($q, 'deferredfeedback', 10, 1); + + // Simulate some data submitted by the student. + $this->process_submission(['answer' => 'This is my wonderful essay!', 'answerformat' => FORMAT_HTML]); + $this->finish(); + + // Process a manual comment. + $this->setUser($teacher); + $this->manual_grade('Not good enough!', 10, FORMAT_HTML); + $this->render(); + $this->save_quba(); + + // Set display option userinfoinhistory to HIDDEN. + $displayoptions = new question_display_options(); + $displayoptions->history = question_display_options::VISIBLE; + $displayoptions->userinfoinhistory = question_display_options::HIDDEN; + + $this->load_quba(); + $result = $this->quba->render_question($this->slot, $displayoptions); + + // The profile user link should not display. + preg_match("/(.*)<\/a>/", $result, $matches); + $this->assertEquals(false, isset($matches[0])); + + // Set display option userinfoinhistory to SHOW_ALL. + $displayoptions = new question_display_options(); + $displayoptions->history = question_display_options::VISIBLE; + $displayoptions->userinfoinhistory = question_display_options::SHOW_ALL; + + $this->load_quba(); + $result = $this->quba->render_question($this->slot, $displayoptions); + $numsteps = $this->quba->get_question_attempt($this->slot)->get_num_steps(); + + // All steps in the result should contain user profile link. + preg_match_all("/(.*)<\/a>/", $result, $matches); + $this->assertEquals($numsteps, count($matches[0])); + + // Set the userinfoinhistory to student id. + $displayoptions = new question_display_options(); + $displayoptions->history = question_display_options::VISIBLE; + $displayoptions->userinfoinhistory = $student->id; + + $this->load_quba(); + $result = $this->quba->render_question($this->slot, $displayoptions); + + // The step just show the user profile link if the step's userid is different with student id. + preg_match_all("/(.*)<\/a>/", $result, $matches); + $this->assertEquals(1, count($matches[0])); + } }