From eb5d0d7d599077e6220e21c04dabf3b63dd1a86b Mon Sep 17 00:00:00 2001 From: Peter Date: Tue, 11 Dec 2018 11:39:21 +0800 Subject: [PATCH] MDL-40227 mod_lesson: Use localised float formatting * Leverage PARAM_LOCALISEDFLOAT * Store all numbers with standard '.' dec formatting * Show all numbers based on locale settings * Behat test to cover different cases using numeric questions and modified locale setting --- mod/lesson/classes/local/numeric/helper.php | 79 ++++++++ mod/lesson/lang/en/lesson.php | 2 + mod/lesson/locallib.php | 14 +- mod/lesson/pagetypes/numerical.php | 98 ++++++++-- ...son_numerical_question_with_locale.feature | 135 ++++++++++++++ mod/lesson/tests/numeric_helper_test.php | 170 ++++++++++++++++++ 6 files changed, 482 insertions(+), 16 deletions(-) create mode 100644 mod/lesson/classes/local/numeric/helper.php create mode 100644 mod/lesson/tests/behat/lesson_numerical_question_with_locale.feature create mode 100644 mod/lesson/tests/numeric_helper_test.php diff --git a/mod/lesson/classes/local/numeric/helper.php b/mod/lesson/classes/local/numeric/helper.php new file mode 100644 index 00000000000..01ad8f80b7f --- /dev/null +++ b/mod/lesson/classes/local/numeric/helper.php @@ -0,0 +1,79 @@ +. + +/** + * Lesson's numeric helper lib. + * + * Contains any helper functions for the numeric pagetyep + * + * @package mod_lesson + * @copyright 2020 Peter Dias + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +namespace mod_lesson\local\numeric; + +/** + * Lesson numeric page helper + * + * @copyright 2020 Peter Dias + * @package core_lesson + */ +class helper { + + /** + * Helper function to unformat a given numeric value from locale specific values with n:n signifying ranges to standards + * with decimal point numbers/ranges + * + * @param string $value The value to be formatted + * @return string|float|bool $formattedvalue unformatted value + * String - If it is a range it will return a value e.g. 2:4 + * Float - if it's a properly formatted float + * Null - If empty and could not be converted + */ + public static function lesson_unformat_numeric_value(string $value) { + if (strpos($value, ':')) { + list($min, $max) = explode(':', $value); + $formattedvalue = unformat_float($min) . ':' . unformat_float($max); + } else { + $formattedvalue = unformat_float($value); + } + + return $formattedvalue; + } + + /** + * Helper function to format a given value into locale specific values with n:n signifying ranges + * + * @param string|number $value The value to be formatted + * @return string $formattedvalue Formatted value OR $value if not numeric + */ + public static function lesson_format_numeric_value($value) : string { + $formattedvalue = $value; + if (strpos($value, ':')) { + list($min, $max) = explode(':', $value); + $formattedvalue = $min . ':' . $max; + if (is_numeric($min) && is_numeric($max)) { + $formattedvalue = format_float($min, strlen($min), true, true) . ':' + . format_float($max, strlen($max), true, true); + } + } else { + $formattedvalue = is_numeric($value) ? format_float($value, strlen($value), true, true) : $value; + } + + return $formattedvalue; + } + +} diff --git a/mod/lesson/lang/en/lesson.php b/mod/lesson/lang/en/lesson.php index fc14d267a06..997176cb307 100644 --- a/mod/lesson/lang/en/lesson.php +++ b/mod/lesson/lang/en/lesson.php @@ -399,6 +399,8 @@ $string['numberofpagesviewed'] = 'Number of questions answered: {$a}'; $string['numberofpagesviewedheader'] = 'Number of questions answered'; $string['numberofpagesviewednotice'] = 'Number of questions answered: {$a->nquestions} (You should answer at least {$a->minquestions})'; $string['numerical'] = 'Numerical'; +$string['numericanswer_help'] = 'You can specify a number, or a range of numbers by using colon. For example 2:5 means any answer between 2 and 5 including them are correct.'; +$string['numericanswer'] = 'Numeric answer'; $string['offlinedatamessage'] = 'You have worked on this attempt using a mobile device. Data was last saved to this site {$a} ago. Please check that you do not have any unsaved work.'; $string['ongoing'] = 'Display ongoing score'; $string['ongoing_help'] = 'If enabled, each page will display the student\'s current points earned out of the total possible thus far.'; diff --git a/mod/lesson/locallib.php b/mod/lesson/locallib.php index 30a8d4b5b62..9b1cb3ae41d 100644 --- a/mod/lesson/locallib.php +++ b/mod/lesson/locallib.php @@ -1458,9 +1458,11 @@ abstract class lesson_add_page_form_base extends moodleform { * @param string $label, null means default * @param bool $required * @param string $format + * @param array $help Add help text via the addHelpButton. Must be an array which contains the string identifier and + * component as it's elements * @return void */ - protected final function add_answer($count, $label = null, $required = false, $format= '') { + protected final function add_answer($count, $label = null, $required = false, $format= '', array $help = []) { if ($label === null) { $label = get_string('answer', 'lesson'); } @@ -1473,13 +1475,17 @@ abstract class lesson_add_page_form_base extends moodleform { $this->_form->setDefault('answer_editor['.$count.']', array('text' => '', 'format' => FORMAT_HTML)); } else { $this->_form->addElement('text', 'answer_editor['.$count.']', $label, - array('size' => '50', 'maxlength' => '200')); + array('size' => '50', 'maxlength' => '200')); $this->_form->setType('answer_editor['.$count.']', PARAM_TEXT); } if ($required) { $this->_form->addRule('answer_editor['.$count.']', get_string('required'), 'required', null, 'client'); } + + if ($help) { + $this->_form->addHelpButton("answer_editor[$count]", $help['identifier'], $help['component']); + } } /** * Convenience function: Adds an response editor @@ -4530,6 +4536,7 @@ abstract class lesson_page extends lesson_base { $this->answers[$i]->lessonid = $this->lesson->id; $this->answers[$i]->pageid = $this->id; $this->answers[$i]->timecreated = $this->timecreated; + $this->answers[$i]->answer = null; } if (isset($properties->answer_editor[$i])) { @@ -4542,6 +4549,9 @@ abstract class lesson_page extends lesson_base { $this->answers[$i]->answer = $properties->answer_editor[$i]; $this->answers[$i]->answerformat = FORMAT_MOODLE; } + } else { + // If there is no data posted which means we want to reset the stored values. + $this->answers[$i]->answer = null; } if (!empty($properties->response_editor[$i]) && is_array($properties->response_editor[$i])) { diff --git a/mod/lesson/pagetypes/numerical.php b/mod/lesson/pagetypes/numerical.php index 014daac50f6..ff7bfa24688 100644 --- a/mod/lesson/pagetypes/numerical.php +++ b/mod/lesson/pagetypes/numerical.php @@ -28,6 +28,8 @@ defined('MOODLE_INTERNAL') || die(); /** Numerical question type */ define("LESSON_PAGE_NUMERICAL", "8"); +use mod_lesson\local\numeric\helper; + class lesson_page_type_numerical extends lesson_page { protected $type = lesson_page::TYPE_QUESTION; @@ -48,8 +50,9 @@ class lesson_page_type_numerical extends lesson_page { return $this->typeidstring; } public function display($renderer, $attempt) { - global $USER, $CFG, $PAGE; - $mform = new lesson_display_answer_form_shortanswer($CFG->wwwroot.'/mod/lesson/continue.php', array('contents'=>$this->get_contents(), 'lessonid'=>$this->lesson->id)); + global $USER, $PAGE; + $mform = new lesson_display_answer_form_numerical(new moodle_url('/mod/lesson/continue.php'), + array('contents' => $this->get_contents(), 'lessonid' => $this->lesson->id)); $data = new stdClass; $data->id = $PAGE->cm->id; $data->pageid = $this->properties->id; @@ -109,10 +112,10 @@ class lesson_page_type_numerical extends lesson_page { } public function check_answer() { - global $CFG; $result = parent::check_answer(); - $mform = new lesson_display_answer_form_shortanswer($CFG->wwwroot.'/mod/lesson/continue.php', array('contents'=>$this->get_contents())); + $mform = new lesson_display_answer_form_numerical(new moodle_url('/mod/lesson/continue.php'), + array('contents' => $this->get_contents())); $data = $mform->get_data(); require_sesskey(); @@ -124,12 +127,11 @@ class lesson_page_type_numerical extends lesson_page { $result->response = ''; $result->newpageid = 0; - if (!isset($data->answer) || !is_numeric($data->answer)) { + if (!isset($data->answer)) { $result->noanswer = true; return $result; } else { - // Just doing default PARAM_RAW, not doing PARAM_INT because it could be a float. - $result->useranswer = (float)$data->answer; + $result->useranswer = $data->answer; } $result->studentanswer = $result->userresponse = $result->useranswer; $answers = $this->get_answers(); @@ -201,7 +203,8 @@ class lesson_page_type_numerical extends lesson_page { } else { $cells[] = ':'; } - $cells[] = format_text($answer->answer, $answer->answerformat, $options); + $formattedanswer = helper::lesson_format_numeric_value($answer->answer); + $cells[] = format_text($formattedanswer, $answer->answerformat, $options); $table->data[] = new html_table_row($cells); $cells = array(); @@ -258,7 +261,8 @@ class lesson_page_type_numerical extends lesson_page { unset($stats["total"]); foreach ($stats as $valentered => $ntimes) { $data = ''; + 'disabled="disabled" readonly="readonly" value="'. + s(format_float($valentered, strlen($valentered), true, true)).'" />'; $percent = $ntimes / $total * 100; $percent = round($percent, 2); $percent .= "% ".get_string("enteredthis", "lesson"); @@ -272,7 +276,8 @@ class lesson_page_type_numerical extends lesson_page { empty($answerdata->answers)))) { // Get in here when the user answered or for the last answer. $data = ''; + 'disabled="disabled" readonly="readonly" value="'. + s(format_float($useranswer->useranswer, strlen($useranswer->useranswer), true, true)).'">'; if (isset($pagestats[$this->properties->id][$useranswer->useranswer])) { $percent = $pagestats[$this->properties->id][$useranswer->useranswer] / $pagestats[$this->properties->id]["total"] * 100; $percent = round($percent, 2); @@ -321,6 +326,12 @@ class lesson_page_type_numerical extends lesson_page { */ public function update_form_data(stdClass $data) : stdClass { $answercount = count($this->get_answers()); + + // If no answers provided, then we don't need to check anything. + if (!$answercount) { + return $data; + } + // Check for other answer entry. $lastanswer = $data->{'answer_editor[' . ($answercount - 1) . ']'}; if (strpos($lastanswer, LESSON_OTHER_ANSWERS) !== false) { @@ -354,7 +365,10 @@ class lesson_add_page_form_numerical extends lesson_add_page_form_base { $answercount = $this->_customdata['lesson']->maxanswers; for ($i = 0; $i < $answercount; $i++) { $this->_form->addElement('header', 'answertitle'.$i, get_string('answer').' '.($i+1)); - $this->add_answer($i, null, ($i < 1)); + $this->add_answer($i, null, ($i < 1), '', [ + 'identifier' => 'numericanswer', + 'component' => 'mod_lesson' + ]); $this->add_response($i); $this->add_jumpto($i, null, ($i == 0 ? LESSON_NEXTPAGE : LESSON_THISPAGE)); $this->add_score($i, null, ($i===0)?1:0); @@ -367,6 +381,64 @@ class lesson_add_page_form_numerical extends lesson_add_page_form_base { $this->add_jumpto($newcount, get_string('allotheranswersjump', 'lesson'), LESSON_NEXTPAGE); $this->add_score($newcount, get_string('allotheranswersscore', 'lesson'), 0); } + + /** + * We call get data when storing the data into the db. Override to format the floats properly + * + * @return object|void + */ + public function get_data() : ?stdClass { + $data = parent::get_data(); + + if (!empty($data->answer_editor)) { + foreach ($data->answer_editor as $key => $answer) { + $data->answer_editor[$key] = helper::lesson_unformat_numeric_value($answer); + } + } + + return $data; + } + + /** + * Return submitted data if properly submitted or returns NULL if validation fails or + * if there is no submitted data with formatted numbers + * + * @return object submitted data; NULL if not valid or not submitted or cancelled + */ + public function get_submitted_data() : ?stdClass { + $data = parent::get_submitted_data(); + + if (!empty($data->answer_editor)) { + foreach ($data->answer_editor as $key => $answer) { + $data->answer_editor[$key] = helper::lesson_unformat_numeric_value($answer); + } + } + + return $data; + } + + /** + * Load in existing data as form defaults. Usually new entry defaults are stored directly in + * form definition (new entry form); this function is used to load in data where values + * already exist and data is being edited (edit entry form) after formatting numbers + * + * + * @param stdClass|array $defaults object or array of default values + */ + public function set_data($defaults) { + if (is_object($defaults)) { + $defaults = (array) $defaults; + } + + $editor = 'answer_editor'; + foreach ($defaults as $key => $answer) { + if (substr($key, 0, strlen($editor)) == $editor) { + $defaults[$key] = helper::lesson_format_numeric_value($answer); + } + } + + parent::set_data($defaults); + } } class lesson_display_answer_form_numerical extends moodleform { @@ -402,8 +474,7 @@ class lesson_display_answer_form_numerical extends moodleform { $mform->addElement('hidden', 'pageid'); $mform->setType('pageid', PARAM_INT); - $mform->addElement('text', 'answer', get_string('youranswer', 'lesson'), $attrs); - $mform->setType('answer', PARAM_FLOAT); + $mform->addElement('float', 'answer', get_string('youranswer', 'lesson'), $attrs); if ($hasattempt) { $this->add_action_buttons(null, get_string("nextpage", "lesson")); @@ -411,5 +482,4 @@ class lesson_display_answer_form_numerical extends moodleform { $this->add_action_buttons(null, get_string("submit", "lesson")); } } - } diff --git a/mod/lesson/tests/behat/lesson_numerical_question_with_locale.feature b/mod/lesson/tests/behat/lesson_numerical_question_with_locale.feature new file mode 100644 index 00000000000..a3daa905bf0 --- /dev/null +++ b/mod/lesson/tests/behat/lesson_numerical_question_with_locale.feature @@ -0,0 +1,135 @@ +@mod @mod_lesson +Feature: In a lesson activity, I need to edit pages in the lesson taking into account locale settings + + Background: + Given the following "users" exist: + | username | firstname | lastname | email | + | teacher1 | Teacher | 1 | teacher1@example.com | + | student1 | Student | 1 | student1@example.com | + And the following "courses" exist: + | fullname | shortname | category | + | Course 1 | C1 | 0 | + And the following "course enrolments" exist: + | user | course | role | + | teacher1 | C1 | editingteacher | + | student1 | C1 | student | + And the following "language customisations" exist: + | component | stringid | value | + | core_langconfig | decsep | # | + And I log in as "teacher1" + And I am on "Course 1" course homepage with editing mode on + And I add a "Lesson" to section "1" and I fill the form with: + | Name | Test lesson name | + | Description | Test lesson description | + | Allow student review | Yes | + And I follow "Test lesson name" + And I follow "Add a question page" + And I set the field "Select a question type" to "Numerical" + And I press "Add a question page" + And I set the following fields to these values: + | Page title | Hardest question ever | + | Page contents | 1 + 1? | + | id_answer_editor_0 | 2#87 | + | id_response_editor_0 | Correct answer | + | id_jumpto_0 | End of lesson | + | id_score_0 | 1 | + | id_answer_editor_1 | 2#1:2#8 | + | id_response_editor_1 | Incorrect answer | + | id_jumpto_1 | This page | + | id_score_1 | 0 | + And I press "Save page" + And I log out + + Scenario: Edit a numerical question with the locale specific variables + Given I log in as "teacher1" + And I am on "Course 1" course homepage with editing mode on + And I follow "Test lesson name" + And I click on "Edit" "link" in the "region-main" "region" + And I follow "Hardest question ever" + Then I should see "2#87" + And I should see "2#1:2#8" + And I log out + + Scenario: View the detailed page of lesson + Given I log in as "teacher1" + And I am on "Course 1" course homepage with editing mode on + And I follow "Test lesson name" + And I click on "Edit" "link" in the "region-main" "region" + And I click on "Expanded" "link" in the "region-main" "region" + Then I should see "2#87" + And I should see "2#1:2#8" + And I log out + + Scenario: Attempt the lesson successfully as a student + Given I log in as "student1" + And I am on "Course 1" course homepage + And I follow "Test lesson name" + And I should see "1 + 1?" + And I set the following fields to these values: + | Your answer | 2#87 | + And I press "Submit" + Then I should see "Correct answer" + And I should not see "Incorrect answer" + And I press "Continue" + And I should see "Congratulations - end of lesson reached" + And I should see "Your score is 1 (out of 1)." + And I log out + + Scenario: Attempt the lesson unsuccessfully as a student + Given I log in as "student1" + And I am on "Course 1" course homepage + And I follow "Test lesson name" + And I should see "1 + 1?" + And I set the following fields to these values: + | Your answer | 2#7 | + And I press "Submit" + Then I should not see "Correct answer" + And I should see "Incorrect answer" + And I press "Continue" + And I should see "Congratulations - end of lesson reached" + And I should see "Your score is 0 (out of 1)." + And I log out + + Scenario: Attempt the lesson successfully as a student and review + Given I log in as "student1" + And I am on "Course 1" course homepage + And I follow "Test lesson name" + And I should see "1 + 1?" + And I set the following fields to these values: + | Your answer | 2#87 | + And I press "Submit" + Then I should see "Correct answer" + And I should not see "Incorrect answer" + And I press "Continue" + And I should see "Congratulations - end of lesson reached" + And I should see "Your score is 1 (out of 1)." + And I follow "Review lesson" + Then I should see "1 + 1?" + And the following fields match these values: + | Your answer | 2#87 | + And I log out + + Scenario: Edit lesson question page with updated locale setting and wrong answer + Given I log in as "teacher1" + And the following "language customisations" exist: + | component | stringid | value | + | core_langconfig | decsep | , | + And I am on "Course 1" course homepage with editing mode on + And I follow "Test lesson name" + Then I click on "Edit" "link" in the "region-main" "region" + And I follow "Hardest question ever" + Then I should see "2,87" + And I should see "2,1:2,8" + And I log out + And I log in as "student1" + And I am on "Course 1" course homepage + And I follow "Test lesson name" + And I should see "1 + 1?" + And I set the following fields to these values: + | Your answer | 2,7 | + And I press "Submit" + And I should see "Incorrect answer" + And I should not see "Correct answer" + And I press "Continue" + And I should see "Congratulations - end of lesson reached" + And I should see "Your score is 0 (out of 1)." diff --git a/mod/lesson/tests/numeric_helper_test.php b/mod/lesson/tests/numeric_helper_test.php new file mode 100644 index 00000000000..da932920d5a --- /dev/null +++ b/mod/lesson/tests/numeric_helper_test.php @@ -0,0 +1,170 @@ +. + +/** + * Unit tests for page types classes + * + * @package mod_lesson + * @category test + * @copyright 2020 Peter Dias + * @license http://www.gnu.org/copyleft/gpl.html GNU Public License + */ + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +use mod_lesson\local\numeric\helper; + +/** + * This class contains the test cases for the numeric helper functions + * + * @copyright 2020 Peter Dias + * @license http://www.gnu.org/copyleft/gpl.html GNU Public License + */ +class mod_lesson_numeric_type_helper_test extends advanced_testcase { + /** + * Test the lesson_unformat_numeric_value function. + * + * @dataProvider lesson_unformat_dataprovider + * @param $decsep + * @param $tests + */ + public function test_lesson_unformat_numeric_value($decsep, $tests) { + $this->define_local_decimal_separator($decsep); + + foreach ($tests as $test) { + $this->assertEquals($test[1], helper::lesson_unformat_numeric_value($test[0])); + } + } + + /** + * Test the lesson_format_numeric_value function. + * + * @dataProvider lesson_format_dataprovider + * @param $decsep + * @param $tests + */ + public function test_lesson_format_numeric_value($decsep, $tests) { + $this->define_local_decimal_separator($decsep); + + foreach ($tests as $test) { + $this->assertEquals($test[1], helper::lesson_format_numeric_value($test[0])); + } + } + + /** + * Provide various cases for the unformat test function + * + * @return array + */ + public function lesson_unformat_dataprovider() { + return [ + "Using a decimal as a separator" => [ + "decsep" => ".", + "test" => [ + ["2.1", 2.1], + ["1:4.2", "1:4.2"], + ["2,1", 2], + ["1:4,2", "1:4"], + ["", null] + ] + ], + "Using a comma as a separator" => [ + "decsep" => ",", + "test" => [ + ["2,1", 2.1], + ["1:4,2", "1:4.2"], + ["2.1", 2.1], + ["1:4.2", "1:4.2"], + ] + ], + "Using a X as a separator" => [ + "decsep" => "X", + "test" => [ + ["2X1", 2.1], + ["1:4X2", "1:4.2"], + ["2.1", 2.1], + ["1:4.2", "1:4.2"], + ] + ] + ]; + } + + /** + * Provide various cases for the unformat test function + * + * @return array + */ + public function lesson_format_dataprovider() { + return [ + "Using a decimal as a separator" => [ + "decsep" => ".", + "test" => [ + ["2.1", 2.1], + ["1:4.2", "1:4.2"], + ["2,1", "2,1"], + ["1:4,2", "1:4,2"] + ] + ], + "Using a comma as a separator" => [ + "decsep" => ",", + "test" => [ + ["2,1", "2,1"], + ["1:4,2", "1:4,2"], + ["2.1", "2,1"], + [2.1, "2,1"], + ["1:4.2", "1:4,2"], + ] + ], + "Using a X as a separator" => [ + "decsep" => "X", + "test" => [ + ["2X1", "2X1"], + ["1:4X2", "1:4X2"], + ["2.1", "2X1"], + ["1:4.2", "1:4X2"], + ] + ] + ]; + } + + + /** + * Define a local decimal separator. + * + * It is not possible to directly change the result of get_string in + * a unit test. Instead, we create a language pack for language 'xx' in + * dataroot and make langconfig.php with the string we need to change. + * The default example separator used here is 'X'; on PHP 5.3 and before this + * must be a single byte character due to PHP bug/limitation in + * number_format, so you can't use UTF-8 characters. + * + * @param string $decsep Separator character. Defaults to `'X'`. + */ + protected function define_local_decimal_separator(string $decsep = 'X') { + global $SESSION, $CFG; + + $SESSION->lang = 'xx'; + $langconfig = "dataroot . '/lang/xx'; + check_dir_exists($langfolder); + file_put_contents($langfolder . '/langconfig.php', $langconfig); + + // Ensure the new value is picked up and not taken from the cache. + $stringmanager = get_string_manager(); + $stringmanager->reset_caches(true); + } +}