-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[user] Add getSites functions in User class #9102
[user] Add getSites functions in User class #9102
Conversation
if ($user->hasPermission('user_accounts_multisite')) { | ||
$sites = \Utility::getSiteList(false); | ||
} else { | ||
$sites = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you should be using $user->getStudySites()
instead of reimplementing the logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeffersoncasimir getStudySites only includes study sites, but I believe in this case we want all sites that the user has access to
@jeffersoncasimir @driusan (and @ridz1208 because you seem interested in this development) I added 2 functions to the User class:
I'm still trying to set up Docker on my VM to be able to debug the test suites (it was failing yesterday), but I'm trying something out in my last commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been tested and works as expected. Great work!
} else { | ||
$siteLimitQuery = ''; | ||
$params = []; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this is necessary.
user_accounts.class.inc already has:
/**
* Tells the base class that this page's provisioner can support the
* HasAnyPermissionOrUserSiteMatch filter.
*
* @return ?array of permissions or null
*/
public function allSitePermissionNames() : ?array
{
return ['user_accounts_multisite'];
}
so shouldn't the HasAnyPermissionOrUserSiteMatch filter take care of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allSitePermissionNames function takes care of filtering out users who don't belong to one of the user's sites, but this code makes it so that only the user's sites are shown in the "Site" column of the table.
With the change from user_accounts.class.inc but without this change, there were empty strings in between commas:
@jeffersoncasimir @driusan I removed all of the user_accounts code, changed the name, and the description. The PR now only includes the 2 functions that can be used elsewhere in the code. |
Brief summary of changes
This PR adds 2 functions to the user class. One returns an array of Site objects that the user has access to. The other returns an associative array with the format CenterID => SiteName for all of the sites that the user has access to.
Testing instructions (if applicable)
Link(s) to related issue(s)