Loris icon indicating copy to clipboard operation
Loris copied to clipboard
verified publisher icon aces

[SQL][Instrument List][Battery Manager] Move DDE Enabled to Test_Battery

Open skarya22 opened this issue 1 year ago • 12 comments

Brief summary of changes

  • Changed the logic for DDE being enabled to be moved from Config to Test_Battery so that it can be managed on a visit-instrument basis instead of just instrument.
  • Modified Battery Manager to have a section for editing which battery has DDE enabled
  • Modifies Raisinbread to match the sql changes
image

Testing instructions (if applicable)

  1. Confirm that the sql patch copies over the selected DDE instruments to the test_battery field properly
  2. Test all affected tools and confirm they still work as intended
  3. Confirm that the correct tests show the DDE option in instrument list for the visit-instrument tuple as defined in test_battery

skarya22 avatar May 14 '24 17:05 skarya22

@driusan removing from release unless you wanted it for 26 ?

ridz1208 avatar May 28 '24 16:05 ridz1208

No, I thought I had already removed it from the release.

driusan avatar Jun 03 '24 15:06 driusan

Move from Instrument_Manager to Battery_Manager

skarya22 avatar Jul 30 '24 15:07 skarya22

That stack trace has a project/libraries/NDB_BVL_Battery.class.inc file in it. Are you sure you don't have overrides that are breaking @skarya22's change?

driusan avatar Sep 04 '24 17:09 driusan

@shonibare from the stack trace it looks like the problem is that there is likely an override like @driusan mentioned in NDB_BVL_Battery.class.inc, specifically here: image

$config->getSetting("DoubleDataEntryInstruments") should no longer be present in the function, but the stack trace has "Config setting DoubleDataEntryInstruments does not exist in database"

skarya22 avatar Sep 04 '24 19:09 skarya22

@skarya22 could you take a look at this function https://github.com/aces/Loris/blob/b5e10cabd89fa6efed76a0b529f446a414c9b641/php/libraries/NDB_Config.class.inc#L430

shonibare avatar Sep 04 '24 19:09 shonibare

@shonibare That function is alright, other parts of LORIS need it, however it should not be called with "DoubleDataEntryInstruments" as the name since that was removed from Config

skarya22 avatar Sep 05 '24 13:09 skarya22

@shonibare That function is alright, other parts of LORIS need it, however it should not be called with "DoubleDataEntryInstruments" as the name since that was removed from Config

@skarya22 is it not breaking on your VM? cos I did fetch your PR and ran the patch. I also searched for this DoubleDataEntryInstruments in the files in your PR in my VM but didn't find it.

shonibare avatar Sep 05 '24 14:09 shonibare

Yeah it works for me image

skarya22 avatar Sep 05 '24 15:09 skarya22

Confirmed that this is working when I check out Saagar's PR, run the patch, and compile: image

I also tested the other relevant modules and those all loaded also

CamilleBeau avatar Sep 19 '24 15:09 CamilleBeau

@driusan I think I made the necessary adjustments. I could not find anything in the swagger documentation to update however. 0.0.4-dev: image

0.0.3: image

skarya22 avatar Sep 24 '24 16:09 skarya22

@skarya22 can you rebase this? There is a conflict

driusan avatar Oct 15 '24 15:10 driusan

@skarya22 There are still conflicts that prevent this from being merged

driusan avatar Oct 29 '24 17:10 driusan

@driusan sorry everything has conflicts after the lint changes this morning

skarya22 avatar Oct 29 '24 19:10 skarya22