O3-4931: Add ability to evaluate expressions in extensions #1422
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Size Change: -163 kB (-2.49%) Total Size: 6.4 MB
ℹ️ View Unchanged
|
CynthiaKamau
force-pushed
the
extension-expressions
branch
from
a10ab77 to
fb07729
Compare
Contributor
|
Thank you so much @CynthiaKamau for working on this!! Sorry you have been waiting for weeks for a review - Ian has been super swamped with some very urgent complex projects. Thank you for your patience and again, thank you SO MUCH for contributing this in follow up to Ian's suggestions here. I made a ticket at https://openmrs.atlassian.net/browse/O3-4931 to reflect/track this work plan so it's not just hanging out in the heap of slack :P While we await a review, I have a few questions, please forgive me as I think they're basic, but this will help me understand and communicate this exciting work more widely.
Thank you again!!!! I'm super impressed by your turnaround time in response to Ian's suggestions on Slack 🙏 CCing @denniskigen because I think he'll want to be aware of this important work, and @NethmiRodrigo because this directly relates to Clinical Views (IIUC) which she's been working on / thinking about a lot lately, and we'll want to use this in our HIV Package support work for DRC. |
gracepotma
changed the title
Contributor
|
Per Ian's direct request, assigning this to @samuelmale as the primary reviewer. |
ibacher
reviewed
Aug 1, 2025
Member
ibacher
left a comment
ibacher
left a comment
There was a problem hiding this comment.
Thanks @CynthiaKamau! I think this PR as-written does much more than it should. What we should be adding here is a core building block for functionalities that can be properly leveraged by supplying data at sensible points in the application. Additionally, I've had a re-think about how this should be implemented. See my rather long comment on useAssignExtensionExpressionContext().
| if (!evaluateAsBoolean(displayConditionExpression, { session })) { | ||
| // Get patient UUID from URL and add to evaluation context | ||
| const patientUuid = getPatientUuidFromUrl(); | ||
| let evaluationContext: any = { ...extensionExpressionContext, session }; |
Member
There was a problem hiding this comment.
Suggested change
| let evaluationContext: any = { ...extensionExpressionContext, session }; | |
| let evaluationContext = { ...extensionExpressionContext, session }; |
Comment on lines
95
to
97
| export interface ExtensionExpressionContextStore { | ||
| [key: string]: any; | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| export interface ExtensionExpressionContextStore { | |
| [key: string]: any; | |
| } | |
| export type ExtensionExpressionContextStore = Record<string | symbol | number, unknown>; |
Comment on lines
165
to
250
| // Automatic patient context management | ||
| let currentPatientUuid: string | null = null; | ||
| let patientDataCache: { [uuid: string]: any } = {}; | ||
|
|
||
| async function fetchPatientData(patientUuid: string) { | ||
| // Check cache first | ||
| if (patientDataCache[patientUuid]) { | ||
| return patientDataCache[patientUuid]; | ||
| } | ||
|
|
||
| try { | ||
| const response = await openmrsFetch(`/ws/rest/v1/patient/${patientUuid}`); | ||
| const patientData = response.data; | ||
|
|
||
| // Cache the result | ||
| patientDataCache[patientUuid] = patientData; | ||
|
|
||
| return patientData; | ||
| } catch (error) { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| function updatePatientContext() { | ||
| const patientUuid = getPatientUuidFromUrl(); | ||
|
|
||
| if (patientUuid !== currentPatientUuid) { | ||
| currentPatientUuid = patientUuid; | ||
|
|
||
| if (patientUuid) { | ||
| // Fetch patient data and update context | ||
| fetchPatientData(patientUuid).then((patientData) => { | ||
| // Only update if we're still on the same patient page | ||
| if (patientData && currentPatientUuid === patientUuid) { | ||
| const currentContext = extensionExpressionContextStore.getState(); | ||
| const newContext: any = { | ||
| ...currentContext, | ||
| patient: patientData, | ||
| patientUuid: patientUuid, | ||
| }; | ||
|
|
||
| extensionExpressionContextStore.setState(newContext); | ||
| } | ||
| }); | ||
| } else { | ||
| // Clear patient context when not on patient page | ||
| const currentContext = extensionExpressionContextStore.getState(); | ||
| const newContext = { ...currentContext }; | ||
| delete newContext.patient; | ||
| delete newContext.patientUuid; | ||
| extensionExpressionContextStore.setState(newContext); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Update patient context on URL changes | ||
| function handleUrlChange() { | ||
| updatePatientContext(); | ||
| updateOutputStoreToCurrent(); | ||
| } | ||
|
|
||
| // Listen for URL changes | ||
| window.addEventListener('popstate', handleUrlChange); | ||
|
|
||
| // Override pushState and replaceState to detect programmatic navigation | ||
| const originalPushState = history.pushState; | ||
| const originalReplaceState = history.replaceState; | ||
|
|
||
| history.pushState = function (...args) { | ||
| originalPushState.apply(history, args); | ||
| setTimeout(handleUrlChange, 0); | ||
| }; | ||
|
|
||
| history.replaceState = function (...args) { | ||
| originalReplaceState.apply(history, args); | ||
| setTimeout(handleUrlChange, 0); | ||
| }; | ||
|
|
||
| // Initial setup | ||
| updatePatientContext(); | ||
|
|
||
| function getPatientUuidFromUrl() { | ||
| const match = /\/patient\/([a-zA-Z0-9\-]+)\/?/.exec(location.pathname); | ||
| return match && match[1]; | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
A few things on this section of code:
- Automatic patient context is not a concern that should be handled at the level of the esm-extensions. esm-extensions should be left quite generic. It definitely shouldn't be monitoring the URL for changes or trying to parse the patient from that context. Among other things, that URL schema only works in the context of the patient chart, but this would make it something that takes up processing cycles in every context. Instead all of this logic should be in the patient-chart-app.
- Because of the way the patient chart works, we currently generally pump the correct patient into the extension context using the
stateproperty. Maybe we can devise a
| order?: Array<string>; | ||
| configure?: ExtensionSlotConfigureValueObject; | ||
| /** Expression to determine whether this extension slot should be displayed */ | ||
| displayExpression?: string; |
Member
There was a problem hiding this comment.
This doesn't seem like it should be a property of extension slots. Code rendering extension slots can already make context-specific decisions to render them or not. Instead this should only apply to extensions.
| import { isOnline as isOnlineFn } from '@openmrs/esm-utils'; | ||
| import { isEqual, merge } from 'lodash-es'; | ||
| import { checkStatusFor } from './helpers'; | ||
|
|
| let evaluationContext: any = { ...extensionExpressionContext, session }; | ||
|
|
||
| if (patientUuid) { | ||
| evaluationContext = { ...extensionExpressionContext, session, patientUuid }; |
Member
There was a problem hiding this comment.
I don't really see a lot of value to trying to always add a patientUuid field here. What possible expression do we need to support that depends on the patientUuid?
| * | ||
| * @param context Additional context data to provide to extension display expressions | ||
| */ | ||
| export function useAssignExtensionExpressionContext(context: Record<string, any>) { |
Member
There was a problem hiding this comment.
Always prefer unknown where we do not know or care about the type of an object. Using any effectively opts-out of type-checking and makes it too easy to introduce unintended errors.
Suggested change
| export function useAssignExtensionExpressionContext(context: Record<string, any>) { | |
| export function useAssignExtensionExpressionContext(context: Record<string, unknown>) { |
Comment on lines
49
to
54
| const currentState = extensionExpressionContextStore.getState(); | ||
| const newState = { ...currentState }; | ||
| Object.keys(context).forEach((key) => { | ||
| delete newState[key]; | ||
| }); | ||
| extensionExpressionContextStore.setState(newState); |
Member
There was a problem hiding this comment.
Rather than writing things this way, it's better to take advantage of setState() to do:
extensionExpressionContextStore.setState((currentState) => {
const result = {};
Object.entries(currentState).forEach((key, value) => {
if (!Object.hasOwn(context, key)) {
result[key] = value;
}
});
return result;
});
Member
There was a problem hiding this comment.
So, while I realize this was in my little specification, now that I look at it, I think there's actually a better way to implement extension expression context that will work much better and that's to be enable ExtensionSlot's to pass certain variables to the context via the state. So, when we render an extensionSlot it has a state property that is generally passed down to the extension.
Perhaps what we do is to push this state into the extension system itself, e.g., so when we call useExtensionSlot(), we also pass in the slot state, which can be associated with the slot initially in the call to registerExtensionSlot() and then, with a second useEffect() hook and a new function in esm-extensions that updates the state property of the slot in extensionInternalStore (which doesn't currently exist). That way, we can use the ExtensionSlot state directly in expression evaluation. This would also mean that since most extension slots in the patient chart already pass the patient as part of their state, we'd probably have to do very little work to make this mechanism work for the patient-chart (other than ensuring that the nav-group correctly passes any state to any dashboards underneath it).
It also means that each extension could evaluate rules against slot-specific data, which should help make this more generally useful.
Contributor
Author
There was a problem hiding this comment.
Thanks @ibacher , let me make the necessary changes
Contributor
Author
|
samuelmale
reviewed
Aug 4, 2025
| */ | ||
|
|
||
| import { type Session, type SessionStore, sessionStore, userHasAccess } from '@openmrs/esm-api'; | ||
| /* eslint-disable no-console */ |
Member
There was a problem hiding this comment.
Seems like a debugging leftover
| ); | ||
| }; | ||
|
|
||
| export const extensionSlotConfigSchema = { |
Comment on lines
366
to
497
| } catch (e) { | ||
| console.error(`Error while evaluating expression ${displayConditionExpression}`, e); | ||
| // if the expression has an error, we do not display the extension | ||
| } catch (error) { |
Member
There was a problem hiding this comment.
I think it's important to log the error to the console for debugging purposes.
| /** | ||
| * If supplied, this is used to determine the display expression for the extension. | ||
| * This is used to determine if the extension is displayed in the UI. | ||
| * If not supplied, the extension will be displayed in the UI by default. |
Member
There was a problem hiding this comment.
the extension will be displayed in the UI by default
Well, assuming the logged-in user possesses the required privileges.
| extensionConfig?.['Display expression']?.expression ?? | ||
| (typeof extension.displayExpression === 'string' | ||
| ? extension.displayExpression | ||
| : (extension.displayExpression as any)?.expression) ?? |
Member
There was a problem hiding this comment.
Ideally, extension.displayExpression should be a string property. We won't have any issues if the schema is updated.
CynthiaKamau
force-pushed
the
extension-expressions
branch
2 times, most recently
from
1cc660f to
35df8ab
Compare
samuelmale
reviewed
Aug 11, 2025
| "moduleName": "Module name", | ||
| "modulesWithMissingDependenciesWarning": "Some modules have unresolved backend dependencies", | ||
| "numberValidationMessage": "Value must be a number", | ||
| "objectPropertyValidationMessage": "Property {{key}}: {{error}}", |
Member
There was a problem hiding this comment.
Did you remove this by mistake?
| getExtensionSlotsConfigStore, | ||
| } from '@openmrs/esm-config'; | ||
| import { evaluateAsBoolean } from '@openmrs/esm-expression-evaluator'; | ||
| import { evaluateAsBoolean, type VariablesMap } from '@openmrs/esm-expression-evaluator'; |
| if (!evaluateAsBoolean(displayConditionExpression, { session })) { | ||
| const context = | ||
| slotState && typeof slotState === 'object' ? { session, ...slotState } : { session, slotState }; | ||
| if (!evaluateExpression(displayConditionExpression, context)) { |
Member
There was a problem hiding this comment.
Can you use the framework's built-in expression evaluator?
| slotName, | ||
| state, | ||
| ) => | ||
| extensionInternalStore.setState((slotState) => { |
Member
There was a problem hiding this comment.
Suggested change
| extensionInternalStore.setState((slotState) => { | |
| extensionInternalStore.setState((currentState) => { |
| }); | ||
|
|
||
| export function setExtensionSlotState(slotName: string, state: ExtensionSlotCustomState) { | ||
| extensionInternalStore.setState((slotState) => { |
Member
There was a problem hiding this comment.
Again, it's kind of misleading to refer to this as slotState. currentState is a better name.
samuelmale
reviewed
Aug 14, 2025
| if (!evaluateAsBoolean(displayConditionExpression, { session })) { | ||
| const context: VariablesMap = | ||
| slotState && typeof slotState === 'object' ? { session, ...slotState } : { session, slotState }; | ||
| if (!evaluateAsBooleanAsync(displayConditionExpression, context)) { |
Member
There was a problem hiding this comment.
Hhhmmm, usingevaluateAsBooleanAsync(...) will always return a Promise, hence this condition will always be false unless you resolve the Promise. Can you use evaluateAsBoolean(...) instead?
Contributor
|
Thanks for your answers above @CynthiaKamau!
When I go to https://ethiohri-dev.globalhealthapp.net/openmrs/spa (logged in), I only see the home page. Where in the code or config or admin does someone configure the logic for what is shown/hidden depending on certain patient variables? |
CynthiaKamau
force-pushed
the
extension-expressions
branch
from
9875f2a to
1fd0499
Compare
Contributor
Author
The evaluation expressions were working on the patient chart clinical views, the expressions were being added in the dashboard.meta file, like here |
samuelmale
reviewed
Aug 25, 2025
| const displayConditionExpression = extensionConfig?.['Display conditions']?.expression ?? null; | ||
| if (displayConditionExpression !== null) { | ||
| const displayConditionExpression = | ||
| extensionConfig?.['Display conditions']?.expression ?? extension.expression ?? null; |
Member
There was a problem hiding this comment.
@CynthiaKamau So, because you provide an implicit config value of "" for the expression under "Display conditions", this statement will never resolve expressions defined via extension def in the routes file (extension.expression). This is because extensionConfig?.['Display conditions']?.expression will never be null or undefined. You should instead:
Suggested change
| extensionConfig?.['Display conditions']?.expression ?? extension.expression ?? null; | |
| extensionConfig?.['Display conditions']?.expression || extension.expression || null; |
Member
There was a problem hiding this comment.
@CynthiaKamau, I've also tested out this feature using the extension config, and it doesn't seem to work. This is because extensionConfig.['Display conditions'] points to the module's config, and not the extension config provided via the slot. Below is my demo config:
{
"@openmrs/esm-appointments-app": {
"appointments-metrics-slot": {
"configure": {
"metrics-card-scheduled-appointments": {
"Display conditions": {
"privileges": [],
"expression": "false"
}
}
}
}
}
}
Member
There was a problem hiding this comment.
Hmmm... that means several other features are broken (see line 345 which should be handling that).
Contributor
Author
There was a problem hiding this comment.
@CynthiaKamau, I've also tested out this feature using the extension config, and it doesn't seem to work. This is because
extensionConfig.['Display conditions']points to the module's config, and not the extension config provided via the slot. Below is my demo config:{ "@openmrs/esm-appointments-app": { "appointments-metrics-slot": { "configure": { "metrics-card-scheduled-appointments": { "Display conditions": { "privileges": [], "expression": "false" } } } } } }
I am using the same approach used in handling privileges, which are also part of the display conditions. Since state comes from an extension, if there is no state passed from your extension the evaluation will always be false
Member
There was a problem hiding this comment.
@CynthiaKamau have you tested this out at runtime to confirm that it works using extension config? If so, can you show me a sample config?
Contributor
Author
There was a problem hiding this comment.
@CynthiaKamau have you tested this out at runtime to confirm that it works using extension config? If so, can you show me a sample config?
{
"@openmrs/esm-patient-chart-app": {
"extensionSlots": {
"patient-info-slot": {
"configure": {
"patient-vitals-info": {
"Display conditions": {
"expression": "patient.person.gender === 'F'"
}
}
}
}
}
}
}
CynthiaKamau
force-pushed
the
extension-expressions
branch
from
1fd0499 to
ef54a03
Compare
ibacher
reviewed
Sep 8, 2025
Comment on lines
18
to
22
| useEffect(() => { | ||
| if (state) { | ||
| setExtensionSlotState(slotName, state); | ||
| } | ||
| }, [slotName, state]); |
Member
There was a problem hiding this comment.
Why do we have this check when it seems like by accepting null or undefined here, we remove the need for calling updateInternalExtensionSotre() in the ExtensionSlot at all.
Member
There was a problem hiding this comment.
That said, what we actually need here is a guard against running this on initial render, since registerExtensionSlot() already does that. That way we guard against an unnecessary cycle of cascading updates.
3 tasks
ibacher
approved these changes
Oct 25, 2025
This hopefully addresses some timing issues, but is also reflective of the order these things would run in the a real-world scenario.
Might be better to just swap to JestDom?
This will hopefully make things less flaky...
ibacher
approved these changes
Oct 27, 2025
ibacher
reviewed
Oct 27, 2025
| * Recomputes all configuration derived stores based on current state of input stores. | ||
| * Called whenever any input store (configInternalStore, temporaryConfigStore, configExtensionStore) changes. | ||
| */ | ||
| function recomputeAllConfigs() { |
Member
There was a problem hiding this comment.
This should be roughly equivalent to what was happening before, albeit with less chaining.
| const newState = { slots: { ...oldState.slots, ...newSlotStoreEntries } }; | ||
| if (!equals(oldState, newState)) { | ||
|
|
||
| if (!equals(oldState.slots, newState.slots)) { |
Member
There was a problem hiding this comment.
These comparison guards should mean that we're calling setState() less frequently
| throw `Unknown expression type ${expression}. Expressions must either be a string or pre-compiled string.`; | ||
| } | ||
|
|
||
| if (typeof expression === 'string' && expression.trim().length === 0) { |
Member
There was a problem hiding this comment.
One issue here is that we didn't prevent attempted evaluation of an empty string
|
|
||
| // Create context once for all extensions in this slot | ||
| const slotState = internalState.slots[slotName]?.state; | ||
| const expressionContext = slotState && typeof slotState === 'object' ? { session, ...slotState } : { session }; |
Member
There was a problem hiding this comment.
Using a single expressionContext is more efficient but may enable subtle errors if an expression modifies global state as a side-effect.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
Add ability to evaluate expressions in extensions
Screenshots
Screen.Recording.2025-07-14.at.13.54.37.mov
Related Issue
Other