[Core: Candidate] Fix getFirstVisit method

[davidblader]

Brief summary of changes

ran into this while trying to run assign_missing_instruments
got:
PHP Fatal error: Uncaught DomainException: Attempt to use pselectRow on a query that returns multiple rows in /var/www/loris/php/libraries/Database.class.inc:764 Stack trace: #0 /var/www/loris/php/libraries/Database.class.inc(892): Database->pselectRow('SELECT Visit_la...', Array) #1 /var/www/loris/php/libraries/Candidate.class.inc(630): Database->pselectOne('SELECT Visit_la...', Array) #2 /var/www/loris/tools/assign_missing_instruments.php(93): Candidate->getFirstVisit() #3 /var/www/loris/tools/assign_missing_instruments.php(148): populateVisitLabel(Array, 'Initial_Assessm...')
this updates the query to ensure a singular result is returned

To test this change...

• try running assign_missing_instruments.php, also try creating & starting a visit

[johnsaigle]

I just did some testing of your error and I was able to reproduce it. It's definitely a problem.

I don't think I like the approach of hamstringing pselectOne in this way. The limit clause is there to tell developers that they're using the function correctly -- in this case it's doing exactly what it's supposed to do.

My opinion would be to attempt a rewrite of the query. Even if it becomes fairly unreadable, that's what code comments are for.

Another possible approach would be to change the signature of getFirstVisit to return an array instead of a string. This way it will typically return a one-element array. In the case where two results appear (i.e. when a candidate does two visits on the same day), the calling code can determine which or both of the visit labels is appropriate to use.

What do you think?

In any case the real issue here is in the Candidate class and not in the Database class , so I don't think we should change the Database.

[christinerogers]

I'd agree, leaning towards fixing getFirstVisit() - including the function description (and maybe even name?) -- given we're now plausibly allowing for multiple first visits to be returned.

[driusan]

I agree with @johnsaigle. I don't like this change. If you're calling pselectRow you should be providing a query that only returns one row. The right solution is to fix the places that are providing non-unique queries to pselectRow/One.

[johnsaigle]

assign_missing_instruments.php no longer throws the error that it did before.

Addressed by David

Addressed by David

[johnsaigle]

Looks good to me and fixes the error. Someone else should probably review the query because I don't know enough to tell if it's optimal.

[johnsaigle]

Adding Needs Work based on the discussion in Dave's review.

[johnsaigle]

Looks like the unit tests needs to be updated.

[zaliqarosli]

i'm trying to use this fix to the script and i'm still getting this type error
PHP Fatal error: Uncaught TypeError: Return value of Candidate::getFirstVisit() must be of the type string, null returned in /var/www/loris/php/libraries/Candidate.class.inc:647 Stack trace: #0 /var/www/loris/tools/assign_missing_instruments.php(93): Candidate->getFirstVisit() #1 /var/www/loris/tools/assign_missing_instruments.php(148): populateVisitLabel(Array, 'V3') #2 {main} thrown in /var/www/loris/php/libraries/Candidate.class.inc on line 647
i'm not quite sure why the empty string still isn't returning 😕

[zaliqarosli]

@davidblader i just figured out that if pselectCol returns an empty array, getFirstVisit returns null. you need an empty() check!

[zaliqarosli]

you're also going to have to add return $ProjectList[$this->getProjectID()] ?? ''; to line 412

[zaliqarosli]

passed testing and approved! will approve again once passes travis 👍

[PapillonMcGill]

@zaliqarosli @davidblader
line 4232+ of Travis report (PHP 7.2)
There was 1 failure:

1. CandidateTest::testGetFirstVisitReturnsFirstVisitLabel
   Failed asserting that two strings are equal.
   --- Expected
   +++ Actual
   @@ @@
   -'V01'
   +''
   /app/test/unittests/CandidateTest.php:362

the test at line 362 of CandidateTest expected to have 'V01' as result but received and empty string.
Hope it help

[xlecours]

Note on Travis error.
My guess is that the test is failing because it is says that the second call of pselectOne should return V01; but there is no second call to that function since pselectCol is called instead.
The test should say something like

$this->_dbMock->expects($this->at(1))
            ->method('pselectCol')
            ->willReturn('V01');

My review of the PR
Considering that:

1. We should be able to write a query that is returning a single value. The use of pselectCol seems to be a hack to circumvent poor database structure.
2. The function is use at two places and in both cases there is already a Timepoint in the calling code block where the purpose of the call is to compare the returned visit label with the timepoint's visit label.
3. There is a VisitNo column that SHOULD be reliable.

Can we remove that function and either have a isFirstVisit function in the Timepoint class or simply check if $timepoint->getVisitNo() === 1 ?

[zaliqarosli]

nice, i like this solution 👍

[zaliqarosli]

@davidblader i had a lot of candidates in my RaisinBread with multiple timepoints of VisitNo=1, but i'm passing manual test on this assuming that the real data on projects are clean. Might be nice to add a try catch block in getFirstVisit() or assign_missing_instruments.php that catches the DomainException Attempt to use pselectRow on a query that returns multiple rows if the pselectOne call fails and throws a new error Candidate $candID has two first visits where we expect one. Please clean the data or something like that to the console. but i won't block this PR on that request