Skip to content

Commit

Permalink
[user_accounts] Use userID to update examiners table (#7462)
Browse files Browse the repository at this point in the history 
Currently when updating the examiners table in edit_user, it is assumed that a unique examiner exists based on their full name instead of their userID. This causes 2 problems:

- If an examiner with the same name as another examiner is edited, the older one will be replaced in the table and will no longer be considered an examiner. I.e. only one examiner can exist per full name.
- If a user's name is edited, then a new row will be created in the examiners table and there will be 2 rows for the same user.

This fixes this issue by assuming in the user_accounts module that examiners have a unique userID instead of assuming examiners have a unique full name in the user accounts module.

However, since examiners do not have to be users, the examiners module was edited to assume that an examiner added is not a user. The examiner module does not ask for any identifying info other than the full name, so it would be impossible to know which user is being referred to if we are accepting that different users may exist with the same name.

The resulting implementation is that only one non-user examiner can be added per full name, but any LORIS users that are added as examiners do not have to have a unique name.

The instrument library file is also edited to add an email beside the full name in the examiners list examiners with duplicate names.

Included is a cleanup script that gets rid of the duplicate userIDs in the examiner table which were added because of a name change in "edit user".

A script was not made for adding back examiners that may have been overwritten by a new user with the same name, because there is no way to determine this based on the previous implementation of examiners.
  • Loading branch information
@CamilleBeau
CamilleBeau authored Oct 6, 2021
1 parent 82b5046 commit 92970de
Show file tree
Hide file tree
Showing 8 changed files with 204 additions and 78 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ database (PR #5260)
- Fix edge-case that gave a confusing error message when changing password (PR #5956)
- Fix bug where examiner form field was incorrectly hidden (PR #6085)
- Fix special character double escaping in instruments (PR #6223)
- Fix duplicate examiners created / examiners overwritten (PR #7462)

### Modules
#### Candidate Profile
Expand Down Expand Up @@ -78,6 +79,7 @@ exception). It is recommended to run this tool for existing projects (PR #5270)
be used by projects having custom modules not in LORIS. (PR #5913)
- Duplicate filenames in the data release module will cause an error when downloading. Make sure to remove all filename duplications before upgrading to this version. (PR #6461)
- New tool for detecting and reporting the presence of double escaped special characters in the database instruments (PR #6477)
- Run `tools/single_use/Remove_duplicate_examiners.php` to remove duplicate examiners that may have been created before bugfix. Make sure to run this script _before_ running the `SQL/New_patches/2021-09-28-Unique_examiners.sql`. (PR #7462)

### Notes For Developers
- The tool `phpstan` has been added to our automated test suite. (PR #4928)
Expand Down
4 changes: 2 additions & 2 deletions SQL/0000-00-00-schema.sql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1193,8 +1193,8 @@ CREATE TABLE `examiners` (
`radiologist` tinyint(1) default 0 NOT NULL,
`userID` int(10) unsigned DEFAULT NULL,
PRIMARY KEY (`examinerID`),
UNIQUE KEY `full_name` (`full_name`),
KEY `FK_examiners_2` (`userID`),
UNIQUE KEY `unique_examiner` (`full_name`,`userID`),
UNIQUE KEY `FK_examiners_2` (`userID`),
CONSTRAINT `FK_examiners_2` FOREIGN KEY (`userID`) REFERENCES `users` (`ID`) ON DELETE SET NULL ON UPDATE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

Expand Down
11 changes: 11 additions & 0 deletions SQL/New_patches/2021-09-28-Unique_examiners.sql
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
-- Note: For projects with duplicate examiners, patch must be run AFTER running
-- tools/single_use/Remove_duplicate_examiners.php

-- Remove constraint on full name
-- Change userID to be unique
-- Make full name / userID combined unique
ALTER TABLE examiners
DROP INDEX `full_name`,
DROP INDEX `FK_examiners_2`,
ADD UNIQUE KEY `unique_examiner` (`full_name`,`userID`),
ADD UNIQUE KEY `FK_examiners_2` (`userID`);
1 change: 1 addition & 0 deletions modules/examiner/jsx/examinerIndex.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ class ExaminerIndex extends Component {
name: 'examiner',
type: 'text',
}},
{label: 'Email', show: true},
{label: 'ID', show: false},
{label: 'Site', show: true, filter: {
name: 'site',
Expand Down
31 changes: 15 additions & 16 deletions modules/examiner/php/examiner.class.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ class Examiner extends \NDB_Menu_Filter_Form
LEFT JOIN certification c ON
(c.examinerID=e.examinerID and c.pass = 'certified')
LEFT JOIN test_names tn ON (tn.ID = c.testID)
LEFT JOIN users u ON u.ID=e.userID
WHERE 1=1 ";
} else {

Expand All @@ -83,12 +84,14 @@ class Examiner extends \NDB_Menu_Filter_Form
LEFT JOIN certification c ON
(c.examinerID=e.examinerID and c.pass = 'certified')
LEFT JOIN test_names tn ON (tn.ID = c.testID)
LEFT JOIN users u ON u.ID=e.userID
WHERE FIND_IN_SET(epr.centerID, '$centerIDs')";
}

// set the class variables
$this->columns = array(
'e.full_name as Examiner',
'u.Email',
'e.examinerID as ID',
'psc.Name as Site',
'e.radiologist as Radiologist',
Expand All @@ -100,6 +103,7 @@ class Examiner extends \NDB_Menu_Filter_Form
$this->order_by = 'e.full_name';
$this->headers = array(
'Examiner',
'Email',
'ID',
'Site',
'Radiologist',
Expand Down Expand Up @@ -216,31 +220,24 @@ class Examiner extends \NDB_Menu_Filter_Form
$radiologist = 1;
}

//Check if examiner is already in the database 'examiners', if not add them
// Check if examiner is already in the database 'examiners', if not add them
// Assumption made that only non-users are added to the table here
// Otherwise, would need more information to identify which loris user
// the name refers to
$examinerID = $DB->pselectOne(
"SELECT examinerID
FROM examiners
WHERE full_name=:fullName",
WHERE full_name=:fullName
AND userID IS NULL",
array('fullName' => $fullName)
);

//Check if examiner is already a loris user, if so add foreign key
$isUser = $DB->pselectOne(
"SELECT ID FROM users WHERE Real_name=:fullName",
array('fullName' => $fullName)
);

$user_id = null;
if (!empty($isUser)) {
$user_id = $isUser;
}
if (empty($examinerID)) {
$DB->insert(
'examiners',
array(
'full_name' => $fullName,
'radiologist' => $radiologist,
'userID' => $user_id,
'radiologist' => $radiologist
)
);
} else {
Expand Down Expand Up @@ -278,7 +275,9 @@ class Examiner extends \NDB_Menu_Filter_Form
"SELECT e.examinerID
FROM examiners e
LEFT JOIN examiners_psc_rel epr ON (e.examinerID=epr.examinerID)
WHERE e.full_name=:fullName AND epr.centerID=:siteID",
WHERE e.full_name=:fullName
AND epr.centerID=:siteID
AND e.userID IS NULL",
array(
'fullName' => $fullName,
'siteID' => $siteID,
Expand All @@ -305,7 +304,7 @@ class Examiner extends \NDB_Menu_Filter_Form
$examinerID = $DB->pselectOne(
"SELECT examinerID
FROM examiners
WHERE full_name=:fullName",
WHERE full_name=:fullName AND userID IS NULL",
array('fullName' => $fullName)
);

Expand Down
110 changes: 60 additions & 50 deletions modules/user_accounts/php/edit_user.class.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,9 @@ class Edit_User extends \NDB_Form
unset($values['ProjectIDs']);
// END PROJECT UPDATE

// EXAMINER UPDATE
// SETUP EXAMINER VALUES
// The examiner will be updated after user is created
// so as to ensure an ID exists if new user
if ($editor->hasPermission('examiner_multisite')) {
// get all fields that are related to examiners
$ex_curr_sites = array();
Expand All @@ -323,13 +325,63 @@ class Edit_User extends \NDB_Form
$ex_pending = $v;
}
}
foreach ($ex_curr_sites as $k => $v) {
unset($values[$k]);
}
}
unset($values['examiner_radiologist']);
unset($values['examiner_pending']);
// END SETUP EXAMINER VALUES

// make the set
$set = array();
foreach ($values as $key => $value) {
// Password updates are handled separately
if (!empty($value) && $key != 'Password_hash') {
$set[$key] = $value;
} else {
$set[$key] = null;
}
}
// update the user
if ($this->isCreatingNewUser()) {
// insert a new user
\User::insert($set);
$user = \User::factory($set['UserID']);
$uid = $user->getData('ID');
foreach ($us_curr_sites as $site) {
$DB->insert(
'user_psc_rel',
array(
"UserID" => $uid,
"CenterID" => $site,
)
);
}
foreach ($us_curr_projects as $project) {
$DB->insert(
'user_project_rel',
array(
"UserID" => $uid,
"ProjectID" => $project,
)
);
}
} else {
// update the user
$user = \User::factory($this->identifier);
$user->update($set);
}

// START EXAMINER UPDATE
if ($editor->hasPermission('examiner_multisite')) {

$examinerID = $DB->pselect(
"SELECT e.examinerID
FROM examiners e
WHERE e.full_name=:fn",
WHERE e.userID=:uid",
array(
"fn" => $values['Real_name'],
"uid" => $uid,
)
);

Expand All @@ -342,6 +394,7 @@ class Edit_User extends \NDB_Form
// If examiner not in table and radiologist, pending and current
// sites fields set add the examiner to the examiner table
$ex_radiologist = $ex_radiologist === 'Y' ? 1 : 0;

$DB->insert(
'examiners',
array(
Expand All @@ -353,8 +406,8 @@ class Edit_User extends \NDB_Form
$examinerID = $DB->pselectOne(
"SELECT examinerID
FROM examiners
WHERE full_name=:fullName",
array('fullName' => $values['Real_name'])
WHERE userID=:uid",
array('uid' => $uid)
);
} elseif (!empty($examinerID)
&& ((!empty($ex_radiologist)
Expand All @@ -370,10 +423,10 @@ class Edit_User extends \NDB_Form
'examiners',
array(
'radiologist' => $ex_radiologist,
'userID' => $uid,
'full_name' => $values['Real_name']
),
array(
"full_name" => $values['Real_name'],
'userID' => $uid
)
);
$examinerID = $examinerID[0]['examinerID'];
Expand Down Expand Up @@ -428,7 +481,6 @@ class Edit_User extends \NDB_Form
)
);
}
unset($values[$k]);
}

//de-activate examiner if sites where no longer checked
Expand All @@ -444,50 +496,8 @@ class Edit_User extends \NDB_Form
);
}
}
unset($values['examiner_radiologist']);
unset($values['examiner_pending']);
//END EXAMINER UPDATE

// make the set
$set = array();
foreach ($values as $key => $value) {
// Password updates are handled separately
if (!empty($value) && $key != 'Password_hash') {
$set[$key] = $value;
} else {
$set[$key] = null;
}
}
// update the user
if ($this->isCreatingNewUser()) {
// insert a new user
\User::insert($set);
$user = \User::factory($set['UserID']);
$uid = $user->getData('ID');
foreach ($us_curr_sites as $site) {
$DB->insert(
'user_psc_rel',
array(
"UserID" => $uid,
"CenterID" => $site,
)
);
}
foreach ($us_curr_projects as $project) {
$DB->insert(
'user_project_rel',
array(
"UserID" => $uid,
"ProjectID" => $project,
)
);
}
} else {
// update the user
$user = \User::factory($this->identifier);
$user->update($set);
}

// Now set the password. Note that this field is named incorrectly
// and represents a plaintext password, not a hash.
if (isset($values['Password_hash'])) {
Expand Down
38 changes: 28 additions & 10 deletions php/libraries/NDB_BVL_Instrument.class.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1053,13 +1053,14 @@ abstract class NDB_BVL_Instrument extends NDB_Page
"SELECT ID FROM test_names WHERE Test_name=:tst_name",
array('tst_name' => $this->testName)
);
$results = $db->pselect(
"SELECT c.examinerID, e.full_name
$results = $db->pselectWithIndexKey(
"SELECT c.examinerID, e.full_name, u.Email
FROM certification c
JOIN examiners e
ON (c.examinerID = e.examinerID)
JOIN examiners_psc_rel epr
ON (epr.examinerID=e.examinerID)
LEFT JOIN users u ON u.ID=e.userID
WHERE c.testID =:tid
AND c.pass =:cert_id
AND epr.centerID =:cid
Expand All @@ -1068,24 +1069,41 @@ abstract class NDB_BVL_Instrument extends NDB_Page
'tid' => $test_id,
'cert_id' => 'certified',
'cid' => $centerID,
)
),
'examinerID'
);
} else {
$results = $db->pselect(
"SELECT e.examinerID, e.full_name
$results = $db->pselectWithIndexKey(
"SELECT e.examinerID, e.full_name, u.Email
FROM examiners e
JOIN examiners_psc_rel epr
ON (epr.examinerID=e.examinerID)
LEFT JOIN users u ON u.ID=e.userID
WHERE epr.centerID=:centID
ORDER BY full_name",
array('centID' => $centerID)
array('centID' => $centerID),
'examinerID'
);
}

$examiners = array('' => '');
$examiners = [];
if (is_array($results) && !empty($results)) {
foreach ($results AS $row) {
$examiners[$row['examinerID']] = $row['full_name'];
foreach ($results AS $eid => $row) {
$name = $row['full_name'];
if (in_array($name, $examiners)) {
// If name already in examiners, set first case of name
// with email (if exists)
$duplicateID = array_search($name, $examiners);
$duplicateEmail = $results[$duplicateID]['Email'];
$emailString = empty($duplicateEmail) ? "" :
" ({$duplicateEmail})";
$examiners[$duplicateID] = $name . $emailString;
// Set new case of name with email as well
$emailString = empty($row['Email']) ? "" :
" ({$row['Email']})";
$examiners[$eid] = $name . $emailString;
} else {
$examiners[$eid] = $name;
}
}
}
return $examiners;
Expand Down
Loading

0 comments on commit 92970de

Please sign in to comment.