From 291143fe6a821a07f8097e26e38f69efa3c84848 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Sun, 25 Jan 2015 00:33:26 +0100 Subject: [PATCH 1/3] [ticket/9109] Properly document and calculate the group settings with value 0 PHPBB3-9109 --- phpBB/includes/functions_privmsgs.php | 49 +++++++++++++-- phpBB/includes/ucp/ucp_pm_compose.php | 10 +-- phpBB/language/en/acp/groups.php | 4 +- .../fixtures/get_max_setting_from_group.xml | 62 +++++++++++++++++++ .../get_max_setting_from_group_test.php | 60 ++++++++++++++++++ 5 files changed, 170 insertions(+), 15 deletions(-) create mode 100644 tests/functions_privmsgs/fixtures/get_max_setting_from_group.xml create mode 100644 tests/functions_privmsgs/get_max_setting_from_group_test.php diff --git a/phpBB/includes/functions_privmsgs.php b/phpBB/includes/functions_privmsgs.php index ab0f092f87..ef6532f59d 100644 --- a/phpBB/includes/functions_privmsgs.php +++ b/phpBB/includes/functions_privmsgs.php @@ -2139,17 +2139,56 @@ function set_user_message_limit() { global $user, $db, $config; - // Get maximum about from user memberships - if it is 0, there is no limit set and we use the maximum value within the config. - $sql = 'SELECT MAX(g.group_message_limit) as max_message_limit + // Get maximum about from user memberships + $message_limit = phpbb_get_max_setting_from_group($db, $user->data['user_id'], 'message_limit'); + + // If it is 0, there is no limit set and we use the maximum value within the config. + $user->data['message_limit'] = (!$message_limit) ? $config['pm_max_msgs'] : $message_limit; +} + +/** + * Get the maximum PM setting for the groups of the user + * + * @param \phpbb\db\driver\driver_interface $db + * @param int $user_id + * @param string $setting Only 'max_recipients' and 'message_limit' are supported + * @return int The maximum setting for all groups of the user, unless one group has '0' + */ +function phpbb_get_max_setting_from_group(\phpbb\db\driver\driver_interface $db, $user_id, $setting) +{ + if ($setting !== 'max_recipients' && $setting !== 'message_limit') + { + throw new InvalidArgumentException('Setting "' . $setting . '" is not supported'); + } + + // Get maximum number of allowed recipients + $sql = 'SELECT MAX(g.group_' . $setting . ') as max_setting FROM ' . GROUPS_TABLE . ' g, ' . USER_GROUP_TABLE . ' ug - WHERE ug.user_id = ' . $user->data['user_id'] . ' + WHERE ug.user_id = ' . (int) $user_id . ' AND ug.user_pending = 0 AND ug.group_id = g.group_id'; $result = $db->sql_query($sql); - $message_limit = (int) $db->sql_fetchfield('max_message_limit'); + $max_setting = (int) $db->sql_fetchfield('max_setting'); $db->sql_freeresult($result); - $user->data['message_limit'] = (!$message_limit) ? $config['pm_max_msgs'] : $message_limit; + if ($max_setting) + { + // If the user is limited by a group, check whether he is also set to + // unlimited in another group (value 0) + $sql = 'SELECT g.group_' . $setting . ' as max_setting + FROM ' . GROUPS_TABLE . ' g, ' . USER_GROUP_TABLE . ' ug + WHERE ug.user_id = ' . (int) $user_id . ' + AND ug.user_pending = 0 + AND ug.group_id = g.group_id + AND g.group_max_recipients = 0'; + $result = $db->sql_query($sql); + $is_unlimited = $db->sql_fetchfield('max_setting'); + $db->sql_freeresult($result); + + $max_setting = ($is_unlimited === false) ? $max_setting : 0; + } + + return $max_setting; } /** diff --git a/phpBB/includes/ucp/ucp_pm_compose.php b/phpBB/includes/ucp/ucp_pm_compose.php index 0064fb89fc..8c69346f03 100644 --- a/phpBB/includes/ucp/ucp_pm_compose.php +++ b/phpBB/includes/ucp/ucp_pm_compose.php @@ -504,15 +504,9 @@ function compose_pm($id, $mode, $action, $user_folders = array()) } // Get maximum number of allowed recipients - $sql = 'SELECT MAX(g.group_max_recipients) as max_recipients - FROM ' . GROUPS_TABLE . ' g, ' . USER_GROUP_TABLE . ' ug - WHERE ug.user_id = ' . $user->data['user_id'] . ' - AND ug.user_pending = 0 - AND ug.group_id = g.group_id'; - $result = $db->sql_query($sql); - $max_recipients = (int) $db->sql_fetchfield('max_recipients'); - $db->sql_freeresult($result); + $max_recipients = phpbb_get_max_setting_from_group($db, $user->data['user_id'], 'max_recipients'); + // If it is 0, there is no limit set and we use the maximum value within the config. $max_recipients = (!$max_recipients) ? $config['pm_max_recipients'] : $max_recipients; // If this is a quote/reply "to all"... we may increase the max_recpients to the number of original recipients diff --git a/phpBB/language/en/acp/groups.php b/phpBB/language/en/acp/groups.php index a7700ed681..020f171b1b 100644 --- a/phpBB/language/en/acp/groups.php +++ b/phpBB/language/en/acp/groups.php @@ -83,7 +83,7 @@ $lang = array_merge($lang, array( 'GROUP_MEMBERS' => 'Group members', 'GROUP_MEMBERS_EXPLAIN' => 'This is a complete listing of all the members of this usergroup. It includes separate sections for leaders, pending and existing members. From here you can manage all aspects of who has membership of this group and what their role is. To remove a leader but keep them in the group use Demote rather than delete. Similarly use Promote to make an existing member a leader.', 'GROUP_MESSAGE_LIMIT' => 'Group private message limit per folder', - 'GROUP_MESSAGE_LIMIT_EXPLAIN' => 'This setting overrides the per-user folder message limit. A value of 0 means the user default limit will be used.', + 'GROUP_MESSAGE_LIMIT_EXPLAIN' => 'This setting overrides the per-user folder message limit. The maximum for all groups of the user is used to determinate the actual value.
Set this value to 0 to overwrite the setting for all users of this group with the board-wide setting.', 'GROUP_MODS_ADDED' => 'New group leaders added successfully.', 'GROUP_MODS_DEMOTED' => 'Group leaders demoted successfully.', 'GROUP_MODS_PROMOTED' => 'Group members promoted successfully.', @@ -92,7 +92,7 @@ $lang = array_merge($lang, array( 'GROUP_OPEN' => 'Open', 'GROUP_PENDING' => 'Pending members', 'GROUP_MAX_RECIPIENTS' => 'Maximum number of allowed recipients per private message', - 'GROUP_MAX_RECIPIENTS_EXPLAIN' => 'The maximum number of allowed recipients in a private message. If 0 is entered, the board-wide setting is used.', + 'GROUP_MAX_RECIPIENTS_EXPLAIN' => 'The maximum number of allowed recipients in a private message. The maximum for all groups of the user is used to determinate the actual value.
Set this value to 0 to overwrite the setting for all users of this group with the board-wide setting.', 'GROUP_OPTIONS_SAVE' => 'Group wide options', 'GROUP_PROMOTE' => 'Promote to group leader', 'GROUP_RANK' => 'Group rank', diff --git a/tests/functions_privmsgs/fixtures/get_max_setting_from_group.xml b/tests/functions_privmsgs/fixtures/get_max_setting_from_group.xml new file mode 100644 index 0000000000..b10bf782ec --- /dev/null +++ b/tests/functions_privmsgs/fixtures/get_max_setting_from_group.xml @@ -0,0 +1,62 @@ + + + + group_id + group_desc + group_message_limit + group_max_recipients + + 1 + + 1 + 3 + + + 2 + + 2 + 4 + + + 3 + + 0 + 0 + +
+ + user_id + group_id + user_pending + + 1 + 1 + 0 + + + 1 + 2 + 0 + + + 1 + 3 + 0 + + + 2 + 1 + 0 + + + 2 + 2 + 0 + + + 3 + 3 + 0 + +
+ diff --git a/tests/functions_privmsgs/get_max_setting_from_group_test.php b/tests/functions_privmsgs/get_max_setting_from_group_test.php new file mode 100644 index 0000000000..48d42ec48a --- /dev/null +++ b/tests/functions_privmsgs/get_max_setting_from_group_test.php @@ -0,0 +1,60 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +require_once dirname(__FILE__) . '/../../phpBB/includes/functions_privmsgs.php'; + +class phpbb_functions_privmsgs_get_max_setting_from_group_test extends phpbb_database_test_case +{ + public function getDataSet() + { + return $this->createXMLDataSet(dirname(__FILE__).'/fixtures/get_max_setting_from_group.xml'); + } + + /** @var \phpbb\db\driver\driver_interface */ + protected $db; + + protected function setUp() + { + parent::setUp(); + + $this->db = $this->new_dbal(); + } + + static public function get_max_setting_from_group_data() + { + return array( + array(1, 0, 'message_limit'), + array(2, 2, 'message_limit'), + array(3, 0, 'message_limit'), + array(1, 0, 'max_recipients'), + array(2, 4, 'max_recipients'), + array(3, 0, 'max_recipients'), + ); + } + + /** + * @dataProvider get_max_setting_from_group_data + */ + public function test_get_max_setting_from_group($user_id, $expected, $setting) + { + $this->assertEquals($expected, phpbb_get_max_setting_from_group($this->db, $user_id, $setting)); + } + + /** + * @expectedException InvalidArgumentException + */ + public function test_get_max_setting_from_group_throws() + { + phpbb_get_max_setting_from_group($this->db, ANONYMOUS, 'not_a_setting'); + } +} From 5cc0488f74633a9f28011ed935a3b7e902ba4497 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 3 Feb 2015 23:13:37 +0100 Subject: [PATCH 2/3] [ticket/9109] Only query database once in phpbb_get_max_setting_from_group More test cases have been added, too. PHPBB3-9109 --- phpBB/includes/functions_privmsgs.php | 25 ++++--------------- .../fixtures/get_max_setting_from_group.xml | 21 ++++++++++++++++ .../get_max_setting_from_group_test.php | 4 +++ 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/phpBB/includes/functions_privmsgs.php b/phpBB/includes/functions_privmsgs.php index ef6532f59d..10a1b8d4ad 100644 --- a/phpBB/includes/functions_privmsgs.php +++ b/phpBB/includes/functions_privmsgs.php @@ -2162,33 +2162,18 @@ function phpbb_get_max_setting_from_group(\phpbb\db\driver\driver_interface $db, } // Get maximum number of allowed recipients - $sql = 'SELECT MAX(g.group_' . $setting . ') as max_setting + $sql = 'SELECT MIN(g.group_' . $setting . ') as min_setting, MAX(g.group_' . $setting . ') as max_setting FROM ' . GROUPS_TABLE . ' g, ' . USER_GROUP_TABLE . ' ug WHERE ug.user_id = ' . (int) $user_id . ' AND ug.user_pending = 0 AND ug.group_id = g.group_id'; $result = $db->sql_query($sql); - $max_setting = (int) $db->sql_fetchfield('max_setting'); + $row = $db->sql_fetchrow($result); $db->sql_freeresult($result); + $max_setting = (int) $row['max_setting']; + $min_setting = (int) $row['min_setting']; - if ($max_setting) - { - // If the user is limited by a group, check whether he is also set to - // unlimited in another group (value 0) - $sql = 'SELECT g.group_' . $setting . ' as max_setting - FROM ' . GROUPS_TABLE . ' g, ' . USER_GROUP_TABLE . ' ug - WHERE ug.user_id = ' . (int) $user_id . ' - AND ug.user_pending = 0 - AND ug.group_id = g.group_id - AND g.group_max_recipients = 0'; - $result = $db->sql_query($sql); - $is_unlimited = $db->sql_fetchfield('max_setting'); - $db->sql_freeresult($result); - - $max_setting = ($is_unlimited === false) ? $max_setting : 0; - } - - return $max_setting; + return ($min_setting > 0) ? $max_setting : 0; } /** diff --git a/tests/functions_privmsgs/fixtures/get_max_setting_from_group.xml b/tests/functions_privmsgs/fixtures/get_max_setting_from_group.xml index b10bf782ec..c78d63f7cb 100644 --- a/tests/functions_privmsgs/fixtures/get_max_setting_from_group.xml +++ b/tests/functions_privmsgs/fixtures/get_max_setting_from_group.xml @@ -23,6 +23,12 @@ 0 0 + + 4 + + 0 + 5 + user_id @@ -58,5 +64,20 @@ 30 + + 4 + 4 + 0 + + + 5 + 3 + 1 + + + 5 + 2 + 0 +
diff --git a/tests/functions_privmsgs/get_max_setting_from_group_test.php b/tests/functions_privmsgs/get_max_setting_from_group_test.php index 48d42ec48a..3eb7866802 100644 --- a/tests/functions_privmsgs/get_max_setting_from_group_test.php +++ b/tests/functions_privmsgs/get_max_setting_from_group_test.php @@ -36,9 +36,13 @@ class phpbb_functions_privmsgs_get_max_setting_from_group_test extends phpbb_dat array(1, 0, 'message_limit'), array(2, 2, 'message_limit'), array(3, 0, 'message_limit'), + array(4, 0, 'message_limit'), + array(5, 2, 'message_limit'), array(1, 0, 'max_recipients'), array(2, 4, 'max_recipients'), array(3, 0, 'max_recipients'), + array(4, 5, 'max_recipients'), + array(5, 4, 'max_recipients'), ); } From c76d797cb2c4abb97c4e887b8c175626c7c9ba1e Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 3 Feb 2015 23:17:04 +0100 Subject: [PATCH 3/3] [ticket/9109] Improve docblock and fix spelling errors PHPBB3-9109 --- phpBB/includes/functions_privmsgs.php | 1 + phpBB/language/en/acp/groups.php | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/functions_privmsgs.php b/phpBB/includes/functions_privmsgs.php index 10a1b8d4ad..0465f3b00f 100644 --- a/phpBB/includes/functions_privmsgs.php +++ b/phpBB/includes/functions_privmsgs.php @@ -2153,6 +2153,7 @@ function set_user_message_limit() * @param int $user_id * @param string $setting Only 'max_recipients' and 'message_limit' are supported * @return int The maximum setting for all groups of the user, unless one group has '0' + * @throws \InvalidArgumentException If selected group setting is not supported */ function phpbb_get_max_setting_from_group(\phpbb\db\driver\driver_interface $db, $user_id, $setting) { diff --git a/phpBB/language/en/acp/groups.php b/phpBB/language/en/acp/groups.php index 020f171b1b..421075ce5e 100644 --- a/phpBB/language/en/acp/groups.php +++ b/phpBB/language/en/acp/groups.php @@ -83,7 +83,7 @@ $lang = array_merge($lang, array( 'GROUP_MEMBERS' => 'Group members', 'GROUP_MEMBERS_EXPLAIN' => 'This is a complete listing of all the members of this usergroup. It includes separate sections for leaders, pending and existing members. From here you can manage all aspects of who has membership of this group and what their role is. To remove a leader but keep them in the group use Demote rather than delete. Similarly use Promote to make an existing member a leader.', 'GROUP_MESSAGE_LIMIT' => 'Group private message limit per folder', - 'GROUP_MESSAGE_LIMIT_EXPLAIN' => 'This setting overrides the per-user folder message limit. The maximum for all groups of the user is used to determinate the actual value.
Set this value to 0 to overwrite the setting for all users of this group with the board-wide setting.', + 'GROUP_MESSAGE_LIMIT_EXPLAIN' => 'This setting overrides the per-user folder message limit. The maximum for all groups of the user is used to determine the actual value.
Set this value to 0 to overwrite the setting for all users of this group with the board-wide setting.', 'GROUP_MODS_ADDED' => 'New group leaders added successfully.', 'GROUP_MODS_DEMOTED' => 'Group leaders demoted successfully.', 'GROUP_MODS_PROMOTED' => 'Group members promoted successfully.', @@ -92,7 +92,7 @@ $lang = array_merge($lang, array( 'GROUP_OPEN' => 'Open', 'GROUP_PENDING' => 'Pending members', 'GROUP_MAX_RECIPIENTS' => 'Maximum number of allowed recipients per private message', - 'GROUP_MAX_RECIPIENTS_EXPLAIN' => 'The maximum number of allowed recipients in a private message. The maximum for all groups of the user is used to determinate the actual value.
Set this value to 0 to overwrite the setting for all users of this group with the board-wide setting.', + 'GROUP_MAX_RECIPIENTS_EXPLAIN' => 'The maximum number of allowed recipients in a private message. The maximum for all groups of the user is used to determine the actual value.
Set this value to 0 to overwrite the setting for all users of this group with the board-wide setting.', 'GROUP_OPTIONS_SAVE' => 'Group wide options', 'GROUP_PROMOTE' => 'Promote to group leader', 'GROUP_RANK' => 'Group rank',