Broken lookups in EHR forms #958
Conversation
|
This scenario sounds very familiar. FWIW, I think this situation is already handled in Ext4 forms, or at least anything that inherits from LDK's Ext4 FormPanel, by these lines: UserEditableCombo has code to ensure an incoming value is present in the store or otherwise added. The "allowChooseOther: false" basically means it will accept incoming data but the user cannot see the 'Other' option. Given you are hitting this I assume Grids dont have a similar check? Could this be accomplished more simply by performing a similar check here: Or perhaps more appropriately into something like LABKEY.ext4.Util.getGridEditorConfig(), or EHR's method that calls that? |
This reverts commit 29fe97b.
|
hi @labkey-martyp: two comments: When you call ensureLookups, I dont think this considers whether any of the stores have loaded, right? Especially if the combo's store hasnt loaded, immediately querying whether this record exists or not could result in inappropriately adding a value, I believe. Async issues are less likely on a dev machine with small data, but that is not hard to get with real data. I don't remember all the ins and outs of when Ext4 grids create the editor, and I am pretty sure there is code in EHR forms to proactively identify and load copies of the lookup stores; however, this problem is still possible. This is one of the reasons why feeding into UserEditableCombo is highly preferable, IMO, since that is tested and should handle all of this. Why not do something exactly like the link I posted above for FormPanel: https://github.com/bimberlabinternal/LabDevKitModules/blob/8af7221e79bdd0291c74e4f7be4921f0eaf97714/LDK/resources/web/LDK/form/Panel.js#L18? Just add UserEditableCombo all the time for combos, with allowChooseOther=false. As as I look at this, that code should probably check whether item.plugins already has a UserEditableCombo and only add it if not present. Adding the plugin again when it exists probably wouldnt break anything, but it's also unnecessary. If showing brackets around unknown values is mission critical (I agree it's a reasonable feature), then I think a simple and non-invasive approach could be to build that logic directly into LABKEY.Ext4.Combo's innerTpl. We could support the property "_isUnknownLookup" on the record. If that's true, that innerTpl would show brackets. UserEditableCombo could set this property here: https://github.com/bimberlabinternal/LabDevKitModules/blob/8af7221e79bdd0291c74e4f7be4921f0eaf97714/LDK/resources/web/LDK/plugin/UserEditableCombo.js#L84 |
Ok, I updated to add the UserEditableCombo by default to these fields with the "other" selection turned off. I'll continue to do some testing to make sure that doesn't have unintended consequences. Showing the bracket is not mission critical and it would be challenging to handle all the possible tpls in the LK core extjs components and specific PRC overrides, as well as where brackets are already being added to the grid values in renderers. |
| // const xtype = editor?.xtype; | ||
| const xtype = editor?.xtype; | ||
| if ((xtype === 'combo' || xtype === 'labkey-combo' || xtype === 'ehr-simplecombo') | ||
| // && !editor.plugins?.find(p => p.ptype === 'ldk-usereditablecombo')) { |
labkey-bpatel
left a comment
labkey-bpatel
left a comment
There was a problem hiding this comment.
Had looked at this PR before going on leave but didn't get a chance to approve it.
Rationale
This improves broken lookup handling when updating existing tasks in EHR forms. Currently there are cases where a broken lookup, when updating an existing task, will disappear when clicked on. There is then no way to get it back without refreshing the form. This could lead to confusion or even data loss.
This update changes any combo types with broken lookups to the UserEditableCombo type, as that handles this use case better ensuring the broken lookup in the grid does not disappear when clicked and is in the dropdown list of the combo.
Changes