diff --git a/lang/en/question.php b/lang/en/question.php index 0c1403284040c..977fede5b85ee 100644 --- a/lang/en/question.php +++ b/lang/en/question.php @@ -100,8 +100,13 @@ $string['defaultinfofor'] = 'The default category for questions shared in context \'{$a}\'.'; $string['defaultmarkmustbepositive'] = 'The default mark must be positive.'; $string['deletecoursecategorywithquestions'] = 'There are questions in the question bank associated with this course category. If you proceed, they will be deleted. You may wish to move them first, using the question bank interface.'; -$string['deletequestioncheck'] = 'Are you absolutely sure you want to delete \'{$a}\'?'; -$string['deletequestionscheck'] = 'Are you absolutely sure you want to delete the following questions?

{$a}'; +$string['deletequestioncheck'] = 'This will delete the following question and all its versions:

{$a}'; +$string['deletequestionscheck'] = 'This will delete the following questions and all their versions:

{$a}'; +$string['deleteselectedquestioncheck'] = 'This will delete selected versions of the following question:

{$a}'; +$string['deletequestiontitle'] = 'Delete question?'; +$string['deletequestiontitle_plural'] = 'Delete questions?'; +$string['deleteversiontitle'] = 'Delete selected version?'; +$string['deleteversiontitle_plural'] = 'Delete selected versions?'; $string['deletingbehaviour'] = 'Deleting question behaviour \'{$a}\''; $string['deletingqtype'] = 'Deleting question type \'{$a}\''; $string['didnotmatchanyanswer'] = '[Did not match any answer]'; @@ -288,9 +293,10 @@ $string['questioncatsfor'] = 'Question categories for \'{$a}\''; $string['questiondoesnotexist'] = 'This question does not exist'; $string['questionname'] = 'Question name'; +$string['questionnameandquestionversion'] = '{$a->name} v{$a->version}'; $string['questionno'] = 'Question {$a}'; $string['questionsaveerror'] = 'Errors occur during saving question - ({$a})'; -$string['questionsinuse'] = '(* Questions marked with an asterisk are used somewhere, for example in a quiz. Therefore, if you proceed, these questions will not really be deleted, they will just be hidden.)'; +$string['questionsinuse'] = '* Denotes questions which can\'t be deleted because they are in use. Instead, they will be hidden in the question bank unless you select \'Show old questions\'.'; $string['questionsmovedto'] = 'Questions still in use moved to "{$a}" in the parent course category.'; $string['questionsrescuedfrom'] = 'Questions saved from context {$a}.'; $string['questionsrescuedfrominfo'] = 'These questions (some of which may be hidden) were saved when context {$a} was deleted because they are still used by some quizzes or other activities.'; diff --git a/question/bank/deletequestion/classes/delete_action_column.php b/question/bank/deletequestion/classes/delete_action_column.php index 361f25bc25064..3481246f6f178 100644 --- a/question/bank/deletequestion/classes/delete_action_column.php +++ b/question/bank/deletequestion/classes/delete_action_column.php @@ -96,6 +96,9 @@ protected function get_url_icon_and_label(\stdClass $question): array { 'q' . $question->id => 1, 'sesskey' => sesskey()); $deleteparams = array_merge($deleteparams, $this->returnparams); + if ($this->qbank->base_url()->get_param('deleteall')) { + $deleteparams['deleteall'] = 1; + } $url = new \moodle_url($this->deletequestionurl, $deleteparams); return [$url, 't/delete', $this->strdelete]; } diff --git a/question/bank/deletequestion/classes/helper.php b/question/bank/deletequestion/classes/helper.php new file mode 100644 index 0000000000000..5f7fd246be342 --- /dev/null +++ b/question/bank/deletequestion/classes/helper.php @@ -0,0 +1,117 @@ +. + +namespace qbank_deletequestion; + +/** + * Class helper of qbank_deletequestion. + * + * @package qbank_deletequestion + * @copyright 2023 The Open University + * @since Moodle 4.2 + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class helper { + + /** + * Get the confirmation message of delete question. + * + * @param array $questionids List of id questions. + * @param bool $deleteallversions Delete all question version or not. + * @return array List confirmation message. + */ + public static function get_delete_confirmation_message(array $questionids, bool $deleteallversions): array { + global $DB; + $questionnames = ''; + $inuse = false; + $hasmutipleversions = false; + $questionversions = []; + $countselectedquestion = count($questionids); + if ($deleteallversions) { + $listofquestions = \question_bank::get_all_versions_of_questions($questionids); + foreach ($listofquestions as $questionbankentry) { + if (count($questionbankentry) > 1 && !$hasmutipleversions) { + $hasmutipleversions = true; + } + // Flip the array to list question by question id. [ qid => qversion ]. + $questionversions += array_flip($questionbankentry); + } + // Flatten an array. + $questionids = array_merge(...$listofquestions); + } + [$questionsql, $params] = $DB->get_in_or_equal($questionids, SQL_PARAMS_NAMED); + $questions = $DB->get_records_select('question', 'id ' . $questionsql, $params, + 'name ASC', 'id, name'); + foreach ($questions as $question) { + if (questions_in_use([$question->id])) { + $questionnames .= '* '; + $inuse = true; + } + $questionname = format_string($question->name); + if (isset($questionversions[$question->id])) { + $a = new \stdClass(); + $a->name = $questionname; + $a->version = $questionversions[$question->id]; + $questionnames .= get_string('questionnameandquestionversion', + 'question', $a) . '
'; + } else { + $questionnames .= $questionname . '
'; + } + } + if ($inuse) { + $questionnames .= '
'.get_string('questionsinuse', 'question'); + } + $confirmtitle = [ + 'confirmtitle' => $countselectedquestion > 1 ? get_string('deleteversiontitle_plural', + 'question') : get_string('deleteversiontitle', 'question'), + ]; + $message = get_string('deleteselectedquestioncheck', 'question', $questionnames); + if ($deleteallversions) { + $confirmtitle = [ + 'confirmtitle' => get_string('deletequestiontitle', 'question'), + ]; + $message = get_string('deletequestioncheck', 'question', $questionnames); + if ($countselectedquestion > 1) { + $confirmtitle = [ + 'confirmtitle' => get_string('deletequestiontitle_plural', 'question'), + ]; + $message = get_string('deletequestionscheck', 'question', $questionnames); + } + } + + return [$confirmtitle, $message]; + } + + /** + * Delete questions has (single/multiple) version. + * + * @param array $questionids List of questionid. + * @param bool $deleteallversions Delete all question version or not. + */ + public static function delete_questions(array $questionids, bool $deleteallversions): void { + if ($deleteallversions) { + // Get all the question id from multidimensional array. + $listofquestions = \question_bank::get_all_versions_of_questions($questionids); + // Flatten an array. + $questionids = array_merge(...$listofquestions); + } + foreach ($questionids as $questionid) { + $questionid = (int) $questionid; + question_require_capability_on($questionid, 'edit'); + question_delete_question($questionid); + } + } +} diff --git a/question/bank/deletequestion/delete.php b/question/bank/deletequestion/delete.php index dbb697aafecd7..081ec3b6740a1 100644 --- a/question/bank/deletequestion/delete.php +++ b/question/bank/deletequestion/delete.php @@ -33,6 +33,7 @@ $returnurl = optional_param('returnurl', 0, PARAM_LOCALURL); $cmid = optional_param('cmid', 0, PARAM_INT); $courseid = optional_param('courseid', 0, PARAM_INT); +$deleteall = optional_param('deleteall', false, PARAM_BOOL); if ($returnurl) { $returnurl = new moodle_url($returnurl); @@ -79,12 +80,7 @@ $deleteselected = required_param('deleteselected', PARAM_RAW); if ($confirm == md5($deleteselected)) { if ($questionlist = explode(',', $deleteselected)) { - // For each question either hide it if it is in use or delete it. - foreach ($questionlist as $questionid) { - $questionid = (int)$questionid; - question_require_capability_on($questionid, 'edit'); - question_delete_question($questionid); - } + \qbank_deletequestion\helper::delete_questions($questionlist, $deleteall); } redirect($returnurl); } else { @@ -98,18 +94,11 @@ // Make a list of all the questions that are selected. $rawquestions = $_REQUEST; // This code is called by both POST forms and GET links, so cannot use data_submitted. $questionlist = ''; // Comma separated list of ids of questions to be deleted. - $questionnames = ''; // String with names of questions separated by
with an asterix in front of those that are in use. - $inuse = false; // Set to true if at least one of the questions is in use. foreach ($rawquestions as $key => $value) { // Parse input for question ids. if (preg_match('!^q([0-9]+)$!', $key, $matches)) { $key = $matches[1]; $questionlist .= $key.','; question_require_capability_on((int)$key, 'edit'); - if (questions_in_use(array($key))) { - $questionnames .= '* '; - $inuse = true; - } - $questionnames .= $DB->get_field('question', 'name', array('id' => $key)) . '
'; } } if (!$questionlist) { // No questions were selected. @@ -117,16 +106,17 @@ } $questionlist = rtrim($questionlist, ','); - // Add an explanation about questions in use. - if ($inuse) { - $questionnames .= '
'.get_string('questionsinuse', 'question'); - } - $deleteurl = new \moodle_url('/question/bank/deletequestion/delete.php', - array('deleteselected' => $questionlist, 'confirm' => md5($questionlist), - 'sesskey' => sesskey(), 'returnurl' => $returnurl, 'cmid' => $cmid, 'courseid' => $courseid)); - + $deleteurl = new \moodle_url('/question/bank/deletequestion/delete.php', [ + 'deleteselected' => $questionlist, 'deleteall' => $deleteall, 'confirm' => md5($questionlist), + 'sesskey' => sesskey(), 'returnurl' => $returnurl, 'cmid' => $cmid, 'courseid' => $courseid, + ]); $continue = new \single_button($deleteurl, get_string('delete'), 'post'); - echo $OUTPUT->confirm(get_string('deletequestionscheck', 'question', $questionnames), $continue, $returnurl); + + $questionids = explode(',', $questionlist); + [$displayoptions, $message] = qbank_deletequestion\helper::get_delete_confirmation_message($questionids, + $deleteall); + + echo $OUTPUT->confirm($message, $continue, $returnurl, $displayoptions); } echo $OUTPUT->footer(); diff --git a/question/bank/deletequestion/tests/behat/delete_question_column.feature b/question/bank/deletequestion/tests/behat/delete_question_column.feature index ac6603f1f8391..11838054ae588 100644 --- a/question/bank/deletequestion/tests/behat/delete_question_column.feature +++ b/question/bank/deletequestion/tests/behat/delete_question_column.feature @@ -51,7 +51,7 @@ Feature: Use the qbank plugin manager page for deletequestion And I click on "First question second" "checkbox" And I click on "With selected" "button" And I click on question bulk action "deleteselected" - And I click on "Delete" "button" in the "Confirm" "dialogue" + And I click on "Delete" "button" in the "Delete questions?" "dialogue" Then I should not see "First question" And I should not see "First question second" @@ -68,6 +68,6 @@ Feature: Use the qbank plugin manager page for deletequestion And I click on "First question" "checkbox" And I click on "With selected" "button" And I click on question bulk action "deleteselected" - When I click on "Delete" "button" in the "Confirm" "dialogue" + When I click on "Delete" "button" in the "Delete question?" "dialogue" Then I should not see "Third question" And "foo" "autocomplete_selection" should exist diff --git a/question/bank/deletequestion/tests/helper_test.php b/question/bank/deletequestion/tests/helper_test.php new file mode 100644 index 0000000000000..3af3f6d42b504 --- /dev/null +++ b/question/bank/deletequestion/tests/helper_test.php @@ -0,0 +1,177 @@ +. + +namespace qbank_deletequestion; + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/question/engine/tests/helpers.php'); + +/** + * Class containing unit tests for the helper class + * + * @package qbank_deletequestion + * @copyright 2023 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class helper_test extends \advanced_testcase { + + /** + * @var \context_module module context. + */ + protected $context; + + /** + * @var \stdClass course object. + */ + protected $course; + + /** + * @var \component_generator_base question generator. + */ + protected $qgenerator; + + /** + * @var \stdClass quiz object. + */ + protected $quiz; + + /** + * Called before every test. + */ + protected function setUp(): void { + parent::setUp(); + self::setAdminUser(); + $this->resetAfterTest(); + + $datagenerator = $this->getDataGenerator(); + $this->course = $datagenerator->create_course(); + $this->quiz = $datagenerator->create_module('quiz', ['course' => $this->course->id]); + $this->qgenerator = $datagenerator->get_plugin_generator('core_question'); + $this->context = \context_module::instance($this->quiz->cmid); + } + + /** + * Test get a confirmation message when deleting the question in the (question bank/history) page. + * + * @covers \qbank_deletequestion\helper::get_delete_confirmation_message + */ + public function test_get_delete_confirmation_message(): void { + $qcategory = $this->qgenerator->create_question_category(['contextid' => $this->context->id]); + $question = $this->qgenerator->create_question('shortanswer', null, ['category' => $qcategory->id, + 'name' => 'Question 1']); + $questionfirstversionid = $question->id; + + // Verify confirmation title and confirmation message with question not in use in question bank page. + $deleteallversions = true; + [$title1, $message1] = \qbank_deletequestion\helper::get_delete_confirmation_message([$questionfirstversionid], + $deleteallversions); + $this->assertEquals(['confirmtitle' => get_string('deletequestiontitle', 'question')], + $title1); + $this->assertEquals(get_string('deletequestioncheck', 'question', + $question->name). ' v1' . '
', $message1); + + // Create a new version and adding it to a quiz. + $question2 = $this->qgenerator->update_question($question, null, ['name' => 'Question 1']); + $questionsecondversionid = $question2->id; + + // Verify confirmation title and confirmation message with question has multiple versions in question bank page. + $listnameofquestionversion2 = $question->name . ' v1' . '
' . $question2->name . ' v2' .'
'; + [$title2, $message2] = \qbank_deletequestion\helper::get_delete_confirmation_message([$questionsecondversionid], + $deleteallversions); + $this->assertEquals(['confirmtitle' => get_string('deletequestiontitle', 'question')], + $title2); + $this->assertEquals(get_string('deletequestioncheck', 'question', + $listnameofquestionversion2), $message2); + + // Verify confirmation title and confirmation message with multiple question selected. + $listnameofquestionversion3 = $question->name . ' v1' . '
' . $question2->name . ' v2' .'
'; + [$title3, $message3] = \qbank_deletequestion\helper::get_delete_confirmation_message([$questionfirstversionid, + $questionsecondversionid], $deleteallversions); + $this->assertEquals(['confirmtitle' => get_string('deletequestiontitle_plural', 'question')], + $title3); + $this->assertEquals(get_string('deletequestionscheck', 'question', + $listnameofquestionversion3), $message3); + + // Add second question version to the quiz to become question in use. + quiz_add_quiz_question($questionsecondversionid, $this->quiz); + + // Verify confirmation message with question in use and has multiple versions in question bank page. + $listnameofquestionversion4 = $question->name . ' v1' . '
' . '* ' . $question2->name . ' v2' . '
'; + $message4 = \qbank_deletequestion\helper::get_delete_confirmation_message([$questionsecondversionid], + $deleteallversions)[1]; + $this->assertEquals(get_string('deletequestioncheck', 'question', + $listnameofquestionversion4) . '
' . get_string('questionsinuse', + 'question'), $message4); + + // Verify confirmation title and confirmation message in history page with one question selected. + $deleteallversions = false; + [$title5, $message5] = \qbank_deletequestion\helper::get_delete_confirmation_message([$questionfirstversionid], + $deleteallversions); + $this->assertEquals(['confirmtitle' => get_string('deleteversiontitle', 'question')], + $title5); + $this->assertEquals(get_string('deleteselectedquestioncheck', 'question', + $question->name) . '
', $message5); + + // Verify confirmation title and confirmation message in history page with multiple question selected. + $listnameofquestionversion6 = 'Question 1
* Question 1
'; + [$title6, $message6] = \qbank_deletequestion\helper::get_delete_confirmation_message([$questionfirstversionid, + $questionsecondversionid], $deleteallversions); + $this->assertEquals(['confirmtitle' => get_string('deleteversiontitle_plural', 'question')], + $title6); + $this->assertEquals(get_string('deleteselectedquestioncheck', 'question', + $listnameofquestionversion6) . '
'. get_string('questionsinuse', 'question'), + $message6); + } + + /** + * Test delete questions have single/multiple version. + * + * @covers \qbank_deletequestion\helper::delete_questions + */ + public function test_delete_question_has_multiple_version() { + global $DB; + $qcategory = $this->qgenerator->create_question_category(['contextid' => $this->context->id]); + $question1 = $this->qgenerator->create_question('shortanswer', null, ['category' => $qcategory->id, + 'name' => 'Question 1 version 1']); + $question1v1id = $question1->id; + // Create a new version for question 1. + $question1v2 = $this->qgenerator->update_question($question1, null, ['name' => 'Question 1 version 2']); + $question1v2id = $question1v2->id; + + $question2 = $this->qgenerator->create_question('shortanswer', null, ['category' => $qcategory->id, + 'name' => 'Question 2 version 1']); + $question2v1id = $question2->id; + + $question3 = $this->qgenerator->create_question('shortanswer', null, ['category' => $qcategory->id, + 'name' => 'Question 3 version 1']); + $question3v1id = $question3->id; + + // Do. + \qbank_deletequestion\helper::delete_questions([$question1v2id, $question2v1id], true); + + // All the versions of question1 will be deleted. + $this->assertFalse($DB->record_exists('question', ['id' => $question1v1id])); + $this->assertFalse($DB->record_exists('question', ['id' => $question1v2id])); + + // The question2 have single version will be deleted. + $this->assertFalse($DB->record_exists('question', ['id' => $question2v1id])); + + // Check that we did not delete too much. + $this->assertTrue($DB->record_exists('question', ['id' => $question3v1id])); + } +} diff --git a/question/edit.php b/question/edit.php index d9d694ba5cb3e..dc1b6d21b6db7 100644 --- a/question/edit.php +++ b/question/edit.php @@ -38,6 +38,7 @@ $PAGE->set_primary_active_tab('home'); } +$thispageurl->param('deleteall', 1); $questionbank = new core_question\local\bank\view($contexts, $thispageurl, $COURSE, $cm); $context = $contexts->lowest(); diff --git a/question/engine/bank.php b/question/engine/bank.php index 866d7d152806c..42d7b17178a34 100644 --- a/question/engine/bank.php +++ b/question/engine/bank.php @@ -309,6 +309,30 @@ public static function get_all_versions_of_question(int $questionid): array { return $DB->get_records_sql($sql, [$questionid]); } + /** + * Get all the versions of questions. + * + * @param array $questionids Array of question ids. + * @return array List of versions of questions. + */ + public static function get_all_versions_of_questions(array $questionids): array { + global $DB; + + [$listquestionid, $params] = $DB->get_in_or_equal($questionids, SQL_PARAMS_NAMED); + $sql = "SELECT qv.questionid, qv.version, qv.questionbankentryid + FROM {question_versions} qv + JOIN {question_versions} qv2 ON qv.questionbankentryid = qv2.questionbankentryid + WHERE qv2.questionid $listquestionid + ORDER BY qv.questionbankentryid, qv.version DESC"; + $result = []; + $rows = $DB->get_recordset_sql($sql, $params); + foreach ($rows as $row) { + $result[$row->questionbankentryid][$row->version] = $row->questionid; + } + + return $result; + } + /** * @return question_finder a question finder. */ diff --git a/question/tests/behat/delete_questions.feature b/question/tests/behat/delete_questions.feature index 4eb57105222ef..4048d92b583d9 100644 --- a/question/tests/behat/delete_questions.feature +++ b/question/tests/behat/delete_questions.feature @@ -6,14 +6,14 @@ Feature: A teacher can delete questions in the question bank Background: Given the following "users" exist: - | username | firstname | lastname | email | - | teacher1 | Teacher | 1 | teacher1@example.com | + | username | firstname | lastname | email | + | teacher1 | Teacher | 1 | teacher1@example.com | And the following "courses" exist: | fullname | shortname | format | - | Course 1 | C1 | weeks | + | Course 1 | C1 | weeks | And the following "course enrolments" exist: - | user | course | role | - | teacher1 | C1 | editingteacher | + | user | course | role | + | teacher1 | C1 | editingteacher | And the following "question categories" exist: | contextlevel | reference | name | | Course | C1 | Test questions | @@ -51,6 +51,8 @@ Feature: A teacher can delete questions in the question bank | Test used question to be deleted | 1 | 0 | When I am on the "Course 1" "core_question > course question bank" page And I choose "Delete" action for "Test used question to be deleted" in the question bank + And I should see "This will delete the following question and all its versions:" + And I should see "* Denotes questions which can't be deleted because they are in use. Instead, they will be hidden in the question bank unless you select 'Show old questions'." And I press "Delete" Then I should not see "Test used question to be deleted" And I set the field "Also show old questions" to "1" @@ -69,3 +71,16 @@ Feature: A teacher can delete questions in the question bank And I press "Delete" And I set the field "Also show old questions" to "1" Then I should not see "Broken question" + + @javascript + Scenario: Delete question has multiple versions in question bank page + Given I am on the "Course 1" "core_question > course question bank" page logged in as "teacher1" + When the following "core_question > updated questions" exist: + | questioncategory | question | questiontext | + | Test questions | Test question to be deleted | Test question to be deleted version 2 | + And I choose "Delete" action for "Test question to be deleted" in the question bank + And I should see "This will delete the following question and all its versions:" + And I should not see "* Denotes questions which can't be deleted because they are in use. Instead, they will be hidden in the question bank unless you select 'Show old questions'." + And I press "Delete" + Then I should not see "Test question to be deleted" + And I should not see "Test question to be deleted version2" diff --git a/question/tests/version_test.php b/question/tests/version_test.php index 2a431637a951c..363af81e20e00 100644 --- a/question/tests/version_test.php +++ b/question/tests/version_test.php @@ -288,4 +288,36 @@ public function test_get_all_versions_of_question() { $this->assertEquals(array_slice($questiondefinition, 1, 1)[0]->questionid, $questionid2); $this->assertEquals(array_slice($questiondefinition, 2, 1)[0]->questionid, $questionid1); } + + /** + * Test that all the versions of questions are available from the method. + * + * @covers ::get_all_versions_of_questions + */ + public function test_get_all_versions_of_questions() { + global $DB; + + $questionversions = []; + $qcategory = $this->qgenerator->create_question_category(['contextid' => $this->context->id]); + $question = $this->qgenerator->create_question('shortanswer', null, + [ + 'category' => $qcategory->id, + 'idnumber' => 'id1' + ]); + $questionversions[1] = $question->id; + + // Create a new version. + $question = $this->qgenerator->update_question($question, null, ['idnumber' => 'id2']); + $questionversions[2] = $question->id; + // Change the id number and get the question object. + $question = $this->qgenerator->update_question($question, null, ['idnumber' => 'id3']); + $questionversions[3] = $question->id; + + $questionbankentryid = $DB->get_record('question_versions', ['questionid' => $question->id], 'questionbankentryid'); + + $questionversionsofquestions = question_bank::get_all_versions_of_questions([$question->id]); + $questionbankentryids = array_keys($questionversionsofquestions)[0]; + $this->assertEquals($questionbankentryid->questionbankentryid, $questionbankentryids); + $this->assertEquals($questionversions, $questionversionsofquestions[$questionbankentryids]); + } }