Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Core] PSCID uniqueness #8411

Merged
merged 6 commits into from
Jun 27, 2023
Merged

Conversation

xlecours
Copy link
Contributor

@xlecours xlecours commented Feb 24, 2023

Brief summary of changes

Add PSCID uniqueness check to Candidate::createNew.

To test:

  1. Make sure PSCID generation os set to user in yoru config file.
  2. Create a candidate
  3. try to create a second candidate with the same PSCID
  4. Error should say that PSCID must be unique

@@ -241,7 +241,7 @@ class Candidates extends Endpoint implements \LORIS\Middleware\ETagCalculator
$pscid,
$project->getId()
);
} catch (\LorisException | \InvalidArgumentException $e) {
} catch (\Exception | \LorisException | \InvalidArgumentException $e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When is a \LorisException or an \InvalidArgumentException not an \Exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When PSCID is not provided and generation is user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean that by adding \Exception I should remove the other two because they are subclasses?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That was my question, yes.

@laemtl laemtl added the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Mar 7, 2023
@xlecours
Copy link
Contributor Author

xlecours commented Mar 7, 2023

@kongtiaowang would you know why this is failing? I'am not changing the file that triggers the error.

@driusan
Copy link
Collaborator

driusan commented Mar 7, 2023

It looks like it's the test that you're adding in the PR that's failing?


There was 1 failure:

1) LorisApiCandidatesTest::testPostCandidatesUserGeneratedPSCID
Failed asserting that 201 matches expected 400.

/app/raisinbread/test/api/LorisApiCandidatesTest.php:405
phpvfscomposer:///app/vendor/phpunit/phpunit/phpunit:60

@xlecours
Copy link
Contributor Author

xlecours commented Mar 7, 2023

My bad, I was looking a the warning. Thanks, I'll look into it.

@xlecours xlecours marked this pull request as ready for review March 21, 2023 17:39
@xlecours xlecours added Add to Release Notes PR change should be highlighted in Release notes (important security, features and bugfixes) Document at Release PR adds or changes a feature such that the wiki (or other documentation) must be updated Beginner Friendly PR or Issue appears to be easy for someone to use to familiarize themselves with LORIS and removed Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix labels Mar 21, 2023
Copy link
Contributor

@zaliqarosli zaliqarosli left a comment

Choose a reason for hiding this comment

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

API docs says PSCID uniqueness throws 409 but i believe this throws 400. can we resolve this either in the docs or in the endpoint?

otherwise, the code does work to not let candidate creation happen if PSCID already exists

curl -H "Authorization: Bearer $token" https://copn-stg.loris.ca/api/v0.0.3/candidates -d '{"Candidate":{"PSCID":"MNI0054","Project":"COPN","Site":"Montreal Neurological Institute","DoB":"1998-01","Sex":"Female"}}'
{"error":"PSCID must be unique"}

@zaliqarosli zaliqarosli linked an issue Jun 12, 2023 that may be closed by this pull request
@zaliqarosli
Copy link
Contributor

@xlecours i can review/test once testing is fixed/passes

@driusan
Copy link
Collaborator

driusan commented Jun 21, 2023

Looks like it's just PHPCS:


FILE: ...nner/work/Loris/Loris/php/exceptions/ConflictException.class.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 7 | ERROR | [x] Tag value for @license tag indented incorrectly;
   |       |     expected 1 spaces but found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Copy link
Contributor

@zaliqarosli zaliqarosli left a comment

Choose a reason for hiding this comment

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

duplicate PSCID is not created but no error is thrown

lorisadmin@copn-dev:/var/www/loris/project$ curl -H "Authorizatio/api/v0.0.3/candidates -d '{"Candidate":{"PSCID":"MNI0054","Project":"COPN","Site":"Montreal Neurological Institute","DoB":"1998-01-01","Sex":"Female"}}'
{"CandID":"148989","Project":"COPN","PSCID":"MNI0054","Site":"Montreal Neurological Institute","EDC":null,"DoB":"1998-01-01","Sex":"Female"}
lorisadmin@copn-dev:/var/www/loris/project$ curl -H "Authorization: Bearer $/api/v0.0.3/candidates -d '{"Candidate":{"PSCID":"MNI0054","Project":"COPN","Site":"Montreal Neurological Institute","DoB":"1998-01-01","Sex":"Female"}}'

@xlecours
Copy link
Contributor Author

xlecours commented Jun 22, 2023

duplicate PSCID is not created but no error is thrown

@zaliqarosli Can you try with -v curl option and the apache error log if any please?

@zaliqarosli
Copy link
Contributor

@xlecours 500 error, Conflict Exception class not found ..

[Mon Jun 26 13:07:31.861163 2023] [php:error] [pid 2258] [client 192.168.122.1:42732] PHP Fatal error:  Uncaught Error: Class "ConflictException" not found in /var/www/loris/project/libraries/Candidate.class.inc:276\nStack trace:\n#0 /var/www/loris/project/modules/api/php/endpoints/candidates.class.inc(242): Candidate::createNew()\n#1 /var/www/loris/project/modules/api/php/endpoints/candidates.class.inc(107): LORIS\\api\\Endpoints\\Candidates->_handlePOST()\n#2 /var/www/loris/src/Middleware/ETag.php(78): LORIS\\api\\Endpoints\\Candidates->handle()\n#3 /var/www/loris/src/Http/Endpoint.php(46): LORIS\\Middleware\\ETag->process()\n#4 /var/www/loris/project/modules/api/php/endpoint.class.inc(65): LORIS\\Http\\Endpoint->process()\n#5 /var/www/loris/project/modules/api/php/module.class.inc(125): LORIS\\Api\\Endpoint->process()\n#6 /var/www/loris/src/Router/ModuleRouter.php(77): LORIS\\api\\Module->handle()\n#7 /var/www/loris/src/Middleware/ExceptionHandlingMiddleware.php(54): LORIS\\Router\\ModuleRouter->handle()\n#8 /var/www/loris/src/Router/BaseRouter.php(126): LORIS\\Middleware\\ExceptionHandlingMiddleware->process()\n#9 /var/www/loris/src/Middleware/ResponseGenerator.php(50): LORIS\\Router\\BaseRouter->handle()\n#10 /var/www/loris/src/Middleware/ContentLength.php(52): LORIS\\Middleware\\ResponseGenerator->process()\n#11 /var/www/loris/htdocs/index.php(55): LORIS\\Middleware\\ContentLength->process()\n#12 {main}\n  thrown in /var/www/loris/project/libraries/Candidate.class.inc on line 276
[Mon Jun 26 13:07:31.861343 2023] [php:notice] [pid 2258] [client 192.168.122.1:42732] [CRITICAL] /var/www/loris/project/libraries/Candidate.class.inc:276: Uncaught Error: Class "ConflictException" not found in /var/www/loris/project/libraries/Candidate.class.inc:276\nStack trace:\n#0 /var/www/loris/project/modules/api/php/endpoints/candidates.class.inc(242): Candidate::createNew()\n#1 /var/www/loris/project/modules/api/php/endpoints/candidates.class.inc(107): LORIS\\api\\Endpoints\\Candidates->_handlePOST()\n#2 /var/www/loris/src/Middleware/ETag.php(78): LORIS\\api\\Endpoints\\Candidates->handle()\n#3 /var/www/loris/src/Http/Endpoint.php(46): LORIS\\Middleware\\ETag->process()\n#4 /var/www/loris/project/modules/api/php/endpoint.class.inc(65): LORIS\\Http\\Endpoint->process()\n#5 /var/www/loris/project/modules/api/php/module.class.inc(125): LORIS\\Api\\Endpoint->process()\n#6 /var/www/loris/src/Router/ModuleRouter.php(77): LORIS\\api\\Module->handle()\n#7 /var/www/loris/src/Middleware/ExceptionHandlingMiddleware.php(54): LORIS\\Router\\ModuleRouter->handle()\n#8 /var/www/loris/src/Router/BaseRouter.php(126): LORIS\\Middleware\\ExceptionHandlingMiddleware->process()\n#9 /var/www/loris/src/Middleware/ResponseGenerator.php(50): LORIS\\Router\\BaseRouter->handle()\n#10 /var/www/loris/src/Middleware/ContentLength.php(52): LORIS\\Middleware\\ResponseGenerator->process()\n#11 /var/www/loris/htdocs/index.php(55): LORIS\\Middleware\\ContentLength->process()\n#12 {main}\n  thrown

this might be specific to my set up. i'll look into it, but anything obvious sticking out to you?

@driusan
Copy link
Collaborator

driusan commented Jun 26, 2023

Have you run make or make dev?

Copy link
Contributor

@zaliqarosli zaliqarosli left a comment

Choose a reason for hiding this comment

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

tested again and it works great 💪

@zaliqarosli zaliqarosli added the Passed Manual Tests PR has undergone proper testing by at least one peer label Jun 26, 2023
@driusan driusan merged commit c521659 into aces:24.1-release Jun 27, 2023
@ridz1208 ridz1208 added this to the 24.1.5 milestone Jul 10, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Add to Release Notes PR change should be highlighted in Release notes (important security, features and bugfixes) Beginner Friendly PR or Issue appears to be easy for someone to use to familiarize themselves with LORIS Document at Release PR adds or changes a feature such that the wiki (or other documentation) must be updated Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[api] Able to create duplicate candidates with same PSCID
5 participants