Skip to content

Add brackets in UserEditableCombo dropdowns around broken lookups #247

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

Closed
wants to merge 4 commits into from

Conversation

labkey-martyp
Copy link

Rationale

UserEditableCombo is now the default combo input if there are broken lookups. This adds a tpl to put the broken lookups in the dropdown in brackets.

Related Pull Requests

Changes

  • Add tpl to dropdown

@bbimber
Copy link
Collaborator

bbimber commented May 27, 2025

@labkey-martyp: can you please make this an opt-in feature with a config option like 'useBracketsForUnknownValues: false" or something to that effect, and turn this on the in ~2 places in EHR that rely on this feature? There are valid use-cases where users are expected to add values and this is going to change those.

Note: the LabKey query XML already supports a config option for lookups called "useRawValue". That was added a while after UserEditableCombo was created. One intent of this was to allow a sort of "soft" lookup, where the field has a lookup configured, but when rendering the raw value is used (i.e., no brackets for broken lookups). A more invasive but logical extension of what you're doing here would be to read this property in the client-side code that translates from the query metadata into Ext4 field config.

@bbimber
Copy link
Collaborator

bbimber commented May 27, 2025

thanks!

@bbimber
Copy link
Collaborator

bbimber commented May 27, 2025

hi @labkey-martyp: I believe EHR frequently uses LABKEY.ext4.ComboBox, right? Or does it inherit through LDK? If it's using the former, have you considered how this tpl override would interact with this?

https://github.com/LabKey/platform/blob/309189f08e712deeca426d744f3dbff30eae9e39/core/webapp/extWidgets/LabkeyCombo.js#L39

@labkey-martyp
Copy link
Author

Ok yeah the brackets should be opt-in now.

I'm not familiar with the soft link version of useRawValue. I've only seen it in the context of using the fk value instead of default display column. It was my understanding it still indicates a broken lookup if the value was not in the fk table.

@labkey-martyp
Copy link
Author

hi @labkey-martyp: I believe EHR frequently uses LABKEY.ext4.ComboBox, right? Or does it inherit through LDK? If it's using the former, have you considered how this tpl override would interact with this?

https://github.com/LabKey/platform/blob/309189f08e712deeca426d744f3dbff30eae9e39/core/webapp/extWidgets/LabkeyCombo.js#L39

I'll check it out

@bbimber
Copy link
Collaborator

bbimber commented May 27, 2025

@labkey-martyp: I cant speak to every center, but the fact that UserEditableCombo exists implies there are cases where there is a lookup defined, but that it's acceptable for users to enter another value. That is what i'm calling a "soft" lookup here.

Like you point out, useRawValue=true causes grids to render without brackets when there are broken lookups, which is highly similar to what is happening on the client here. I dont think this is an important enough different to trigger a lot of new development here, but if the relevant EHR/LDK code ever gets touched, reading and interpreting useRawValue=true would make some sense.

@labkey-martyp
Copy link
Author

hi @labkey-martyp: I believe EHR frequently uses LABKEY.ext4.ComboBox, right? Or does it inherit through LDK? If it's using the former, have you considered how this tpl override would interact with this?

https://github.com/LabKey/platform/blob/309189f08e712deeca426d744f3dbff30eae9e39/core/webapp/extWidgets/LabkeyCombo.js#L39

Yeah you're right. The innerTpl is getting overwritten and it's virtually impossible between the core LK extjs component and EHR customizations how to handle the innerTpl.

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.

2 participants