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
DDIB should not output two subject columns. #2263
Conversation
Currently it outputs the original column and the translated column, this depends on subtle behavior in matchColumns() to not break things. Subtle is bad and we don't need both columns.
api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, I looked into something like this as well. I think there's one more issue though that needs to be resolved for this to work. If you try to import ParticipantId by it's label "Participant ID", like the FileWatcherDatasetTest, it will fail validation if that ParticipantId translate column is not always there. Which really seems like an unintentional validation. So not sure if we should be matching by column label as well, or if importing by column label should not be allowed and the test data just needs to be fixed.
I kicked off a bunch of tests on this branch. Here's the failing FileWatcher tests. So far that's all that's failing.
….java Co-authored-by: Adam Rauch <adam@labkey.com>
I think the test is wrong. This dataset has a column (name="ParticipantId" label="Participant Id") and a column (name ="Participant Id" and label="Participant Id"). Notice the space. The code correctly matches the input column "Participant Id" preferentially to the column named "Participant Id" rather than the one labeled "Participant Id". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Looks good.
Rationale
DatasetDataIteratorBuilder should not output two subject columns.
Currently it outputs the original column and the translated column, this depends on subtle behavior in matchColumns() to not break things. Subtle is bad and we don't need both columns.
Related Pull Requests
https://github.com/LabKey/premium/pull/246
Changes