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 accounts] Fixing users being always inserted into examiner table upon save #3829

Merged
merged 11 commits into from
Aug 6, 2018
Merged

[user accounts] Fixing users being always inserted into examiner table upon save #3829

Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b8a37e4
[user accounts] Fixing examiners being always inserted into Examiner …
cmadjar Jul 16, 2018
c2e0bb1
removing forgotten print_r statements
cmadjar Jul 16, 2018
e72ee5a
Fixing Travis errors
cmadjar Jul 16, 2018
340e28f
Travis complaining again
cmadjar Jul 16, 2018
4f78836
Muhammad's feedback
cmadjar Jul 16, 2018
1d8bf97
Zain's feedback
cmadjar Jul 16, 2018
970dc3c
corrected misplaced closing curly bracket
cmadjar Jul 17, 2018
0c661ea
phpcs
cmadjar Jul 17, 2018
6382ad5
more phpcs
cmadjar Jul 17, 2018
797b3d7
Rida's feedback on the logic
cmadjar Jul 30, 2018
e4cd8fb
phpcs
ridz1208 Aug 4, 2018
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
53 changes: 34 additions & 19 deletions modules/user_accounts/php/edit_user.class.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ class Edit_User extends \NDB_Form
}
unset($values['CenterIDs']);
// END multi-site UPDATE

// EXAMINER UPDATE
if ($editor->hasPermission('examiner_multisite')) {
// get all fields that are related to examiners
Expand All @@ -278,9 +279,6 @@ class Edit_User extends \NDB_Form
}
}

//modify examiner radiologist to be in the binary format 0-1
$ex_radiologist = $ex_radiologist === 'Y' ? 1 : 0;

$examinerID = $DB->pselect(
"SELECT e.examinerID
FROM examiners e
Expand All @@ -291,11 +289,14 @@ class Edit_User extends \NDB_Form
);

// START EXAMINER UPDATE
// If examiner not in table add him,
// otherwise update the radiologist field and get the current sites
$values = \Utility::nullifyEmpty($values, 'examiner_radiologist');
if (empty($examinerID)) {

if (!empty($ex_radiologist)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment explaining the condition here is a good idea

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

&& !empty($ex_pending)
&& !empty($ex_curr_sites)
&& empty($examinerID)
) {
// 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 @@ -304,14 +305,30 @@ class Edit_User extends \NDB_Form
'userID' => $uid,
)
);
$examinerID = $examinerID = $DB->pselectOne(
$examinerID = $DB->pselectOne(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why we need to extract the new examinerID here, it doesn't seem to be used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmadjar I was wrong about this, it is used 👍

"SELECT examinerID
FROM examiners
WHERE full_name=:fullName",
FROM examiners
WHERE full_name=:fullName",
array('fullName' => $values['Real_name'])
);
} else {

// } elseif (!empty($examinerID)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented code should not be submitted

// && ((!empty($ex_radiologist)
// && !empty($ex_pending)
// && !empty($ex_curr_sites))
// || (empty($ex_radiologist)
// && empty($ex_pending)
// && empty($ex_curr_sites)))
// ) {
} elseif (!empty($examinerID)
&& ((!empty($ex_radiologist)
&& !empty($ex_pending)
&& !empty($ex_curr_sites))
|| empty($ex_curr_sites))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmadjar, addressing your most recent change, shouldn't the else if statement be:

else if (!empty($examinerID)
		 && !empty($ex_pending) 
		 && !empty($ex_radiologist)) 
		 && (empty($ex_curr_sites) || !empty($ex_curr_sites))
		) {

		 	// The case where the examiner already exists 
		 	// and has either all sites being deleted or the sites are being changed
		  }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @um4r12 ,

Thanks for the sugesstion. Actually, I tried that and it does not work as it is impossible to be in the following condition !empty($examinerID) && !empty($ex_pending) && !empty($ex_radiologist)) && empty($ex_curr_sites) in the frontend. To remove all the sites from an examiner, you have to unselect radiologist and ex_pending. Currently, only lets you be in either of the following conditions:

  • !empty($examinerID) && !empty($ex_pending) && !empty($ex_radiologist)) && !empty($ex_curr_sites)
  • !empty($examinerID) && empty($ex_pending) && empty($ex_radiologist)) && empty($ex_curr_sites) which I simplified in my last commit by !empty($examinerID) && empty($ex_curr_sites)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmadjar is correct, its not ideal but is acceptable in the scope of this bugfix. a revap of the logic is def needed to prevent unsetting of all fields and to sync the validation checks to the processing.

) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fault in logic above, if examiner ID exists, there is no way the examiner pending and approval can be reset to null (the columns in SQL are not nullable, examiners can not be deleted and there is no code below to handle that case) thus the last segment of this statemnt should be removed.

|| (empty($ex_radiologist)
                && empty($ex_pending)
                && empty($ex_curr_sites))

The possible cases are:

  • !empty($examinerID) && !empty($ex_pending) && !empty($ex_radiologist)) &&
    • empty($ex_curr_sites) -> examiner existed but we just removed all its sites
    • ||
    • !empty($ex_curr_sites) -> examiner existed and we changed its sites
  • empty($examinerID) && !empty($ex_pending) && !empty($ex_radiologist)) &&
    • !empty($ex_curr_sites) -> examiner is new and we added its sites

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ridz1208 I changed the logic as you suggested. FYI, the following case is impossible: !empty($examinerID) && !empty($ex_pending) && !empty($ex_radiologist) && empty($ex_curr_sites) due to data entry checks. If there are no sites for the examiner, then Radiologist and Pending fields must be emptied out in order for the form to save. That is why I originally added the second "or" with all empty fields.

|| ((empty($ex_radiologist)
                && empty($ex_pending)
                && empty($ex_curr_sites)))

But indeed, in case the requirements for data entry change in the form, probably best to simplify the 'or' part of the logic to || empty($ex_curr_sites)

Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are correct but that means that there is a bigger fautl in logic in the field validation in the front end. Thats def not within the scope of this PR since its not a bugfix but rather a minor revamp.

This should do for the moment. I will try to test as soon as I can.

// If examiner already exists in the examiner table AND
// radiologist, pending and current sites fields all set or
// unset update the examiner table with new values
$ex_radiologist = $ex_radiologist === 'Y' ? 1 : 0;
$DB->update(
'examiners',
array(
Expand All @@ -327,8 +344,8 @@ class Edit_User extends \NDB_Form
//get existing sites for examiner
$prev_sites = $DB->pselect(
"SELECT centerID
FROM examiners_psc_rel epr
WHERE examinerID=:eid",
FROM examiners_psc_rel epr
WHERE examinerID=:eid",
array("eid" => $examinerID)
);

Expand All @@ -337,14 +354,13 @@ class Edit_User extends \NDB_Form
array_push($ex_prev_sites, $center['centerID']);
}
}

foreach ($ex_curr_sites as $k => $v) {

//Check if examiner already in db for site
$result = $DB->pselectRow(
"SELECT epr.centerID
FROM examiners_psc_rel epr
WHERE epr.examinerID=:eid AND epr.centerID=:cid",
FROM examiners_psc_rel epr
WHERE epr.examinerID=:eid AND epr.centerID=:cid",
array(
"eid" => $examinerID,
"cid" => $v,
Expand Down Expand Up @@ -393,7 +409,6 @@ class Edit_User extends \NDB_Form
}
unset($values['examiner_radiologist']);
unset($values['examiner_pending']);

//END EXAMINER UPDATE

// make the set
Expand Down