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.

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

[NDB_BVL_Instrument_LINST] Add Metadata Fields after adding to dictionary #9667

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

skarya22
Copy link
Contributor

@skarya22 skarya22 commented Mar 4, 2025

Brief summary of changes

  • raisinbread BMI instrument was not opening due to a change I made in [NDB_BVL_Instrument] Hide inactive examiners for site from selection, unless if already selected #9416 where getExaminerFields in NDB_BVL_Instrument now relied on data being present in the instrument's dictionary.
  • Unfortunately, when loading a LINST instrument with Direct Entry enabled, it would first run _addMetadataFields, and then add metadata fields to the dictionary
  • _addMetadataFields would then call getExaminerFields causing it to fail, since getExaminerFields relies on the dictionary having metadata, but the metadata is added after _addMetadataFields
  • The change has been made so _addMetadataFields is run after metadata is entered in the instance's dictionary. Further, metadata fields are only added to the instances dictionary if direct entry is not enabled. Otherwise they were added to the dictionary when they did not exist.

WHEN REVIEWING THIS PR, PRESS THE GEAR ICON, THEN HIDE WHITESPACE

Testing instructions (if applicable)

  1. Try to open the raisinbread LINST instrument
  2. See that it opens
  3. Try setting direct entry to true, see that it still opens

Link(s) to related issue(s)

@skarya22 skarya22 requested a review from adamdaudrich March 4, 2025 20:35
@skarya22 skarya22 added 27.0.0 - Bugs Bugs Found in LORIS 27 testing Priority: High PR or issue should be prioritised over others for review and testing Module: instruments PR or issue related to instruments module labels Mar 4, 2025
Copy link
Collaborator

@adamdaudrich adamdaudrich left a comment

Choose a reason for hiding this comment

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

This was complicated !
but - Yes it makes sense to call _addMetaDataFields after building the dictionary !
However, I am getting an error, as follows :

[Tue Mar 04 19:31:14.362177 2025] [php:error] [pid 15578] [client 192.168.122.1:56428] PHP Fatal error: Uncaught TypeError: NDB_BVL_InstrumentStatus_ControlPanel::__construct(): Argument #2 ($testname) must be of type string, null given, called in /var/www/loris/php/libraries/NDB_BVL_Instrument.class.inc on line 2797 and defined in /var/www/loris/php/libraries/NDB_BVL_InstrumentStatus_ControlPanel.class.inc:57\nStack trace:\n#0 /var/www/loris/php/libraries/NDB_BVL_Instrument.class.inc(2797): NDB_BVL_InstrumentStatus_ControlPanel->__construct()\n#1 /var/www/loris/src/Middleware/UserPageDecorationMiddleware.php(266): NDB_BVL_Instrument->getControlPanel()\n#2 /var/www/loris/src/Middleware/PageDecorationMiddleware.php(59): LORIS\\Middleware\\UserPageDecorationMiddleware->process()\n#3 /var/www/loris/php/libraries/NDB_Page.class.inc(731): LORIS\\Middleware\\PageDecorationMiddleware->process()\n#4 /var/www/loris/php/libraries/NDB_BVL_Instrument.class.inc(3236): NDB_Page->process()\n#5 /var/www/loris/modules/instruments/php/module.class.inc(97): NDB_BVL_Instrument->process()\n#6 /var/www/loris/src/Middleware/ResponseGenerator.php(51): LORIS\\instruments\\Module->handle()\n#7 /var/www/loris/src/Middleware/AuthMiddleware.php(64): LORIS\\Middleware\\ResponseGenerator->process()\n#8 /var/www/loris/src/Router/ModuleRouter.php(75): LORIS\\Middleware\\AuthMiddleware->process()\n#9 /var/www/loris/src/Middleware/ExceptionHandlingMiddleware.php(55): LORIS\\Router\\ModuleRouter->handle()\n#10 /var/www/loris/src/Router/BaseRouter.php(138): LORIS\\Middleware\\ExceptionHandlingMiddleware->process()\n#11 /var/www/loris/src/Middleware/ResponseGenerator.php(51): LORIS\\Router\\BaseRouter->handle()\n#12 /var/www/loris/src/Middleware/ContentLength.php(53): LORIS\\Middleware\\ResponseGenerator->process()\n#13 /var/www/loris/htdocs/index.php(74): LORIS\\Middleware\\ContentLength->process()\n#14 {main}\n thrown in /var/www/loris/php/libraries/NDB_BVL_InstrumentStatus_ControlPanel.class.inc on line 57, referer: https://adaudrich-dev.loris.ca/instrument_list/?candID=300001&sessionID=1

]
);
} else {
if ($this->DataEntryType!=="DirectEntry") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if ($this->DataEntryType!=="DirectEntry") {
// The sequence of if statements below building the data dictionary is designed to
// mimic the same logic in the _addMetadataFields() function of
// NDB_BVL_Instrument to keep the dictionary in sync with the fields added
// to the form, make sure to modify it accordingly
if ($this->DataEntryType!=="DirectEntry") {

Copy link
Collaborator

Choose a reason for hiding this comment

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

@saagar, small suggestion. I would also suggest adding a similar statement at the top of metadata fields in case someone decides to modify that part and forgets this

Copy link

Choose a reason for hiding this comment

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

@ridz1208 -- think you have the wrong person!

@ridz1208
Copy link
Collaborator

ridz1208 commented Mar 5, 2025

@skarya22 I have not tested it but I will help Adam give it a test. For the changes themselves, I think its good enough for a bugfix. I do think a better fix is warranted for the next release and it could come with a slight refactor of some of the convoluted logic in this class.

@skarya22 skarya22 added this to the 27.0.0 milestone Mar 5, 2025
@skarya22
Copy link
Contributor Author

skarya22 commented Mar 5, 2025

@adamdaudrich Can you let me know what you were trying to do when you received that error so I can recreate it? I can't find that error

@adamdaudrich
Copy link
Collaborator

@adamdaudrich Can you let me know what you were trying to do when you received that error so I can recreate it? I can't find that error

I got the controlPanel() error when loading bmi.linst. It essentially means control panel is not getting a testname (type: string).

@skarya22
Copy link
Contributor Author

skarya22 commented Mar 6, 2025

@adamdaudrich that's strange, I am able to open the bmi instrument from candidate profile with no errors :/

I opened https://my-vm-dev.loris.ca/instruments/bmi/?commentID=300010MTL01010221465351037&sessionID=10&candID=300010

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
27.0.0 - Bugs Bugs Found in LORIS 27 testing Module: instruments PR or issue related to instruments module Priority: High PR or issue should be prioritised over others for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[raisinbread] Can't open bmi instrument
4 participants