Conversation
regisoc
added
Category: Feature
github-actions
bot
added
the
RaisinBread
MaximeBICMTL
left a comment
MaximeBICMTL
left a comment
There was a problem hiding this comment.
I know this is just a work-in-progress, but I did a small review notably of the Javascript side.
I know this requires some work, but I would heavily prefer to use Typescript and function components for the UI, untyped JS and class components are a lot harder to maintain in my opinion and I would rather not have them in newer code.
This is my main concern, outside of that, all the nits do not need to be addressed, those are just me stating my opinions on some pieces of code.
modules/redcap/jsx/redcapIndex.js
Outdated
| baseURL={loris.BaseURL} | ||
| moduleURL={loris.BaseURL + '/redcap'} | ||
| hasPermission={loris.userHasPermission} |
There was a problem hiding this comment.
I am not sure about passing these values as props to the module.
- The first two can be obtained from the
lorisinstance, no need to use props IMO. - The latter one is not used right now (but I guess that may change).
| baseURL: props.baseURL, | ||
| data: {}, | ||
| error: false, | ||
| isLoaded: false, |
There was a problem hiding this comment.
Maybe just use data !== null instead of isLoaded ?
|
|
||
| return ( | ||
| <div> | ||
| {this.state.data == null |
There was a problem hiding this comment.
I don't think this.state.data can ever be null, it is initialized with an empty object {} or a JSON received from the back-end. I would also avoid loose equality in our code (use === instead of ==).
| super(props); | ||
|
|
||
| this.state = { | ||
| baseURL: props.baseURL, |
There was a problem hiding this comment.
I would avoid putting constant data in a component state, I would rather use loris.BaseURL directly.
| this.state = { | ||
| baseURL: props.baseURL, | ||
| moduleURL: props.moduleURL, | ||
| data: {}, | ||
| error: false, | ||
| isLoaded: false, | ||
| }; |
There was a problem hiding this comment.
Same comments about constant data in state and isLoaded.
| * @return {*} a formatted table cell for a given column | ||
| */ | ||
| formatColumn(column, cell, rowData, rowHeaders) { | ||
| let result = (<td>{cell}</td>); |
There was a problem hiding this comment.
I would use return directly in the matchswitch instead of putting things in result, using break, and then returning.
| // The fields configured for display/hide. | ||
| let fields = [ |
There was a problem hiding this comment.
Same comment about constant data.
|
@MaximeBICMTL only requesting another review, but we already talked about the JS/TS update to be done later on. |
|
@regisoc Just saw your review request, will look into that PR and test it whenever I have the time (will be done this month). |
MaximeBICMTL
left a comment
MaximeBICMTL
left a comment
There was a problem hiding this comment.
I found some bugs during testing (during the dictionary import part in particular), see PR regisoc#1 for the bug fixes that made it work for me.
Once the fixes are in I'll approve this PR.
2 participants
Brief summary of changes
This PR adds a UI for designated users with new permission
redcap_ui_view.The UI is mainly targeted at people managing REDCap data integration, and adapts to one or multiple connected REDCap instances and projects.
The current UI composed by 3 tabs (more can be added) to give information and help navigate, this is particularly useful for issues arising when importing REDCap data e.g. missing notifications, network issues, out-of-sync instruments dictionary.
Images of current included tabs
Notifications received:
REDCap Dictionary in use:
REDCap import issues in issue tracker:
This PR also updates the
redcap2linst:module/redcap/toolsas a module specific tool.redcap_dictionarytable update when parsing the redcap dictionary.To discuss:
Testing instructions (if applicable)
redcapmodule in modules manager.Tools > REDCapmenu item.