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.

Sign up for GitHub

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

[instrument_list/battery] Clean-up battery retrieval function #7167

Merged
merged 4 commits into from
Aug 26, 2021
Merged

[instrument_list/battery] Clean-up battery retrieval function #7167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a69147
[instrument_list/battery] Clean-up battery retrieval function
ridz1208 Dec 3, 2020
f2cd8cf
changelog
ridz1208 Dec 3, 2020
d47c750
missing session ID
ridz1208 Dec 3, 2020
ad121d0
first commit in a long loooong time
ridz1208 Jun 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ requesting a new account and will be displayed in the User Accounts module (PR #
#### API Documentation (**New Module**)
- New module mostly intended for developers, this module provides a user interface to inspect and try LORIS modules API.
### Clean Up
- *Add item here*
- Removal of unused variables and unnecessary branching from `getBattery()` and `getBatteryVerbose()` functions (PR #7167)
### Notes For Existing Projects
- New function Candidate::getSubjectForMostRecentVisit replaces Utility::getSubprojectIDUsingCandID, adding ability to determine which subproject a candidate belongs to given their most recent visit.
- LINST instrument class was modified to implement the getFullName() and getSubtestList() functions thus making entries in the test_names and instrument_subtests tables respectively unnecessary for LINST instruments (PR #7169)
Expand Down
79 changes: 4 additions & 75 deletions modules/instrument_list/php/instrument_list.class.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,19 +188,14 @@ class Instrument_List extends \NDB_Menu_Filter
// set template data
$this->tpl_data['candID'] = $this->candidate->getCandID();
$this->tpl_data['sessionID'] = $this->timePoint->getSessionID();
$this->tpl_data['stage'] = \Utility::getStageUsingCandID(
new CandID(strval($this->tpl_data['candID']))
);

// get behavioural battery for this visit (time point)
$battery = new \NDB_BVL_Battery;
$battery->selectBattery($this->timePoint->getSessionID());

$this->tpl_data['stage'] = \Utility::getStageUsingCandID(
new CandID(strval($this->tpl_data['candID']))
);

// get the list of instruments
$listOfInstruments = $this->getBatteryInstrumentList(
$this->tpl_data['stage']
);
$listOfInstruments = $battery->getBatteryVerbose();

// display list of instruments
if (!empty($listOfInstruments)) {
Expand Down Expand Up @@ -321,72 +316,6 @@ class Instrument_List extends \NDB_Menu_Filter
return $html;
}

/**
* Gets an associative array of instruments which are members of the current
* battery for instrument list module
*
* @param string $stage Either 'visit' or 'screening' - determines
* whether to register only screening instruments
* or visit instruments
*
* @return array an associative array containing Test_name,
* Full_name, Sub_group, CommentID
*/
function getBatteryInstrumentList($stage=null)
{
$DB = \Database::singleton();

// assert that a battery has already been selected
if (is_null($this->timePoint)) {
throw new \Exception("No battery selected");
}

// craft the select query
$query = "SELECT DISTINCT f.Test_name,
t.Full_name,
f.CommentID,
CONCAT('DDE_', f.CommentID) AS DDECommentID,
ts.Subgroup_name as Sub_group,
ts.group_order as Subgroup_order,
t.isDirectEntry";
if (!is_null($stage)) {
$query .= ", b.instr_order as instrument_order";
}
$query .= " FROM flag f
JOIN test_names t ON (f.Test_name=t.Test_name)
JOIN test_subgroups ts ON (ts.ID = t.Sub_group)
LEFT JOIN session s ON (s.ID=f.SessionID) ";
$qparams = ['SID' => $this->timePoint->getSessionID()];

if (!is_null($stage)) {
$query .= "
LEFT JOIN test_battery b
ON ((t.Test_name=b.Test_name OR b.Test_name IS NULL)
AND (s.SubprojectID=b.SubprojectID OR b.SubprojectID IS NULL)
AND (s.Visit_label=b.Visit_label OR b.Visit_label IS NULL)
AND (s.CenterID=b.CenterID OR b.CenterID IS NULL)) ";
}
$query .= " WHERE f.SessionID=:SID
AND LEFT(f.CommentID, 3) != 'DDE'";

if (\Utility::columnsHasNull('test_subgroups', 'group_order')) {
$query .= " ORDER BY Subgroup_name";
} else {
$query .= " ORDER BY Subgroup_order";
}
if (!is_null($stage)) {
$query .= ", instrument_order";
} else {
$query .= ", Full_name";
}

// get the list of instruments
$rows = $DB->pselect($query, $qparams);

// return all the data selected
return $rows;
}

/**
* Include the column formatter required to display the feedback link colours
* in the candidate_list menu
Expand Down
79 changes: 27 additions & 52 deletions php/libraries/NDB_BVL_Battery.class.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,16 +277,9 @@ class NDB_BVL_Battery
* Gets an array of instruments which are members of the currently
* selected session's battery
*
* @param string $stage Either 'visit' or 'screening' - determines
* whether to register only screening instruments
* or visit instruments
* @param integer $SubprojectID The SubprojectID of that we want the battery for.
* @param string $visit_label The visit label of the battery that we want to
* retrieve
*
* @return array an array of instrument names
*/
function getBattery($stage=null, $SubprojectID=null, $visit_label=null)
function getBattery()
{
$DB = Database::singleton();
// assert that a battery has already been selected
Expand All @@ -309,73 +302,55 @@ class NDB_BVL_Battery

// return an array of test names
return $tests;
} // end getBattery()
}

/**
* Gets an associative array of instruments which are members of the current
* battery
*
* @param string $stage Either 'visit' or 'screening' - determines
* whether to register only screening instruments
* or visit instruments
* @param integer $SubprojectID The SubprojectID of that we want the battery for.
*
* @return array an associative array containing Test_name,
* Full_name, Sub_group, CommentID
*/
function getBatteryVerbose($stage=null, $SubprojectID=null)
function getBatteryVerbose()
{
$DB = Database::singleton();
// assert that a battery has already been selected
$this->_assertBatterySelected();

// craft the select query
$query = "SELECT f.Test_name,
t.Full_name,
f.CommentID,
CONCAT('DDE_', f.CommentID) AS DDECommentID,
ts.Subgroup_name as Sub_group,
ts.group_order as Subgroup_order,
t.isDirectEntry";
if (!is_null($stage)) {
$query .= ", b.instr_order as instrument_order";
}
$query .= " FROM flag f
JOIN test_names t ON (f.Test_name=t.Test_name)
JOIN test_subgroups ts ON (ts.ID = t.Sub_group) ";
$qparams = ['SID' => $this->sessionID];

if (!is_null($stage)) {
$query .= "
LEFT JOIN test_battery b ON (t.Test_name=b.Test_name) ";
}
$query .= " WHERE f.SessionID=:SID
AND f.CommentID NOT LIKE 'DDE%'";
if (!is_null($stage) && !is_null($SubprojectID)) {
$query .= " AND (
b.SubprojectID IS NULL
OR b.SubprojectID=:SubprojID
)";
$qparams['SubprojID'] = $SubprojectID;
}

if (Utility::columnsHasNull('test_subgroups', 'group_order')) {
$query = "SELECT DISTINCT f.Test_name,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be converted to a group BY to avoid start next stage duplication issues

Copy link
Contributor

Choose a reason for hiding this comment

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

does this still need to be resolved?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes but its unrelated to the survey_accounts bug if thats what you were thinking

t.Full_name,
f.CommentID,
CONCAT('DDE_', f.CommentID) AS DDECommentID,
ts.Subgroup_name as Sub_group,
ts.group_order as Subgroup_order,
t.isDirectEntry,
b.instr_order as instrument_order
FROM flag f
JOIN test_names t ON (f.Test_name=t.Test_name)
JOIN test_subgroups ts ON (ts.ID = t.Sub_group)
LEFT JOIN session s ON (s.ID=f.SessionID)
LEFT JOIN test_battery b
ON ((t.Test_name=b.Test_name OR b.Test_name IS NULL)
AND (s.SubprojectID=b.SubprojectID OR b.SubprojectID IS NULL)
AND (s.Visit_label=b.Visit_label OR b.Visit_label IS NULL)
Comment on lines +335 to +336
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you check for Null subproject IDs or visit labels?
Shouldnt the instruments in the test battery always have those information?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, see https://github.com/aces/Loris/blob/main/modules/battery_manager/help/battery_manager.md

AND (s.CenterID=b.CenterID OR b.CenterID IS NULL))
WHERE f.SessionID=:SID
AND LEFT(f.CommentID, 4) != 'DDE_' ";
if (\Utility::columnsHasNull('test_subgroups', 'group_order')) {
$query .= " ORDER BY Subgroup_name";
} else {
$query .= " ORDER BY Subgroup_order";
}
if (!is_null($stage)) {
$query .= ", instrument_order";
} else {
$query .= ", Full_name";
}
//print_r($query);
$query .= ", instrument_order, Full_name";
$qparams = ['SID' => $this->sessionID];

// get the list of instruments
$rows = $DB->pselect($query, $qparams);

// return all the data selected
return $rows;
} // end getBatteryVerbose()
Copy link
Contributor

Choose a reason for hiding this comment

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

There s the same thing line 284 for the getBattery function

}


/**
Expand Down
6 changes: 1 addition & 5 deletions tools/assign_missing_instruments.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,7 @@ function populateVisitLabel($result, $visit_label)
$timePoint->getCenterID(),
$isFirstVisit
);
$actual_battery =$battery->getBattery(
$timePoint->getCurrentStage(),
$result['subprojectID'],
$visit_label
);
$actual_battery =$battery->getBattery();

$diff = array_unique(array_diff($defined_battery, $actual_battery));
if (!empty($diff)) {
Expand Down
4 changes: 2 additions & 2 deletions tools/fix_timepoint_date_problems.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ function diagnose($sessionID, $dateType = null, $newDate = null)
// compute subject age for the current stage
$ageArray = Utility::calculateAge($dateBirth, $dateOfStage);
$age = ($ageArray['year'] * 12 + $ageArray['mon']) * 30
+ $ageArray['day'];
+ $ageArray['day'];
if ($age < 0) {
$age = 0;
}
Expand All @@ -733,7 +733,7 @@ function diagnose($sessionID, $dateType = null, $newDate = null)
$success = $battery->selectBattery(new \SessionID(strval($sessionID)));

// get the existing battery for the stage
$existingTests = $battery->getBattery($stage);
$existingTests = $battery->getBattery();

// determine the correct list of instruments
$neededTests = Utility::lookupBattery($age, $stage);
Expand Down