diff --git a/docs/changelog/6.x.x.txt b/docs/changelog/6.x.x.txt index 73c38aa64..d1b171f34 100644 --- a/docs/changelog/6.x.x.txt +++ b/docs/changelog/6.x.x.txt @@ -11,6 +11,8 @@ - Run on registration and alert on new user have been converted to a single workflow. - Added a Photogallery prototype to the default installation. + - Changed the Group property dbCacheTimeout to be groupCacheTimeout and made + it cache all the users in a group during that time. - Added an option for hosters to limit the number of assets that may be created on a WebGUI site. - Added a change url function that will allow an editor to change the URL of diff --git a/docs/migration.txt b/docs/migration.txt index 8191226d8..d0e03db35 100644 --- a/docs/migration.txt +++ b/docs/migration.txt @@ -13,6 +13,7 @@ CONTENTS 4. Scheduler Migration 5. System Changes Affecting Migration 6. Automatic list of Assets in Help System. +7. Group Membership 1. Wobject/Asset Migration @@ -910,3 +911,29 @@ is more flexible, is more reliable, and is compliant with the new mail system. 6.1 Starting in WebGUI 6.7, there is now an automatic list of all Assets in each site's WebGUI configuration file. For this to work correctly, each Asset should have a Help entry named "assetName add/edit". + + +7. Group Membership +------------------------------------- +Starting in WebGUI 7.0, there are several change to how group membership +works. The biggest change is that there are two methods for getting +the users in a group. + +$group->getUsers will return all users who have are members via the WebGUI API. + +$group->getAllUsers will return a list of all users who are members of the +current group or any of its subgroups via _any_ method. Previously, there +was no way to get a complete list of all users, and you had to request +a recursive fetch of users. getAllUsers is always recursive. + +So basically, you will want to convert your code to use getAllUsers instead +of getUsers. + +Next, there has been a long standing bug in WebGUI regarding the +dbCacheTimeout group property. It didn't work as a timeout, but rather +as an expiration property for adding users to groups via database queries +AND LDAP. In WebGUI 7.0, this property has been renamed groupCacheTimeout +and it actually functions as a cache timeout. Note that the timeout may +extend the amount of time that a member is in a group, so don't set an +arbitrarily long timeout. + diff --git a/lib/WebGUI/Asset.pm b/lib/WebGUI/Asset.pm index e27e215af..9002a441e 100644 --- a/lib/WebGUI/Asset.pm +++ b/lib/WebGUI/Asset.pm @@ -599,7 +599,7 @@ sub getEditForm { my $clause; if ($self->session->user->isInGroup(3)) { my $group = WebGUI::Group->new($self->session,4); - my $contentManagers = $group->getUsers(1); + my $contentManagers = $group->getAllUsers(); push (@$contentManagers, $self->session->user->userId); $clause = "userId in (".$self->session->db->quoteAndJoin($contentManagers).")"; } else { diff --git a/lib/WebGUI/Asset/FilePile.pm b/lib/WebGUI/Asset/FilePile.pm index 29af0a062..075253d43 100644 --- a/lib/WebGUI/Asset/FilePile.pm +++ b/lib/WebGUI/Asset/FilePile.pm @@ -93,7 +93,7 @@ sub edit { my $clause; if ($self->session->user->isInGroup(3)) { my $group = WebGUI::Group->new($self->session,4); - my $contentManagers = $group->getUsers(1); + my $contentManagers = $group->getAllUsers(); push (@$contentManagers, $self->session->user->userId); $clause = "userId in (".$self->session->db->quoteAndJoin($contentManagers).")"; } else { diff --git a/lib/WebGUI/AssetBranch.pm b/lib/WebGUI/AssetBranch.pm index 3d2d8cd06..aeb10d962 100644 --- a/lib/WebGUI/AssetBranch.pm +++ b/lib/WebGUI/AssetBranch.pm @@ -175,7 +175,7 @@ sub www_editBranch { } my $clause; if ($self->session->user->isInGroup(3)) { - my $contentManagers = WebGUI::Group->new($self->session,4)->getUsers(1); + my $contentManagers = WebGUI::Group->new($self->session,4)->getAllUsers(); push (@$contentManagers, $self->session->user->userId); $clause = "userId in (".$self->session->db->quoteAndJoin($contentManagers).")"; } else { diff --git a/lib/WebGUI/Group.pm b/lib/WebGUI/Group.pm index bbd8211a5..37d15c690 100755 --- a/lib/WebGUI/Group.pm +++ b/lib/WebGUI/Group.pm @@ -64,7 +64,8 @@ This package provides an object-oriented way of managing WebGUI groups and group $arrayRef = $group->getGroupsFor(); $arrayRef = $self->session->user->getGroups($userId); $arrayRef = $group->getGroupsIn($recursive); - $arrayRef = $group->getUsers($recursive); + $arrayRef = $group->getUsers(); ##WebGUI defined groups only + $arrayRef = $group->getAllUsers(); ##All users in all groups in this group $boolean = $self->session->user->isInGroup($groupId); $boolean = $group->userIsAdmin($userId,$groupId); $epoch = $group->userGroupExpireDate($userId,$groupId); @@ -113,6 +114,7 @@ not be added to any group. Groups may not be added to themselves. sub addGroups { my $self = shift; my $groups = shift; + WebGUI::Cache->new($self->session, $self->getId)->delete; $self->session->stow->delete("isInGroup"); foreach my $gid (@{$groups}) { next if ($gid eq '1'); @@ -149,6 +151,7 @@ An override for the default offset of the grouping. Specified in seconds. sub addUsers { my $self = shift; my $users = shift; + WebGUI::Cache->new($self->session, $self->getId)->delete; $self->session->stow->delete("isInGroup"); my $expireOffset = shift || $self->get("expireOffset"); foreach my $uid (@{$users}) { @@ -207,6 +210,21 @@ sub autoDelete { } +#------------------------------------------------------------------- + +=head2 clearCaches ( ) + +Clears caches for this group. + +=cut + +sub clearCaches { + my $self = shift; + WebGUI::Cache->new($self->session, $self->getId)->delete; + $self->session->stow->delete("isInGroup"); + $self->session->stow->delete("gotGroupsInGroup"); +} + #------------------------------------------------------------------- =head2 dateCreated ( ) @@ -231,6 +249,7 @@ Deletes this group and all references to it. sub delete { my $self = shift; + $self->clearCaches; $self->session->db->write("delete from groups where groupId=?", [$self->getId]); $self->session->db->write("delete from groupings where groupId=?", [$self->getId]); $self->session->db->write("delete from groupGroupings where inGroup=? or groupId=?", [$self->getId, $self->getId]); @@ -256,11 +275,10 @@ An array reference containing the list of group ids to delete from. sub deleteGroups { my $self = shift; my $groups = shift; - $self->session->stow->delete("isInGroup"); + $self->clearCaches; foreach my $gid (@{$groups}) { $self->session->db->write("delete from groupGroupings where groupId=? and inGroup=?",[$gid, $self->getId]); } - $self->session->stow->delete("gotGroupsInGroup"); } @@ -279,11 +297,10 @@ An array reference containing a list of users. sub deleteUsers { my $self = shift; my $users = shift; - $self->session->stow->delete("isInGroup"); + $self->clearCaches; foreach my $uid (@{$users}) { $self->session->db->write("delete from groupings where groupId=? and userId=?",[$self->getId, $uid]); } - $self->session->stow->delete("gotGroupsForUser"); } #------------------------------------------------------------------- @@ -667,13 +684,57 @@ sub getGroupsIn { #------------------------------------------------------------------- -=head2 getUsers ( [ recursive, withoutExpired ] ) +=head2 getAllUsers ( [ withoutExpired ] ) -Returns an array reference containing a list of users that belong to this group. +Returns an array reference containing a list of users that belong to this group +and in any group that belongs to this group. -=head3 recursive +=head3 withoutExpired -A boolean value to determine whether the method should return the users directly in the group or to follow the entire groups of groups hierarchy. Defaults to "0". +A boolean that if set true will return the users list minus the expired groupings. + +=cut +sub getAllUsers { + my $self = shift; + my $withoutExpired = shift; + my $loopCount = shift; + my $expireTime = 0; + my $cache = WebGUI::Cache->new($self->session, $self->getId); + my $value = $cache->get; + return $value if defined $value; + $self->session->errorHandler->warn('GROUP: non-cached lookup for '. $self->name); + my @users = (); + push @users, + @{ $self->getUsers($withoutExpired) }, + @{ $self->getDatabaseUsers() }, + @{ $self->getKarmaUsers() }, + @{ $self->getScratchUsers() }, + @{ $self->getIpUsers() }, + ; + ++$loopCount; + if ($loopCount > 99) { + $self->session->errorHandler->fatal("Endless recursive loop detected while determining groups in group.\nRequested groupId: ".$self->getId); + } + my $groups = $self->getGroupsIn(); + ##Have to iterate twice due to the withoutExpired clause. + foreach my $groupId (@{ $groups }) { + my $subGroup = WebGUI::Group->new($self->session, $groupId); + push @users, @{ $subGroup->getAllUsers(1, $withoutExpired, $loopCount) }; + } + my %users = map { $_ => 1 } @users; + @users = keys %users; + $cache->set(\@users, $self->groupCacheTimeout); + return \@users; +} + + +#------------------------------------------------------------------- + +=head2 getUsers ( [ withoutExpired ] ) + +Returns an array reference containing a list of users that have been added +to this WebGUI group directly, rather than by other methods of group membership +like IP address, LDAP, dbQuery or scratchFilter. =head3 withoutExpired @@ -682,38 +743,12 @@ A boolean that if set true will return the users list minus the expired grouping =cut sub getUsers { my $self = shift; - my $recursive = shift; my $withoutExpired = shift; - my $loopCount = shift; my $expireTime = 0; - my $cache = WebGUI::Cache->new($self->session, $self->getId); - my $value = $cache->get; - return $value if defined $value; if ($withoutExpired) { $expireTime = $self->session->datetime->time(); } my @users = $self->session->db->buildArray("select userId from groupings where expireDate > ? and groupId = ?", [$expireTime, $self->getId]); - push @users, - @{ $self->getDatabaseUsers() }, - @{ $self->getKarmaUsers() }, - @{ $self->getScratchUsers() }, - @{ $self->getIpUsers() }, - ; - if ($recursive) { - ++$loopCount; - if ($loopCount > 99) { - $self->session->errorHandler->fatal("Endless recursive loop detected while determining groups in group.\nRequested groupId: ".$self->getId); - } - my $groups = $self->getGroupsIn(); - ##Have to iterate twice due to the withoutExpired clause. - foreach my $groupId (@{ $groups }) { - my $subGroup = WebGUI::Group->new($self->session, $groupId); - push @users, @{ $subGroup->getUsers(1, $withoutExpired, $loopCount) }; - } - } - my %users = map { $_ => 1 } @users; - @users = keys %users; - $cache->set(\@users, $self->groupCacheTimeout); return \@users; } @@ -748,8 +783,7 @@ sub karmaThreshold { my $self = shift; my $value = shift; if (defined $value) { - $self->session->stow->delete('isInGroup'); - $self->session->stow->delete("gotGroupsInGroup"); + $self->clearCaches; $self->set("karmaThreshold",$value); } return $self->get("karmaThreshold"); @@ -772,8 +806,7 @@ sub ipFilter { my $self = shift; my $value = shift; if (defined $value) { - $self->session->stow->delete("gotGroupsInGroup"); - $self->session->stow->delete('isInGroup'); + $self->clearCaches; $self->set("ipFilter",$value); } return $self->get("ipFilter"); @@ -933,9 +966,8 @@ sub dbQuery { my $self = shift; my $value = shift; if (defined $value) { + $self->clearCaches; $self->set("dbQuery",$value); - $self->session->stow->delete("gotGroupsInGroup"); - $self->session->stow->delete("isInGroup"); } return $self->get("dbQuery"); } @@ -956,9 +988,8 @@ sub databaseLinkId { my $self = shift; my $value = shift; if (defined $value) { + $self->clearCaches; $self->set("databaseLinkId",$value); - $self->session->stow->delete("gotGroupsInGroup"); - $self->session->stow->delete("isInGroup"); } return $self->get("databaseLinkId"); } diff --git a/lib/WebGUI/Mail/Send.pm b/lib/WebGUI/Mail/Send.pm index 66220ba36..52bbe672f 100644 --- a/lib/WebGUI/Mail/Send.pm +++ b/lib/WebGUI/Mail/Send.pm @@ -356,7 +356,7 @@ sub send { my $group = WebGUI::Group->new($self->session, $group); $self->{_message}->head->replace("bcc", undef); $self->{_message}->head->replace("cc", undef); - foreach my $userId (@{$group->getUsers(1,1)}) { + foreach my $userId (@{$group->getAllUsers(1)}) { my $user = WebGUI::User->new($self->session, $userId); if ($user->profileField("email")) { $self->{_message}->head->replace("To",$user->profileField("email")); diff --git a/lib/WebGUI/User.pm b/lib/WebGUI/User.pm index fdf2ef198..191a209c9 100644 --- a/lib/WebGUI/User.pm +++ b/lib/WebGUI/User.pm @@ -264,7 +264,7 @@ sub isInGroup { ### Lookup the actual groupings. my $group = WebGUI::Group->new($self->session,$gid); ### Check for groups of groups. - my $users = $group->getUsers(1); + my $users = $group->getAllUsers(); foreach my $user (@{$users}) { $isInGroup->{$user}{$gid} = 1; if ($uid eq $user) { diff --git a/t/Group.t b/t/Group.t index 217522ebe..d54040980 100644 --- a/t/Group.t +++ b/t/Group.t @@ -74,7 +74,7 @@ my @ipTests = ( ); -plan tests => (87 + scalar(@scratchTests) + scalar(@ipTests)); # increment this value for each test you create +plan tests => (88 + scalar(@scratchTests) + scalar(@ipTests)); # increment this value for each test you create my $session = WebGUI::Test->session; @@ -97,6 +97,7 @@ is ($g->dateCreated(), $g->lastUpdated(), 'lastUpdated = create time'); is_deeply ($g->getGroupsIn(), [3], 'Admin group added by default to this group'); is_deeply ($g->getGroupsFor(), [], 'Group not added to any other group'); is_deeply ($g->getUsers(), [], 'No users added by default'); +is_deeply ($g->getAllUsers(), [3], 'No users added by default in any method'); is ($g->autoAdd(), 0, 'auto Add is off by default'); is ($g->autoDelete(), 0, 'auto Delete is off by default'); is ($g->isEditable(), 1, 'isEditable is on by default'); @@ -262,10 +263,10 @@ cmp_bag($gA->getUsers, [@aUsers], 'users in group A'); cmp_bag($gC->getUsers, [@cUsers], 'users in group C'); cmp_bag($gZ->getUsers, [@zUsers], 'users in group Z'); -cmp_bag($gB->getUsers(1), [@bUsers, @aUsers, @cUsers, @zUsers, 3], 'users in group B, recursively'); -cmp_bag($gA->getUsers(1), [@aUsers, @zUsers, 3], 'users in group A, recursively'); -cmp_bag($gC->getUsers(1), [@cUsers, 3], 'users in group C, recursively'); -cmp_bag($gZ->getUsers(1), [@zUsers, 3], 'users in group Z, recursively'); +cmp_bag($gB->getAllUsers(), [@bUsers, @aUsers, @cUsers, @zUsers, 3], 'users in group B, recursively'); +cmp_bag($gA->getAllUsers(), [@aUsers, @zUsers, 3], 'users in group A, recursively'); +cmp_bag($gC->getAllUsers(), [@cUsers, 3], 'users in group C, recursively'); +cmp_bag($gZ->getAllUsers(), [@zUsers, 3], 'users in group Z, recursively'); ##Database based user membership in groups @@ -286,6 +287,7 @@ cmp_bag($mobUsers, [map {$_->userId} @mob], 'verify SQL table built correctly'); is( $gY->databaseLinkId, 0, "Group Y's databaseLinkId is set to WebGUI"); $gY->dbQuery(q!select userId from myUserTable!); is( $session->stow->get('isInGroup'), undef, 'setting dbQuery clears cached isInGroup'); +WebGUI::Cache->new($session, $gZ->getId)->delete(); ##Delete cached key for testing my @mobIds = map { $_->userId } @mob; @@ -298,10 +300,10 @@ cmp_bag( is( $mob[0]->isInGroup($gY->getId), 1, 'mob[0] is in group Y after setting dbQuery'); is( $mob[0]->isInGroup($gZ->getId), 1, 'mob[0] isInGroup Z'); -ok( isIn($mob[0]->userId, @{ $gY->getUsers() }), 'mob[0] in list of group Y users'); +ok( isIn($mob[0]->userId, @{ $gY->getAllUsers() }), 'mob[0] in list of group Y users'); ok( !isIn($mob[0]->userId, @{ $gZ->getUsers() }), 'mob[0] not in list of group Z users'); -ok( isIn($mob[0]->userId, @{ $gZ->getUsers(1) }), 'mob[0] in list of group Z users, recursively'); +ok( isIn($mob[0]->userId, @{ $gZ->getAllUsers() }), 'mob[0] in list of group Z users, recursively'); ##Karma tests @@ -346,7 +348,7 @@ is_deeply( ); $session->setting->set('useKarma', 1); -$session->stow->delete('isInGroup'); ##Clear cache since previous data is wrong +$gK->clearCaches; ##Clear cache since previous data is wrong is_deeply( [ (map { $_->isInGroup($gK->getId) } @chameleons) ], @@ -365,7 +367,7 @@ $session->setting->set('useKarma', $defaultKarmaSetting); ##Scratch tests my $gS = WebGUI::Group->new($session, "new"); -$gK->name('Group S'); +$gS->name('Group S'); $gC->addGroups([$gS->getId]); # B @@ -416,9 +418,9 @@ cmp_bag( ); cmp_bag( - $gS->getUsers, - [ (map { $_->{user}->userId() } grep { $_->{expect} } @scratchTests) ], - 'getUsers for group with scratch' + $gS->getAllUsers, + [ ( (map { $_->{user}->userId() } grep { $_->{expect} } @scratchTests), 3) ], + 'getAllUsers for group with scratch' ); foreach my $subSession (@sessionBank) { @@ -457,8 +459,8 @@ cmp_bag( ); cmp_bag( - $gI->getUsers, - [ (map { $_->{user}->userId() } grep { $_->{expect} } @ipTests) ], + $gI->getAllUsers, + [ ( (map { $_->{user}->userId() } grep { $_->{expect} } @ipTests), 3) ], 'getUsers for group with IP filter' );