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

Refactor upload service #214

Merged
merged 14 commits into from
Nov 29, 2023
Merged

Refactor upload service #214

merged 14 commits into from
Nov 29, 2023

Conversation

hannasage
Copy link

@hannasage hannasage commented Nov 23, 2023

Purpose

This PR refactors our common submission functionality, including document uploads and the submission payload, out into its own file complete with a standard function and hook adapter. This unifies the code that handles this intricate process, and creates a uniform developer experience.

Linked Issues to Close

N/A

Approach

First, we established that we'd need two interfaces as we sometimes submit actions from pages with just static data, no form input, and sometimes we submit from pages with said form data via react-hook-form. Check out MedicaidForm and ToggleRaiResponseWithdraw to see this variance.

Next, we ported over the document upload process from the big block of code int he MedicaidForm submit handler function, and split it out into well-named functions for easy maintenance and readability.

Finally, we orchestrated document uploads and form data submission via the main submit function.

Assorted Notes/Considerations/Learning

There is an addition of a map with key/value pairs representing a file's object attribute key and the UI friendly title. If you do not add your attachment type in and it's not currently available in the map, your attachment defaults to the object key (i.e. "cmsForm179" instead of "CMS Form 179")

Examples

For use with react-hook-form:

const handleSubmit: SubmitHandler<MedicaidFormSchema> = async (formData) => {
  try {
    await submit<MedicaidFormSchema>({
      data: formData,
      endpoint: "/submit",
      user,
      authority: Authority.MED_SPA,
    });
    // Open success modal
  } catch (e) {
    console.error(e);
    // Open error modal
  }
};

<form onSubmit={form.handleSubmit(handleSubmit)}> 
   ... 
</form>

For use without any input data or uploaded docs:

const { mutate, isLoading, isSuccess, error } = useSubmissionService<{
  id: string;
}>({
  data: { id: id! },
  endpoint: buildActionUrl(type!),
  user,
});

// Triggers error modal when an error is returned
useEffect(() => if (error) setErrorModalOpen(true), [error]}

<Button onClick={() => {
    mutate()
    // Open success modal
}>Submit</Button>

src/services/ui/src/api/submissionService.ts Outdated Show resolved Hide resolved
if (data?.attachments) {
const attachments = data.attachments as AttachmentsSchema;
const validFilesetKeys = Object.keys(attachments).filter(key => attachments[key] !== undefined);
const preSignedURLs = await Promise.all(validFilesetKeys.map(() => getPreSignedUrl()));
Copy link
Author

Choose a reason for hiding this comment

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

@mdial89f @13bfrancis We get one pre-signed URL object back per GROUP of files since every key can hold an array of 1 or more. This was copied functionality from the original approach, but this DOES mean every file in a group associated to a single key is uploaded with the same URL and given the same key. This seems like a flaw, is it?

I believe we should be getting a pre-signed URL per file, not per file group. That way every file has its own upload url and key. But this is making some assumptions about how the upload service and retrieval of a file works.

const getPreSignedUrl = async (): Promise<PreSignedURL> => {
// TODO: Can this be a GET instead of POST w/ empty body
return await API.post("os", "/getUploadUrl", {
body: {},
Copy link
Author

Choose a reason for hiding this comment

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

@mdial89f Can this be a GET? POST with an empty body seems like we're maybe compensating for an external services requirement, and if that isn't the case, I think we should refactor.

Comment on lines +70 to +83
case buildActionUrl(Action.ISSUE_RAI):
return {
...data,
...userDetails,
requestedDate: seaToolFriendlyTimestamp,
attachments: attachments ? buildAttachmentObject(attachments) : null
};
case buildActionUrl(Action.RESPOND_TO_RAI):
return {
...data,
...userDetails,
responseDate: seaToolFriendlyTimestamp,
attachments: attachments ? buildAttachmentObject(attachments) : null
};
Copy link
Author

Choose a reason for hiding this comment

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

I like the idea of switch-ing on our endpoint value because it can help us identify areas where our payloads are literally the same, but have a renamed attribute here and there. We should strive to keep this switch to as few special cases as possible and group similar payloads.

For instance, requestedDate and responseDate don't need to be two fields in two payloads...that's just a timestamp. We can derive whether the timestamp is for a response or request based on the action itself.

@hannasage hannasage self-assigned this Nov 27, 2023
@hannasage hannasage marked this pull request as ready for review November 27, 2023 17:22
Copy link
Collaborator

@13bfrancis 13bfrancis left a comment

Choose a reason for hiding this comment

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

I like this. If I get my branch deployed and finished you can use that one for testing if you would like

@hannasage hannasage merged commit cdfd578 into master Nov 29, 2023
14 of 15 checks passed
@hannasage hannasage deleted the refactor-upload-service branch November 29, 2023 18:07
Copy link
Contributor

🎉 This PR is included in version 1.5.0-val.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants