[RFC|Merged] Enhanced Team Page

[bantu]

Some more queries (untested).

add($group_id)

Code: Select all

UPDATE phpbb_groups
SET group_teampage = MAX(group_teampage) + 1
WHERE group_id = $group_id
    AND group_teampage = 0
GROUP BY group_id

I've added the "AND group_teampage = 0" to ensure you can only add groups where group_teampage is not already enabled.

delete($group_id)

Code: Select all

$db->sql_transaction('begin');

UPDATE phpbb_groups g1, phpbb_groups g2
SET g1.group_teampage = g1.group_teampage - 1
WHERE g1.group_teampage > g2.group_teampage
  AND g2.group_id = $group_id
  AND g2.group_teampage <> 0

UPDATE phpbb_groups
SET group_teampage = 0
WHERE group_id = $group_id

$db->sql_transaction('commit'); 

move_up($group_id) and move_up($group_id) is basically swapping two integers. You have to ensure you're not at the bottom or top there. Transactions should also be used there.

Not sure if the "UPDATE phpbb_groups g1, phpbb_groups g2" works on all DBMSes. - naderman says no

[nickvergessen]

http://github.com/nickvergessen/phpbb3/ ... bda09c5bd4

• The huge switch block should probably be devided into separate functions (lesson learned from 3.0.x).
  Most of those functions/methods will only take the group_id as a parameter anyway.
• functions/methods should use the function/method keywords (public, private, protected, static).
• functions/methods should have proper documentation about the arguments they take and return
• Queries using the primary key do not need sql_query_limit(). There is only one row for what you're looking for anyway.

Fixed

• Some queries can probably be joined together. For 'add' it could probably be

  Code: Select all

  UPDATE phpbb_groups SET group_teampage = MAX(group_teampage) + 1 WHERE group_id = $var GROUP BY group_id

  or something like that. This way we should be protected from any race conditions there.

MySQL does not allow GROUP BY on UPDATE http://dev.mysql.com/doc/refman/5.1/en/update.html

• This code block appears four times.

  Code: Select all

  $sql = 'SELECT group_teampage
    FROM ' . GROUPS_TABLE . '
    WHERE group_id = ' . $group_id;
$result = $db->sql_query_limit($sql, 1);
$current_teampage = (int) $db->sql_fetchfield('group_teampage');
$db->sql_freeresult($result);  

• If 0 in "SET group_teampage = 0" is something special it should probably be a (class) constant.

Fixed

There should be some documentation about how the integer field is organised.

I wonder where I should post something like that.

Maybe everything should be put into a separate class.

Only in a separate class or also in a separate file?

[bantu]

There should be some documentation about how the integer field is organised.

I wonder where I should post something like that.

When you create a new class anyway, I'd say in the documentation block above the class definition.

Maybe everything should be put into a separate class.

Only in a separate class or also in a separate file?

Good question. Not sure about that. It would probably be better if it is in it's own file, so you can easily reuse it from other places (MODs).

[bantu]

Looking at the new code:

• You can leave out the function prefix "teampage_", because you're already in the class "phpbb_teampage".
  I'd for example change "teampage_action_add($group_id)" to "add_group($group_id)". The word "action" doesn't really say anything in this case.
• I'd suggest to make GROUP_NOT_TEAM a class constant in the phpbb_teampage class.

  Code: Select all

  const GROUP_DISABLED = 0;  

  or "GROUP_NOT_LISTED" or something like that.
  You can access the constant from within the class like this

  Code: Select all

  self::GROUP_DISABLED

  . From outside the class you can access it like this

  Code: Select all

  phpbb_teampage::GROUP_DISABLED

• Please use SQL transactions to group multiple update queries together.
• All functions are missing the documentation for the returned value. If no value is returned you can use "@return void"
• Since phpbb_teampage has no constructor and no properties, it might make sense to implement it fully as a static class.
• function get_group_teampage should not always call trigger_error() if the group doesn't exist. This might not be what someone wants in some cases. trigger_error() should probably be called from function manage_teampage() in acp_groups.php (preferred). Or make it an optional parameter (not preferred).
  You might be able to reuse this function in phpBB/includes/functions_user.php, because there is "$sql = 'SELECT group_teampage" again.
• Non-trivial if statements should have a comment to explain what you're checking for. For example "if ($current_teampage > 1)". You're checking if the group is already at the top, right?

[nickvergessen]

Fixed most of the points in http://github.com/nickvergessen/phpbb3/ ... 54b34eca16

Little thing that came up to my mind.
We could change group_legend to uint aswell and using the class with an additional-parameter could introduce a sortable and easy to manage group-legend aswell.

There are currently to MODs in the DB having a dl-count of 6000 in total, but as it's very easy to implement, with the already existing functions I think that should go into the core.

[A_Jelly_Doughnut]

There are currently to MODs in the DB having a dl-count of 6000 in total, but as it's very easy to implement, with the already existing functions I think that should go into the core.

+1. This seems like one of those features that people would use if it was there, but the MOD isn't worth it

[bantu]

Little thing that came up to my mind.
We could change group_legend to uint aswell and using the class with an additional-parameter could introduce a sortable and easy to manage group-legend aswell.

There are currently to MODs in the DB having a dl-count of 6000 in total, but as it's very easy to implement, with the already existing functions I think that should go into the core.

Sounds like it would make sense. I guess we/you can turn the class you wrote into a more generic class so it can be used for the team page and the group legend.

By the way, I guess you also have to provide an option to have it the way it is right now in 3.0.x, sorting by group_name.

[nickvergessen]

The legend is now managed the same way. Default is taken from currently being active and than sort by group name.

I didn't add an option for sort by group name, as you can just move groups up and down so this is done.

[bantu]

I didn't add an option for sort by group name, as you can just move groups up and down so this is done.

That's not optimal. You would have to adjust it everytime you add a group.

[nickvergessen]

Added another config for that now:
http://github.com/nickvergessen/phpbb3/ ... 08b1ef2a5a