Skip to content

Conversation

@labkey-jeckels
Copy link
Contributor

Rationale

22.7 got stricter related to metadata on extensible tables. LSID columns should be considered selectable, though they don't need to shown in any user-facing views.

Related Pull Requests

Changes

  • Stop declaring that LSID is unselectable on a handful of ehr tables

@labkey-jeckels labkey-jeckels merged commit 458a1fe into release22.7-SNAPSHOT Nov 22, 2022
@labkey-jeckels labkey-jeckels deleted the 22.7_fb_46761_ehrExtensibleTables branch November 22, 2022 02:02
@bbimber
Copy link
Collaborator

bbimber commented Nov 22, 2022

@labkey-jeckels if I understand this, could this be quite a bit bigger in scope than just here? why did this change? The original reason for making these unselectable (and it was a decision years ago), is that it makes basically no sense to show the actual wrapped column in the table and it's confusing to the user for it to be an option.

@labkey-jeckels
Copy link
Contributor Author

@bbimber See the related PR for more background on the change.

This change touched the "extensible" core EHR tables - project, protocol, snomed_tags, and protocol_counts. It didn't affect the LSID column on dataset tables, as they weren't causing problems. And those are the only place I can find in the code that wrap the LSID column as opposed to others, like the project or protocol columns.

I agree that we want to preserve the behavior of not letting users choose the wrapped column directly for some of these lookups. However, I'd also argue that the right behavior for the LSID column is to allow it to be selected (though not shown by default), and have the wrapped columns be considered unselectable.

@bbimber
Copy link
Collaborator

bbimber commented Nov 22, 2022

Hi @labkey-jeckels, yeah, i got the dataset LSID vs. extensible table part. I just compared 22.11 vs 22.7 and this seems OK. I didnt remember how the columns defined in the extensible table appeared. They appear top-level like normal columns (as opposed to being a lookup through LSID), so I agree this change doesnt make that big a difference.

You might remember that EHR's datasets have a bunch of other wrapped column-based lookups, and these appear in the customize view UI as the wrapped column with a lookup tree to the target table. That style UI is where it frequently makes sense to keep the wrapped column as unselectable; however, that doesnt apply here.

Anyway, thanks for the info - this seems fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants