smart-on-fhir / ehi-app

EHI Export API Client Reference Implementation
https://ehi-app.smarthealthit.org/
Apache License 2.0
4 stars 1 forks source link

ensure download link is a complete url #30

Closed Dtphelan1 closed 1 year ago

Dtphelan1 commented 1 year ago

Closes #11 for real this time, catching a mistake w/r/t download links.

Dtphelan1 commented 1 year ago

Your comment implies that this variable is being used to simultaneously represent things in the front-end and things in the backend, but it is not. While it is true that routes on the front-end use the /jobs slug, the use of that path is independent from this variable; here, EXPORT_ROUTE is only used describe the base-relative-path for all export job-related actions on the server.

If you feel like that approach is too fragile for the API (i.e. there isn't a strong guarantee that all these operations, like updating a job and getting downloads off of it, will have the same base url in the future) or if you feel it's unnecessary (i.e. there aren't enough methods here to justify using a constant) I'm open to changing it. But your commented implied a third thing that I don't think is the case.

vlad-ignatov commented 1 year ago

Oh, right, these are all used against the same server. My bad.

Let me clarify what I meant otherwise. I would throw away the constant and simplify the entire file to something like:

import { ExportJobSummary, ExportJob } from "../types";
import { request } from "./fetchHelpers";

export function getExportJobLink(id: string) {
  // Needs the actual server URL since this is used in an <a> tag, not in the request library
  return `${process!.env!.REACT_APP_EHI_SERVER}/jobs/${id}/download`;
}

export async function getExportJobs(signal?: AbortSignal) {
  return request<ExportJobSummary[]>("/jobs", { signal });
}

export async function getExportJob(id: string, signal?: AbortSignal) {
  return request<ExportJob>(`/jobs/${id}`, { signal });
}

export async function updateExportStatus(id: string, newStatus: "approve" | "reject") {
  return request<ExportJob>(`/jobs/${id}`, {
    method: "post",
    headers: {
      Accept: "application/json",
      "Content-Type": "application/json",
    },
    body: JSON.stringify({ action: newStatus }),
  });
}

This makes the file smaller and easier to read and understand. Placing the slug in a constant is only useful if it can change. It will never change (once we settle on something). However, since we are still in the prototype stage, things can change and that is what I mean by "some room for improvement/simplification in the future" - I would personally be happier with the above code, but not yet since the API is not set in stone.

In other words this should currently be good for merging and should get the job done. Later, when the project is functionally complete we can go through another stage of simplifying, optimizing and finalizing, and we can then go back to this and see if it is still applicable.

Dtphelan1 commented 1 year ago

We are on the same page now, thanks for clarifying! At a minimum, I'll include the Type parameters to the request calls