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] Enable PHPCS for fix_timepoint_date_problems.php #6798

Merged

Conversation

laemtl
Copy link
Contributor

@laemtl laemtl commented Jun 30, 2020

Replaces #5984

This PR also makes changes to the User and Timepoint class, to fix return type inaccuracies.

To test

  • checkout the branch
  • run PHPCS and check that there is no errors:
    npm run lint:php
  • run the tool and check that there is no errors:
    php tools/fix_timepoint_date_problems.php

@laemtl laemtl changed the title 2020 01 20 phpcs fix timepoint date problems [Tools] Enable PHPCS for fix_timepoint_date_problems.php Jun 30, 2020
@laemtl laemtl added the Testing PR contains test plan or automated test code (or config files for Travis) label Jun 30, 2020
@laemtl laemtl force-pushed the 2020-01-20_PHPCS-fix_timepoint_date_problems branch from 949b7cc to e756f28 Compare July 9, 2020 16:08
@laemtl laemtl changed the base branch from master to main July 21, 2020 15:39
@laemtl laemtl force-pushed the 2020-01-20_PHPCS-fix_timepoint_date_problems branch 2 times, most recently from c7960b9 to ac476f6 Compare July 22, 2020 00:48
@laemtl laemtl added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Aug 3, 2020
@laemtl laemtl force-pushed the 2020-01-20_PHPCS-fix_timepoint_date_problems branch from ac476f6 to 86453cf Compare August 3, 2020 22:28
@laemtl laemtl removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Aug 3, 2020
@laemtl laemtl added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Aug 17, 2020
@laemtl laemtl force-pushed the 2020-01-20_PHPCS-fix_timepoint_date_problems branch from 86453cf to fefd9f0 Compare August 18, 2020 00:37
@laemtl laemtl added State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) and removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) labels Aug 18, 2020
@laemtl laemtl force-pushed the 2020-01-20_PHPCS-fix_timepoint_date_problems branch from fefd9f0 to cba2763 Compare August 20, 2020 18:04
@laemtl laemtl removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Aug 20, 2020
@ridz1208 ridz1208 removed the Testing PR contains test plan or automated test code (or config files for Travis) label Sep 1, 2020
@jesscall
Copy link
Contributor

jesscall commented Sep 3, 2020

Hi @laemtl!

I was wondering if you could provide some information on how this could be tested?

Thanks!

Copy link
Contributor

@jesscall jesscall left a comment

Choose a reason for hiding this comment

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

Hi @laemtl!
Here are some suggested improvements:

  1. Check that args are set
  2. When using diagnose, I am getting an error telling me it cannot diagnose a non-started timepoint even though the visits have been started.
$ php fix_timepoint_date_problems.php diagnose 676061
Candidate: 676061 

 Timepoint V1 ; SubProjectID: 2 ; Effective DOB: 1980-02-02 ; (SessionID): 2145 
Error: failed to get the list of needed instruments for candidate (676061), timepoint (2145):
Error: Cannot diagnose the non-started timepoint (2145) for candidate (676061)!

 Timepoint V2 ; SubProjectID: 1 ; Effective DOB: 1980-02-02 ; (SessionID): 2147 
Error: failed to get the list of needed instruments for candidate (676061), timepoint (2147):
Error: Cannot diagnose the non-started timepoint (2147) for candidate (676061)!

 Timepoint V3 ; SubProjectID: 2 ; Effective DOB: 1980-02-02 ; (SessionID): 2148 
Error: failed to get the list of needed instruments for candidate (676061), timepoint (2148):
Error: Cannot diagnose the non-started timepoint (2148) for candidate (676061)!

 Timepoint V4 ; SubProjectID: 3 ; Effective DOB: 1980-02-02 ; (SessionID): 2149 
Error: failed to get the list of needed instruments for candidate (676061), timepoint (2149):
Error: Cannot diagnose the non-started timepoint (2149) for candidate (676061)!

 Timepoint V5 ; SubProjectID: 3 ; Effective DOB: 1980-02-02 ; (SessionID): 2150 
Error: failed to get the list of needed instruments for candidate (676061), timepoint (2150):
Error: Cannot diagnose the non-started timepoint (2150) for candidate (676061)!

 Timepoint V6 ; SubProjectID: 3 ; Effective DOB: 1980-02-02 ; (SessionID): 2151 
Error: failed to get the list of needed instruments for candidate (676061), timepoint (2151):
Error: Cannot diagnose the non-started timepoint (2151) for candidate (676061)!

Screen Shot 2020-09-03 at 5 09 03 PM

  1. fix_date option returns an error (on dob and on visit)
$ php fix_timepoint_date_problems.php fix_date 676061 2020-01-01 dob
PHP Notice:  Undefined index: UserID in /var/www/loris/php/libraries/User.class.inc on line 256
PHP Fatal error:  Uncaught TypeError: Return value of User::getUsername() must be of the type string, null returned in /var/www/loris/php/libraries/User.class.inc:256
Stack trace:
#0 /var/www/loris/tools/fix_timepoint_date_problems.php(415): User->getUsername()
#1 /var/www/loris/tools/fix_timepoint_date_problems.php(222): fixDate(Object(LORIS\StudyEntities\Candidate\CandID), 'dob', '2020-01-01')
#2 {main}
  thrown in /var/www/loris/php/libraries/User.class.inc on line 256

$ php fix_timepoint_date_problems.php fix_date 676061 2020-01-01 V1 2145
PHP Notice:  Undefined index: UserID in /var/www/loris/php/libraries/User.class.inc on line 256
PHP Fatal error:  Uncaught TypeError: Return value of User::getUsername() must be of the type string, null returned in /var/www/loris/php/libraries/User.class.inc:256
Stack trace:
#0 /var/www/loris/tools/fix_timepoint_date_problems.php(415): User->getUsername()
#1 /var/www/loris/tools/fix_timepoint_date_problems.php(222): fixDate(Object(LORIS\StudyEntities\Candidate\CandID), 'v1', '2020-01-01')
#2 {main}
  thrown in /var/www/loris/php/libraries/User.class.inc on line 256

* HELP SCREEN
* display and stop processing if action=help
*/
if (empty($argv[1]) || $argv[1] == 'help') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Exit script after displaying help message if args are missing. Also check that argv[2], [3]..etc are set, otherwise:

PHP Notice:  Undefined offset: 1 in /var/www/loris/tools/fix_timepoint_date_problems.php on line 92
PHP Notice:  Undefined offset: 2 in /var/www/loris/tools/fix_timepoint_date_problems.php on line 95
PHP Fatal error:  Uncaught TypeError: Argument 1 passed to LORIS\StudyEntities\Candidate\ValidatableIdentifier::__construct() must be of the type string, null given, called in /var/www/loris/tools/fix_timepoint_date_problems.php on line 95 and defined in /var/www/loris/src/StudyEntities/Candidate/ValidatableIdentifier.php:58
Stack trace:
#0 /var/www/loris/tools/fix_timepoint_date_problems.php(95): LORIS\StudyEntities\Candidate\ValidatableIdentifier->__construct(NULL)
#1 {main}
  thrown in /var/www/loris/src/StudyEntities/Candidate/ValidatableIdentifier.php on line 58

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @jesscall for all you suggestions. I will have a look shortly.

@jesscall jesscall added the State: Needs work PR awaiting additional work by the author to proceed label Sep 3, 2020
@laemtl laemtl added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Oct 6, 2020
@laemtl laemtl force-pushed the 2020-01-20_PHPCS-fix_timepoint_date_problems branch from cba2763 to b5d3089 Compare October 6, 2020 21:39
@laemtl laemtl removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Oct 6, 2020
@laemtl laemtl force-pushed the 2020-01-20_PHPCS-fix_timepoint_date_problems branch from b5d3089 to b7c85df Compare October 8, 2020 04:33
@laemtl laemtl removed the State: Needs work PR awaiting additional work by the author to proceed label Oct 8, 2020
@laemtl laemtl force-pushed the 2020-01-20_PHPCS-fix_timepoint_date_problems branch from b7c85df to 3196a14 Compare October 8, 2020 04:42
@laemtl
Copy link
Contributor Author

laemtl commented Oct 8, 2020

@jesscall You were right, there were several problems in this script, thanks to have reported them. I made an attempt to fix them, let me know what you think.
@driusan, @ridz1208 I would like to know if there is a better approach to what has been done L377. I wonder if it may be not convenient to have a Loris user matching your session user.

@laemtl laemtl requested a review from jesscall October 8, 2020 16:11
@laemtl laemtl force-pushed the 2020-01-20_PHPCS-fix_timepoint_date_problems branch 3 times, most recently from abd6e76 to b13c8f9 Compare December 16, 2020 04:28
@laemtl laemtl force-pushed the 2020-01-20_PHPCS-fix_timepoint_date_problems branch from 01583bd to 305856c Compare December 23, 2020 20:33
@laemtl
Copy link
Contributor Author

laemtl commented Jan 8, 2021

@driusan What is missing in this PR to be ready?

@driusan
Copy link
Collaborator

driusan commented Jan 8, 2021

The change to the timepoint class still needs to be reverted.

@laemtl
Copy link
Contributor Author

laemtl commented Jan 8, 2021

@driusan Done! I opened an issue for discussion: #7282

@laemtl laemtl added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Jan 15, 2021
@laemtl
Copy link
Contributor Author

laemtl commented Jan 15, 2021

Blocked by #6825

@driusan driusan removed the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Jan 18, 2021
@driusan
Copy link
Collaborator

driusan commented Jan 18, 2021

#6825 is merged

@laemtl laemtl added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jan 18, 2021
@laemtl laemtl force-pushed the 2020-01-20_PHPCS-fix_timepoint_date_problems branch from e27475c to eec1134 Compare January 18, 2021 20:44
@laemtl laemtl removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jan 18, 2021
@laemtl
Copy link
Contributor Author

laemtl commented Feb 1, 2021

@driusan Ready to be reviewed/merged.

@laemtl laemtl force-pushed the 2020-01-20_PHPCS-fix_timepoint_date_problems branch from eec1134 to 66c8a64 Compare February 11, 2021 18:23
@laemtl
Copy link
Contributor Author

laemtl commented Feb 11, 2021

@driusan Rebased.

@driusan
Copy link
Collaborator

driusan commented Feb 11, 2021

@jesscall GitHub says you requested changes.. can you update your review so I can merge (if you're satisfied..)?

@laemtl laemtl force-pushed the 2020-01-20_PHPCS-fix_timepoint_date_problems branch from 66c8a64 to 6811979 Compare February 15, 2021 17:13
@driusan driusan merged commit d29d39e into aces:main Mar 8, 2021
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
@ridz1208 ridz1208 added this to the 24.0.0 milestone Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing PR contains test plan or automated test code (or config files for Travis)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants