2 files changed, 94 insertions, 24 deletions
diff --git a/phpBB/includes/group_positions.php b/phpBB/includes/group_positions.php
index 951014c0c0..fc44e3249d 100644
--- a/phpBB/includes/group_positions.php
+++ b/phpBB/includes/group_positions.php
@@ -163,25 +163,7 @@ class phpbb_group_positions
 	*/
 	public function move_up($group_id)
 	{
-		$current_value = $this->get_group_value($group_id);
-
-		// Only move the group, if it is in the list and not already on top.
-		if ($current_value > 1)
-		{
-			$this->db->sql_transaction('begin');
-
-			$sql = 'UPDATE ' . GROUPS_TABLE . '
-				SET group_' . $this->field . ' = group_' . $this->field . ' + 1
-				WHERE group_' . $this->field . ' = ' . ($current_value - 1);
-			$this->db->sql_query($sql);
-
-			$sql = 'UPDATE ' . GROUPS_TABLE . '
-				SET group_' . $this->field . ' = group_' . $this->field . ' - 1
-				WHERE group_id = ' . (int) $group_id;
-			$this->db->sql_query($sql);
-
-			$this->db->sql_transaction('commit');
-		}
+		$this->move($group_id, 1);
 	}
 
 	/**
@@ -192,22 +174,51 @@ class phpbb_group_positions
 	*/
 	public function move_down($group_id)
 	{
+		$this->move($group_id, -1);
+	}
+
+	/**
+	* Moves a group up/down
+	*
+	* @param	int		$group_id	group_id of the group to be moved
+	* @param	int		$delta		number of steps:
+	*								- positive = move up
+	*								- negative = move down
+	* @return	void
+	*/
+	public function move($group_id, $delta)
+	{
+		if (!is_int($delta) || !$delta)
+		{
+			return;
+		}
+
+		$move_up = ($delta > 0) ? true : false;
 		$current_value = $this->get_group_value($group_id);
 
 		if ($current_value != self::GROUP_DISABLED)
 		{
 			$this->db->sql_transaction('begin');
 
+			// First we move all groups between our current value and the target value up/down 1,
+			// so we have a gap for our group to move.
 			$sql = 'UPDATE ' . GROUPS_TABLE . '
-				SET group_' . $this->field . ' = group_' . $this->field . ' - 1
-				WHERE group_' . $this->field . ' = ' . ($current_value + 1);
+				SET group_' . $this->field . ' = group_' . $this->field . (($move_up) ? ' + 1' : ' - 1') . '
+				WHERE group_' . $this->field . ' > ' . self::GROUP_DISABLED . '
+					AND group_' . $this->field . (($move_up) ? ' >= ' : ' <= ') . ($current_value - $delta) . '
+					AND group_' . $this->field . (($move_up) ? ' < ' : ' > ') . $current_value;
 			$this->db->sql_query($sql);
 
-			if ($this->db->sql_affectedrows() == 1)
+			// Because there might be fewer groups above/below the group than we wanted to move,
+			// we use the number of changed groups, to update the group.
+			$delta = (int) $this->db->sql_affectedrows();
+
+			if ($delta)
 			{
-				// Only update when we move another one up, otherwise it was the last.
+				// And now finally, when we moved some other groups and built a gap,
+				// we can move the desired group to it.
 				$sql = 'UPDATE ' . GROUPS_TABLE . '
-					SET group_' . $this->field . ' = group_' . $this->field . ' + 1
+					SET group_' . $this->field . ' = group_' . $this->field . (($move_up) ? ' - ' : ' + ') . $delta . '
 					WHERE group_id = ' . (int) $group_id;
 				$this->db->sql_query($sql);
 			}
diff --git a/tests/group_positions/group_positions_test.php b/tests/group_positions/group_positions_test.php
index 7454402194..b68f205b37 100644
--- a/tests/group_positions/group_positions_test.php
+++ b/tests/group_positions/group_positions_test.php
@@ -224,5 +224,64 @@ class phpbb_group_positions_test extends phpbb_database_test_case
 
 		$this->assertEquals($expected, $db->sql_fetchrowset($result));
 	}
+
+	public static function move_data()
+	{
+		return array(
+			array('teampage', 1, 1, array(
+				array('group_id' => 1, 'group_teampage' => 0, 'group_legend' => 0),
+				array('group_id' => 2, 'group_teampage' => 1, 'group_legend' => 0),
+				array('group_id' => 3, 'group_teampage' => 2, 'group_legend' => 1),
+			)),
+			array('teampage', 1, -1, array(
+				array('group_id' => 1, 'group_teampage' => 0, 'group_legend' => 0),
+				array('group_id' => 2, 'group_teampage' => 1, 'group_legend' => 0),
+				array('group_id' => 3, 'group_teampage' => 2, 'group_legend' => 1),
+			)),
+			array('teampage', 3, 3, array(
+				array('group_id' => 1, 'group_teampage' => 0, 'group_legend' => 0),
+				array('group_id' => 2, 'group_teampage' => 2, 'group_legend' => 0),
+				array('group_id' => 3, 'group_teampage' => 1, 'group_legend' => 1),
+			)),
+			array('teampage', 2, 0, array(
+				array('group_id' => 1, 'group_teampage' => 0, 'group_legend' => 0),
+				array('group_id' => 2, 'group_teampage' => 1, 'group_legend' => 0),
+				array('group_id' => 3, 'group_teampage' => 2, 'group_legend' => 1),
+			)),
+			array('teampage', 2, -1, array(
+				array('group_id' => 1, 'group_teampage' => 0, 'group_legend' => 0),
+				array('group_id' => 2, 'group_teampage' => 2, 'group_legend' => 0),
+				array('group_id' => 3, 'group_teampage' => 1, 'group_legend' => 1),
+			)),
+			array('teampage', 2, -3, array(
+				array('group_id' => 1, 'group_teampage' => 0, 'group_legend' => 0),
+				array('group_id' => 2, 'group_teampage' => 2, 'group_legend' => 0),
+				array('group_id' => 3, 'group_teampage' => 1, 'group_legend' => 1),
+			)),
+			array('teampage', 3, -1, array(
+				array('group_id' => 1, 'group_teampage' => 0, 'group_legend' => 0),
+				array('group_id' => 2, 'group_teampage' => 1, 'group_legend' => 0),
+				array('group_id' => 3, 'group_teampage' => 2, 'group_legend' => 1),
+			)),
+		);
+	}
+
+	/**
+	* @dataProvider move_data
+	*/
+	public function test_move($field, $group_id, $increment, $expected)
+	{
+		global $db;
+
+		$db = $this->new_dbal();
+		$test_class = new phpbb_group_positions($db, $field);
+		$test_class->move($group_id, $increment);
+
+		$result = $db->sql_query('SELECT group_id, group_teampage, group_legend
+			FROM ' . GROUPS_TABLE . '
+			ORDER BY group_id ASC');
+
+		$this->assertEquals($expected, $db->sql_fetchrowset($result));
+	}
 }