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

[Tools] delete_candidate CanID Fix #6805

Merged
merged 3 commits into from
Jul 21, 2020

Conversation

laemtl
Copy link
Contributor

@laemtl laemtl commented Jul 6, 2020

Fix in delete_candidate.php tool to use the CandID class.

@laemtl laemtl added Release: Add to release notes PR whose changes should be highlighted in the release notes Category: Bug PR or issue that aims to report or fix a bug and removed Release: Add to release notes PR whose changes should be highlighted in the release notes labels Jul 6, 2020
@@ -150,7 +154,7 @@ function showHelp()
* (second-level relations) should have actions on delete specified in the
* database schema
*
* @param string $CandID Identifying the candidate
* @param CanID $CandID Identifying the candidate
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* @param CanID $CandID Identifying the candidate
* @param CandID $CandID Identifying the candidate

@laemtl laemtl force-pushed the 2020-07-06-delete_candidate_CanID branch from 7045719 to 495cd79 Compare July 6, 2020 17:36
@laemtl laemtl requested a review from driusan July 6, 2020 17:36
@@ -18,11 +18,15 @@
* @license Loris license
* @link https://www.github.com/aces/Loris-Trunk/
*/

use LORIS\StudyEntities\Candidate\CandID;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure this needs to go below the autoload require.

Copy link
Contributor Author

@laemtl laemtl Jul 7, 2020

Choose a reason for hiding this comment

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

@driusan For my personal knowledge, can you explain a bit why? I will take that into account for future changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I might be mistaken, it might not try autoloading until the class is referenced with "new", but how would the class be loaded if the autoloader hasn't been setup yet?

@laemtl laemtl added Testing PR contains test plan or automated test code (or config files for Travis) Blocking PR should be prioritized because it is blocking the progress of another task labels Jul 7, 2020
Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

Minor suggestion, to clean up the header.

tools/delete_candidate.php Outdated Show resolved Hide resolved
@laemtl laemtl force-pushed the 2020-07-06-delete_candidate_CanID branch from 495cd79 to 4a83884 Compare July 7, 2020 22:12
@laemtl laemtl requested a review from christinerogers July 7, 2020 22:12
@laemtl laemtl removed Blocking PR should be prioritized because it is blocking the progress of another task Testing PR contains test plan or automated test code (or config files for Travis) labels Jul 7, 2020
@laemtl laemtl requested a review from driusan July 9, 2020 16:17
Co-authored-by: christinerogers <christinerogers@users.noreply.github.com>
@laemtl
Copy link
Contributor Author

laemtl commented Jul 15, 2020

Thanks @ridz1208

@CamilleBeau CamilleBeau added the Passed manual tests PR has been successfully tested by at least one peer label Jul 16, 2020
@laemtl laemtl changed the base branch from master to main July 21, 2020 15:39
@driusan driusan merged commit 11f4f02 into aces:main Jul 21, 2020
@laemtl laemtl deleted the 2020-07-06-delete_candidate_CanID branch July 21, 2020 19:10
@ridz1208 ridz1208 added this to the 24.0.0 milestone Jul 27, 2020
spell00 pushed a commit to spell00/Loris that referenced this pull request Aug 13, 2020
Fix in delete_candidate.php tool to use the CandID class.

    Resolves aces#6757
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 15, 2021
Fix in delete_candidate.php tool to use the CandID class.

    Resolves aces#6757
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
Fix in delete_candidate.php tool to use the CandID class.

    Resolves aces#6757
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Bug PR or issue that aims to report or fix a bug 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.

delete_candidate.php should use CandID class
6 participants