[DT-2542] NMDS fixes. #2764
[DT-2542] NMDS fixes. #2764
Conversation
|
otchet-broad
requested review from
fboulnois,
kevinmarete and
rushtong
and removed request for
a team
| consentGroup.setPoa(dataUse.getPopulationOriginsAncestry()); | ||
| consentGroup.setOtherPrimary(dataUse.getOther()); | ||
| consentGroup.setNmds(dataUse.getMethodsResearch()); | ||
| consentGroup.setNmds(Boolean.FALSE.equals(dataUse.getMethodsResearch()) ? true : null); |
There was a problem hiding this comment.
We invert the incoming value from true to false so it makes sense that we would need to flip it on the way out as well.
There was a problem hiding this comment.
(perhaps not in this PR) Since this has caused ongoing confusion, should we add/update/refactor this property so that we don't need to invert the values?
There was a problem hiding this comment.
@fboulnois - agree with your thoughts here and had similar ones while I was going through the code. I wanted to do the first implementation showing all of the places that were impacted by the original decisions to make sure I didn't miss anything and then come back and refactor this (and migrate the data in the database as well). I also wanted to catch up with @rushtong on this to see if we could better understand the history of why it was treated differently (and kept that way in his earlier refactor).
There was a problem hiding this comment.
A bit of history that might help inform a future decision:
- We originally asked researchers (we do not ask this any longer) if they would be doing methods development research. See the history on DataAccessRequestApplication.js
- This is the inverse of the DUO term, NMDS: https://github.com/EBISPOT/DUO
- To store those in a Data Use object, we capture these as "methodsResearch"
- We have internal business logic (see the V4 algorithm) in ontology that matches the research purpose on this field but does NOT take the corresponding NMDS dataset value into consideration (see code reference in DataUseMatchCasesV3.java).
- Since we don't collect this in the DAR, and it isn't evaluated on the Dataset for matching, I think this field is entirely moot.
There was a problem hiding this comment.
Maybe we fix this with this PR and add a ticket to go back to product to see if we can remove this from our codebase entirely?
| consentGroup.setPoa(dataUse.getPopulationOriginsAncestry()); | ||
| consentGroup.setOtherPrimary(dataUse.getOther()); | ||
| consentGroup.setNmds(dataUse.getMethodsResearch()); | ||
| consentGroup.setNmds(Boolean.FALSE.equals(dataUse.getMethodsResearch()) ? true : null); |
There was a problem hiding this comment.
(perhaps not in this PR) Since this has caused ongoing confusion, should we add/update/refactor this property so that we don't need to invert the values?
rushtong
left a comment
rushtong
left a comment
There was a problem hiding this comment.
This is a confusing field, thanks for tracing this through 👍🏽
Addresses
https://broadworkbench.atlassian.net/browse/DT-2542
Summary
Have you read CONTRIBUTING.md lately? If not, do that first.