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

ridz1208 · 2020-12-03T18:04:41Z

Brief summary of changes

Removed getBatteryInstrumentList function from the instrument_list module and made it query the data from the correct place which is the NDB_BVL_Battery class
Modified query in NDB_BVL_Battery to remove duplicate returns and avoid "jumpy" test_names depending on if an instrument_order is defined or not (see individual comments below for more details)
Removed unused/inaccurate parameters for Battery functions in Battery class. The parameters STAGE, SUBPROJECTID and VISIT LABEL should all be queried from the session itself (Which was already mandatory to define. see _assertBatterySelected) but are also either completely unsued in these functions or not necessary at all.

Testing instructions (if applicable)

@jesscall @sruthymathew123 @zaliqarosli each of you should test this on at least a handful of timepoints since you all use different functionalities affecting the instrument list module

QPN uses instruments at the "Not Started" stage which might be affected by these modifications
IBIS has several test_batteries which diverge from the "defined batteries" which is what the now deleted function was added for in the first place (see Instrument_list query mod to left join on test battery #2006). Also can test compatibility with survey instruments
CCNA uses instrument ordering as well as manual addition of individual instruments to sessions

make sure:

instrument ordering works when order is defined in the battery table
if ordering is not defined for some or all of the instruments, order should default to Full Name displayed
ALL instruments in the flag table for that session appear in the instrument_list module, even instruments which are not in the test_battery defined for that session (ie survey instruments and manually added instruments)
Control panel properly checks that all instruments are complete in order to allow pass/fail changes and completion of the session (this functionality depends on being able to query all instruments of the session)

ridz1208 · 2020-12-23T14:00:45Z

php/libraries/NDB_BVL_Battery.class.inc

-        }
-
-        if (Utility::columnsHasNull('test_subgroups', 'group_order')) {
+        $query = "SELECT DISTINCT f.Test_name,


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

does this still need to be resolved?

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

ridz1208 · 2021-01-20T00:06:06Z

@jesscall @CamilleBeau @pierre-p-s I need someone from CCNA to test this one, you guys have the more complex batteries and use all sorts of ordering.

read the description carefully, theres a bunch of clean up here and testing should be rigorous

zaliqarosli · 2021-02-11T22:29:55Z

@ridz1208 does this fix a specific issue? I am wondering if this PR indeed fixes the error you get when you try to 'Start Visit Stage' on a session that already has a survey created i.e. a test battery generated, blocking the rest of the instruments in the test battery from getting populated

ridz1208 · 2021-02-11T22:36:05Z

@zaliqarosli no this might fix instruments not displaying in the same order when you load instrument_list multiple times. the issue you are talking about was however fixed in a different PR

pierre-p-s

Tested and the ordering seemed to be working. Pass fail function worked as intended.

pierre-p-s · 2021-02-15T21:17:07Z

php/libraries/NDB_BVL_Battery.class.inc

+      	                AND (s.SubprojectID=b.SubprojectID OR b.SubprojectID IS NULL)
+       	                AND (s.Visit_label=b.Visit_label OR b.Visit_label IS NULL)


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

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

pierre-p-s · 2021-02-15T21:18:58Z

php/libraries/NDB_BVL_Battery.class.inc

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

        // return all the data selected
        return $rows;
-    } // end getBatteryVerbose()


There s the same thing line 284 for the getBattery function

cmadjar · 2021-06-08T13:45:23Z

@ridz1208 can you rebase and integrate Pierre's feedback? Thanks!

ridz1208 · 2021-06-15T12:37:23Z

@pierre-p-s please re-review/re-test

@zaliqarosli if you can find some time to test (see description for more)

pierre-p-s · 2021-06-28T18:09:08Z

Manual test and review worked.

However we found an issue in the patch 2020-02-10_NewModulePermissions.sql in SQL/New_patches with @ridz1208. Should I make an issue?

ridz1208 · 2021-06-28T18:14:49Z

@pierre-p-s let's investigate that together for another 10 minutes before doing that.

ridz1208 · 2021-08-05T18:10:15Z

@zaliqarosli waiting on your testing

zaliqarosli · 2021-08-12T17:41:21Z

testing on QPN data:

instruments defined at the "Not Started" stage populated in the test battery table on instrument_list page i.e. not affected by changes in this PR
instrument ordering and default ordering by Full Name if instr_order field is NULL works as described in this PR
survey instruments (not defined in test_battery) show up on instrument_list page

failed:

I am able to set Session BVL QC status to 'Complete' even if instruments are 'In progress'. PR description suggests I shouldn't be able to do this. @ridz1208 is that right?

ridz1208 · 2021-08-12T19:13:07Z

@zaliqarosli testing ITEM 4 involves making sure you can only toggle the VISIT to pass or fail when all instruments are set to complete. Not BVL QC status nor BVL QC type if that's the only issue you found it means this passes tests

zaliqarosli

passes testing

ridz1208 · 2021-08-12T19:42:12Z

@driusan all urs

ridz1208 assigned zaliqarosli, sruthymathew123 and jesscall Dec 3, 2020

ridz1208 force-pushed the battery_in_the_wrong_place branch from 439aa19 to 4c6b64b Compare December 3, 2020 21:01

ridz1208 mentioned this pull request Dec 22, 2020

[23.0][battery_manager] Issue with test battery assignnment for duplicate tests #7130

Closed

ridz1208 commented Dec 23, 2020

View reviewed changes

ridz1208 added the State: Needs work PR awaiting additional work by the author to proceed label Dec 23, 2020

zaliqarosli added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jan 11, 2021

ridz1208 removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) State: Needs work PR awaiting additional work by the author to proceed labels Jan 20, 2021

pierre-p-s reviewed Feb 15, 2021

View reviewed changes

cmadjar assigned ridz1208 and unassigned sruthymathew123, jesscall and zaliqarosli Jun 8, 2021

ridz1208 added 4 commits June 15, 2021 08:19

[instrument_list/battery] Clean-up battery retrieval function

8a69147

changelog

f2cd8cf

missing session ID

d47c750

first commit in a long loooong time

ad121d0

ridz1208 force-pushed the battery_in_the_wrong_place branch from 7acf8a7 to ad121d0 Compare June 15, 2021 12:35

pierre-p-s added the Passed manual tests PR has been successfully tested by at least one peer label Jun 28, 2021

cmadjar unassigned ridz1208 Aug 10, 2021

cmadjar assigned zaliqarosli Aug 10, 2021

zaliqarosli approved these changes Aug 12, 2021

View reviewed changes

driusan merged commit 697f13d into aces:main Aug 26, 2021

ridz1208 added this to the 24.0.0 milestone Nov 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ridz1208 commented Dec 3, 2020

ridz1208 Dec 23, 2020

zaliqarosli Feb 11, 2021

ridz1208 Feb 11, 2021

ridz1208 commented Jan 20, 2021

zaliqarosli commented Feb 11, 2021 •

edited

Loading

ridz1208 commented Feb 11, 2021

pierre-p-s left a comment

pierre-p-s Feb 15, 2021

ridz1208 Jun 15, 2021

pierre-p-s Feb 15, 2021

cmadjar commented Jun 8, 2021 •

edited

Loading

ridz1208 commented Jun 15, 2021

pierre-p-s commented Jun 28, 2021

ridz1208 commented Jun 28, 2021

ridz1208 commented Aug 5, 2021

zaliqarosli commented Aug 12, 2021

ridz1208 commented Aug 12, 2021

zaliqarosli left a comment

ridz1208 commented Aug 12, 2021

		AND (s.SubprojectID=b.SubprojectID OR b.SubprojectID IS NULL)
		AND (s.Visit_label=b.Visit_label OR b.Visit_label IS NULL)

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

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

Conversation

ridz1208 commented Dec 3, 2020

Brief summary of changes

Testing instructions (if applicable)

ridz1208 Dec 23, 2020

Choose a reason for hiding this comment

zaliqarosli Feb 11, 2021

Choose a reason for hiding this comment

ridz1208 Feb 11, 2021

Choose a reason for hiding this comment

ridz1208 commented Jan 20, 2021

zaliqarosli commented Feb 11, 2021 • edited Loading

ridz1208 commented Feb 11, 2021

pierre-p-s left a comment

Choose a reason for hiding this comment

pierre-p-s Feb 15, 2021

Choose a reason for hiding this comment

ridz1208 Jun 15, 2021

Choose a reason for hiding this comment

pierre-p-s Feb 15, 2021

Choose a reason for hiding this comment

cmadjar commented Jun 8, 2021 • edited Loading

ridz1208 commented Jun 15, 2021

pierre-p-s commented Jun 28, 2021

ridz1208 commented Jun 28, 2021

ridz1208 commented Aug 5, 2021

zaliqarosli commented Aug 12, 2021

ridz1208 commented Aug 12, 2021

zaliqarosli left a comment

Choose a reason for hiding this comment

ridz1208 commented Aug 12, 2021

zaliqarosli commented Feb 11, 2021 •

edited

Loading

cmadjar commented Jun 8, 2021 •

edited

Loading