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

LORIS instrument fields naming rules clash with REDCap import #9526

Open
maximemulder opened this issue Jan 8, 2025 · 2 comments
Open

LORIS instrument fields naming rules clash with REDCap import #9526

maximemulder opened this issue Jan 8, 2025 · 2 comments
Labels
Area: Instruments PR or issue related instruments Category: Bug PR or issue that aims to report or fix a bug

Comments

@maximemulder
Copy link
Contributor

maximemulder commented Jan 8, 2025

While working on the REDCap module, I encountered a bug where two fields of a REDCap instrument imported in LORIS would be present in the generated LINST file but absent in the NDB_BVL_Instrument_LINST data dictionary: work_status and marital_status.

This bug seems to come from these lines, which basically ignore any field with _status in its name:

if (strpos($fieldname, "_status") !== false) {
continue;
}

This is annoying as we would prefer to not have this kind of naming restrictions for the REDCap instruments we wish to import. From a quick glance at the code, there are probably other LINST naming requirements that need to be addressed in the REDCap module.

Here is my quick guess at the potential solutions:

Solution Implementation User experience
Check and forbid special names during the REDCap instrument import Easy Bad
Escape special names in the REDCap-imported instruments Medium Medium
Create a new LINST-agnostic type of instrument for REDCap imported instruments Hard Good
Refactor LINST to remove special names Hard Good

There may be other solutions I have not thought of. The last two are not really feasible on the short term.

These naming requirements might also affect other instrument-related features that do not handle them ?

@maximemulder
Copy link
Contributor Author

Pinging @driusan @regisoc for opinions.

@maximemulder maximemulder added Area: Instruments PR or issue related instruments Category: Bug PR or issue that aims to report or fix a bug labels Jan 8, 2025
@driusan
Copy link
Collaborator

driusan commented Jan 9, 2025

Any requirements from LINST are inherited from NDB_BVL_Instrument. (The format is effectively the same as running lorisform_parser on a PHP instrument.)

addMetaDataFields() fields are ignored because they get automatically re-added by LORIS. _status is special because the addElement wrappers add them back in a group. NDB_BVL_Instrument (whether PHP or LINST) expect X_status to be a "not answered" field for X to explicitly indicate the question was not answered. NDB_BVL_Instrument (the PHP base class that LINST extends) does some stuff on save where it clears out X if X_status is set. Simply removing the statement referenced would likely result in either data loss or SQL errors, not just graphical layout problems. I'm not sure if XIN rules has any implications..

Note that these come from NDB_BVL_Instrument, not NDB_BVL_Instrument_LINST. A different instrument type would also extend the same class and inherit the same assumptions and restrictions and need similar workarounds to what LINST has. As a result, I think that one of the first 2 options is the only short term solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Instruments PR or issue related instruments Category: Bug PR or issue that aims to report or fix a bug
Projects
None yet
Development

No branches or pull requests

2 participants