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

cmadjar · 2018-07-16T17:23:03Z

This PR fixes the issue that whenever a user has the examiner_multisite permission and creates a new user, the new user will always be added to the examiner table even if no examiner options were set.

This bug was found by @ZainVirani so assigning him to be a reviewer to this new PR.

…table

johnsaigle · 2018-07-16T17:58:08Z

@cmadjar This requires phpcs

xlecours · 2018-07-16T18:06:35Z

modules/user_accounts/php/edit_user.class.inc

+                         ))
+            ) {
+                print_r('other situation');
+                print_r($ex_radiologist);


Was that for debuging?

xlecours · 2018-07-16T18:16:05Z

modules/user_accounts/php/edit_user.class.inc

+                          && empty($ex_curr_sites)
+                         ))
+            ) {
+                print_r('other situation');


Was that for debuging?

oops. Yes. I will remove it. Sorry :S

xlecours · 2018-07-16T18:16:12Z

modules/user_accounts/php/edit_user.class.inc

+                && !empty($ex_curr_sites)
+                && empty($examinerID)
+            ) {
+                print_r("radio, pending, curr site not empty + examiner empty");


Was that for debuging?

cmadjar · 2018-07-16T18:37:09Z

@xlecours removed the print_r statements.
@johnsaigle fixed the travis issues hopefully.

um4r12 · 2018-07-16T18:26:01Z

modules/user_accounts/php/edit_user.class.inc

-                     'userID'      => $uid,
+                        'full_name'   => $values['Real_name'],
+                        'radiologist' => $ex_radiologist,
+                        'userID'      => $uid,
                    )
                );
                $examinerID = $examinerID = $DB->pselectOne(


Is this supposed to be just $examinerID = $DB->pselectOne(...), instead of $examinerID = $examinerID = $DB->pselectOne(...)

jesscall · 2018-07-16T19:29:24Z

Created a user under User Accounts with no Examiner affiliations both before and after checking your branch out and it works as intended!

ZainVirani

I reviewed code and tested to verify my comments, but I really think I would like another set of eyes on this.

ZainVirani · 2018-07-16T20:05:48Z

modules/user_accounts/php/edit_user.class.inc

@@ -293,9 +290,16 @@ class Edit_User extends \NDB_Form
            // START EXAMINER UPDATE
            // If examiner not in table add him,


unrelated but can we change this to "If examiner not in the table add them,"

ZainVirani · 2018-07-16T20:11:34Z

modules/user_accounts/php/edit_user.class.inc

-            $values = \Utility::nullifyEmpty($values, 'examiner_radiologist');
-            if (empty($examinerID)) {
-
+            if (!empty($ex_radiologist)


A comment explaining the condition here is a good idea

ZainVirani · 2018-07-16T20:11:49Z

modules/user_accounts/php/edit_user.class.inc

                    array('fullName' => $values['Real_name'])
                );
-            } else {
-
+            } elseif (!empty($examinerID)


A comment explaining the condition logic here would be good

ZainVirani · 2018-07-16T20:15:04Z

modules/user_accounts/php/edit_user.class.inc

+                && empty($examinerID)
+            ) {
+                $ex_radiologist = $ex_radiologist === 'Y' ? 1 : 0;
+                $values         = \Utility::nullifyEmpty(


This function call is useless here, we assigned $values[examiner_radiologist] to $ex_radiologist earlier and then checked for !empty($ex_radiologist). The call should be made outside of this if statement.
I also don't see the point of making this call at all, the value is not used again and it's unset after this code.

good point. Now that I think of it, this is probably not useful. Will remove it.

ZainVirani · 2018-07-16T20:17:21Z

modules/user_accounts/php/edit_user.class.inc

+                && empty($ex_curr_sites)))
+            ) {
+                $ex_radiologist = $ex_radiologist === 'Y' ? 1 : 0;
+                $values         = \Utility::nullifyEmpty(


See comment on line 299.

ZainVirani · 2018-07-16T20:19:31Z

modules/user_accounts/php/edit_user.class.inc

@@ -304,14 +308,25 @@ class Edit_User extends \NDB_Form
                     'userID'      => $uid,
                    )
                );
-                $examinerID = $examinerID = $DB->pselectOne(
+                $examinerID = $DB->pselectOne(


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

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

ZainVirani · 2018-07-16T20:29:03Z

modules/user_accounts/php/edit_user.class.inc

-                     "cid" => $v,
-                    )
-                );
+                foreach ($ex_curr_sites as $k => $v) {


you're missing a } before this line (and subsequently have an extra at the end), otherwise new examiners won't have any site affiliation. Previously this code was run regardless of the empty status of $examinerID, and now it is only run when it is set. Setting the examiner status of an existing user for the first time breaks the page as a result.

This one is the one that sketches me out the most. I know that it does break when this condition is hit, but I didn't check if moving the } would fix it. Give it a shot and lmk @cmadjar.

Nice catch!! Your suggestion worked.

cmadjar

@ZainVirani Thanks for the comments! They should all have been taken care of in the last commit. Please re-review at your convenience.
Thanks again!

cmadjar · 2018-07-16T20:50:32Z

modules/user_accounts/php/edit_user.class.inc

@@ -293,9 +290,16 @@ class Edit_User extends \NDB_Form
            // START EXAMINER UPDATE
            // If examiner not in table add him,


cmadjar · 2018-07-16T20:54:51Z

modules/user_accounts/php/edit_user.class.inc

-            $values = \Utility::nullifyEmpty($values, 'examiner_radiologist');
-            if (empty($examinerID)) {
-
+            if (!empty($ex_radiologist)


cmadjar · 2018-07-16T20:56:26Z

modules/user_accounts/php/edit_user.class.inc

+                && empty($examinerID)
+            ) {
+                $ex_radiologist = $ex_radiologist === 'Y' ? 1 : 0;
+                $values         = \Utility::nullifyEmpty(


good point. Now that I think of it, this is probably not useful. Will remove it.

cmadjar · 2018-07-16T21:01:51Z

modules/user_accounts/php/edit_user.class.inc

                    array('fullName' => $values['Real_name'])
                );
-            } else {
-
+            } elseif (!empty($examinerID)


cmadjar · 2018-07-16T21:02:01Z

modules/user_accounts/php/edit_user.class.inc

+                && empty($ex_curr_sites)))
+            ) {
+                $ex_radiologist = $ex_radiologist === 'Y' ? 1 : 0;
+                $values         = \Utility::nullifyEmpty(


cmadjar · 2018-07-16T22:21:31Z

modules/user_accounts/php/edit_user.class.inc

-                     "cid" => $v,
-                    )
-                );
+                foreach ($ex_curr_sites as $k => $v) {


Nice catch!! Your suggestion worked.

change made

Accidental approval

ZainVirani

One small change still required.

ZainVirani · 2018-07-17T14:22:42Z

modules/user_accounts/php/edit_user.class.inc

-                foreach ($prev_sites as $row => $center) {
-                    array_push($ex_prev_sites, $center['centerID']);
-                }
+            }


Sorry, I think this should be on line 350 instead of here. The original file had the next foreach loop inside of the else statement.

ZainVirani · 2018-07-17T14:23:46Z

modules/user_accounts/php/edit_user.class.inc

@@ -304,14 +308,25 @@ class Edit_User extends \NDB_Form
                     'userID'      => $uid,
                    )
                );
-                $examinerID = $examinerID = $DB->pselectOne(
+                $examinerID = $DB->pselectOne(


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

cmadjar · 2018-07-17T14:51:04Z

@ZainVirani Good catch! I corrected the misplaced closing curly bracket. Please re-review at your convenience. Thanks a lot!

ridz1208 · 2018-07-26T01:07:31Z

@ZainVirani please re-review

@jesscall please re-test on ccna DB (make sure that when examiner sites are checked the examiner entry in the table contains the userID reference, make sure the removal of examiners also works)

zaliqarosli

it kinda bothers me that an if/elseif statement isn't followed by an else :p but other than that, it looks good!

ridz1208

fault in logic

ridz1208 · 2018-07-27T21:34:53Z

modules/user_accounts/php/edit_user.class.inc

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


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

@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!

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.

ridz1208 · 2018-07-27T21:36:07Z

@ZainVirani @zaliqarosli wouldn't mind a double check on the logic, look at what the code is handling below to know what the if statement has to check for

um4r12 · 2018-07-30T19:49:16Z

modules/user_accounts/php/edit_user.class.inc

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


@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 }

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)

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

cmadjar · 2018-08-01T19:47:24Z

@ridz1208 @um4r12 Please re-review. Thanks!

stale

ridz1208 · 2018-08-04T05:45:17Z

modules/user_accounts/php/edit_user.class.inc

                    array('fullName' => $values['Real_name'])
                );
-            } else {
-
+//            } elseif (!empty($examinerID)


commented code should not be submitted

ridz1208 · 2018-08-04T05:52:13Z

@cmadjar I hope you dont mind, I pushed directly to your branch to fix phpcs

[user accounts] Fixing examiners being always inserted into Examiner …

b8a37e4

…table

cmadjar added Category: Bug PR or issue that aims to report or fix a bug [branch] major labels Jul 16, 2018

cmadjar added this to the 20.0.0 milestone Jul 16, 2018

cmadjar requested a review from ZainVirani July 16, 2018 17:23

xlecours previously requested changes Jul 16, 2018

View reviewed changes

cmadjar added 2 commits July 16, 2018 14:28

removing forgotten print_r statements

c2e0bb1

Fixing Travis errors

e72ee5a

um4r12 suggested changes Jul 16, 2018

View reviewed changes

cmadjar added 2 commits July 16, 2018 14:41

Travis complaining again

340e28f

Muhammad's feedback

4f78836

jesscall added the Passed manual tests PR has been successfully tested by at least one peer label Jul 16, 2018

um4r12 previously approved these changes Jul 16, 2018

View reviewed changes

ZainVirani suggested changes Jul 16, 2018

View reviewed changes

ZainVirani removed the Passed manual tests PR has been successfully tested by at least one peer label Jul 16, 2018

Zain's feedback

1d8bf97

cmadjar commented Jul 16, 2018

View reviewed changes

ZainVirani suggested changes Jul 17, 2018

View reviewed changes

corrected misplaced closing curly bracket

970dc3c

cmadjar added 2 commits July 17, 2018 13:08

phpcs

0c661ea

more phpcs

6382ad5

zaliqarosli added the Passed manual tests PR has been successfully tested by at least one peer label Jul 24, 2018

ridz1208 removed the [branch] major label Jul 26, 2018

ridz1208 self-assigned this Jul 26, 2018

zaliqarosli approved these changes Jul 26, 2018

View reviewed changes

ZainVirani approved these changes Jul 26, 2018

View reviewed changes

ridz1208 previously requested changes Jul 27, 2018

View reviewed changes

ridz1208 added State: Needs work PR awaiting additional work by the author to proceed and removed Passed manual tests PR has been successfully tested by at least one peer labels Jul 27, 2018

Rida's feedback on the logic

797b3d7

cmadjar removed the State: Needs work PR awaiting additional work by the author to proceed label Jul 30, 2018

um4r12 previously requested changes Jul 30, 2018

View reviewed changes

ridz1208 added the Critical to release PR or issue is key for the release to which it has been assigned label Aug 3, 2018

ridz1208 reviewed Aug 4, 2018

View reviewed changes

phpcs

e4cd8fb

ridz1208 added the Passed manual tests PR has been successfully tested by at least one peer label Aug 4, 2018

ridz1208 merged commit 9a41406 into aces:20.0-release Aug 6, 2018

cmadjar deleted the Fixing_examiners_being_always_inserted_into_Examiner_table branch September 24, 2021 19:05

		@@ -293,9 +290,16 @@ class Edit_User extends \NDB_Form
		// START EXAMINER UPDATE
		// If examiner not in table add him,

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

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

Conversation

cmadjar commented Jul 16, 2018

johnsaigle commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmadjar commented Jul 16, 2018

Choose a reason for hiding this comment

jesscall commented Jul 16, 2018 • edited Loading

ZainVirani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmadjar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZainVirani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmadjar commented Jul 17, 2018

ridz1208 commented Jul 26, 2018

zaliqarosli left a comment

Choose a reason for hiding this comment

ridz1208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ridz1208 commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmadjar commented Aug 1, 2018

Choose a reason for hiding this comment

ridz1208 commented Aug 4, 2018

jesscall commented Jul 16, 2018 •

edited

Loading