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

New Data Dictionary framework and module #6936

Merged
merged 60 commits into from
Apr 22, 2021
Merged

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Aug 26, 2020

This updates the LORIS Data Framework to include a concept of a data dictionary that a module may manage. It adds a new "dictionary" module to manage the module dictionary but leaves the datadict module in place because it's currently the only place to manage the parameter_type table used by the DQT.

The extension of the data framework to include dictionaries also includes many enhancements from the current "data dictionary" in LORIS which only includes a field name and description. Types, Scopes, and Cardinalities are added which more thoroughly describe the data and will eventually be able to be used to build richer data-type sensitive interfaces in places like the DQT. DictionaryItems implement the AccessibleResource interface to enable modules to give column-level granularity over data.

Modules provide categories of DictionaryItems, which allows them to group items in the frontend. This is primarily to make it possible to filter the instruments from the "instruments" module (in the current data_dict module, the Source From options are literally populated by a call to \Utility::getAllInstruments()) in a manageable way, but can be used by any module that wants to group the fields.

Instruments written in PHP must now implement a new getDataDictionary() abstract function to describe the fields provided by the instrument, but a LorisFormDictionaryImpl is provided which populates it in the same way that the lorisform_parser uses it to populate the parameter_type table. The candidate_parameters and imaging_browser also implement dictionaries in this PR.

NB: I intend to try to break this PR into smaller, easier to review chunks, but it may be helpful to reviewers to have a PR with the entire dictionary overhaul for context.

@driusan driusan added Release: Add to release notes PR whose changes should be highlighted in the release notes Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects labels Aug 26, 2020
@driusan
Copy link
Collaborator Author

driusan commented Aug 26, 2020

@HenriRabalais can you have a look at this to see if it's possible to describe the biobank's data? The place to start would probably be the $module->getDataDictionary() function and some of the examples of modules that implement it.
@zaliqarosli can you look at it to see if it's missing anything (in particular types) wrt. the work you've done on instruments that would be needed?

driusan added a commit to driusan/Loris that referenced this pull request Aug 27, 2020
This adds the classes representing the data scope, data type,
data cardinality, DictionaryItem (and dictionary Category) from
PR aces#6936 but does not use yet use them.
driusan added a commit to driusan/Loris that referenced this pull request Aug 27, 2020
This implements the known types of types from PR aces#6936.
@driusan driusan mentioned this pull request Aug 27, 2020
@driusan
Copy link
Collaborator Author

driusan commented Aug 27, 2020

Blocked by #6938, #6939

@driusan driusan added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Aug 27, 2020
* @license http://www.gnu.org/licenses/gpl-3.0.txt GPLv3
*/

class Fields extends \NDB_Page
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for NDB_Page. implements RequestHandlerInetrface would be enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't see what the benefit of that would be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better cohesion and reduced coupling. If you only need the handle function, and maybe _hasAccess, implementing RequestHandlerInterface is enough. I don't see what $identifer and $commentid means for a datadictionnary field. $fieldoptions, $dateoption adds confusion. What is the use of AddFile, AddSelect, ...? Will it use its LorisForm?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be honest I'd like to remove $identifier and $commentid from the page class in general, but when I tried doing that in #7414 the scope of the change exploded so I held back to do it later.

I don't think having methods that are uncalled adds any overhead and the properties in the page class add minimal. I just had a look through the code and hasAccess does seem to be the main benefit. Other than that, I think there's benefit in having a root to the class hierarchy, but I can change this to manually call hasAccess if you want.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made #7431 to work through what would be ideal in general outside of this PR.

modules/dictionary/php/moduledict.class.inc Outdated Show resolved Hide resolved
modules/dictionary/php/moduledict.class.inc Show resolved Hide resolved
@zaliqarosli
Copy link
Contributor

@driusan can you point me to where i should be looking? this is a big PR and i don't know what it is exactly you want me to look at to see if something is missing. and what kind of 'types' are you referring to? thanks

@driusan
Copy link
Collaborator Author

driusan commented Sep 9, 2020

The src/Data directory parts of this PR which is separated out into PR #6939

@driusan driusan mentioned this pull request Nov 13, 2020
5 tasks
driusan added a commit to driusan/Loris that referenced this pull request Nov 26, 2020
This implements the known types of types from PR aces#6936.
@driusan driusan added the State: Needs work PR awaiting additional work by the author to proceed label Nov 26, 2020
@driusan
Copy link
Collaborator Author

driusan commented Nov 26, 2020

Multiselects should have multiple cardinality. (See comment on #6939)

driusan added a commit to driusan/Loris that referenced this pull request Dec 9, 2020
This adds the classes representing the data scope, data type,
data cardinality, DictionaryItem (and dictionary Category) from
PR aces#6936 but does not use yet use them.
driusan added a commit that referenced this pull request Dec 9, 2020
This adds the classes representing the data scope, data type,
data cardinality, DictionaryItem (and dictionary Category) from
PR #6936 but does not use yet use them.

It gives us the ability to describe data dictionaries in LORIS.
driusan added a commit to driusan/Loris that referenced this pull request Dec 9, 2020
…dule

This extracts the portion of aces#6936 which relates to candidate variables
in the `candidate_parameters` dictionary and puts it into a new PR
for reviewability now that the classes which it depends on have been
separately merged.

TODO:
[ ] Address comment about PSCID in aces#6936.
[ ] Project should be an enumeration based on project table
[ ] Subproject should be an enumeration based on subproject table
@driusan
Copy link
Collaborator Author

driusan commented Dec 9, 2020

Now blocked by #7176, #7177, #7179, and #7180

driusan added a commit to driusan/Loris that referenced this pull request Dec 10, 2020
…dule

This extracts the portion of aces#6936 which relates to candidate variables
in the `candidate_parameters` dictionary and puts it into a new PR
for reviewability now that the classes which it depends on have been
separately merged.

TODO:
[ ] Address comment about PSCID in aces#6936.
[ ] Project should be an enumeration based on project table
[ ] Subproject should be an enumeration based on subproject table
driusan added a commit to driusan/Loris that referenced this pull request Feb 12, 2021
…dule

This extracts the portion of aces#6936 which relates to candidate variables
in the `candidate_parameters` dictionary and puts it into a new PR
for reviewability now that the classes which it depends on have been
separately merged.

TODO:
[ ] Address comment about PSCID in aces#6936.
[ ] Project should be an enumeration based on project table
[ ] Subproject should be an enumeration based on subproject table
driusan added a commit that referenced this pull request Mar 12, 2021
…dule (#7176)

This extracts the portion of #6936 which relates to candidate variables
in the candidate_parameters dictionary and puts it into a new PR
for reviewability. It adds a data dictionary to the candidate parameters class.
@driusan driusan force-pushed the NewDataDictionary branch from 0f161e2 to 312c1ed Compare April 12, 2021 16:56
@driusan
Copy link
Collaborator Author

driusan commented Apr 12, 2021

@maltheism rebase done

@xlecours my previous comment seems to be hidden since rebasing.. could you update or dismiss your review? I think I've responded to everything in it.

@xlecours
Copy link
Contributor

xlecours commented Apr 22, 2021

@xlecours my previous comment seems to be hidden since rebasing.. could you update or dismiss your review? I think I've responded to everything in it.

@driusan I have added a comment there: #6936 (comment)
Yay or Nay? Just let me know, I'll approve or review the changes accordingly.

@driusan driusan removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Apr 22, 2021
@xlecours xlecours dismissed their stale review April 22, 2021 17:43

Suggestion made, discussed.

@driusan driusan merged commit 2fceb83 into aces:main Apr 22, 2021
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
This adds the classes representing the data scope, data type,
data cardinality, DictionaryItem (and dictionary Category) from
PR aces#6936 but does not use yet use them.

It gives us the ability to describe data dictionaries in LORIS.
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
…dule (aces#7176)

This extracts the portion of aces#6936 which relates to candidate variables
in the candidate_parameters dictionary and puts it into a new PR
for reviewability. It adds a data dictionary to the candidate parameters class.
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
This updates the LORIS Data Framework to include a concept of a data dictionary that a module may manage. It adds a new "dictionary" module to manage the module dictionary but leaves the datadict module in place because it's currently the only place to manage the parameter_type table used by the DQT.

The extension of the data framework to include dictionaries also includes many enhancements from the current "data dictionary" in LORIS which only includes a field name and description. Types, Scopes, and Cardinalities are added which more thoroughly describe the data and will eventually be able to be used to build richer data-type sensitive interfaces in places like the DQT. DictionaryItems implement the AccessibleResource interface to enable modules to give column-level granularity over data.

Modules provide categories of DictionaryItems, which allows them to group items in the frontend. This is primarily to make it possible to filter the instruments from the "instruments" module (in the current data_dict module, the Source From options are literally populated by a call to \Utility::getAllInstruments()) in a manageable way, but can be used by any module that wants to group the fields.

Instruments written in PHP must now implement a new getDataDictionary() abstract function to describe the fields provided by the instrument, but a LorisFormDictionaryImpl is provided which populates it in the same way that the lorisform_parser uses it to populate the parameter_type table.
@ridz1208 ridz1208 added this to the 24.0.0 milestone Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed manual tests PR has been successfully tested by at least one peer Release: Add to release notes PR whose changes should be highlighted in the release notes Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants