(feat) O3-3773 Support for Patient Program Summary in Patient Chart #1969

kajambiya · 2024-08-22T05:13:47Z

Requirements

This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
My work includes tests or is validated by existing tests.

Summary

This PR introduces summary program summary to patient chart.

Screenshots

Related Issue

Other

hadijahkyampeire

Mostly looks good, thanks @kajambiya , just a few comments

hadijahkyampeire · 2024-09-16T08:56:23Z

packages/esm-patient-chart-app/src/encounter-tile/utils/helpers.ts

+  else return encounter[param] ? encounter[param] : '--';
+}
+
+export function formatDateTime(dateString: string): any {


Let's use the already existing Date formatters from the framework

hadijahkyampeire · 2024-09-16T08:57:26Z

packages/esm-patient-chart-app/src/encounter-tile/utils/helpers.ts

+}
+
+export function obsArrayDateComparator(left, right) {
+  return formatDateTime(right.obsDatetime) - formatDateTime(left.obsDatetime);


Same here, we can leverage the framework functions, check on my PR, I got a way around this

ibacher · 2024-09-17T19:54:15Z

packages/esm-patient-chart-app/src/encounter-tile/utils/helpers.ts

+import { formatDate, parseDate } from '@openmrs/esm-framework';
+
+export function getEncounterValues(encounter, param: string, isDate?: Boolean) {
+  if (isDate) return dayjs(encounter[param]).format('DD-MMM-YYYY');


Please use the framework rather than dayjs for formatting dates and never hard-code the date format.

ibacher · 2024-09-17T19:56:17Z

packages/esm-patient-chart-app/src/encounter-tile/utils/helpers.ts

+
+export function getObsFromEncounters(encounters, obsConcept) {
+  const filteredEnc = encounters?.find((enc) => enc.obs.find((obs) => obs.concept.uuid === obsConcept));
+  return getObsFromEncounter(filteredEnc, obsConcept);


Why do we need this filtering step? This just seems to add complexity for no real gain.

ibacher · 2024-09-17T19:56:37Z

packages/esm-patient-chart-app/src/encounter-tile/utils/helpers.ts

+  return formatDateTime(right.obsDatetime) - formatDateTime(left.obsDatetime);
+}
+
+export function findObs(encounter, obsConcept): Record<string, any> {


Please don't use any when a better type can be used (as is the case here).

ibacher · 2024-09-17T19:57:45Z

packages/esm-patient-chart-app/src/encounter-tile/utils/helpers.ts

+
+  if (isTrueFalseConcept) {
+    if (
+      (obs?.value?.uuid != 'cf82933b-3f3f-45e7-a5ab-5d31aaee3da3' && obs?.value?.name?.name !== 'Unknown') ||


Please don't hard-code the meaning of concepts to specific UUIDs like this. It is inherently not-sharrable.

ibacher · 2024-09-17T19:57:55Z

packages/esm-patient-chart-app/src/encounter-tile/utils/helpers.ts

+  }
+
+  if (type === 'location') {
+    return encounter.location.name;


Suggested change

return encounter.location.name;

return encounter.location.display;

ibacher · 2024-09-17T19:59:24Z

packages/esm-patient-chart-app/src/encounter-tile/utils/helpers.ts

+
+  if (typeof obs.value === 'object' && obs.value?.names) {
+    return (
+      obs.value?.names?.find((conceptName) => conceptName.conceptNameType === 'SHORT')?.name || obs.value.name.name


Why are we using the SHORT name in preference to the preferred name? Is there a good reason for this choice?

do you mean concept.display or concept.preferredName ?

display is always the preferred name in the user's locale.

ibacher · 2024-09-17T20:00:02Z

packages/esm-patient-chart-app/src/index.ts

@@ -112,6 +113,11 @@ export const stopVisitPatientSearchActionButton = getSyncLifecycle(stopVisitActi
  moduleName,
 });

+export const patientProgramSummary = getSyncLifecycle(PatientSummaryOverviewList, {


This seems like it should be part of the programs app and not the patient-chart-app.

The data is related to a program but some people may classify it differently.

Right, that doesn't explain to me why it belongs here though. If it's program-specific, surely it's part of the programs app?

It is not program specific, I think the name might be misleading , it’s more of a clinical view summary. I have updated the name

Part of my point is that this is a public interface intended to be consumed by multiple configurations written by implementations that may or may not understand what the original purpose of a widget is; thus naming things so that they are consistent, discoverable, and not confusing is of vital importance here.

ibacher · 2024-10-09T15:09:21Z

packages/esm-patient-chart-app/src/index.ts

@@ -112,6 +113,11 @@ export const stopVisitPatientSearchActionButton = getSyncLifecycle(stopVisitActi
  moduleName,
 });

+export const clinicalViewsSummary = getSyncLifecycle(ClinicalViewsSummary, {


Please use getAsyncLifecycle() here.

ibacher · 2024-10-09T15:11:51Z

packages/esm-patient-chart-app/src/index.ts

@@ -112,6 +113,11 @@ export const stopVisitPatientSearchActionButton = getSyncLifecycle(stopVisitActi
  moduleName,
 });

+export const patientProgramSummary = getSyncLifecycle(PatientSummaryOverviewList, {


Part of my point is that this is a public interface intended to be consumed by multiple configurations written by implementations that may or may not understand what the original purpose of a widget is; thus naming things so that they are consistent, discoverable, and not confusing is of vital importance here.

ibacher · 2024-10-09T15:12:23Z

...ges/esm-patient-chart-app/src/clinical-views/components/clinical-views-summary.component.tsx

+
+  const tilesData = useMemo(
+    () =>
+      tilesDefinitions?.map((tile: any) => ({


This should be correctly typed.

Suggested change

tilesDefinitions?.map((tile: any) => ({

tilesDefinitions?.map((tile=> ({

ibacher · 2024-10-09T15:12:37Z

...ges/esm-patient-chart-app/src/clinical-views/components/clinical-views-summary.component.tsx

+  const tilesData = useMemo(
+    () =>
+      tilesDefinitions?.map((tile: any) => ({
+        title: tile.tileHeader,


If this is coming straight from config, it should be translatable.

I added the Tile type instead

ibacher · 2024-10-09T15:13:10Z

...ges/esm-patient-chart-app/src/clinical-views/components/clinical-views-summary.component.tsx

+  const config = useConfig();
+  const tilesDefinitions = config.tilesDefinitions;


Suggested change

const config = useConfig();

const tilesDefinitions = config.tilesDefinitions;

const { tileDefinitions } = useConfig</* ConfigType goes here */>();

ibacher · 2024-10-09T15:13:44Z

...ges/esm-patient-chart-app/src/clinical-views/components/clinical-views-summary.component.tsx

@@ -0,0 +1,46 @@
+import React, { useMemo } from 'react';
+import { useConfig } from '@openmrs/esm-framework';
+import { getEncounterTileColumns } from '../../encounter-tile/utils/encounter-tile-config-builder';


Organization-wise here, the encounter-tile is part of the clinical views, so surely it should be under the components for that?

ibacher · 2024-10-09T15:14:36Z

packages/esm-patient-chart-app/src/encounter-tile/components/encounter-tile.component.tsx

+  );
+};
+
+export const MemoizedEncounterTile = React.memo(EncounterTile);


Suggested change

export const MemoizedEncounterTile = React.memo(EncounterTile);

export const EncounterTile = React.memo(EncounterTileInternal);

It's better to reserve weird names for the non-exported bits of an API.

ibacher · 2024-10-09T15:17:04Z

packages/esm-patient-chart-app/src/encounter-tile/components/encounter-tile.component.tsx

+    return (
+      <div className={styles.tileBox}>
+        <div className={styles.tileBoxColumn}>
+          <span className={styles.tileTitle}> {column.header} </span>


Spaces here seem wrong?

ibacher · 2024-10-09T15:18:10Z

packages/esm-patient-chart-app/src/encounter-tile/hooks/useLastEncounter.ts

+  'obs:(uuid,obsDatetime,voided,groupMembers,concept:(uuid,name:(uuid,name)),value:(uuid,name:(uuid,name),' +
+  'names:(uuid,conceptNameType,name))),form:(uuid,name))';
+
+export function useLastEncounter(patientUuid: string, encounterType: string) {


Suggested change

export function useLastEncounter(patientUuid: string, encounterType: string) {

export function useMostRecentEncounter(patientUuid: string, encounterType: string) {

ibacher · 2024-10-09T15:20:44Z

packages/esm-patient-chart-app/src/encounter-tile/components/tile.scss

+  font-size: 16px;
+  line-height: 1.33;
+  letter-spacing: 0.32px;


Shouldn't we be using Carbon's typography defaults here and everywhere else in this file?

ibacher

Thank you! This widget is getting much closer. A few more nit-picking comments on the code.

Now looking at the visual of this widget, we should bring it in line with the existing RefApp chart tiles specifically:

The h4 header font rendering is inconsistent with other tiles in the chart. Both the font-size (20px vs 16px) is off and the font-weight (400 vs 600).
The header ::after element is not correctly sized (border-bottom-width is 2px too large and padding-top is off by 1px — 2px in this component vs 3px in others)

Oddly, when located in the top-of-all-patient-dashboards-slot, as shown above the box appears too wide, but when located in the same slot as the other elements, the width is too small:

ibacher · 2024-10-21T13:26:49Z

...ges/esm-patient-chart-app/src/clinical-views/components/clinical-views-summary.component.tsx

+}
+
+const ClinicalViewsSummary: React.FC<OverviewListProps> = ({ patientUuid }) => {
+  const config = useConfig() as ConfigObject;


Suggested change

const config = useConfig() as ConfigObject;

const config = useConfig<ConfigObject>();

ibacher · 2024-10-21T13:28:46Z

packages/esm-patient-chart-app/src/clinical-views/utils/encounter-tile-config-builder.ts

+export const getEncounterTileColumns = (tileDefinition: MenuCardProps, t?: TFunction) => {
+  const columns: Array<FormattedCardColumn> = tileDefinition.columns?.map((column: ColumnDefinition) => ({
+    key: column.id,
+    header: column.title,


Header should be translatable.

ibacher · 2024-10-21T13:29:31Z

packages/esm-patient-chart-app/src/clinical-views/utils/encounter-tile-config-builder.ts

+
+export const getEncounterTileColumns = (tileDefinition: MenuCardProps, t?: TFunction) => {
+  const columns: Array<FormattedCardColumn> = tileDefinition.columns?.map((column: ColumnDefinition) => ({
+    key: column.id,


It seems like bad practice to require users to supply the key. This could presumably be constructed from the title property, right?

ibacher · 2024-10-21T13:33:24Z

...ges/esm-patient-chart-app/src/clinical-views/components/clinical-views-summary.component.tsx

+const ClinicalViewsSummary: React.FC<OverviewListProps> = ({ patientUuid }) => {
+  const config = useConfig() as ConfigObject;
+  const { t } = useTranslation();
+  const tilesDefinitions = config.tilesDefinitions;


It would be better to name this "tileDefinitions". The double plural is weird.

brandones · 2024-11-11T16:37:35Z

Ping @ibacher

ibacher

Not sure I've covered everything, but here are some things that need to be cleaned-up before this is mergeable. Goal here is to make sure that the code is in a format that the community can support over the long term. Thanks!

ibacher · 2024-11-22T16:23:45Z

...-patient-chart-app/src/clinical-views/components/encounter-tile/encounter-tile.component.tsx

+import { CodeSnippetSkeleton, Tile, Column, Layer } from '@carbon/react';
+import React, { useMemo } from 'react';
+import styles from './tile.scss';
+import { groupColumnsByEncounterType } from '../../utils/helpers';
+import { useLastEncounter } from '../../hooks/useLastEncounter';
+import { LazyCell } from '../../../lazy-cell/lazy-cell.component';
+import { useTranslation } from 'react-i18next';
+import { type FormattedCardColumn } from '../../utils/encounter-tile-config-builder';
+import { useLayoutType } from '@openmrs/esm-framework';


Please review our Coding Conventions on code organization and how imports should be ordered.

ibacher · 2024-11-22T16:24:41Z

...-patient-chart-app/src/clinical-views/components/encounter-tile/encounter-tile.component.tsx

+    <Layer className={styles.layer}>
+      <Tile className={styles.tile}>
+        <div className={isTablet ? styles.tabletHeading : styles.desktopHeading}>
+          <h4 className={styles.title}>{headerTitle}</h4>


Any string displayed to the user needs to be translatable.

The strings from the config are translated already

ibacher · 2024-11-22T16:26:24Z

...-patient-chart-app/src/clinical-views/components/encounter-tile/encounter-tile.component.tsx

+  );
+};
+
+export const EncounterTile = React.memo(EncounterTileInternal);


Why create two separate consts for this?

export const EncounterTile = React.memo(({ patientUuid, columns, headerTitle }: EncounterTileProps) => { // code goes here });

Also works.

ibacher · 2024-11-22T16:27:51Z

...-patient-chart-app/src/clinical-views/components/encounter-tile/encounter-tile.component.tsx

+              key={encounterType}
+              patientUuid={patientUuid}
+              encounterType={encounterType}
+              columns={columns as FormattedCardColumn[]}


Why do we need to cast the column types here? This seems like an unnecessary escape hatch when we have full control over:

The types in question

The thing that's being cast to a type

The function returning the thing that needs to be cast

ibacher · 2024-11-22T16:29:38Z

...-patient-chart-app/src/clinical-views/components/encounter-tile/encounter-tile.component.tsx

+      <div className={styles.tileBox}>
+        {columns.map((column, ind) => (
+          <div key={ind} className={styles.tileBoxColumn}>
+            <span className={styles.tileTitle}>{t(column.title)}</span>
+            <span className={styles.tileValue}>--</span>
+          </div>
+        ))}
+      </div>


So, here, an error happens and instead of reporting that to the user we just render a blank set of cells? That doesn't seem right... I'd expect us to render a message to the user explaining that the encounter data could not be loaded.

ibacher · 2024-11-22T16:52:30Z

packages/esm-patient-chart-app/src/clinical-views/utils/helpers.ts

+  obsConcept: string,
+  isDate?: Boolean,
+  isTrueFalseConcept?: Boolean,
+  type?: string,


We only support a fixed set of types, so the type here should probably reflect that.

ibacher · 2024-11-22T16:52:56Z

packages/esm-patient-chart-app/src/clinical-views/utils/helpers.ts

+  isTrueFalseConcept?: Boolean,
+  type?: string,
+  fallbackConcepts?: Array<string>,
+  secondaryConcept?: string,


What is a "secondaryConcept"? How does this differ from "fallbackConcepts"? These are the things that we need to explain via comments.

a secondary concept is used in a question wit the option other and a user is required to add their response in another question, which in his case is captured in the secondary concept.

A fallback concept is used in instances where a tile has an alternative value, for example, enrollment date and re-enrollment date. If there is no enrollment date, then the value of the re-enrollment date can be displayed instead
cc : @eudson

ibacher · 2024-11-22T16:53:43Z

packages/esm-patient-chart-app/src/clinical-views/utils/helpers.ts

+
+  if (secondaryConcept && typeof obs.value === 'object' && obs.value.names) {
+    const primaryValue = obs.value.name.display;
+    if (primaryValue === t('Other non-coded')) {


This is, again, not what the translation system should be used for. Capture the UUID of the concept via the configuration system.

ibacher · 2024-11-22T16:54:25Z

packages/esm-patient-chart-app/src/clinical-views/utils/helpers.ts

+
+export function getObsFromEncounters(encounters: Encounter, obsConcept: string) {
+  const filteredEnc = encounters?.find((enc) => enc.obs.find((obs) => obs.concept.uuid === obsConcept));
+  return getObsFromEncounter(filteredEnc, obsConcept);


This is one of the many reasons that I think getObsFromEncounter needs refactoring. It's doing too many things with too many concerns.

ibacher · 2024-11-22T16:55:26Z

packages/esm-patient-common-lib/src/types/index.ts

@@ -1,3 +1,5 @@
+import { OpenmrsResource } from '@openmrs/esm-framework';


Suggested change

import { OpenmrsResource } from '@openmrs/esm-framework';

import { type OpenmrsResource } from '@openmrs/esm-framework';

kajambiya force-pushed the O3-3773 branch from 1320c6f to 4946dbd Compare August 22, 2024 05:33

kajambiya force-pushed the O3-3773 branch 2 times, most recently from eaf5653 to 6b763ca Compare September 4, 2024 05:28

kajambiya changed the title ~~(feat) O3-3773 Support Summary Tiles in Patient Chart~~ (feat) O3-3773 Support for Patient Program Summary in Patient Chart Sep 4, 2024

kajambiya requested review from hadijahkyampeire, ibacher and ebambo September 4, 2024 06:13

kajambiya marked this pull request as ready for review September 4, 2024 06:36

kajambiya requested a review from samuelmale September 4, 2024 06:37

kajambiya force-pushed the O3-3773 branch from 6b763ca to 64b77fd Compare September 6, 2024 06:19

hadijahkyampeire reviewed Sep 16, 2024

View reviewed changes

ibacher requested changes Sep 17, 2024

View reviewed changes

CynthiaKamau force-pushed the O3-3773 branch from 64b77fd to 36a0e2a Compare October 4, 2024 17:50

CynthiaKamau requested a review from ibacher October 4, 2024 17:52

CynthiaKamau force-pushed the O3-3773 branch 3 times, most recently from cb49d63 to cab9b91 Compare October 8, 2024 13:30

ibacher requested changes Oct 9, 2024

View reviewed changes

CynthiaKamau force-pushed the O3-3773 branch 2 times, most recently from d5c1461 to b9b0d00 Compare October 12, 2024 07:25

ibacher requested changes Oct 21, 2024

View reviewed changes

CynthiaKamau force-pushed the O3-3773 branch from b9b0d00 to 72c35b9 Compare October 22, 2024 18:43

CynthiaKamau requested a review from ibacher October 29, 2024 15:59

ibacher requested changes Nov 22, 2024

View reviewed changes

kajambiya and others added 4 commits November 26, 2024 16:23

Introduce summary card

d54f690

add logic

dfeb69f

clean up

814eb05

code review

7a6c637

CynthiaKamau force-pushed the O3-3773 branch from 72c35b9 to 8dcedbb Compare November 26, 2024 13:24

code review

bd4e5dc

CynthiaKamau force-pushed the O3-3773 branch from 8dcedbb to bd4e5dc Compare November 26, 2024 13:32

	return encounter.location.name;
	return encounter.location.display;

	tilesDefinitions?.map((tile: any) => ({
	tilesDefinitions?.map((tile=> ({

		const config = useConfig();
		const tilesDefinitions = config.tilesDefinitions;

	const config = useConfig();
	const tilesDefinitions = config.tilesDefinitions;
	const { tileDefinitions } = useConfig</* ConfigType goes here */>();

	export const MemoizedEncounterTile = React.memo(EncounterTile);
	export const EncounterTile = React.memo(EncounterTileInternal);

	export function useLastEncounter(patientUuid: string, encounterType: string) {
	export function useMostRecentEncounter(patientUuid: string, encounterType: string) {

	const config = useConfig() as ConfigObject;
	const config = useConfig<ConfigObject>();

		@@ -1,3 +1,5 @@
		import { OpenmrsResource } from '@openmrs/esm-framework';

	import { OpenmrsResource } from '@openmrs/esm-framework';
	import { type OpenmrsResource } from '@openmrs/esm-framework';

(feat) O3-3773 Support for Patient Program Summary in Patient Chart #1969

Are you sure you want to change the base?

(feat) O3-3773 Support for Patient Program Summary in Patient Chart #1969

Conversation

kajambiya commented Aug 22, 2024 • edited by denniskigen Loading

Requirements

Summary

Screenshots

Related Issue

Other

hadijahkyampeire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandones commented Nov 11, 2024

ibacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kajambiya commented Aug 22, 2024 •

edited by denniskigen

Loading