[RFC|Merged] Enhanced Team Page

These requests for comments/change have lead to an implemented feature that has been successfully merged into the 3.1/Ascraeus branch. Everything listed in this forum will be available in phpBB 3.1.
User avatar
bantu
3.0 Release Manager
3.0 Release Manager
Posts: 557
Joined: Thu Sep 07, 2006 11:22 am
Location: Karlsruhe, Germany
Contact:

Re: The team page

Post by 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 :-P

User avatar
nickvergessen
Former Team Member
Posts: 733
Joined: Sun Oct 07, 2007 11:54 am
Location: Stuttgart, Germany
Contact:

Re: The team page

Post by nickvergessen »

http://github.com/nickvergessen/phpbb3/ ... bda09c5bd4
bantu wrote:
  • 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
bantu wrote:
  • 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
bantu wrote:
  • 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
bantu wrote:There should be some documentation about how the integer field is organised.
I wonder where I should post something like that.
bantu wrote:Maybe everything should be put into a separate class.
Only in a separate class or also in a separate file?
Member of the Development-TeamNo Support via PM

User avatar
bantu
3.0 Release Manager
3.0 Release Manager
Posts: 557
Joined: Thu Sep 07, 2006 11:22 am
Location: Karlsruhe, Germany
Contact:

Re: The team page

Post by bantu »

nickvergessen wrote:
bantu wrote: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.
nickvergessen wrote:
bantu wrote: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).

User avatar
bantu
3.0 Release Manager
3.0 Release Manager
Posts: 557
Joined: Thu Sep 07, 2006 11:22 am
Location: Karlsruhe, Germany
Contact:

Re: The team page

Post by 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?

User avatar
nickvergessen
Former Team Member
Posts: 733
Joined: Sun Oct 07, 2007 11:54 am
Location: Stuttgart, Germany
Contact:

Re: The team page

Post by 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.
Member of the Development-TeamNo Support via PM

User avatar
A_Jelly_Doughnut
Registered User
Posts: 1780
Joined: Wed Jun 04, 2003 4:23 pm

Re: The team page

Post by A_Jelly_Doughnut »

nickvergessen wrote: 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 :)
A_Jelly_Doughnut

User avatar
bantu
3.0 Release Manager
3.0 Release Manager
Posts: 557
Joined: Thu Sep 07, 2006 11:22 am
Location: Karlsruhe, Germany
Contact:

Re: The team page

Post by bantu »

nickvergessen wrote: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.

User avatar
nickvergessen
Former Team Member
Posts: 733
Joined: Sun Oct 07, 2007 11:54 am
Location: Stuttgart, Germany
Contact:

Re: The team page

Post by 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.
Member of the Development-TeamNo Support via PM

User avatar
bantu
3.0 Release Manager
3.0 Release Manager
Posts: 557
Joined: Thu Sep 07, 2006 11:22 am
Location: Karlsruhe, Germany
Contact:

Re: The team page

Post by bantu »

nickvergessen wrote: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.

User avatar
nickvergessen
Former Team Member
Posts: 733
Joined: Sun Oct 07, 2007 11:54 am
Location: Stuttgart, Germany
Contact:

Re: The team page

Post by nickvergessen »

Member of the Development-TeamNo Support via PM

Post Reply