Moodle

Match question type: should be able to use the multilang filter on the answers

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 1.9.9, 2.0, 2.1.4, 2.2.1
  • Fix Version/s: 2.1.5, 2.2.2
  • Component/s: Questions
  • Labels:
  • Testing Instructions:
    Hide

    0. Make sure you have the multi-lang filter enabled on your site, and applying to Content and headings.

    1. Create a match question and enter the following subquestions and answers:

    • Portugese: PT
    • Chinese: <b class="shouldbestripped">ZH</b>
    • Current lanauage: <span lang="en" class="multilang">EN</span><span lang="cz" class="multilang">CS</span><span lang="fr" class="multilang">FR</span>

    (Feel free to alter that last one so it works in your favourite langauge.)

    2. Save the question and re-edit it. Verify that the <b> tags have been stripped out, but that the third answer has not been damaged.

    3. Preview the question, and verify that it works. That is, you should see three choices like PT, ZH and EN, randomly shuffled, in the drop-down menus.

    Show
    0. Make sure you have the multi-lang filter enabled on your site, and applying to Content and headings. 1. Create a match question and enter the following subquestions and answers: Portugese: PT Chinese: <b class="shouldbestripped">ZH</b> Current lanauage: <span lang="en" class="multilang">EN</span><span lang="cz" class="multilang">CS</span><span lang="fr" class="multilang">FR</span> (Feel free to alter that last one so it works in your favourite langauge.) 2. Save the question and re-edit it. Verify that the <b> tags have been stripped out, but that the third answer has not been damaged. 3. Preview the question, and verify that it works. That is, you should see three choices like PT, ZH and EN, randomly shuffled, in the drop-down menus.
  • Affected Branches:
    MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
  • Fixed Branches:
    MOODLE_21_STABLE, MOODLE_22_STABLE
  • Pull from Repository:
  • Pull Master Branch:
  • Pull Master Diff URL:

Description

Things have changed since this problem was originally reported, but the situation now is:

1. You should be able to enter match answers that work with the multilang filter, but no other HTML. That is, the answers are PARAM_TEXT. This is currently working.

2. Questions with multilang syntax in the answers should work. That was broken.

Original description:

The in_array() check in line 246 (1.9) / 264 (2.0) of questiontype.php doesn't align with what's added to the array in the next line. I think it's probably fairly rare that people have tags in this field, but it leads to unexpected behavior with some answers added twice or not being treated as equal. I can get the same answer three times in the drop-down menu along with the expected unpredictable grading if I add the following answers (tested with the most recent 2.0):

<b>a</b>

<i>a</i>

a

Issue Links

testing discovered

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-31100 Unit test failure messages mangle UTF8

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-31101 format_string mangles < and > characters

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Tim Hunt added a comment -

Yes, you are right. Thanks for spotting this.

I cannot immediately work out the correct fix, because I cannot immediately work out what should go into the $allanswers array - should that be the version with tags stripped, or not.

Is there any chance you coudl work out the fix, then I can review it and commit it? Don't worry if you are as busy as I am. Thanks.

Show
Tim Hunt added a comment - Yes, you are right. Thanks for spotting this. I cannot immediately work out the correct fix, because I cannot immediately work out what should go into the $allanswers array - should that be the version with tags stripped, or not. Is there any chance you coudl work out the fix, then I can review it and commit it? Don't worry if you are as busy as I am. Thanks.
Hide
Permalink
Adriane Boyd added a comment -

I pretty sure that $allanswers should have the formatted text because I think it's used to align duplicates between the response and the just-generated reduced set, which might not have chosen the same id to represent a particular duplicate. I think that's what's going on, anyway. They will both refer to the formatted text displayed in the drop-down.

It turns out that the same formatting needs to be taken into account in the grading, too. I'll attach a patch which works in my simple tests. There might be something I've overlooked.

Show
Adriane Boyd added a comment - I pretty sure that $allanswers should have the formatted text because I think it's used to align duplicates between the response and the just-generated reduced set, which might not have chosen the same id to represent a particular duplicate. I think that's what's going on, anyway. They will both refer to the formatted text displayed in the drop-down. It turns out that the same formatting needs to be taken into account in the grading, too. I'll attach a patch which works in my simple tests. There might be something I've overlooked.
Hide
Permalink
Adriane Boyd added a comment -

Or perhaps it would be better/simpler to strip tags and format when it's saved in the database? Maybe it could be part of the form validation that won't let you save unless $ans == strip_tags($ans)?

Show
Adriane Boyd added a comment - Or perhaps it would be better/simpler to strip tags and format when it's saved in the database? Maybe it could be part of the form validation that won't let you save unless $ans == strip_tags($ans)?
Hide
Permalink
Tim Hunt added a comment -

Generally, we sanitise text on output. I think the logic is that cleaning up HTML (or converting it to plain text) is a complex operation, so the algorithms are likely to improve over time. Will try to review your patch soon, but probably not this week.

Show
Tim Hunt added a comment - Generally, we sanitise text on output. I think the logic is that cleaning up HTML (or converting it to plain text) is a complex operation, so the algorithms are likely to improve over time. Will try to review your patch soon, but probably not this week.
Hide
Permalink
Tim Hunt added a comment -

I need to look at this, see if it still affects 2.1+, and if so, apply the fix.

Show
Tim Hunt added a comment - I need to look at this, see if it still affects 2.1+, and if so, apply the fix.
Hide
Permalink
Tim Hunt added a comment -

The correct fix is to call format_string on the choices. Submitting for integration.

Show
Tim Hunt added a comment - The correct fix is to call format_string on the choices. Submitting for integration.
Hide
Permalink
Sam Hemelryk added a comment -

Thanks Tim this has been integrated now.

Show
Sam Hemelryk added a comment - Thanks Tim this has been integrated now.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Note this has been detected to cause unittests to fail under 21/22/master with information:

Failed

qtype_match_walkthrough_test.test_match_with_tricky_html_choices (from _question_type_match_simpletest_testwalkthrough.php)

Failing for the past 16 builds (Since #107 )
Took 25 ms.
Error Message

Content [<div id="q1" class="que match deferredfeedback notyetanswered"><div class="info"><div class="state">Not yet answered</div><div class="grade">Marked out of 3.00</div><div class="questionflag"><img src="http://localhost/theme/image.php?theme=standard&amp;image=i%2Funflagged&amp;rev=281" alt="Not flagged" /></div>
</div><div class="content"><div class="formulation"><h3 class="accesshide">Question text</h3><input type="hidden" name="q0u6n8KZ655:1_:sequencecheck" value="1" /><div class="qtext">Classify the animals.</div><div class="ablock"><table class="answer"><tbody><tr class="r0"><td class="text">(1, 2]</td><td class="control"><select id="menuq0u6n8KZ655:1_sub0" class="select menuq0u6n8KZ655:1_sub0" name="q0u6n8KZ655:1_sub0"><option selected="selected" value="0">Choose...</option><option value="1">1 ≤ x ≤ 2</option><option value="2">1 < x ≤ 2</option><option value="3">1 ≤ x < 2</option></select> </td></tr><tr class="r1"><td class="text">[1, 2]</td><td class="control"><select id="menuq0u6n8KZ655:1_sub1" class="select menuq0u6n8KZ655:1_sub1" name="q0u6n8KZ655:1_sub1"><option selected="selected" value="0">Choose...</option><option value="1">1 ≤ x ≤ 2</option><option value="2">1 < x ≤ 2</option><option value="3">1 ≤ x < 2</option></select> </td></tr><tr class="r0"><td class="text">[1, 2)</td><td class="control"><select id="menuq0u6n8KZ655:1_sub2" class="select menuq0u6n8KZ655:1_sub2" name="q0u6n8KZ655:1_sub2"><option selected="selected" value="0">Choose...</option><option value="1">1 ≤ x ≤ 2</option><option value="2">1 < x ≤ 2</option><option value="3">1 ≤ x < 2</option></select> </td></tr></tbody></table></div></div></div></div>] does not contain an enabled <select> with name q0u6n8KZ655:1_sub0 and choices Choose..., 1 ≤ x ≤ 2, 1 < x ≤ 2, 1 ≤ x < 2 at [/question/engine/simpletest/helpers.php line 511]

Ciao

Show
Eloy Lafuente (stronk7) added a comment - Note this has been detected to cause unittests to fail under 21/22/master with information: Failed qtype_match_walkthrough_test.test_match_with_tricky_html_choices (from _question_type_match_simpletest_testwalkthrough.php) Failing for the past 16 builds (Since #107 ) Took 25 ms. Error Message Content [<div id= "q1" class= "que match deferredfeedback notyetanswered" ><div class= "info" ><div class= "state" >Not yet answered</div><div class= "grade" >Marked out of 3.00</div><div class= "questionflag" ><img src= "http: //localhost/theme/image.php?theme=standard&amp;image=i%2Funflagged&amp;rev=281" alt= "Not flagged" /></div> </div><div class= "content" ><div class= "formulation" ><h3 class= "accesshide" >Question text</h3><input type= "hidden" name= "q0u6n8KZ655:1_:sequencecheck" value= "1" /><div class= "qtext" >Classify the animals.</div><div class= "ablock" ><table class= "answer" ><tbody><tr class= "r0" ><td class= "text" >(1, 2]</td><td class= "control" ><select id= "menuq0u6n8KZ655:1_sub0" class= "select menuq0u6n8KZ655:1_sub0" name= "q0u6n8KZ655:1_sub0" ><option selected= "selected" value= "0" >Choose...</option><option value= "1" >1 ≤ x ≤ 2</option><option value= "2" >1 < x ≤ 2</option><option value= "3" >1 ≤ x < 2</option></select> </td></tr><tr class= "r1" ><td class= "text" >[1, 2]</td><td class= "control" ><select id= "menuq0u6n8KZ655:1_sub1" class= "select menuq0u6n8KZ655:1_sub1" name= "q0u6n8KZ655:1_sub1" ><option selected= "selected" value= "0" >Choose...</option><option value= "1" >1 ≤ x ≤ 2</option><option value= "2" >1 < x ≤ 2</option><option value= "3" >1 ≤ x < 2</option></select> </td></tr><tr class= "r0" ><td class= "text" >[1, 2)</td><td class= "control" ><select id= "menuq0u6n8KZ655:1_sub2" class= "select menuq0u6n8KZ655:1_sub2" name= "q0u6n8KZ655:1_sub2" ><option selected= "selected" value= "0" >Choose...</option><option value= "1" >1 ≤ x ≤ 2</option><option value= "2" >1 < x ≤ 2</option><option value= "3" >1 ≤ x < 2</option></select> </td></tr></tbody></table></div></div></div></div>] does not contain an enabled <select> with name q0u6n8KZ655:1_sub0 and choices Choose..., 1 ≤ x ≤ 2, 1 < x ≤ 2, 1 ≤ x < 2 at [/question/engine/simpletest/helpers.php line 511] Ciao
Hide
Permalink
Tim Hunt added a comment -

The cause of the error is MDL-31101, which I will now try to fix.

Show
Tim Hunt added a comment - The cause of the error is MDL-31101 , which I will now try to fix.
Hide
Permalink
Tim Hunt added a comment -

I think I have fixed it, but Petr disagrees.

Show
Tim Hunt added a comment - I think I have fixed it, but Petr disagrees.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

MDL-31101 has been integrated and tests (both issue ones and CI ones) are passing, so I'm sending this to own-testing again. Thanks!

Show
Eloy Lafuente (stronk7) added a comment - MDL-31101 has been integrated and tests (both issue ones and CI ones) are passing, so I'm sending this to own-testing again. Thanks!
Hide
Permalink
Rajesh Taneja added a comment -

Works Great
Thanks for fixing this, Tim.

Show
Rajesh Taneja added a comment - Works Great Thanks for fixing this, Tim.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

This virus has been spread upstream, everybody will be infected soon. Congrats, you did it!

Closing, ciao

Show
Eloy Lafuente (stronk7) added a comment - This virus has been spread upstream, everybody will be infected soon. Congrats, you did it! Closing, ciao

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: