[BI-2778] Deprecate additionalinfo usage for level and use obs unit lvl#500
[BI-2778] Deprecate additionalinfo usage for level and use obs unit lvl#500
Conversation
HMS17
requested review from
a team,
davedrp and
jloux-brapi
and removed request for
a team and
davedrp
| if (isSubEntityDataset(ous)) { | ||
| Comparator<BrAPIObservationUnit> subUnitComparator = Comparator.comparing(BrAPIObservationUnit::getObservationUnitName, new IntOrderComparator()); | ||
| Comparator<BrAPIObservationUnit> ouNameComparator = Comparator.comparing(row -> (row.getAdditionalInfo().get(BrAPIAdditionalInfoFields.EXP_UNIT_ID).toString()), new IntOrderComparator()); | ||
| Comparator<BrAPIObservationUnit> ouNameComparator = Comparator.comparing(row -> (getTopLevel(row).getLevelName()), new IntOrderComparator()); |
There was a problem hiding this comment.
Is it ok to swap the logic for exp unit id with level name?
There was a problem hiding this comment.
I do not think so, EXP_UNIT and EXP_UNIT_ID are two separate values. For the sort, we want to know the top level dataset observation unit associated with the sub entity unit, not just the level name.
There was a problem hiding this comment.
Then I'm confused, why is this comparing level name then instead of exp_unit_id. And if it is comparing level name, why is it using an IntOrderComparator?
There was a problem hiding this comment.
Ah, I don't know why for some reason I interpreted this as you suggesting a change rather than pointing out my change was invalid. Good catch, I'll fix that.
| row.put(topLvlName + " " + OBSERVATION_UNIT_ID_SUFFIX, topLvlOuId); | ||
| } | ||
| row.put(ExperimentObservation.Columns.EXP_UNIT_ID, ou.getAdditionalInfo().get(BrAPIAdditionalInfoFields.EXP_UNIT_ID).getAsString()); | ||
| row.put(ExperimentObservation.Columns.EXP_UNIT_ID, StringUtils.capitalize(getTopLevel(ou).getLevelName())); |
There was a problem hiding this comment.
Is it ok to swap the logic for exp unit id with level name?
There was a problem hiding this comment.
For EXP_UNIT column in a row we want level name. EXP_UNIT_ID column meanwhile is the id of the specific observation unit from the top level dataset.
There was a problem hiding this comment.
RIght, but this new commit is putting the level name in the exp unit id column is it not?
There was a problem hiding this comment.
Ah I misinterpreted, yeah good catch this is incorrect.
jloux-brapi
left a comment
jloux-brapi
left a comment
There was a problem hiding this comment.
Definitely some things going wrong with the testing, not quite sure what.
The changes I suggested seemed to introduce an NPE, when I reverted that, I am still noticing some issues with the UI I have never seen before.
For example, the sub entity data Obs Unit Id column says Undefined Obs Unit Id where it should say the level name.
2 participants
Description
Story: BI-2778 - Deprecate additionalinfo usage for level and use obs unit level
Removed calls to set observation level info in additionalInfo and replaced calls to get observation level info from additionalInfo with calls to the observation level associated with the observation object.
This should not be merged until after the migration of BI-2779 is merged.
Dependencies
bi-web: feature/BI-2778 (old)
bi-web: feature/BI-2778 (new)
Testing
Test areas related to experiments to ensure nothing is broken:
-- Upload experiment
-- Create sub entity dataset
-- View experiment
-- Download top level experiment dataset
-- Download sub entity experiment dataset
-- Download case mentioned in comments of card
Checklist: