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.

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] Use userID to update examiners table #7462

Merged

Conversation

CamilleBeau
Copy link
Contributor

@CamilleBeau CamilleBeau commented Jun 7, 2021

Brief summary of changes

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 PR 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 in this PR 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.

Testing instructions (if applicable)

1.Request an account and select "Examiner role"
2. check that a row in the examiners table exists for this newly requested user
3. From user_accounts, edit this user to change the name.
4. Check that the full name is updated in the examiners table for that user, and that only one row exists for that userID
5. Create an account for a user with the same name as an already-existing examiner
6. Make sure that a new row with a different userID is created in the examiners table. Both the older user and the newer user should have rows in the examiner table.
7. Test the examiners module according to the module testing instructions
8. Make sure that duplicate names in the examiner drop down of an instrument are identified with an email (if one of the names is from a non-user examiner, there will be no email next to that one)
9. From outside of this PR, change the name of a user with examiner credentials. Check that they exist twice in the examiners table (once with old name, once with new name)
10. From inside this PR, run the Remove_duplicate_examiners.php script. Make sure that it gets rid of the duplicate examiner with the old name in the examiners table.
11. Make sure that examinerIDs are updated appropriately in the certification table.
12. Make sure that rows are removed from the examiners_psc_rel table.
13. Make sure that no rows are deleted/updated for the correct examinerID

Link(s) to related issue(s)

@driusan
Copy link
Collaborator

driusan commented Jun 7, 2021

@jesscall maybe you can review?

@jesscall
Copy link
Contributor

jesscall commented Jun 8, 2021

Looks good, just waiting for a cleanup script to be written

@CamilleBeau CamilleBeau added the State: Needs work PR awaiting additional work by the author to proceed label Jun 16, 2021
@jesscall
Copy link
Contributor

@CamilleBeau let me know when this is ready for re-rview

@CamilleBeau CamilleBeau removed the State: Needs work PR awaiting additional work by the author to proceed label Jun 16, 2021
@CamilleBeau
Copy link
Contributor Author

@jesscall Ready for review!

@driusan driusan added the Critical to release PR or issue is key for the release to which it has been assigned label Sep 14, 2021
@racostas racostas self-requested a review September 24, 2021 18:48
@racostas
Copy link
Contributor

Step 5. "Create an account for a user with the same name as an already-existing examiner" isn't working for me, got 500 error. (I'm crating the account as Admin->add User).

@racostas racostas added the State: Needs work PR awaiting additional work by the author to proceed label Sep 24, 2021
@CamilleBeau
Copy link
Contributor Author

@racostas I couldn't replicate this error. I was able to request an account for a user who had the same name (mine for testing) as 2 other users in the DB. You were testing this on CCNA data right? The login module has an override on CCNA

@racostas
Copy link
Contributor

@racostas I couldn't replicate this error. I was able to request an account for a user who had the same name (mine for testing) as 2 other users in the DB. You were testing this on CCNA data right? The login module has an override on CCNA

Strange. I would suggest somebody else to test. I tested in a fresh version of Loris 23.0.3 using RB not in CCNA but could have some missconfig. Steps from 1 to 4 worked fine nevertheless.

@CamilleBeau
Copy link
Contributor Author

Strange. I would suggest somebody else to test. I tested in a fresh version of Loris 23.0.3 using RB not in CCNA but could have some missconfig. Steps from 1 to 4 worked fine nevertheless.

I was able to replicate the error Rolando found after all, and I have committed a fix. Ready for re-testing @racostas

@CamilleBeau CamilleBeau removed the State: Needs work PR awaiting additional work by the author to proceed label Sep 28, 2021
Copy link
Contributor

@racostas racostas left a comment

Choose a reason for hiding this comment

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

LGTM now.

@racostas racostas added the Passed manual tests PR has been successfully tested by at least one peer label Sep 30, 2021
@driusan driusan merged commit 92970de into aces:23.0-release Oct 6, 2021
@ridz1208 ridz1208 added this to the 23.0.8 milestone Oct 19, 2021
driusan pushed a commit that referenced this pull request Nov 24, 2021
This adds an empty value to the array of examiner names. This was mistakenly taken out in #7462, and is causing the first value to be selected by default in instruments.
driusan added a commit to driusan/Loris that referenced this pull request Jan 27, 2022
This was broken in PR aces#7462, so add it back to the same
place that it was removed from for more compatibility.

This also fixes the LINST instruments which weren't fixed
by adding it before the addSelect.
@@ -0,0 +1,85 @@
<?php
Copy link
Collaborator

Choose a reason for hiding this comment

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

before SQL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical to release PR or issue is key for the release to which it has been assigned Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants