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

LIMS-1238: Speed up query to list persons on visit #886

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

LIMS-1238: Speed up query to list persons on visit #886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
aab52aa
LIMS-1238: Speed up query to list persons on visit
Dec 20, 2024
6b281f3
LIMS-1238: Fix some tests
Dec 20, 2024
6431ffd
Fix some more tests
Dec 20, 2024
8dd089c
LIMS-1238: Fix some more tests
Dec 20, 2024
f575763
LIMS-1238: Fix some more tests
Dec 20, 2024
c69a451
LIMS-1238: Fix swapping of personId and currentUserId
Dec 20, 2024
02ee1c8
LIMS-1238: Fix more tests
Dec 22, 2024
886d752
LIMS-1238: Fix more tests
Dec 22, 2024
46f1021
LIMS-1238: Fix more tests
Dec 22, 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
5 changes: 1 addition & 4 deletions api/src/Controllers/UserController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ class UserController extends Page
'pjid' => '\d+',
'peid' => '\d+',
'uid' => '\d+',
'sid' => '\d+',
'visit' => '\w+\d+-\d+',
'location' => '(\w|-|\/)+',
'all' => '\d',
Expand Down Expand Up @@ -184,7 +183,6 @@ function _get_users()
$this->user->hasPermission('manage_users'),
$this->user->personId,
$this->argOrEmptyString('gid'),
$this->argOrEmptyString('sid'),
$this->argOrEmptyString('pjid'),
$this->argOrEmptyString('visit'),
$this->def_arg('per_page', 15),
Expand Down Expand Up @@ -214,7 +212,6 @@ function _get_users()
$this->user->hasPermission('manage_users'),
$this->user->personId,
$this->argOrEmptyString('gid'),
$this->argOrEmptyString('sid'),
$this->argOrEmptyString('pjid'),
$this->argOrEmptyString('visit'),
$this->def_arg('per_page', 15),
Expand Down Expand Up @@ -362,4 +359,4 @@ function _update_permission()
);
$this->_output(array('TYPE' => $this->arg('TYPE'), 'DESCRIPTION' => $desc));
}
}
}
56 changes: 20 additions & 36 deletions api/src/Model/Services/UserData.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,8 @@ function removeGroupPermission($userGroupId, $permisionId)
$this->db->pq("DELETE FROM usergroup_has_permission WHERE usergroupid=:1 and permissionid=:2", array($userGroupId, $permisionId));
}

private function addPersonOrProposalSearch($proposalid, $personId, &$args): string
{
$whereClause = ' AND (prhp.proposalid=:' . (sizeof($args) + 1) . ' OR lc.proposalid=:' . (sizeof($args) + 2) . ' OR p.personid=:' . (sizeof($args) + 3) . ')';
array_push($args, $proposalid);
array_push($args, $proposalid);
array_push($args, $personId);
return $whereClause;
}

function getUsers($getCount, $isStaffMember, $stringMatch, $page, $sortBy = null, $pid=null, $proposalid = null, $personId = null, $isManager = false, $currentUserId = null, $gid = null, $sid = null, $pjid = null, $visitName = null, $perPage = 15, $isAscending = true, $isAll = false, $onlyLogins = false)
function getUsers($getCount, $isStaffMember, $stringMatch, $page, $sortBy = null, $pid=null, $proposalid = null, $personId = null, $isManager = false, $currentUserId = null, $gid = null, $pjid = null, $visitName = null, $perPage = 15, $isAscending = true, $isAll = false, $onlyLogins = false)
{
$args = array();

Expand All @@ -94,9 +86,9 @@ function getUsers($getCount, $isStaffMember, $stringMatch, $page, $sortBy = null
else
$where = 'p.login IS NOT NULL';

if ($personId == "" && $stringMatch == "" && $gid == "" && $sid == "" && $visitName == "" && $pjid == "")
if ($personId == "" && $stringMatch == "" && $gid == "" && $visitName == "" && $pjid == "")
{
//secured by making sure the user has access toproposalid
//secured by making sure the user has access to proposalid
return $this->getUsersForProposal($where, $getCount, $page, $sortBy, $proposalid, $currentUserId, $perPage, $isAscending, $start, $end);
}

Expand All @@ -105,12 +97,15 @@ function getUsers($getCount, $isStaffMember, $stringMatch, $page, $sortBy = null
$group = 'GROUP BY p.personid';

// This blocks means that non-staff can only see users on their proposal, except when they looking at a visit
// (i.e. the proposal that was checkin page.php is added to the where clause)
if ((($personId && !$isManager) // if you not a manager: you looking for a person
|| (!$isStaffMember && !$visitName)) // if you are not a staff member and not loking at a specific visit
// (i.e. the proposal that was checking is added to the where clause)
if ((($personId && !$isManager) // if you're not a manager: you're looking for a person
|| (!$isStaffMember && !$visitName)) // if you are not a staff member and not looking at a specific visit
|| $pid) // if you are looking for user based on a proposal, but this
{
$where .= $this->addPersonOrProposalSearch($proposalid, $currentUserId, $args);
$where .= ' AND (prhp.proposalid=:' . (sizeof($args) + 1) . ' OR lc.proposalid=:' . (sizeof($args) + 2) . ' OR p.personid=:' . (sizeof($args) + 3) . ')';
array_push($args, $proposalid, $proposalid, $currentUserId);
$join .= 'LEFT OUTER JOIN proposalhasperson prhp ON prhp.personid = p.personid
LEFT OUTER JOIN labcontact lc ON lc.personid = p.personid ';
}

if ($personId)
Expand All @@ -122,45 +117,38 @@ function getUsers($getCount, $isStaffMember, $stringMatch, $page, $sortBy = null
if ($stringMatch)
{
$st = sizeof($args) + 1;
$where .= " AND (lower(p.familyname) LIKE lower(CONCAT(CONCAT('%',:" . $st .
"),'%')) OR lower(p.givenname) LIKE lower(CONCAT(CONCAT('%',:" .
($st + 1) . "),'%')) OR lower(p.login) LIKE lower(CONCAT(CONCAT('%',:" . ($st + 2) . "),'%')))";
$where .= " AND (p.familyname LIKE CONCAT('%',:" . $st . ",'%') OR p.givenname LIKE CONCAT('%',:" . ($st + 1) . ",'%') OR p.login LIKE CONCAT('%',:" . ($st + 2) . ",'%'))";
for ($i = 0; $i < 3; $i++)
{
array_push($args, $stringMatch);
}
}

// TODO: the following statements were not previously coded as mutually exclusive, however logically they must be as no attempt was made to extend the JOIN statement
// - it may be worth reviewing this, however.
if ($gid)
{
$join = 'INNER JOIN usergroup_has_person uhp ON uhp.personid = p.personid';
$join .= 'INNER JOIN usergroup_has_person uhp ON uhp.personid = p.personid';
$where .= ' AND uhp.usergroupid=:' . (sizeof($args) + 1);
array_push($args, $gid);
}
else if ($sid)
{
// TODO: this is an invalid DB table - does this need to be removed entirely?
$join = 'INNER JOIN blsession_has_person shp ON shp.personid = p.personid';
$where .= ' AND shp.sessionid=:' . (sizeof($args) + 1);
array_push($args, $sid);
}
else if ($visitName)
{
$pattern = '/([A-z]+)(\d+)-(\d+)/';
preg_match($pattern, $visitName, $matches);
if (!sizeof($matches))
$this->_error('No such visit');
$extc = "count(ses.sessionid) as visits, TO_CHAR(max(ses.startdate), 'DD-MM-YYYY') as last, shp.remote, shp.role,";
$join = 'INNER JOIN session_has_person shp ON shp.personid = p.personid
$join .= 'INNER JOIN session_has_person shp ON shp.personid = p.personid
INNER JOIN blsession s ON shp.sessionid = s.sessionid
INNER JOIN proposal pr ON pr.proposalid = s.proposalid
LEFT OUTER JOIN session_has_person shp2 ON p.personid = shp2.personid
LEFT OUTER JOIN blsession ses ON ses.sessionid = shp2.sessionid AND ses.startdate < s.startdate';
$where .= " AND shp.remote IS NOT NULL AND CONCAT(pr.proposalcode, pr.proposalnumber, '-', s.visit_number) LIKE :" . (sizeof($args) + 1);
$where .= " AND shp.remote IS NOT NULL AND pr.proposalcode = :" . (sizeof($args) + 1) . " AND pr.proposalnumber = :" . (sizeof($args) + 2) . " AND s.visit_number = :" . (sizeof($args) + 3);
$group = 'GROUP BY p.personid, p.givenname, p.familyname, p.login';
array_push($args, $visitName);
array_push($args, $matches[1], $matches[2], $matches[3]);
}
else if ($pjid)
{
$join = 'INNER JOIN project_has_person php ON p.personid = php.personid';
$join .= 'INNER JOIN project_has_person php ON p.personid = php.personid';
$where .= ' AND php.projectid=:' . (sizeof($args) + 1);
$extc = "CONCAT(p.personid, '-', php.projectid) as ppid,";
array_push($args, $pjid);
Expand All @@ -170,8 +158,6 @@ function getUsers($getCount, $isStaffMember, $stringMatch, $page, $sortBy = null
{
$tot = $this->db->pq("SELECT count(distinct p.personid) as tot
FROM person p
LEFT OUTER JOIN proposalhasperson prhp ON prhp.personid = p.personid
LEFT OUTER JOIN labcontact lc ON lc.personid = p.personid
$join
WHERE $where", $args);

Expand All @@ -194,8 +180,6 @@ function getUsers($getCount, $isStaffMember, $stringMatch, $page, $sortBy = null

$rows = $this->db->paginate("SELECT $extc p.personid, p.givenname, p.familyname, CONCAT(p.givenname, ' ', p.familyname) as fullname, p.login, p.emailaddress, p.phonenumber, l.name as labname, l.address, l.city, l.postcode, l.country
FROM person p
LEFT OUTER JOIN proposalhasperson prhp ON prhp.personid = p.personid
LEFT OUTER JOIN labcontact lc ON lc.personid = p.personid
LEFT OUTER JOIN laboratory l ON l.laboratoryid = p.laboratoryid
$join
WHERE $where
Expand Down
6 changes: 3 additions & 3 deletions api/tests/Controllers/UserControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ public function testRemoveGroupPermissionWithNoGidSpecifiedReturnsError(): void
public function testGetUsersWithInvalidPersonIdReturnsNoData(): void
{
$this->userController->args['PERSONID'] = 88;
$this->dataLayerStub->expects($this->exactly(1))->method('getUsers')->with(false, false, '', '', '', '', '', 88, false, 231312, '', '', '', '', 15, true)->willReturn(array());
$this->dataLayerStub->expects($this->exactly(1))->method('getUsers')->with(false, false, '', '', '', '', '', 88, false, 231312, '', '', '', 15, true)->willReturn(array());
$this->slimStub->shouldReceive('halt')->times(1)->with(400, '{"status":400,"message":"No such user"}')->andThrow(new \Exception);
$this->expectException(\Exception::class);

Expand All @@ -255,7 +255,7 @@ public function testGetUsersWithPersonIdReturnsNoTotalData(): void
{
$response = $this->setUpCommonResponse();
$this->userController->args['PERSONID'] = 88;
$this->dataLayerStub->expects($this->exactly(1))->method('getUsers')->with(false, false, '', '', '', '', '', 88, false, 231312, '', '', '', '', 15, true)->willReturn(array(9));
$this->dataLayerStub->expects($this->exactly(1))->method('getUsers')->with(false, false, '', '', '', '', '', 88, false, 231312, '', '', '', 15, true)->willReturn(array(9));

$this->userController->_get_users();
$this->assertEquals(9, $response->getBody());
Expand Down Expand Up @@ -563,4 +563,4 @@ public function testGetPermissionsWithPidSpecifiedReturnsData(): void
$this->userController->_get_permissions();
$this->assertEquals(61, $response->getBody());
}
}
}
Loading
Loading