From 2909976697db33eb7ebb9bb100d4fc0eb1fc5d9a Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 9 Feb 2014 16:23:46 +0100 Subject: [PATCH 1/9] [ticket/216] Use get_move_module_data() for acquiring module data for moving This will get rid of a query code that was duplicated over 4 methods in portal_module. B3P-216 --- acp/portal_module.php | 46 ++++++++++++++++------------------ tests/acp/move_module_test.php | 10 ++++++++ 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/acp/portal_module.php b/acp/portal_module.php index f4577839..0ed7da73 100644 --- a/acp/portal_module.php +++ b/acp/portal_module.php @@ -829,6 +829,24 @@ class portal_module } } + /** + * Get module_data required for moving it + * + * @param int $module_id ID of the module that should be moved + * @return array|null Module_data or empty if not successful + */ + public function get_move_module_data($module_id) + { + $sql = 'SELECT module_order, module_column, module_classname + FROM ' . PORTAL_MODULES_TABLE . ' + WHERE module_id = ' . (int) $module_id; + $result = $this->db->sql_query_limit($sql, 1); + $module_data = $this->db->sql_fetchrow($result); + $this->db->sql_freeresult($result); + + return $module_data; + } + /** * Move module up one row * @@ -836,12 +854,7 @@ class portal_module */ public function move_module_up($module_id) { - $sql = 'SELECT module_order, module_column - FROM ' . PORTAL_MODULES_TABLE . ' - WHERE module_id = ' . (int) $module_id; - $result = $this->db->sql_query_limit($sql, 1); - $module_data = $this->db->sql_fetchrow($result); - $this->db->sql_freeresult($result); + $module_data = $this->get_move_module_data($module_id); if (($module_data !== false) && ($module_data['module_order'] > 1)) { @@ -876,12 +889,7 @@ class portal_module */ protected function move_module_down($module_id) { - $sql = 'SELECT module_order, module_column - FROM ' . PORTAL_MODULES_TABLE . ' - WHERE module_id = ' . (int) $module_id; - $result = $this->db->sql_query_limit($sql, 1); - $module_data = $this->db->sql_fetchrow($result); - $this->db->sql_freeresult($result); + $module_data = $this->get_move_module_data($module_id); if ($module_data !== false) { @@ -915,12 +923,7 @@ class portal_module */ protected function move_module_left($module_id) { - $sql = 'SELECT module_order, module_column, module_classname - FROM ' . PORTAL_MODULES_TABLE . ' - WHERE module_id = ' . (int) $module_id; - $result = $this->db->sql_query_limit($sql, 1); - $module_data = $this->db->sql_fetchrow($result); - $this->db->sql_freeresult($result); + $module_data = $this->get_move_module_data($module_id); if (!isset($this->modules[$module_data['module_classname']])) { @@ -1023,12 +1026,7 @@ class portal_module */ protected function move_module_right($module_id) { - $sql = 'SELECT module_order, module_column, module_classname - FROM ' . PORTAL_MODULES_TABLE . ' - WHERE module_id = ' . (int) $module_id; - $result = $this->db->sql_query_limit($sql, 1); - $module_data = $this->db->sql_fetchrow($result); - $this->db->sql_freeresult($result); + $module_data = $this->get_move_module_data($module_id); if (!isset($this->modules[$module_data['module_classname']])) { diff --git a/tests/acp/move_module_test.php b/tests/acp/move_module_test.php index 892975a9..ca75868d 100644 --- a/tests/acp/move_module_test.php +++ b/tests/acp/move_module_test.php @@ -49,6 +49,16 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data return $this->createXMLDataSet(dirname(__FILE__) . '/fixtures/modules.xml'); } + public function test_get_move_module_data() + { + $module_data = $this->portal_module->get_move_module_data(1); + $this->assertEquals(array( + 'module_order' => 1, + 'module_column' => 2, + 'module_classname' => '\board3\portal\modules\clock', + ), $module_data); + } + public function test_move_module_up() { self::$redirected = false; From 48ae4e63fba64f2a7f33b4c612fca7d019416d44 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 9 Feb 2014 16:34:31 +0100 Subject: [PATCH 2/9] [ticket/216] Do not try to set language if $lang is null in user mock B3P-216 --- tests/mock/user.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/mock/user.php b/tests/mock/user.php index baed62e0..8106e5fc 100644 --- a/tests/mock/user.php +++ b/tests/mock/user.php @@ -34,7 +34,10 @@ class user extends \PHPUnit_Framework_TestCase { include_once(dirname(__FILE__) . '/../../language/en/' . $file . '.php'); - $this->set($lang); + if (isset($lang)) + { + $this->set($lang); + } } else { From feb2f52d4d03b4f44d0550290f21c73b3c8d7b8a Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 11 Feb 2014 22:05:55 +0100 Subject: [PATCH 3/9] [ticket/216] Methods for last module order and handle_after_move() The method for getting the last module order will return the last module order in a column. handle_after_move() will take care of return an error to the user or redirecting if the move was successful. B3P-216 --- acp/portal_module.php | 54 ++++++++++++++++++++---- tests/acp/move_module_test.php | 75 +++++++++++++++++++++++++++++++++- 2 files changed, 118 insertions(+), 11 deletions(-) diff --git a/acp/portal_module.php b/acp/portal_module.php index 0ed7da73..1236933f 100644 --- a/acp/portal_module.php +++ b/acp/portal_module.php @@ -847,6 +847,47 @@ class portal_module return $module_data; } + /** + * Handle output after moving module + * + * @param bool $success Whether moving module was successful + * @param bool $is_row Whether the module move was inside a row + * @return void + */ + public function handle_after_move($success = true, $is_row = false) + { + if (!$success) + { + trigger_error($this->user->lang['UNABLE_TO_MOVE' . (($is_row) ? '_ROW' : '')] . adm_back_link($this->u_action)); + } + + $this->cache->destroy('portal_modules'); + redirect($this->u_action); // redirect in order to get rid of excessive URL parameters + } + + /** + * Get the module order to the last module in the column + * + * @param int $module_column Module column to check + * @return int Module order of the last module in the column + */ + public function get_last_module_order($module_column) + { + $modules = obtain_portal_modules(); + $last_order = 1; + foreach ($modules as $cur_module) + { + if ($cur_module['module_column'] != $module_column) + { + continue; + } + + $last_order = max($last_order, $cur_module['module_order']); + } + + return $last_order; + } + /** * Move module up one row * @@ -873,13 +914,8 @@ class portal_module $this->db->sql_query($sql); } } - else - { - trigger_error($this->user->lang['UNABLE_TO_MOVE_ROW'] . adm_back_link($this->u_action)); - } - - $this->cache->destroy('portal_modules'); - redirect($this->u_action); // redirect in order to get rid of excessive URL parameters + + $this->handle_after_move($updated, true); } /** @@ -887,11 +923,11 @@ class portal_module * * @param int $module_id ID of the module that should be moved */ - protected function move_module_down($module_id) + public function move_module_down($module_id) { $module_data = $this->get_move_module_data($module_id); - if ($module_data !== false) + if ($module_data !== false && $this->get_last_module_order($module_data['module_column']) != $module_data['module_order']) { $sql = 'UPDATE ' . PORTAL_MODULES_TABLE . ' SET module_order = module_order - 1 diff --git a/tests/acp/move_module_test.php b/tests/acp/move_module_test.php index ca75868d..3cfd7cba 100644 --- a/tests/acp/move_module_test.php +++ b/tests/acp/move_module_test.php @@ -33,14 +33,25 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data new \board3\portal\modules\clock(), new \board3\portal\modules\birthday_list(new \phpbb\config\config(array()), $template, $this->db, $user), )); - $cache = $this->getMock('\phpbb\cache\cache', array('destroy', 'sql_exists')); + $cache = $this->getMock('\phpbb\cache\cache', array('destroy', 'sql_exists', 'get', 'put')); $cache->expects($this->any()) ->method('destroy') ->with($this->equalTo('portal_modules')); + $cache->expects($this->any()) + ->method('get') + ->with($this->anything()) + ->will($this->returnValue(false)); $cache->expects($this->any()) ->method('sql_exists') ->with($this->anything()); + $cache->expects($this->any()) + ->method('put') + ->with($this->anything()); $db = $this->db; + $user->set(array( + 'UNABLE_TO_MOVE' => 'UNABLE_TO_MOVE', + 'UNABLE_TO_MOVE_ROW' => 'UNABLE_TO_MOVE_ROW', + )); $this->portal_module = new \board3\portal\acp\portal_module(); } @@ -59,17 +70,77 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data ), $module_data); } + public function data_get_last_module_order() + { + return array( + array(1, 1), + array(2, 2), + array(2, 4), + ); + } + + /** + * @dataProvider data_get_last_module_order + */ + public function test_get_last_module_order($expected, $column) + { + $this->assertEquals($expected, $this->portal_module->get_last_module_order($column)); + } + public function test_move_module_up() { self::$redirected = false; $this->portal_module->move_module_up(2); $this->assertTrue(self::$redirected); - $this->setExpectedTriggerError(E_USER_NOTICE); + $this->setExpectedTriggerError(E_USER_NOTICE, 'UNABLE_TO_MOVE_ROW'); self::$redirected = false; $this->portal_module->move_module_up(2); $this->assertFalse(self::$redirected); } + + public function test_move_module_down() + { + self::$redirected = false; + $this->portal_module->move_module_down(1); + $this->assertTrue(self::$redirected); + + $this->setExpectedTriggerError(E_USER_NOTICE, 'UNABLE_TO_MOVE_ROW'); + self::$redirected = false; + $this->portal_module->move_module_down(1); + $this->assertFalse(self::$redirected); + } + + public function data_handle_after_move() + { + return array( + array(true, false, false), + array(false, false, 'UNABLE_TO_MOVE'), + array(false, true, 'UNABLE_TO_MOVE_ROW'), + ); + } + + /** + * @dataProvider data_handle_after_move + */ + public function test_handle_after_move($success, $is_row, $error) + { + if ($error) + { + $this->setExpectedTriggerError(E_USER_NOTICE, $error); + } + else + { + self::$redirected = false; + } + + $this->portal_module->handle_after_move($success, $is_row); + + if (!$error) + { + $this->assertTrue(self::$redirected); + } + } } function redirect($url) From 817c89e26ef604147f8d4b97acc1861a70fe29bf Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 11 Feb 2014 22:08:11 +0100 Subject: [PATCH 4/9] [ticket/216] Use handle_after_move() in move_module_down() B3P-216 --- acp/portal_module.php | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/acp/portal_module.php b/acp/portal_module.php index 1236933f..48ad1f27 100644 --- a/acp/portal_module.php +++ b/acp/portal_module.php @@ -943,13 +943,8 @@ class portal_module $this->db->sql_query($sql); } } - else - { - trigger_error($this->user->lang['UNABLE_TO_MOVE_ROW'] . adm_back_link($this->u_action)); - } - - $this->cache->destroy('portal_modules'); - redirect($this->u_action); // redirect in order to get rid of excessive URL parameters + + $this->handle_after_move($updated, true); } /** From c7b71f8c0a62805bf4a40f653330807f3262c315 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 11 Feb 2014 23:00:28 +0100 Subject: [PATCH 5/9] [ticket/216] Add move_module_left and move_module_right to unit tests These methods are now covered by unit tests. They might need to be stripped a little bit as the code coverage report currently states that some part of the code is never hit. B3P-216 --- acp/portal_module.php | 20 ++++---- tests/acp/fixtures/modules.xml | 26 ++++++++-- tests/acp/move_module_test.php | 90 ++++++++++++++++++++++++++++++++-- 3 files changed, 116 insertions(+), 20 deletions(-) diff --git a/acp/portal_module.php b/acp/portal_module.php index 48ad1f27..9f1fd57e 100644 --- a/acp/portal_module.php +++ b/acp/portal_module.php @@ -952,7 +952,7 @@ class portal_module * * @param int $module_id ID of the module that should be moved */ - protected function move_module_left($module_id) + public function move_module_left($module_id) { $module_data = $this->get_move_module_data($module_id); @@ -963,7 +963,7 @@ class portal_module $this->c_class = $this->modules[$module_data['module_classname']]; - if ($module_data !== false) + if ($module_data !== false && $module_data['module_column'] > column_string_num('left')) { if($this->c_class->columns & column_string_const(column_num_string($module_data['module_column'] - 1))) { @@ -1043,11 +1043,10 @@ class portal_module } else { - trigger_error($this->user->lang['UNABLE_TO_MOVE'] . adm_back_link($this->u_action)); + $this->handle_after_move(false); } - $this->cache->destroy('portal_modules'); - redirect($this->u_action); // redirect in order to get rid of excessive URL parameters + $this->handle_after_move(true); } /** @@ -1055,7 +1054,7 @@ class portal_module * * @param int $module_id ID of the module that should be moved */ - protected function move_module_right($module_id) + public function move_module_right($module_id) { $module_data = $this->get_move_module_data($module_id); @@ -1066,7 +1065,7 @@ class portal_module $this->c_class = $this->modules[$module_data['module_classname']]; - if ($module_data !== false) + if ($module_data !== false && $module_data['module_column'] < column_string_num('right')) { if($this->c_class->columns & column_string_const(column_num_string($module_data['module_column'] + 1))) { @@ -1146,11 +1145,10 @@ class portal_module } else { - trigger_error($this->user->lang['UNABLE_TO_MOVE'] . adm_back_link($this->u_action)); + $this->handle_after_move(false); } - - $this->cache->destroy('portal_modules'); - redirect($this->u_action); // redirect in order to get rid of excessive URL parameters + + $this->handle_after_move(true); } /** diff --git a/tests/acp/fixtures/modules.xml b/tests/acp/fixtures/modules.xml index c2f193c4..f03aa7b2 100644 --- a/tests/acp/fixtures/modules.xml +++ b/tests/acp/fixtures/modules.xml @@ -8,26 +8,44 @@ 1 \board3\portal\modules\clock - 2 + 1 1 2 \board3\portal\modules\birthday_list + 1 2 - 2 + + + 6 + \board3\portal\modules\donation + 1 + 3 3 \board3\portal\modules\clock - 4 + 3 1 4 \board3\portal\modules\birthday_list - 4 + 3 2 + + 5 + \board3\portal\modules\welcome + 2 + 1 + + + 7 + \board3\portal\modules\custom + 4 + 1 + diff --git a/tests/acp/move_module_test.php b/tests/acp/move_module_test.php index 3cfd7cba..9db44534 100644 --- a/tests/acp/move_module_test.php +++ b/tests/acp/move_module_test.php @@ -28,10 +28,13 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data ->disableOriginalConstructor() ->getMock()); // Mock module service collection + $config = new \phpbb\config\config(array()); $phpbb_container->set('board3.module_collection', array( new \board3\portal\modules\clock(), - new \board3\portal\modules\birthday_list(new \phpbb\config\config(array()), $template, $this->db, $user), + new \board3\portal\modules\birthday_list($config, $template, $this->db, $user), + new \board3\portal\modules\welcome($config, new \phpbb_mock_request, $this->db, $user, $phpbb_root_path, $phpEx), + new \board3\portal\modules\donation($config, $template, $user), )); $cache = $this->getMock('\phpbb\cache\cache', array('destroy', 'sql_exists', 'get', 'put')); $cache->expects($this->any()) @@ -65,7 +68,7 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data $module_data = $this->portal_module->get_move_module_data(1); $this->assertEquals(array( 'module_order' => 1, - 'module_column' => 2, + 'module_column' => 1, 'module_classname' => '\board3\portal\modules\clock', ), $module_data); } @@ -73,9 +76,10 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data public function data_get_last_module_order() { return array( - array(1, 1), - array(2, 2), - array(2, 4), + array(3, 1), + array(1, 2), + array(2, 3), + array(1, 4), ); } @@ -105,6 +109,10 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data $this->portal_module->move_module_down(1); $this->assertTrue(self::$redirected); + self::$redirected = false; + $this->portal_module->move_module_down(1); + $this->assertTrue(self::$redirected); + $this->setExpectedTriggerError(E_USER_NOTICE, 'UNABLE_TO_MOVE_ROW'); self::$redirected = false; $this->portal_module->move_module_down(1); @@ -141,6 +149,78 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data $this->assertTrue(self::$redirected); } } + + public function data_move_module_right() + { + return array( + array(2, 1, 1, 1), + array(6, 1, 2, 2), + array(7, 4, 0, 0), + ); + } + + /** + * @dataProvider data_move_module_right + */ + public function test_move_module_right($module_id, $column_start, $move_right, $move_left) + { + if ($column_start > 3) + { + $this->setExpectedTriggerError(E_USER_ERROR, 'CLASS_NOT_FOUND'); + $this->portal_module->move_module_right($module_id); + return; + } + for ($i = 1; $i <= $move_right; $i++) + { + $data = $this->portal_module->get_move_module_data($module_id); + $this->assertEquals($column_start, $data['module_column']); + $this->portal_module->move_module_right($module_id); + $column_start++; + } + $this->setExpectedTriggerError(E_USER_NOTICE, 'UNABLE_TO_MOVE'); + $this->portal_module->move_module_right($module_id); + } + + public function data_move_module_left() + { + return array( + array(2, 1, 1, 1), + array(6, 1, 2, 2), + array(7, 4, 0, 0), + ); + } + + /** + * @dataProvider data_move_module_left + */ + public function test_move_module_left($module_id, $column_start, $move_right, $move_left) + { + if ($column_start > 3) + { + $this->setExpectedTriggerError(E_USER_ERROR, 'CLASS_NOT_FOUND'); + $this->portal_module->move_module_left($module_id); + return; + } + for ($i = 1; $i <= $move_right; $i++) + { + $data = $this->portal_module->get_move_module_data($module_id); + $this->assertEquals($column_start, $data['module_column']); + $this->portal_module->move_module_right($module_id); + $column_start++; + } + + // We always start in the right column = 3 + $column_start = 3; + for ($i = 1; $i <= $move_left; $i++) + { + $data = $this->portal_module->get_move_module_data($module_id); + $this->assertEquals($column_start, $data['module_column']); + $this->portal_module->move_module_left($module_id); + $column_start--; + } + $this->setExpectedTriggerError(E_USER_NOTICE, 'UNABLE_TO_MOVE'); + $this->portal_module->move_module_left($module_id); + } } function redirect($url) From 1e34c77459d5e5ba28141324d18a9f6924cea99f Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 12 Feb 2014 18:26:31 +0100 Subject: [PATCH 6/9] [ticket/216] Fix non-existing array module_column in portal_module B3P-216 --- acp/portal_module.php | 69 ++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/acp/portal_module.php b/acp/portal_module.php index 9f1fd57e..6abaf28b 100644 --- a/acp/portal_module.php +++ b/acp/portal_module.php @@ -24,6 +24,7 @@ class portal_module protected $c_class; protected $db, $user, $cache, $template, $display_vars, $config, $phpbb_root_path, $phpbb_admin_path, $phpEx, $phpbb_container; protected $root_path, $mod_version_check, $request; + public $module_column = array(); /** @var \phpbb\di\service_collection Portal modules */ protected $modules; @@ -382,13 +383,13 @@ class portal_module // Create an array of already installed modules $portal_modules = obtain_portal_modules(); - $installed_modules = $module_column = array(); + $installed_modules = array(); foreach($portal_modules as $cur_module) { $installed_modules[] = $cur_module['module_classname']; // Create an array with the columns the module is in - $module_column[$cur_module['module_classname']][] = column_num_string($cur_module['module_column']); + $this->module_column[$cur_module['module_classname']][] = column_num_string($cur_module['module_column']); } if ($action == 'move_up') @@ -427,8 +428,8 @@ class portal_module if (in_array($column_string, array('left', 'right'))) { // does the module already exist in the side columns? - if (isset($module_column[$module_classname]) && - (in_array('left', $module_column[$module_classname]) || in_array('right', $module_column[$module_classname]))) + if (isset($this->module_column[$module_classname]) && + (in_array('left', $this->module_column[$module_classname]) || in_array('right', $this->module_column[$module_classname]))) { $submit = false; } @@ -436,10 +437,10 @@ class portal_module elseif (in_array($column_string, array('center', 'top', 'bottom'))) { // does the module already exist in the center columns? - if (isset($module_column[$module_classname]) && - (in_array('center', $module_column[$module_classname]) || - in_array('top', $module_column[$module_classname]) || - in_array('bottom', $module_column[$module_classname]))) + if (isset($this->module_column[$module_classname]) && + (in_array('center', $this->module_column[$module_classname]) || + in_array('top', $this->module_column[$module_classname]) || + in_array('bottom', $this->module_column[$module_classname]))) { $submit = false; } @@ -525,8 +526,8 @@ class portal_module if (in_array($column_string, array('left', 'right'))) { // does the module already exist in the side columns? - if (isset($module_column[$module_class]) && - (in_array('left', $module_column[$module_class]) || in_array('right', $module_column[$module_class]))) + if (isset($this->module_column[$module_class]) && + (in_array('left', $this->module_column[$module_class]) || in_array('right', $this->module_column[$module_class]))) { continue; } @@ -534,10 +535,10 @@ class portal_module elseif (in_array($column_string, array('center', 'top', 'bottom'))) { // does the module already exist in the center columns? - if (isset($module_column[$module_class]) && - (in_array('center', $module_column[$module_class]) || - in_array('top', $module_column[$module_class]) || - in_array('bottom', $module_column[$module_class]))) + if (isset($this->module_column[$module_class]) && + (in_array('center', $this->module_column[$module_class]) || + in_array('top', $this->module_column[$module_class]) || + in_array('bottom', $this->module_column[$module_class]))) { continue; } @@ -633,9 +634,9 @@ class portal_module $column_string = column_num_string($row['module_column'] + 1); // move 1 right if ($column_string == 'right' && - isset($module_column[$row['module_classname']]) && - (in_array('left', $module_column[$row['module_classname']]) || - in_array('right', $module_column[$row['module_classname']]))) + isset($this->module_column[$row['module_classname']]) && + (in_array('left', $this->module_column[$row['module_classname']]) || + in_array('right', $this->module_column[$row['module_classname']]))) { $move_right = false; } @@ -666,9 +667,9 @@ class portal_module $column_string = column_num_string($row['module_column'] - 1); // move 1 left if ($column_string == 'left' && - isset($module_column[$row['module_classname']]) && - (in_array('left', $module_column[$row['module_classname']]) || - in_array('right', $module_column[$row['module_classname']]))) + isset($this->module_column[$row['module_classname']]) && + (in_array('left', $this->module_column[$row['module_classname']]) || + in_array('right', $this->module_column[$row['module_classname']]))) { $move_left = false; } @@ -990,17 +991,17 @@ class portal_module $column_string = column_num_string($module_data['module_column'] - $move_action); // we can only move left to the left & center column if ($column_string == 'left' && - isset($module_column[$module_data['module_classname']]) && - (in_array('left', $module_column[$module_data['module_classname']]) || - in_array('right', $module_column[$module_data['module_classname']]))) + isset($this->module_column[$module_data['module_classname']]) && + (in_array('left', $this->module_column[$module_data['module_classname']]) || + in_array('right', $this->module_column[$module_data['module_classname']]))) { trigger_error($this->user->lang['UNABLE_TO_MOVE'] . adm_back_link($this->u_action)); } elseif ($column_string == 'center' && - isset($module_column[$module_data['module_classname']]) && - (in_array('center', $module_column[$module_data['module_classname']]) || - in_array('top', $module_column[$module_data['module_classname']]) || - in_array('bottom', $module_column[$module_data['module_classname']]))) + isset($this->module_column[$module_data['module_classname']]) && + (in_array('center', $this->module_column[$module_data['module_classname']]) || + in_array('top', $this->module_column[$module_data['module_classname']]) || + in_array('bottom', $this->module_column[$module_data['module_classname']]))) { // we are moving from the right to the center column so we should move to the left column instead $move_action = 2; @@ -1092,17 +1093,17 @@ class portal_module $column_string = column_num_string($module_data['module_column'] + $move_action); // we can only move right to the right & center column if ($column_string == 'right' && - isset($module_column[$module_data['module_classname']]) && - (in_array('left', $module_column[$module_data['module_classname']]) || - in_array('right', $module_column[$module_data['module_classname']]))) + isset($this->module_column[$module_data['module_classname']]) && + (in_array('left', $this->module_column[$module_data['module_classname']]) || + in_array('right', $this->module_column[$module_data['module_classname']]))) { trigger_error($this->user->lang['UNABLE_TO_MOVE'] . adm_back_link($this->u_action)); } elseif ($column_string == 'center' && - isset($module_column[$module_data['module_classname']]) && - (in_array('center', $module_column[$module_data['module_classname']]) || - in_array('top', $module_column[$module_data['module_classname']]) || - in_array('bottom', $module_column[$module_data['module_classname']]))) + isset($this->module_column[$module_data['module_classname']]) && + (in_array('center', $this->module_column[$module_data['module_classname']]) || + in_array('top', $this->module_column[$module_data['module_classname']]) || + in_array('bottom', $this->module_column[$module_data['module_classname']]))) { // we are moving from the left to the center column so we should move to the right column instead $move_action = 2; From d9eaf2f2fdcf24b96e455b520ad5709b1af29c8e Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 12 Feb 2014 18:26:58 +0100 Subject: [PATCH 7/9] [ticket/216] Add error handling for incorrectly moving modules left & right B3P-216 --- acp/portal_module.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/acp/portal_module.php b/acp/portal_module.php index 6abaf28b..0f2efd1e 100644 --- a/acp/portal_module.php +++ b/acp/portal_module.php @@ -976,7 +976,7 @@ class portal_module } else { - // @todo: need an error handle here (i.e. trigger_error()) + $this->handle_after_move(false); } /** @@ -1078,7 +1078,7 @@ class portal_module } else { - // @todo: need an error handle here + $this->handle_after_move(false); } /** From 1d2bc15cf4ccc8eab96ceea28006c39a75ac1047 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 12 Feb 2014 18:27:34 +0100 Subject: [PATCH 8/9] [ticket/216] Get full test coverage on move_module_left & move_module_right B3P-216 --- tests/acp/fixtures/modules.xml | 2 +- tests/acp/move_module_test.php | 56 ++++++++++++++++++++++++++-------- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/tests/acp/fixtures/modules.xml b/tests/acp/fixtures/modules.xml index f03aa7b2..a1358060 100644 --- a/tests/acp/fixtures/modules.xml +++ b/tests/acp/fixtures/modules.xml @@ -14,7 +14,7 @@ 2 \board3\portal\modules\birthday_list - 1 + 2 2 diff --git a/tests/acp/move_module_test.php b/tests/acp/move_module_test.php index 9db44534..cfceaa94 100644 --- a/tests/acp/move_module_test.php +++ b/tests/acp/move_module_test.php @@ -56,6 +56,17 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data 'UNABLE_TO_MOVE_ROW' => 'UNABLE_TO_MOVE_ROW', )); $this->portal_module = new \board3\portal\acp\portal_module(); + $this->update_portal_modules(); + } + + protected function update_portal_modules() + { + $this->portal_module->module_column = array(); + $portal_modules = obtain_portal_modules(); + foreach($portal_modules as $cur_module) + { + $this->portal_module->module_column[$cur_module['module_classname']][] = column_num_string($cur_module['module_column']); + } } public function getDataSet() @@ -77,7 +88,7 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data { return array( array(3, 1), - array(1, 2), + array(2, 2), array(2, 3), array(1, 4), ); @@ -106,16 +117,12 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data public function test_move_module_down() { self::$redirected = false; - $this->portal_module->move_module_down(1); - $this->assertTrue(self::$redirected); - - self::$redirected = false; - $this->portal_module->move_module_down(1); + $this->portal_module->move_module_down(3); $this->assertTrue(self::$redirected); $this->setExpectedTriggerError(E_USER_NOTICE, 'UNABLE_TO_MOVE_ROW'); self::$redirected = false; - $this->portal_module->move_module_down(1); + $this->portal_module->move_module_down(3); $this->assertFalse(self::$redirected); } @@ -153,16 +160,19 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data public function data_move_module_right() { return array( - array(2, 1, 1, 1), - array(6, 1, 2, 2), - array(7, 4, 0, 0), + array(6, 1, 2), + array(6, 1, 1, 2), + array(7, 4, 0), + array(5, 2, 0), + array(1, 1, 1, 3), + array(2, 2, 0), ); } /** * @dataProvider data_move_module_right */ - public function test_move_module_right($module_id, $column_start, $move_right, $move_left) + public function test_move_module_right($module_id, $column_start, $move_right, $add_to_column = false) { if ($column_start > 3) { @@ -170,12 +180,20 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data $this->portal_module->move_module_right($module_id); return; } + + if ($add_to_column) + { + $module_data = $this->portal_module->get_move_module_data($module_id); + $this->portal_module->module_column[$module_data['module_classname']][] = column_num_string($add_to_column); + } + for ($i = 1; $i <= $move_right; $i++) { $data = $this->portal_module->get_move_module_data($module_id); $this->assertEquals($column_start, $data['module_column']); $this->portal_module->move_module_right($module_id); $column_start++; + $this->update_portal_modules(); } $this->setExpectedTriggerError(E_USER_NOTICE, 'UNABLE_TO_MOVE'); $this->portal_module->move_module_right($module_id); @@ -184,16 +202,19 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data public function data_move_module_left() { return array( - array(2, 1, 1, 1), + array(1, 1, 1, 1), array(6, 1, 2, 2), + array(5, 2, 0, 0), + array(6, 1, 2, 1, 2), array(7, 4, 0, 0), + array(2, 2, 0, 0, 1), ); } /** * @dataProvider data_move_module_left */ - public function test_move_module_left($module_id, $column_start, $move_right, $move_left) + public function test_move_module_left($module_id, $column_start, $move_right, $move_left, $add_to_column = false) { if ($column_start > 3) { @@ -201,14 +222,22 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data $this->portal_module->move_module_left($module_id); return; } + for ($i = 1; $i <= $move_right; $i++) { $data = $this->portal_module->get_move_module_data($module_id); $this->assertEquals($column_start, $data['module_column']); $this->portal_module->move_module_right($module_id); + $this->update_portal_modules(); $column_start++; } + if ($add_to_column) + { + $module_data = $this->portal_module->get_move_module_data($module_id); + $this->portal_module->module_column[$module_data['module_classname']][] = column_num_string($add_to_column); + } + // We always start in the right column = 3 $column_start = 3; for ($i = 1; $i <= $move_left; $i++) @@ -216,6 +245,7 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data $data = $this->portal_module->get_move_module_data($module_id); $this->assertEquals($column_start, $data['module_column']); $this->portal_module->move_module_left($module_id); + $this->update_portal_modules(); $column_start--; } $this->setExpectedTriggerError(E_USER_NOTICE, 'UNABLE_TO_MOVE'); From 4655544ed44bc64e4fde1ee166546fb51780ba70 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 18 Feb 2014 00:44:41 +0100 Subject: [PATCH 9/9] [ticket/216] Add method for checking if module can be moved Also fixed minor issues with incorrect checks for module class 'custom'. B3P-216 --- acp/portal_module.php | 117 ++++++++++++++++++--------------- tests/acp/move_module_test.php | 21 ++++++ 2 files changed, 86 insertions(+), 52 deletions(-) diff --git a/acp/portal_module.php b/acp/portal_module.php index 0f2efd1e..1b9fcc10 100644 --- a/acp/portal_module.php +++ b/acp/portal_module.php @@ -146,7 +146,7 @@ class portal_module 'MODULE_SHOW_IMAGE' => (in_array(column_num_string($module_data['module_column']), array('center', 'top', 'bottom'))) ? false : true, )); - if($module_data['module_classname'] != 'custom') + if($module_data['module_classname'] != '\board3\portal\modules\custom') { $groups_ary = explode(',', $module_data['module_group_ids']); @@ -283,7 +283,7 @@ class portal_module if(isset($module_name)) { - if ($module_data['module_classname'] !== 'custom') + if ($module_data['module_classname'] !== '\board3\portal\modules\custom') { add_log('admin', 'LOG_PORTAL_CONFIG', $module_name); } @@ -427,23 +427,11 @@ class portal_module // do we want to add the module to the side columns or to the center columns? if (in_array($column_string, array('left', 'right'))) { - // does the module already exist in the side columns? - if (isset($this->module_column[$module_classname]) && - (in_array('left', $this->module_column[$module_classname]) || in_array('right', $this->module_column[$module_classname]))) - { - $submit = false; - } + $submit = $this->can_move_module(array('left', 'right'), $module_classname); } elseif (in_array($column_string, array('center', 'top', 'bottom'))) { - // does the module already exist in the center columns? - if (isset($this->module_column[$module_classname]) && - (in_array('center', $this->module_column[$module_classname]) || - in_array('top', $this->module_column[$module_classname]) || - in_array('bottom', $this->module_column[$module_classname]))) - { - $submit = false; - } + $submit = $this->can_move_module(array('center', 'top', 'bottom'), $module_classname); } // do not install if module already exists in that column @@ -526,8 +514,7 @@ class portal_module if (in_array($column_string, array('left', 'right'))) { // does the module already exist in the side columns? - if (isset($this->module_column[$module_class]) && - (in_array('left', $this->module_column[$module_class]) || in_array('right', $this->module_column[$module_class]))) + if (!$this->can_move_module(array('left', 'right'), $module_class)) { continue; } @@ -535,10 +522,7 @@ class portal_module elseif (in_array($column_string, array('center', 'top', 'bottom'))) { // does the module already exist in the center columns? - if (isset($this->module_column[$module_class]) && - (in_array('center', $this->module_column[$module_class]) || - in_array('top', $this->module_column[$module_class]) || - in_array('bottom', $this->module_column[$module_class]))) + if (!$this->can_move_module(array('center', 'top', 'bottom'), $module_class)) { continue; } @@ -629,14 +613,11 @@ class portal_module * this only applies to modules in the center column as the side modules * will automatically skip the center column when moving if they need to */ - if ($row['module_classname'] != 'custom') + if ($row['module_classname'] != '\board3\portal\modules\custom') { $column_string = column_num_string($row['module_column'] + 1); // move 1 right - if ($column_string == 'right' && - isset($this->module_column[$row['module_classname']]) && - (in_array('left', $this->module_column[$row['module_classname']]) || - in_array('right', $this->module_column[$row['module_classname']]))) + if ($column_string == 'right' && !$this->can_move_module(array('left', 'right'), $row['module_classname'])) { $move_right = false; } @@ -662,14 +643,11 @@ class portal_module * this only applies to modules in the center column as the side modules * will automatically skip the center column when moving if they need to */ - if ($row['module_classname'] != 'custom') + if ($row['module_classname'] != '\board3\portal\modules\custom') { $column_string = column_num_string($row['module_column'] - 1); // move 1 left - if ($column_string == 'left' && - isset($this->module_column[$row['module_classname']]) && - (in_array('left', $this->module_column[$row['module_classname']]) || - in_array('right', $this->module_column[$row['module_classname']]))) + if ($column_string == 'left' && !$this->can_move_module(array('left', 'right'), $row['module_classname'])) { $move_left = false; } @@ -986,22 +964,15 @@ class portal_module * new column (side columns (left & right) or center columns (top, center, bottom)). * of course this does not apply to custom modules. */ - if ($module_data['module_classname'] != 'custom' && $move_action == 1) + if ($module_data['module_classname'] != '\board3\portal\modules\custom' && $move_action == 1) { $column_string = column_num_string($module_data['module_column'] - $move_action); // we can only move left to the left & center column - if ($column_string == 'left' && - isset($this->module_column[$module_data['module_classname']]) && - (in_array('left', $this->module_column[$module_data['module_classname']]) || - in_array('right', $this->module_column[$module_data['module_classname']]))) + if ($column_string == 'left' && !$this->can_move_module(array('right', 'left'), $module_data['module_classname'])) { trigger_error($this->user->lang['UNABLE_TO_MOVE'] . adm_back_link($this->u_action)); } - elseif ($column_string == 'center' && - isset($this->module_column[$module_data['module_classname']]) && - (in_array('center', $this->module_column[$module_data['module_classname']]) || - in_array('top', $this->module_column[$module_data['module_classname']]) || - in_array('bottom', $this->module_column[$module_data['module_classname']]))) + elseif ($column_string == 'center' && !$this->can_move_module(array('top', 'center', 'bottom'), $module_data['module_classname'])) { // we are moving from the right to the center column so we should move to the left column instead $move_action = 2; @@ -1088,22 +1059,15 @@ class portal_module * new column (side columns (left & right) or center columns (top, center, bottom)). * of course this does not apply to custom modules. */ - if ($module_data['module_classname'] != 'custom' && $move_action == 1) + if ($module_data['module_classname'] != '\board3\portal\modules\custom' && $move_action == 1) { $column_string = column_num_string($module_data['module_column'] + $move_action); // we can only move right to the right & center column - if ($column_string == 'right' && - isset($this->module_column[$module_data['module_classname']]) && - (in_array('left', $this->module_column[$module_data['module_classname']]) || - in_array('right', $this->module_column[$module_data['module_classname']]))) + if ($column_string == 'right' && !$this->can_move_module(array('right', 'left'), $module_data['module_classname'])) { trigger_error($this->user->lang['UNABLE_TO_MOVE'] . adm_back_link($this->u_action)); } - elseif ($column_string == 'center' && - isset($this->module_column[$module_data['module_classname']]) && - (in_array('center', $this->module_column[$module_data['module_classname']]) || - in_array('top', $this->module_column[$module_data['module_classname']]) || - in_array('bottom', $this->module_column[$module_data['module_classname']]))) + elseif ($column_string == 'center' && !$this->can_move_module(array('top', 'center', 'bottom'), $module_data['module_classname'])) { // we are moving from the left to the center column so we should move to the right column instead $move_action = 2; @@ -1248,4 +1212,53 @@ class portal_module } } } + + /** + * Check if module can be moved to desired column(s) + * + * @param array|int $target_column Column(s) the module should be + * moved to + * @param string $module_class Class of the module + * @return bool True if module can be moved to desired column, + * false if not + */ + public function can_move_module($target_column, $module_class) + { + $submit = true; + + if (is_array($target_column)) + { + foreach ($target_column as $column) + { + if (!$this->can_move_module($column, $module_class)) + { + $submit = false; + } + } + } + + // do we want to add the module to the side columns or to the center columns? + if (in_array($target_column, array('left', 'right'))) + { + // does the module already exist in the side columns? + if (isset($this->module_column[$module_class]) && + (in_array('left', $this->module_column[$module_class]) || in_array('right', $this->module_column[$module_class]))) + { + $submit = false; + } + } + elseif (in_array($target_column, array('center', 'top', 'bottom'))) + { + // does the module already exist in the center columns? + if (isset($this->module_column[$module_class]) && + (in_array('center', $this->module_column[$module_class]) || + in_array('top', $this->module_column[$module_class]) || + in_array('bottom', $this->module_column[$module_class]))) + { + $submit = false; + } + } + + return $submit; + } } diff --git a/tests/acp/move_module_test.php b/tests/acp/move_module_test.php index cfceaa94..dd123fd1 100644 --- a/tests/acp/move_module_test.php +++ b/tests/acp/move_module_test.php @@ -251,6 +251,27 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data $this->setExpectedTriggerError(E_USER_NOTICE, 'UNABLE_TO_MOVE'); $this->portal_module->move_module_left($module_id); } + + public function data_can_move_module() + { + return array( + array(false, 'left', '\board3\portal\modules\clock'), + array(false, 'right', '\board3\portal\modules\clock'), + array(true, 'center', '\board3\portal\modules\clock'), + array(true, array('top', 'bottom', 'center'), '\board3\portal\modules\clock'), + array(false, array('left', 'right'), '\board3\portal\modules\clock'), + array(false, 'center', '\board3\portal\modules\birthday_list'), + ); + } + + /** + * @dataProvider data_can_move_module + */ + public function test_can_move_module($expected, $target_column, $module_class) + { + $this->update_portal_modules(); + $this->assertEquals($expected, $this->portal_module->can_move_module($target_column, $module_class)); + } } function redirect($url)