[dictionary] Cohort Filter #9390
Conversation
maximemulder
left a comment
There was a problem hiding this comment.
Sorry for the time I took to review. It works on my machine for the new DQT (old one is not loading but probably not related to this PR). It looks good but I have a few minor comments.
| $cohorts = $DB->pselectCol( | ||
| "SELECT DISTINCT tb.CohortID FROM test_names tn | ||
| JOIN test_battery tb ON tn.Test_name=tb.Test_name | ||
| WHERE tn.Test_name=:tn | ||
| ORDER BY tb.CohortID", | ||
| ['tn' => $cat->getName()] | ||
| ); |
There was a problem hiding this comment.
According to this statement, several variables named $cohorts refer to cohort IDs, hence I would prefer them to be named $cohort_ids.
I also see that some of them are documented as of type ?string[], but cohort IDs are nullable integers in the database. This is probably inaccurate ?
There was a problem hiding this comment.
Thank you! I have updated it to use the cohort name instead of the CohortID, as candidate_list does. It makes more sense as now in the list you see the titles instead of the IDs!
| $list_of_cohorts = []; | ||
| foreach ($projects AS $projectID) { | ||
| $list_of_cohorts = $list_of_cohorts + \Utility::getCohortList($projectID); | ||
| } | ||
| $cohort_options=[]; | ||
| foreach (array_values($list_of_cohorts) as $name) { | ||
| $cohort_options[$name] = $name; | ||
| } |
There was a problem hiding this comment.
I do not like to name a variable $list_of_x as a plural variable name and its type already implies the variable contains a list (or other collection), I'd go back to $cohorts here.
| $list_of_cohorts = []; | |
| foreach ($projects AS $projectID) { | |
| $list_of_cohorts = $list_of_cohorts + \Utility::getCohortList($projectID); | |
| } | |
| $cohort_options=[]; | |
| foreach (array_values($list_of_cohorts) as $name) { | |
| $cohort_options[$name] = $name; | |
| } | |
| $cohorts = []; | |
| foreach ($projects AS $projectID) { | |
| $cohorts = $cohorts + \Utility::getCohortList($projectID); | |
| } | |
| $cohort_options=[]; | |
| foreach (array_values($cohorts) as $name) { | |
| $cohort_options[$name] = $name; | |
| } |
| "SELECT GROUP_CONCAT(DISTINCT c.title) FROM test_names tn | ||
| JOIN test_battery tb ON tn.Test_name=tb.Test_name | ||
| JOIN cohort c ON tb.CohortID=c.CohortID | ||
| WHERE tn.Test_name=:tn | ||
| ORDER BY c.title", | ||
| ['tn' => $cat->getName()] |
There was a problem hiding this comment.
I personally prefer to not use group concats (lists should be lists, not strings with commas), but if that's what the rest of the code does I am willing to accept it.
There was a problem hiding this comment.
Yeah this is what the rest of it was like so probably best to keep it the same
maximemulder
left a comment
There was a problem hiding this comment.
Some nits again. Double check the documentation types and I'll test it again. If it works I'll approve the PR.
|
Thank you for the review and the fix! @maximemulder |
|
@skarya22 This also has conflicts and can't be merged |
|
I don't think this is going to scale because it looks like you're making SQL calls in inner loops but I haven't had a chance to test it yet. |
|
Testing this on a large dataset is in my court |
|
I did not test the performance of this, but if it proves problematic it can be optimized after. |
Brief summary of changes
Testing instructions (if applicable)
CCNA OVERRIDE PR