Skip to content
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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Mar 13, 2024
Merged

[user] Add getSites functions in User class #9102

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
43e4c23
[user_accounts] Limit sites in table filters
CamilleBeau Feb 27, 2024
8a12485
Test plan update
CamilleBeau Feb 27, 2024
c0a2083
fix
CamilleBeau Feb 27, 2024
8be0013
Code cleanliness
CamilleBeau Feb 27, 2024
1fd754a
Add more user site functions & trying something with test
CamilleBeau Mar 1, 2024
c40c2e7
Get rid of user_accounts changes
CamilleBeau Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion modules/user_accounts/php/user_accounts.class.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,24 @@ class User_Accounts extends \DataFrameworkMenu
'Y' => 'Yes',
'N' => 'No',
];
// Get user
$factory = \NDB_Factory::singleton();
$user = $factory->user();

// Without user_acounts_multisite permission, only show user sites
if ($user->hasPermission('user_accounts_multisite')) {
$sites = \Utility::getSiteList(false);
} else {
$sites = [];
Copy link
Contributor

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

Copy link
Contributor Author

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

foreach (\Utility::getSiteList(false) AS $centerID => $centerName) {
if ($user->hasCenter(new \CenterID($centerID))) {
$sites[$centerID] = $centerName;
}
}
}

return [
'sites' => \Utility::getSiteList(false),
'sites' => $sites,
'projects' => \Utility::getProjectList(),
'actives' => $yesNoOptions,
'pendingApprovals' => $yesNoOptions,
Expand Down
14 changes: 13 additions & 1 deletion modules/user_accounts/php/useraccountrowprovisioner.class.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,17 @@ class UserAccountRowProvisioner extends \LORIS\Data\Provisioners\DBRowProvisione
*/
function __construct()
{
// Get user
$factory = \NDB_Factory::singleton();
$user = $factory->user();
if (!$user->hasPermission('user_accounts_multisite')) {
$siteLimitQuery = "LEFT JOIN user_psc_rel upsr2
ON (upsr2.CenterID=upsr.CenterID)
WHERE upsr2.UserID=:uid";
$params = ['uid' => $user->getId()];
} else {
$params = [];
}
Copy link
Collaborator

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?

Copy link
Contributor Author

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.
image

With the change from user_accounts.class.inc but without this change, there were empty strings in between commas:
image

parent::__construct(
"SELECT GROUP_CONCAT(DISTINCT upsr.CenterID) as centerIds,
GROUP_CONCAT(DISTINCT uprr.ProjectID) as projectIds,
Expand All @@ -29,9 +40,10 @@ class UserAccountRowProvisioner extends \LORIS\Data\Provisioners\DBRowProvisione
FROM users u
LEFT JOIN user_psc_rel upsr ON (upsr.UserID=u.ID)
LEFT JOIN user_project_rel uprr ON (uprr.UserID=u.ID)
$siteLimitQuery
GROUP BY u.ID
ORDER BY u.UserID",
[]
$params
);
}

Expand Down
4 changes: 2 additions & 2 deletions modules/user_accounts/test/TestPlan.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ When creating or editing a user: (subtest: edit_user)
13. Check that if generating a new password for a user an email is sent to that user containing the new password (requires
email server).
14. Check that when editing a user account it is not possible to set the password to its actual value (i.e. it needs to change). [Automated]
15. Verify that if the editor does not have permission 'Across all sites add and edit users' then the site drop-down list is populated with
the editor's associated sites, otherwise all sites are displayed.
15. Verify that if the editor does not have permission 'User Accounts: View/Create/Edit User Accounts - All Sites' then the multi-select
site field in both edit user page and the filters for the data table are populated with the editor's associated sites, otherwise all sites are displayed.
16. Check that if the 'Additional user information' entry is set to false in the Configuration module, fields Degree,
Academic Position, Institution, Department, Street Address, City, State/Province, Country and FAX are not shown.
17. Check that the 'Examiner At:' and 'Examiner Status' sections are available only if you have the 'Across all sites add and certify examiners'.
Expand Down
Loading