Skip to content
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

Adding file attachment support #128

Merged
merged 17 commits into from
Nov 2, 2023

Conversation

arodidev
Copy link
Contributor

@arodidev arodidev commented Oct 8, 2023

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 adds support for file attachment capture and submission to the attachments endpoint, via upload or via camera capture.

Screenshots

screen-recording-2023-10-08-at-135526_uv2wAQ9I.mp4

Related Issue

Other

@arodidev arodidev marked this pull request as ready for review October 10, 2023 17:11
@arodidev
Copy link
Contributor Author

@kajambiya kindly point out the variable in question

src/api/api.ts Outdated Show resolved Hide resolved
src/components/inputs/file/file.component.tsx Outdated Show resolved Hide resolved
src/api/api.ts Outdated Show resolved Hide resolved
@arodidev arodidev requested a review from kajambiya October 13, 2023 10:32
src/api/types.ts Outdated Show resolved Hide resolved
src/api/types.ts Outdated Show resolved Hide resolved
@samuelmale
Copy link
Member

@arodidev have you tested this out in edit mode? If so, does it initialize the field value as expected?

@arodidev
Copy link
Contributor Author

@arodidev have you tested this out in edit mode? If so, does it initialize the field value as expected?

Not yet, will look into this.

Comment on lines 316 to 349
export function dataURItoFile(dataURI: string) {
const byteString = atob(dataURI.split(',')[1]);
const mimeString = dataURI
.split(',')[0]
.split(':')[1]
.split(';')[0];

// write the bytes of the string to a typed array
const buffer = new Uint8Array(byteString.length);

for (let i = 0; i < byteString.length; i++) {
buffer[i] = byteString.charCodeAt(i);
}

const blob = new Blob([buffer], { type: mimeString });
return { blob, mimeString };
}

export function fileToBase64(fileObject) {
return new Promise((resolve, reject) => {
const reader = new FileReader();

reader.onloadend = () => {
const base64String = reader.result;
resolve(base64String);
};

reader.onerror = error => {
reject(error);
};

reader.readAsDataURL(fileObject);
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we plan on referencing these functions within business logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove, had used them in a prior implementation.

Comment on lines 33 to 49
const myInitVal = useMemo(() => {
const initialValuesObject = encounterContext.initValues;
const attachmentValue = Object.keys(initialValuesObject)
.filter((key) => key === question.id)
.reduce((cur, key) => {
return Object.assign(cur, { [key]: initialValuesObject[key] });
}, {});
return attachmentValue;
}, [encounterContext]);

const attachmentValue = useMemo(() => {
const firstValue = Object?.values(myInitVal)[0];
if (firstValue) {
const attachment = createGalleryEntry(firstValue?.[0]);
return attachment;
}
}, [myInitVal]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By design, a typical visual component isn't supposed to know how to pick these values from the encounter context. We delegate these tasks to Field Handlers. This was intended to minimize coupling (UI with business logic) and ensure a separation of concerns. Just for consistency, I think you wanna introduce a FileSubmissionHandler

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, will implement.

.filter(field => isEmpty(field.value))
.forEach(field => {
.filter((field) => isEmpty(field.value))
.filter((field) => field.questionOptions.rendering !== 'file')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you follow the handler patterns, this change wouldn't be necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted

Comment on lines +142 to +172
useEffect(() => {
const emptyValues = {
checkbox: [],
toggle: false,
default: '',
};
const attachmentFields = formFields.filter((field) => field.questionOptions.rendering === 'file');

if (attachmentFields.length) {
if (encounter) {
Promise.all(
attachmentFields.map((field) => {
return formFieldHandlers[field.type]?.getInitialValue(encounter, field, formFields);
}),
).then((responses) => {
responses.forEach((responseValue, index) => {
const eachField = attachmentFields[index];

const filteredResponseValue = responseValue['results'].filter(
(eachResponse) => eachResponse.comment === eachField.id,
);

initialValues[eachField.id] = isEmpty(responseValue)
? emptyValues[eachField.questionOptions.rendering] ?? emptyValues.default
: filteredResponseValue;

setInitialValues({ ...initialValues });
});
});
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, let's put this in a handler

Comment on lines 27 to 40
export function createGalleryEntry(data: AttachmentResponse): Attachment {
const attachmentUrl = '/ws/rest/v1/attachment';
return {
id: data.uuid,
src: `${window.openmrsBase}${attachmentUrl}/${data.uuid}/bytes`,
title: data.comment,
description: '',
dateTime: formatDate(new Date(data.dateTime), {
mode: 'wide',
}),
bytesMimeType: data.bytesMimeType,
bytesContentFamily: data.bytesContentFamily,
};
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's a gallery entry? Do we have such a domain defined within the library?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Picked this implementation from esm-patient-attachments-app, which converts the attachment response into a usable attachment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I understand, within the context of that esm, it makes sense to use the term "gallery" because there is an actual gallery but we don't have one here. Can you revise the naming?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Comment on lines 502 to 510
.forEach((field) => {
return saveAttachment(
encounter?.patient.uuid,
field,
field?.questionOptions.concept,
new Date().toISOString(),
encounter?.uuid,
ac,
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again as I suggested before, can we resolve these in parallel?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, will implement

Comment on lines +162 to +167
useEffect(() => {
if (tempInitialValues) {
setInitValues(tempInitialValues);
}
}, [tempInitialValues]);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean the useEffect?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm asking because when introducing new rendering types most of the time we don't have reason to touch this level of code as it's already abstracted at a high level, that's assuming the standard patterns are followed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the initial values are loaded from useInitialValues hook, I am passing down the initial values to the field via context, hence this snippet which updates the state variable holding the initial values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, you don't need this change if you properly follow the standard patterns. Kindly consider introducing a handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do, thank you.

Copy link
Contributor Author

@arodidev arodidev Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also have a session to discuss this further? There are some areas I need clarity on.

@eudson
Copy link
Contributor

eudson commented Nov 1, 2023

@samuelmale this PR needs to be merged. We will work on the handler next sprint (in two weeks), can you please undo your change request ?

@samuelmale samuelmale dismissed their stale review November 1, 2023 16:37

Maybe addressed in a followup commit

@kajambiya kajambiya merged commit e790a4c into openmrs:main Nov 2, 2023
7 checks passed
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.

5 participants