Skip to content
This repository has been archived by the owner on Jan 4, 2023. It is now read-only.

Show errors 500 and 400 on preview #639

Merged
merged 28 commits into from
Oct 12, 2021
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
503a121
feat(river): use HAPI FHIR $validate in preview
simonvadee Sep 14, 2021
bfd46e7
clean(settings): remove PYROG_API_URL
simonvadee Sep 15, 2021
a24267f
clean(docker): use env variables in docker-compose
simonvadee Sep 15, 2021
1fc63ba
fix(preview): update preview serializer
simonvadee Sep 16, 2021
ef15842
feat(app): use OperationOutcomeIssue in preview errors
simonvadee Sep 16, 2021
e18286c
deps: bump "requests" package to 2.26.0
simonvadee Sep 20, 2021
6a312e5
deps: add test dependency to "responses"
simonvadee Sep 20, 2021
480026d
fix(fhir_api): validate() does not raise on non-2XX
simonvadee Sep 20, 2021
81db5ef
tests(adapters): add tests for fhir_api adapter
simonvadee Sep 21, 2021
084980e
test(preview): test preview with validation errors
simonvadee Sep 21, 2021
ca4a42a
refactor(tests): clean fhir_api adapter tests
simonvadee Sep 22, 2021
50d0092
fix(tests): scope of api_client is function
simonvadee Sep 22, 2021
8e4c5b7
fix(fhir_api): differentiate _get and _post
simonvadee Sep 22, 2021
703bd38
fix(tests): scope api_client for "module"
simonvadee Sep 22, 2021
0d0ba75
chore(tests): add fhir_api marker
simonvadee Sep 22, 2021
29cd41b
fix(tests): remove unsued snapshots
simonvadee Sep 22, 2021
5635678
fix[app]: handle api errors in preview with notistack and not only al…
Sep 22, 2021
102de13
refactor(app): creation of an handling error function
Sep 22, 2021
06b813e
fix(app): add handling of server errors
Sep 23, 2021
7a29917
Merge branch 'v4' into mc/app/showErrorsOnPreview
Klochette Oct 11, 2021
43dd5d6
fix(app): merging v4 in branch
Oct 11, 2021
84c6f14
fix(app): corrections from reviews and add functions for different er…
Oct 11, 2021
b88472e
fix(app): fix error display on validation
Oct 11, 2021
1051621
fix(app): river schema reset
Oct 11, 2021
77ee7b0
fix(app): resolve reviews, change api generated to what's in v4
Oct 11, 2021
a6ae5ae
fix(app): delete cast for color
Oct 11, 2021
15bb1b7
fix(app): reset to v4
Oct 12, 2021
3483a36
fix(app): reset ambr
Oct 12, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 99 additions & 48 deletions app/src/features/Preview/Preview.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useState, useEffect } from "react";
import React, { useState, useEffect, useCallback } from "react";

import { IResource } from "@ahryman40k/ts-fhir-types/lib/R4";
import { Icon } from "@blueprintjs/core";
Expand All @@ -17,7 +17,9 @@ import {
useMediaQuery,
} from "@material-ui/core";
import Alert, { Color } from "@material-ui/lab/Alert";
import { FetchBaseQueryError } from "@reduxjs/toolkit/dist/query";
import clsx from "clsx";
import { useSnackbar } from "notistack";
import { useTranslation } from "react-i18next";
import ReactJson from "react-json-view";
import { useParams } from "react-router-dom";
Expand All @@ -29,6 +31,10 @@ import {
useApiOwnersRetrieveQuery,
useApiPreviewCreateMutation,
} from "services/api/endpoints";
import {
apiValidationErrorFromResponse,
ApiValidationError,
} from "services/api/errors";
import {
ExplorationResponse,
OperationOutcomeIssue,
Expand Down Expand Up @@ -136,6 +142,7 @@ const getAlertSeverityFromOperationOutcomeIssue = (
const Preview = (): JSX.Element => {
const classes = useStyles();
const { t } = useTranslation();
const { enqueueSnackbar } = useSnackbar();
const prefersDarkMode = useMediaQuery("(prefers-color-scheme: dark)");
const { mappingId } = useParams<{
mappingId?: string;
Expand All @@ -144,20 +151,19 @@ const Preview = (): JSX.Element => {
const [exploration, setExploration] = useState<
ExplorationResponse | null | undefined
>(undefined);

const [alerts, setAlerts] = useState<
AlertOperationOutcomeIssue[] | undefined
>(undefined);

const [preview, setPreview] = useState<IResource | undefined>(undefined);

const handleAlertClose = (index: number) => {
if (alerts) {
const newAlerts = [...alerts];
newAlerts.splice(index, 1);
setAlerts(newAlerts);
}
};

const [preview, setPreview] = useState<IResource | undefined>(undefined);

const { data: mapping } = useApiResourcesRetrieveQuery(
{ id: mappingId ?? "" },
{ skip: !mappingId }
Expand All @@ -172,39 +178,59 @@ const Preview = (): JSX.Element => {

const [apiExploreCreate] = useApiExploreCreateMutation();

useEffect(() => {
if (mappingId && owner && exploration === undefined) {
setExploration(null);

const explore = async () => {
try {
const exploration = await apiExploreCreate({
explorationRequestRequest: {
owner: owner?.name ?? "",
table: mapping?.primary_key_table ?? "",
resource_id: mappingId,
},
}).unwrap();
setExploration(exploration);
} catch (e) {
setAlerts([
const handleValidationError = useCallback(
(
errors: ApiValidationError<unknown> | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
errors: ApiValidationError<unknown> | undefined,
errors?: ApiValidationError<unknown>,

errorType: number | "FETCH_ERROR" | "PARSING_ERROR" | "CUSTOM_ERROR",
errorField: string
) => {
if (errors) {
Copy link
Contributor

@BPierrick BPierrick Oct 11, 2021

Choose a reason for hiding this comment

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

Declaring your props this way should raise you an error for defining facultative arguments before required ones

You can prevent this like this :

Suggested change
const handleValidationError = useCallback(
(
errors: ApiValidationError<unknown> | undefined,
errorType: number | "FETCH_ERROR" | "PARSING_ERROR" | "CUSTOM_ERROR",
errorField: string
) => {
if (errors) {
const handleValidationError = useCallback(
({
errors?: ApiValidationError<unknown>,
errorType: number | "FETCH_ERROR" | "PARSING_ERROR" | "CUSTOM_ERROR",
errorField: string
}) => {
if (errors) {

const errorEntries = Object.entries(errors);
errorEntries.forEach(([key, text]) => {
enqueueSnackbar(
t<string>("catchValidationErrorPrompt", {
query: errorField,
errorStatus: errorType,
errorKey: key,
errorText: text,
}),
{
severity: "error",
diagnostics: e.error,
code: "internal",
},
]);
}
};
explore();
}
}, [
apiExploreCreate,
exploration,
mapping?.primary_key_table,
mappingId,
owner,
]);
variant: "error",
}
);
});
}
},
[enqueueSnackbar, t]
);

const handleError = useCallback(
(error: FetchBaseQueryError) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Casting the error variable here doesn't raise any eslint error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it doesn't

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm I thought we were in a catch statement

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm I thought we were in a catch statement

const typedError = error;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this line do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nothing anymore, i forgot to delete it

const validationError = apiValidationErrorFromResponse(typedError);
if (validationError)
handleValidationError(validationError, typedError.status, "Preview");
else if (typedError.status === "PARSING_ERROR") {
enqueueSnackbar(
t<string>("catchValidationErrorPrompt", {
query: typedError.status,
errorStatus: typedError.originalStatus.toString(),
errorText: typedError.error,
}),
{ variant: "error" }
);
} else if (
(typedError.status === "FETCH_ERROR" ||
typedError.status === "CUSTOM_ERROR") &&
typedError.data
) {
enqueueSnackbar(`${typedError.status} : ${typedError.data as string}`, {
variant: "error",
});
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This is facultative, but maybe using a switch statement on the typedError.status variable would be more elegant.

[enqueueSnackbar, handleValidationError, t]
);

const [apiPreviewCreate] = useApiPreviewCreateMutation();

Expand All @@ -230,21 +256,46 @@ const Preview = (): JSX.Element => {
severity: getAlertSeverityFromOperationOutcomeIssue(error),
}))
);
} catch (e) {
setAlerts([
{
severity: "error",
diagnostics: e.error,
code: "internal",
},
]);
} catch (error) {
handleError(error as FetchBaseQueryError);
}
};
previewCreate();
}
}
};

// load the data for the table preview list
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a small comment explaining what this useEffect does?

if (mappingId && owner && exploration === undefined) {
setExploration(null);

const explore = async () => {
try {
const exploration = await apiExploreCreate({
Copy link
Contributor

Choose a reason for hiding this comment

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

You are shadowing the exploration variable

explorationRequestRequest: {
owner: owner?.name ?? "",
table: mapping?.primary_key_table ?? "",
resource_id: mappingId,
},
}).unwrap();
setExploration(exploration);
} catch (error) {
handleError(error as FetchBaseQueryError);
}
};
explore();
}
}, [
apiExploreCreate,
exploration,
mapping?.primary_key_table,
mappingId,
owner,
handleError,
enqueueSnackbar,
]);

return (
<Container className={classes.container} maxWidth="xl">
<BackButton className={classes.button} />
Expand Down Expand Up @@ -300,7 +351,7 @@ const Preview = (): JSX.Element => {
<Alert
key={index}
className={classes.alert}
severity={issue.severity}
severity={issue.severity as Color}
Copy link
Contributor

Choose a reason for hiding this comment

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

issue.severity should already be of type Color

onClose={() => handleAlertClose(index)}
>
{issue.diagnostics}
Expand All @@ -318,8 +369,8 @@ const Preview = (): JSX.Element => {
) : (
<div className={classes.texts}>
<Typography>
{t("clickOnAFireIcon")}{" "}
<Icon icon={IconNames.FLAME} className={classes.iconFlame} />{" "}
{t("clickOnAFireIcon")}
<Icon icon={IconNames.FLAME} className={classes.iconFlame} />
{t("inOrderToPreview")}
</Typography>
</div>
Expand Down
8 changes: 5 additions & 3 deletions app/src/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@
"exportMapping": "Export mapping",
"ETLDashboard": "ETL Dashboard",
"newMapping": "New mapping",
"clickOnAFireIcon": "Click on a",
"inOrderToPreview": "icon in order to preview a line as a FHIR instance",
"clickOnAFireIcon": "Click on a ",
"inOrderToPreview": " icon in order to preview a line as a FHIR instance",
"back": "Back",
"useThisGroupIf": "Use this group if...",
"resources": "Resources",
Expand All @@ -124,6 +124,7 @@
"chooseExistingURI": "Choose existing URI",
"selectSource": "Select a source",
"selectMapping": "Select a mapping",
"catchErrorPrompt": "{{ query }} | Error Type : {{ errorStatus }} | {{errorKey}} : {{errorText}}",
"second": "{{ count }} second",
"second_plural": "{{ count }} seconds",
"minute": "{{ count }} minute",
Expand All @@ -148,5 +149,6 @@
"batchErrors": "⚠️ {{errors}} errors",
"batchSuccess": "✅ Success",
"batchCanceled": "🚫 Canceled",
"noAvailableResource": "No available resource"
"noAvailableResource": "No available resource",
"catchValidationErrorPrompt": "{{ query }} | Error Type : {{ errorStatus }} | {{errorKey}} {{errorText}}"
}
10 changes: 5 additions & 5 deletions app/src/services/api/generated/api.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -943,7 +943,7 @@ export type ApiConditionsDestroyApiArg = {
id: string;
};
export type ApiCoreVersionRetrieveApiResponse = unknown;
export type ApiCoreVersionRetrieveApiArg = {};
export type ApiCoreVersionRetrieveApiArg = void;
export type ApiCredentialsListApiResponse = /** status 200 */ Credential[];
export type ApiCredentialsListApiArg = {
/** Which field to use when ordering the results. */
Expand Down Expand Up @@ -1462,14 +1462,14 @@ export type PatchedOwnerRequest = {
name?: string;
credential?: string;
};
export type SeverityEnum = "fatal" | "error" | "warning" | "information";
export type OperationOutcomeIssue = {
severity: SeverityEnum;
severity: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

You definitely shouldn't have those changes in your PR. I guess the merge you made was not from the latest commit of v4 ?

code: string;
diagnostics: string;
location: string[];
expression: string;
location?: string[];
expression?: string;
};
export type SeverityEnum = "fatal" | "error" | "warning" | "information";
export type PreviewResponse = {
instances: {
[key: string]: any;
Expand Down
2 changes: 0 additions & 2 deletions django/common/adapters/fhir_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ def create(self, resource_type: str, payload: dict, auth_token=None):
def validate(self, resource_type: str, payload: dict, auth_token=None):
"""Calls the /<resource_type>/$validate endpoint of HAPI FHIR.
Note that this function does not raise an exception if the status is not 2XX.

Args:
resource_type (str): the resource type
payload (dict): the FHIR instance
Expand All @@ -93,7 +92,6 @@ def validate(self, resource_type: str, payload: dict, auth_token=None):
"resourceType": "OperationOutcome",
"issue": [{"severity": "error", "code": "", "diagnostics": response.text}],
}

def retrieve(self, resource_type, resource_id, auth_token=None):
response = self._get(f"{self._url}/{resource_type}/{resource_id}", auth_token=auth_token)
response.raise_for_status()
Expand Down
21 changes: 21 additions & 0 deletions river-schema.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3433,6 +3433,27 @@ components:
- id
- name
- schema
OperationOutcomeIssue:
type: object
properties:
severity:
type: string
code:
type: string
diagnostics:
type: string
location:
type: array
items:
type: string
expression:
type: string
required:
- code
- diagnostics
- expression
- location
- severity
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why this is here. From v4 branch, OperationOutcomeIssue type should already be described at line 3396 :

OperationOutcomeIssue:
      type: object
      properties:
        severity:
          $ref: '#/components/schemas/SeverityEnum'
        code:
          type: string
        diagnostics:
          type: string
        location:
          type: array
          items:
            type: string
        expression:
          type: string
      required:
      - code
      - diagnostics
      - expression
      - location
      - severity

OwnerRequest:
type: object
properties:
Expand Down
11 changes: 11 additions & 0 deletions tests/common/adapters/__snapshots__/test_fhir_api.ambr
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# name: test_inmemory_fhir_api_validate
<class 'dict'> {
'issue': <class 'list'> [
],
'resourceType': 'OperationOutcome',
'text': <class 'dict'> {
'div': '<p> it went fine bro </p>',
'status': 'generated',
},
}
---
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what is this doing here? @simonvadee @tevariou any idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

must be removed (remains from the merge I guess)

2 changes: 0 additions & 2 deletions tests/common/adapters/test_fhir_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ def test_inmemory_fhir_api_create():
fhir_api.create("Patient", resource)
assert len(fhir_api._db["Patient"]) == 2


def test_inmemory_fhir_api_retrieve():
resource = {"deceased": "not yet"}

Expand Down Expand Up @@ -50,7 +49,6 @@ def test_hapi_fhir_api_create_failure(status, auth_token):
if auth_token:
assert responses.calls[0].request.headers.get("Authorization") == f"Bearer {auth_token}"


@responses.activate
@pytest.mark.parametrize(
"auth_token,response,status",
Expand Down
1 change: 0 additions & 1 deletion tests/river/unit/test_services.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ def test_abort(batch):
def test_retry(batch):
pass


def test_preview(mimic_mapping, snapshot):
# label: patient-resource-id
resource_id = "cktlnp0f300300mmznmyqln70"
Expand Down