From 19168d0934997ee731a0a2007be439f5ae906319 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Wed, 4 Nov 2015 15:07:17 +0100 Subject: [PATCH] MDL-52036 tablelib: Fix the behaviour of resetting table preferences The problem with the previous implementation was that the table's $this->prefs can contain valid non-empty value in its default state - the default column to sort by. On resetting, we must not throw away these default prefs. This patch simplifies the TABLE_VAR_RESET interpretation. If such an HTTP parameter is passed via the request, the table simply behaves as if there were no previously stored preferences (does not matter if coming from the current session, or from the persistent cross-session storage). The logic that decides on whether or not the reset widget should be displayed is put into a new method can_be_reset() with unit tests attached. Finally, the previously private method render_reset_button() is now protected and the reset widget is given a new semantic CSS class. --- lib/tablelib.php | 70 +++++++----- .../fixtures/testable_flexible_table.php | 45 ++++++++ lib/tests/tablelib_test.php | 107 ++++++++++++++++++ 3 files changed, 195 insertions(+), 27 deletions(-) create mode 100644 lib/tests/fixtures/testable_flexible_table.php diff --git a/lib/tablelib.php b/lib/tablelib.php index 06516e9c20a..e2c4b7f2c5e 100644 --- a/lib/tablelib.php +++ b/lib/tablelib.php @@ -458,7 +458,9 @@ class flexible_table { } else if (isset($SESSION->flextable[$this->uniqueid])) { $this->prefs = $SESSION->flextable[$this->uniqueid]; } - if (!$this->prefs) { + + // Set up default preferences if needed. + if (!$this->prefs or optional_param($this->request[TABLE_VAR_RESET], false, PARAM_BOOL)) { $this->prefs = array( 'collapse' => array(), 'sortby' => array(), @@ -526,17 +528,6 @@ class flexible_table { $this->prefs['i_first'] = $ifirst; } - // Allow user to reset table preferences. - if (optional_param($this->request[TABLE_VAR_RESET], 0, PARAM_BOOL) === 1) { - $this->prefs = array( - 'collapse' => array(), - 'sortby' => array(), - 'i_first' => '', - 'i_last' => '', - 'textsort' => $this->column_textsort, - ); - } - // Save user preferences if they have changed. if ($this->prefs != $oldprefs) { if ($this->persistent) { @@ -1416,32 +1407,57 @@ class flexible_table { /** * Generate the HTML for the table preferences reset button. * - * @return string HTML fragment. + * @return string HTML fragment, empty string if no need to reset */ - private function render_reset_button() { - $userprefs = false; - // Loop through the user table preferences for a setting. - foreach ($this->prefs as $preference) { - if (!empty($preference)) { - // We have a preference. - $userprefs = true; - // We only need one. - break; - } - } - // If no table preferences have been set then don't show the reset button. - if (!$userprefs) { + protected function render_reset_button() { + + if (!$this->can_be_reset()) { return ''; } $url = $this->baseurl->out(false, array($this->request[TABLE_VAR_RESET] => 1)); - $html = html_writer::start_div('mdl-right'); + $html = html_writer::start_div('resettable mdl-right'); $html .= html_writer::link($url, get_string('resettable')); $html .= html_writer::end_div(); return $html; } + + /** + * Are there some table preferences that can be reset? + * + * If true, then the "reset table preferences" widget should be displayed. + * + * @return bool + */ + protected function can_be_reset() { + + // Loop through preferences and make sure they are empty or set to the default value. + foreach ($this->prefs as $prefname => $prefval) { + + if ($prefname === 'sortby' and !empty($this->sort_default_column)) { + // Check if the actual sorting differs from the default one. + if (empty($prefval) or $prefval !== array($this->sort_default_column => $this->sort_default_order)) { + return true; + } + + } else if ($prefname === 'collapse' and !empty($prefval)) { + // Check if there are some collapsed columns (all are expanded by default). + foreach ($prefval as $columnname => $iscollapsed) { + if ($iscollapsed) { + return true; + } + } + + } else if (!empty($prefval)) { + // For all other cases, we just check if some preference is set. + return true; + } + } + + return false; + } } diff --git a/lib/tests/fixtures/testable_flexible_table.php b/lib/tests/fixtures/testable_flexible_table.php new file mode 100644 index 00000000000..9787f72cd0a --- /dev/null +++ b/lib/tests/fixtures/testable_flexible_table.php @@ -0,0 +1,45 @@ +. + +/** + * Provides testable_flexible_table class. + * + * @package core + * @subpackage fixtures + * @category test + * @copyright 2015 David Mudrak + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +/** + * Testable subclass of flexible_table providing access to some protected methods. + * + * @copyright 2015 David Mudrak + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class testable_flexible_table extends flexible_table { + + /** + * Provides access to flexible_table::can_be_reset() method. + * + * @return bool + */ + public function can_be_reset() { + return parent::can_be_reset(); + } +} diff --git a/lib/tests/tablelib_test.php b/lib/tests/tablelib_test.php index 3db08c3c82d..1e1fe7b331c 100644 --- a/lib/tests/tablelib_test.php +++ b/lib/tests/tablelib_test.php @@ -27,6 +27,7 @@ defined('MOODLE_INTERNAL') || die(); global $CFG; require_once($CFG->libdir . '/tablelib.php'); +require_once($CFG->libdir . '/tests/fixtures/testable_flexible_table.php'); /** * Test some of tablelib. @@ -498,4 +499,110 @@ class core_tablelib_testcase extends basic_testcase { $this->assertEquals($table5, $table6); } + + /** + * Helper method for preparing tables instances in {@link self::test_can_be_reset()}. + * + * @param string $tableid + * @return testable_flexible_table + */ + protected function prepare_table_for_reset_test($tableid) { + global $SESSION; + + unset($SESSION->flextable[$tableid]); + + $data = $this->generate_data(25, 3); + $columns = array('column0', 'column1', 'column2'); + $headers = $this->generate_headers(3); + + $table = new testable_flexible_table($tableid); + $table->define_baseurl('/invalid.php'); + $table->define_columns($columns); + $table->define_headers($headers); + $table->collapsible(true); + $table->is_persistent(false); + + return $table; + } + + public function test_can_be_reset() { + + // Table in its default state (as if seen for the first time), nothing to reset. + $table = $this->prepare_table_for_reset_test(uniqid('tablelib_test_')); + $table->setup(); + $this->assertFalse($table->can_be_reset()); + + // Table in its default state with default sorting defined, nothing to reset. + $table = $this->prepare_table_for_reset_test(uniqid('tablelib_test_')); + $table->sortable(true, 'column1', SORT_DESC); + $table->setup(); + $this->assertFalse($table->can_be_reset()); + + // Table explicitly sorted by the default column (reverses the order), can be reset. + $table = $this->prepare_table_for_reset_test(uniqid('tablelib_test_')); + $table->sortable(true, 'column1', SORT_DESC); + $_GET['tsort'] = 'column1'; + $table->setup(); + unset($_GET['tsort']); + $this->assertTrue($table->can_be_reset()); + + // Table explicitly sorted twice by the default column (puts back to default order), nothing to reset. + $table = $this->prepare_table_for_reset_test(uniqid('tablelib_test_')); + $table->sortable(true, 'column1', SORT_DESC); + $_GET['tsort'] = 'column1'; + $table->setup(); + $table->setup(); // Set up again to simulate the second page request. + unset($_GET['tsort']); + $this->assertFalse($table->can_be_reset()); + + // Table sorted by other than default column, can be reset. + $table = $this->prepare_table_for_reset_test(uniqid('tablelib_test_')); + $table->sortable(true, 'column1', SORT_DESC); + $_GET['tsort'] = 'column2'; + $table->setup(); + unset($_GET['tsort']); + $this->assertTrue($table->can_be_reset()); + + // Table sorted by the default column after another sorting previously selected. + // This leads to different ORDER BY than just having a single sort defined, can be reset. + $table = $this->prepare_table_for_reset_test(uniqid('tablelib_test_')); + $table->sortable(true, 'column1', SORT_DESC); + $_GET['tsort'] = 'column0'; + $table->setup(); + $_GET['tsort'] = 'column1'; + $table->setup(); + unset($_GET['tsort']); + $this->assertTrue($table->can_be_reset()); + + // Table having some column collapsed, can be reset. + $table = $this->prepare_table_for_reset_test(uniqid('tablelib_test_')); + $_GET['thide'] = 'column2'; + $table->setup(); + unset($_GET['thide']); + $this->assertTrue($table->can_be_reset()); + + // Table having some column explicitly expanded, nothing to reset. + $table = $this->prepare_table_for_reset_test(uniqid('tablelib_test_')); + $_GET['tshow'] = 'column2'; + $table->setup(); + unset($_GET['tshow']); + $this->assertFalse($table->can_be_reset()); + + // Table after expanding a collapsed column, nothing to reset. + $table = $this->prepare_table_for_reset_test(uniqid('tablelib_test_')); + $_GET['thide'] = 'column0'; + $table->setup(); + $_GET['tshow'] = 'column0'; + $table->setup(); + unset($_GET['thide']); + unset($_GET['tshow']); + $this->assertFalse($table->can_be_reset()); + + // Table with some name filtering enabled, can be reset. + $table = $this->prepare_table_for_reset_test(uniqid('tablelib_test_')); + $_GET['tifirst'] = 'A'; + $table->setup(); + unset($_GET['tifirst']); + $this->assertTrue($table->can_be_reset()); + } }