(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

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

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.

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

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.

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

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=> ({

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

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

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) {

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

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>();

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

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

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!

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

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

The main point here was that these use-cases need to be clear in the code, so please add some comments explaining that distinction.

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.

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

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';

gracepotma · 2024-12-13T20:40:29Z

I'm confused, is this ready for rull re-review?

eudson · 2024-12-16T10:22:01Z

I'm confused, is this ready for rull re-review?

@gracepotma what does ready for full re-review means?

gracepotma · 2024-12-17T15:24:27Z

Discussed on today's Content Package check-in call w/ Eudson, Ian, Cynthia, & Jonathan O; apparently this is ready for re-review :) We'll make this a priority to review along with the Tabbed Encounter Widget PR: #1987

ibacher

At a high-level, this looks like it isn't massively distinct from #1987, at least in the backing helpers, etc. Since I'd prefer we didn't duplicate code, is it possible to rework this PR to use the same definitions from there at least for getObsFromEncounter() and similar shared functionality?

ibacher · 2024-12-17T15:17:39Z

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

+    return tileDefinitions?.map((tile: MenuCardProps) => ({
+      title: t(tile.tileHeader),
+      columns: getEncounterTileColumns(tile, t),
+    }));


Suggested change

return tileDefinitions?.map((tile: MenuCardProps) => ({

title: t(tile.tileHeader),

columns: getEncounterTileColumns(tile, t),

}));

return tileDefinitions?.map((tile: MenuCardProps) => ({

title: t(tile.tileHeader),

columns: getEncounterTileColumns(tile, t),

})) ?? [];

ibacher

Couple more small points.

ibacher · 2024-12-17T16:10:17Z

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

+    return <CodeSnippetSkeleton type="multi" role="progressbar" />;
+  }
+
+  if (error || lastEncounter == undefined) {


Shouldn't this just be !lastEncounter?

ibacher · 2024-12-17T16:13:25Z

packages/esm-patient-chart-app/src/clinical-views/hooks/useLastEncounter.ts

+    async (url) => {
+      const cachedData = cache.get(url);
+      if (cachedData) {
+        return cachedData;
+      }


The manual caching here seems unnecessary. It would be simpler to use useSWRImmutable, I think.

ibacher · 2024-12-17T16:14:33Z

packages/esm-patient-chart-app/src/clinical-views/hooks/useLastEncounter.ts

+      if (!response.ok) {
+        throw new Error('Failed to fetch data');
+      }
+      const responseData = await response.json();


Pretty much everything here is redundant. openmrsFetch() already checks if response.ok and raises an error if it isn't. Also await response.json() is already what's available in the response.data field.

CynthiaKamau · 2024-12-18T11:02:29Z

I have restructured a few files that cut across this pr and the encounter list PR

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

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

CynthiaKamau force-pushed the O3-3773 branch from 12d5ba0 to 797618b Compare December 16, 2024 11:02

gracepotma requested a review from ibacher December 17, 2024 15:17

ibacher reviewed Dec 17, 2024

View reviewed changes

kajambiya and others added 10 commits December 18, 2024 11:02

Introduce summary card

283f428

add logic

13fe0c3

clean up

f8d44c0

code review

74a4a84

code review

845ca37

Introduce summary card

5b176ff

add logic

3d716f8

clean up

a34b827

code review

c5789c1

code review

359217f

CynthiaKamau force-pushed the O3-3773 branch from 797618b to e146333 Compare December 18, 2024 10:27

Restructure directories and fix conflicts

1095bcc

CynthiaKamau force-pushed the O3-3773 branch from e146333 to 1095bcc Compare December 18, 2024 10:56

CynthiaKamau requested review from ibacher and CynthiaKamau December 18, 2024 11:02

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

	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

ibacher left a 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

gracepotma commented Dec 13, 2024

eudson commented Dec 16, 2024

gracepotma commented Dec 17, 2024

ibacher left a 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

CynthiaKamau commented Dec 18, 2024

kajambiya commented Aug 22, 2024 •

edited by denniskigen

Loading