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

Results list route #78

Merged
merged 24 commits into from
Oct 14, 2022
Merged

Results list route #78

merged 24 commits into from
Oct 14, 2022

Conversation

jhughes982
Copy link
Collaborator

@jhughes982 jhughes982 commented Oct 12, 2022

  • Added table of results,
  • The data returned from the initial fhir query is a mixture of both patient and observation data, and they are all deeply nested objects of varying data types. I have used the any type in these cases. Is there a better way to add accurate typings here?
  • Moved the fhir provider into app.tsx to expose the necessary methods to Results.tsx. As a result, I have removed a few imports in NewReport.tsx.
  • closes View list of all results #61

@jhughes982 jhughes982 added the enhancement New feature or request label Oct 12, 2022
@jhughes982 jhughes982 requested a review from stefpiatek October 12, 2022 14:26
@jhughes982 jhughes982 self-assigned this Oct 12, 2022
@jhughes982 jhughes982 temporarily deployed to test-env October 12, 2022 14:29 Inactive
@jhughes982 jhughes982 temporarily deployed to test-env October 12, 2022 14:44 Inactive
Copy link
Collaborator

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

Looks good, I think it's worth taking advantage of the fact that we know the data structure if we can (which I think should be fairly straightforward) from the existing typescript libraries for FHIR

const [parsedResults, setParsedResults] = useState<parsedResultsModel | null>(null);

useEffect(() => {
const observationQueryUrl = `${FHIR_URL}/Observation?_profile=http://hl7.org/fhir/uv/genomics-reporting/StructureDefinition/variant&_include=Observation:subject`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the fhir context is instantiated with the base URL, so can just use this and remove the reuse of the env variable

Suggested change
const observationQueryUrl = `${FHIR_URL}/Observation?_profile=http://hl7.org/fhir/uv/genomics-reporting/StructureDefinition/variant&_include=Observation:subject`;
const observationQueryUrl = `/Observation?_profile=http://hl7.org/fhir/uv/genomics-reporting/StructureDefinition/variant&_include=Observation:subject`;

@jhughes982 jhughes982 temporarily deployed to test-env October 13, 2022 13:30 Inactive
@jhughes982 jhughes982 temporarily deployed to test-env October 13, 2022 13:41 Inactive
@jhughes982 jhughes982 marked this pull request as draft October 13, 2022 14:25
@jhughes982 jhughes982 temporarily deployed to test-env October 13, 2022 14:42 Inactive
@jhughes982 jhughes982 marked this pull request as ready for review October 13, 2022 14:52
@jhughes982 jhughes982 temporarily deployed to test-env October 13, 2022 14:53 Inactive
@jhughes982 jhughes982 temporarily deployed to test-env October 13, 2022 14:54 Inactive
@jhughes982 jhughes982 temporarily deployed to test-env October 13, 2022 15:04 Inactive
@jhughes982 jhughes982 temporarily deployed to test-env October 13, 2022 15:24 Inactive
Copy link
Collaborator

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

Looking good, some non-blocking suggestions

Comment on lines 13 to 20
type trimmedObservation = { cDnaChange: string | undefined; observationId: string | undefined };
type patientResult = {
firstName: string | undefined;
lastName: string | undefined;
patientId: string | undefined;
observations: trimmedObservation[];
};
export type parsedResultsModel = patientResult[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the convention for types is to have a capital first letter, or am I getting my languages mixed up?

const [parsedResults, setParsedResults] = useState<parsedResultsModel | null>(null);

useEffect(() => {
const observationQueryUrl = `${FHIR_URL}/Observation?_profile=http://hl7.org/fhir/uv/genomics-reporting/StructureDefinition/variant&_include=Observation:subject`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need the FHIR URL in this, and can remove the import

Suggested change
const observationQueryUrl = `${FHIR_URL}/Observation?_profile=http://hl7.org/fhir/uv/genomics-reporting/StructureDefinition/variant&_include=Observation:subject`;
const observationQueryUrl = `/Observation?_profile=http://hl7.org/fhir/uv/genomics-reporting/StructureDefinition/variant&_include=Observation:subject`;

return subjectIdLong.includes(patientId);
});

if (!patientObservations || patientObservations.length === 0) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

personal preference but I'd aim to make logic branches obvious

Suggested change
if (!patientObservations || patientObservations.length === 0) return;
if (!patientObservations || patientObservations.length === 0) {
return;
}

};

if (!parsedResults) {
return <div>Getting observations.</div>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this getting observations at that point? - I would have thought that's covered in loading spinner but maybe I'm getting confused

if (!observation || !observation.component) return;

const trimmedObservation = observation.component.filter((component) => {
const loincCode = "48004-6";
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be worth having this as a constant at the top level of the file and name it as something like HGVS_CDNA_LOINC

@jhughes982 jhughes982 temporarily deployed to test-env October 14, 2022 08:29 Inactive
@jhughes982 jhughes982 merged commit b5cbcc3 into main Oct 14, 2022
@jhughes982 jhughes982 deleted the results-list-route branch October 14, 2022 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

View list of all results
2 participants