Skip to content

Commit

Permalink
MDL-76298 drag-drop questions: validate the questions are complete
Browse files Browse the repository at this point in the history 
Previously, it was possible to create drag-drop markers and onto image
questions without any drag items or drop zones. This was non-sensical,
and broke statistics calculations.

So, missing validation added, and random guess score calculation made
robust.
  • Loading branch information
@timhunt
timhunt committed Feb 22, 2023
1 parent cb2fa7a commit 7ab2898
Show file tree
Hide file tree
Showing 9 changed files with 95 additions and 9 deletions.
22 changes: 20 additions & 2 deletions question/type/ddimageortext/edit_ddimageortext_form.php
View file
Expand Up @@ -205,9 +205,10 @@ protected function drop_zones_repeated_options() {
public function validation($data, $files) {
$errors = parent::validation($data, $files);
if (!self::file_uploaded($data['bgimage'])) {
$errors["bgimage"] = get_string('formerror_nobgimage', 'qtype_'.$this->qtype());
$errors['bgimage'] = get_string('formerror_nobgimage', 'qtype_'.$this->qtype());
}

$dropfound = false;
$allchoices = array();
for ($i = 0; $i < $data['nodropzone']; $i++) {
$ytoppresent = (trim($data['drops'][$i]['ytop']) !== '');
Expand All @@ -219,6 +220,7 @@ public function validation($data, $files) {
$imagechoicepresent = ($choice !== '0');

if ($imagechoicepresent) {
$dropfound = true;
if (!$ytoppresent) {
$errors["drops[$i]"] = get_string('formerror_noytop', 'qtype_ddimageortext');
} else if (!$ytopisint) {
Expand Down Expand Up @@ -247,25 +249,41 @@ public function validation($data, $files) {
$allchoices[$choice] = $i;
} else {
if ($ytoppresent || $xleftpresent || $labelpresent) {
$dropfound = true;
$errors["drops[$i]"] =
get_string('formerror_noimageselected', 'qtype_ddimageortext');
}
}
}
if (!$dropfound) {
$errors['drops[0]'] = get_string('formerror_droprequired', 'qtype_ddimageortext');
}

$dragfound = false;
for ($dragindex = 0; $dragindex < $data['noitems']; $dragindex++) {
$label = $data['draglabel'][$dragindex];
if ($data['drags'][$dragindex]['dragitemtype'] == 'word') {
if ($label !== '') {
$dragfound = true;
}
$allowedtags = '<br><sub><sup><b><i><strong><em><span>';
$errormessage = get_string('formerror_disallowedtags', 'qtype_ddimageortext', s($allowedtags));
} else {
if (self::file_uploaded($data['dragitem'][$dragindex])) {
$dragfound = true;
}
$allowedtags = '';
$errormessage = get_string('formerror_noallowedtags', 'qtype_ddimageortext');
}
if ($label != strip_tags($label, $allowedtags)) {
$errors["drags[{$dragindex}]"] = $errormessage;
$errors["draglabel[{$dragindex}]"] = $errormessage;
}

}
if (!$dragfound) {
$errors['drags[0]'] = get_string('formerror_dragrequired', 'qtype_ddimageortext');
}

return $errors;
}
}
2 changes: 2 additions & 0 deletions question/type/ddimageortext/lang/en/qtype_ddimageortext.php
View file
Expand Up @@ -39,6 +39,8 @@
$string['dropzone'] = 'Drop zone {$a}';
$string['dropzoneheader'] = 'Drop zones';
$string['formerror_disallowedtags'] = 'Only "{$a}" tags are allowed in this draggable text.';
$string['formerror_dragrequired'] = 'You must add at least one draggable item to this question.';
$string['formerror_droprequired'] = 'You must define at least one drop zone for this question.';
$string['formerror_noallowedtags'] = 'HTML tags are not allowed in this text which is the alt text for a draggable image.';
$string['formerror_noytop'] = 'You must provide a value for the y coordinate for the top left corner of this drop area. You can drag and drop the drop area above to set the coordinates or enter them manually here.';
$string['formerror_noxleft'] = 'You must provide a value for the x coordinate for the top left corner of this drop area. You can drag and drop the drop area above to set the coordinates or enter them manually here.';
Expand Down
7 changes: 6 additions & 1 deletion question/type/ddimageortext/questionbase.php
View file
Expand Up @@ -138,8 +138,13 @@ public function classify_response(array $response) {
}

public function get_random_guess_score() {
$accum = 0;
if (empty($this->places)) {
// Having no places would be nonsensical. However, it used to be possible
// to create questions like that, so avoid errors in this case.
return null;
}

$accum = 0;
foreach ($this->places as $place) {
foreach ($this->choices[$place->group] as $choice) {
if ($choice->infinite) {
Expand Down
13 changes: 13 additions & 0 deletions question/type/ddimageortext/tests/behat/add.feature
View file
Expand Up @@ -99,3 +99,16 @@ Feature: Test creating a drag and drop onto image question
And I click on "Add" "button" in the "Choose a question type to add" "dialogue"
And the following fields match these values:
| id_shuffleanswers | 1 |

@javascript @_file_upload
Scenario: Question must have at least one drag item and one drop zone
When I am on the "Course 1" "core_question > course question bank" page logged in as teacher
And I press "Create a new question ..."
And I set the field "Drag and drop onto image" to "1"
And I click on "Add" "button" in the "Choose a question type to add" "dialogue"
And I set the field "Question name" to "Test question"
And I set the field "Question text" to "Identify the features in this cross-section."
And I upload "question/type/ddimageortext/tests/fixtures/oceanfloorbase.jpg" file to "Background image" filemanager
And I press "Save changes"
Then I should see "You must add at least one draggable item to this question."
And I should see "You must define at least one drop zone for this question."
17 changes: 12 additions & 5 deletions question/type/ddimageortext/tests/form/edit_form_test.php
View file
Expand Up @@ -83,6 +83,13 @@ public function test_item_validation() {
['dragitemtype' => 'word'],
['dragitemtype' => 'word'],
],
'dragitem' => [
0,
0,
0,
0,
0,
],
'draglabel' => [
'frog',
'<b>toad</b>',
Expand All @@ -94,12 +101,12 @@ public function test_item_validation() {

$errors = $form->validation($submitteddata, []);

$this->assertArrayNotHasKey('drags[0]', $errors);
$this->assertArrayNotHasKey('draglabel[0]', $errors);
$this->assertEquals('HTML tags are not allowed in this text which is the alt text for a draggable image.',
$errors['drags[1]']);
$this->assertArrayNotHasKey('drags[2]', $errors);
$this->assertArrayNotHasKey('drags[3]', $errors);
$errors['draglabel[1]']);
$this->assertArrayNotHasKey('draglabel[2]', $errors);
$this->assertArrayNotHasKey('draglabel[3]', $errors);
$this->assertEquals('Only "&lt;br&gt;&lt;sub&gt;&lt;sup&gt;&lt;b&gt;&lt;i&gt;&lt;strong&gt;&lt;em&gt;&lt;span&gt;" ' .
'tags are allowed in this draggable text.', $errors['drags[4]']);
'tags are allowed in this draggable text.', $errors['draglabel[4]']);
}
}
12 changes: 12 additions & 0 deletions question/type/ddimageortext/tests/question_test.php
View file
Expand Up @@ -33,6 +33,7 @@
* @package qtype_ddimageortext
* @copyright 2009 The Open University
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
* @covers qtype_ddimageortext_question
*/
class question_test extends \basic_testcase {

Expand Down Expand Up @@ -91,6 +92,17 @@ public function test_get_random_guess_score_maths() {
$this->assertEquals(0.5, $dd->get_random_guess_score());
}

public function test_get_random_guess_score_broken_question() {
// Before MDL-76298 was fixed, it was possible to create a question with
// no drop zones, and that caused a fatal division by zero error. Because
// people might have questions like that in their database, we need to
// check the calculation is robust to it.
/** @var \qtype_ddimageortext_question $dd */
$dd = \test_question_maker::make_question('ddimageortext');
$dd->places = [];
$this->assertNull($dd->get_random_guess_score());
}

public function test_get_right_choice_for() {
$dd = \test_question_maker::make_question('ddimageortext');
$dd->shufflechoices = false;
Expand Down
16 changes: 15 additions & 1 deletion question/type/ddmarker/edit_ddmarker_form.php
View file
Expand Up @@ -215,11 +215,13 @@ public function validation($data, $files) {
$errors["bgimage"] = get_string('formerror_nobgimage', 'qtype_ddmarker');
}

$dropfound = false;
for ($i = 0; $i < $data['nodropzone']; $i++) {
$choice = $data['drops'][$i]['choice'];
$choicepresent = ($choice !== '0');

if ($choicepresent) {
$dropfound = true;
// Test coords here.
if ($bgimagesize !== null) {
$shape = $data['drops'][$i]['shape'];
Expand All @@ -236,20 +238,32 @@ public function validation($data, $files) {
}
} else {
if (trim($data['drops'][$i]['coords']) !== '') {
$dropfound = true;
$errorcode = 'noitemselected';
$errors["drops[{$i}]"] = get_string('formerror_'.$errorcode, 'qtype_ddmarker');
}
}

}
if (!$dropfound) {
$errors['drops[0]'] = get_string('formerror_droprequired', 'qtype_ddmarker');
}

$markerfound = false;
for ($dragindex = 0; $dragindex < $data['noitems']; $dragindex++) {
$label = $data['drags'][$dragindex]['label'];
if ($label !== '') {
$markerfound = true;
}
if ($label != strip_tags($label, QTYPE_DDMARKER_ALLOWED_TAGS_IN_MARKER)) {
$errors["drags[{$dragindex}]"]
= get_string('formerror_onlysometagsallowed', 'qtype_ddmarker',
s(QTYPE_DDMARKER_ALLOWED_TAGS_IN_MARKER));
}
}
if (!$markerfound) {
$errors['drags[0]'] = get_string('formerror_dragrequired', 'qtype_ddmarker');
}

return $errors;
}

Expand Down
2 changes: 2 additions & 0 deletions question/type/ddmarker/lang/en/qtype_ddmarker.php
View file
Expand Up @@ -53,6 +53,8 @@
Selecting a Marker text will add that text to the shape in the preview.';
$string['followingarewrong'] = 'The following markers have been placed in the wrong area : {$a}.';
$string['followingarewrongandhighlighted'] = 'The following markers were incorrectly placed : {$a}. Highlighted marker(s) are now shown with the correct placement(s).<br /> Click on the marker to highlight the allowed area.';
$string['formerror_dragrequired'] = 'You must add at least one marker to this question.';
$string['formerror_droprequired'] = 'You must define at least one drop zone for this question.';
$string['formerror_nobgimage'] = 'You need to select an image to use as the background for the drag and drop area.';
$string['formerror_noitemselected'] = 'You have specified a drop zone but not chosen a marker that must be dragged to the zone.';
$string['formerror_nosemicolons'] = 'There are no semicolons in your coordinates string. Your coordinates for a {$a->shape} should be expressed as - {$a->coordsstring}.';
Expand Down
13 changes: 13 additions & 0 deletions question/type/ddmarker/tests/behat/add.feature
View file
Expand Up @@ -63,3 +63,16 @@ Feature: Test creating a drag and drop markers question
And the following fields match these values:
| id_showmisplaced | 1 |
| id_shuffleanswers | 1 |

@javascript @_file_upload
Scenario: Question must have at least one marker and one drop zone
When I am on the "Course 1" "core_question > course question bank" page logged in as teacher
And I press "Create a new question ..."
And I set the field "Drag and drop markers" to "1"
And I click on "Add" "button" in the "Choose a question type to add" "dialogue"
And I set the field "Question name" to "Drag and drop markers"
And I set the field "Question text" to "Markers, who need markers?"
And I upload "question/type/ddmarker/tests/fixtures/mkmap.png" file to "Background image" filemanager
And I press "Save changes"
Then I should see "You must add at least one marker to this question."
And I should see "You must define at least one drop zone for this question."

0 comments on commit 7ab2898

Please sign in to comment.