Show errors 500 and 400 on preview #639

Klochette · 2021-09-23T14:08:36Z

Fixes

Fixes #622

Description

Handle errors 500 and 400 on preview with notistack.

Definition of Done

I followed the Arkhn Code Book (I swear!).
I have written tests for the code I added or updated.
I have updated the documentation according to my changes.
I have updated the deployment configuration if needed.

…ert component

BPierrick · 2021-09-28T15:52:41Z

app/src/features/Preview/Preview.tsx

+        const errorKey = Object.keys(errors);
+        errorKey.forEach((key) => {
+          enqueueSnackbar(
+            t<string>("catchValidationErrorPrompt", {
+              query: errorField,
+              errorStatus: errorType,
+              errorKey: key,
+              errorText: errors[key as keyof typeof errors],
+            }),
+            {
+              variant: "error",
+            }
+          );
+        });


I suggest you use Object.entries instead of Object.keys, because of what you do in line 167 => accessing errors[key]

Suggested change

const errorKey = Object.keys(errors);

errorKey.forEach((key) => {

enqueueSnackbar(

t<string>("catchValidationErrorPrompt", {

query: errorField,

errorStatus: errorType,

errorKey: key,

errorText: errors[key as keyof typeof errors],

}),

{

variant: "error",

}

);

});

const errorEntries = Object.entries(errors);

errorEntries.forEach(([key, text]) => {

enqueueSnackbar(

t<string>("catchValidationErrorPrompt", {

query: errorField,

errorStatus: errorType,

errorKey: key,

errorText: text,

}),

{

variant: "error",

}

);

});

BPierrick · 2021-09-28T15:55:40Z

app/src/features/Preview/Preview.tsx

          }
        };
        previewCreate();
      }
    }
  };

+  useEffect(() => {


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

…ror types

BPierrick · 2021-10-11T11:48:12Z

app/src/features/Preview/Preview.tsx

+
+  const handleError = useCallback(
+    (error: FetchBaseQueryError) => {
+      const typedError = error;


What does this line do?

nothing anymore, i forgot to delete it

BPierrick · 2021-10-11T11:48:43Z

app/src/features/Preview/Preview.tsx

+  );
+
+  const handleError = useCallback(
+    (error: FetchBaseQueryError) => {


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

No it doesn't

Nvm I thought we were in a catch statement

BPierrick · 2021-10-11T11:51:33Z

app/src/features/Preview/Preview.tsx

-          setAlerts([
+  const handleValidationError = useCallback(
+    (
+      errors: ApiValidationError<unknown> | undefined,


Suggested change

errors: ApiValidationError<unknown> | undefined,

errors?: ApiValidationError<unknown>,

BPierrick · 2021-10-11T11:53:55Z

app/src/features/Preview/Preview.tsx

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


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) {

BPierrick · 2021-10-11T11:56:58Z

app/src/features/Preview/Preview.tsx

+      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",
+        });
+      }
+    },


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

BPierrick · 2021-10-11T11:58:18Z

app/src/features/Preview/Preview.tsx

+
+      const explore = async () => {
+        try {
+          const exploration = await apiExploreCreate({


You are shadowing the exploration variable

BPierrick · 2021-10-11T12:01:36Z

app/src/services/api/generated/api.generated.ts

 export type OperationOutcomeIssue = {
-  severity: SeverityEnum;
+  severity: string;


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

BPierrick · 2021-10-11T12:05:31Z

river-schema.yml

+    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


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

BPierrick · 2021-10-11T13:20:27Z

tests/common/adapters/__snapshots__/test_fhir_api.ambr

+# 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',
+    },
+  }
+---


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

must be removed (remains from the merge I guess)

BPierrick · 2021-10-11T15:23:40Z

app/src/features/Preview/Preview.tsx

@@ -300,7 +363,7 @@ const Preview = (): JSX.Element => {
          <Alert
            key={index}
            className={classes.alert}
-            severity={issue.severity}
+            severity={issue.severity as Color}


issue.severity should already be of type Color

BPierrick

lgtm

BPierrick · 2021-10-11T16:21:28Z

app/src/features/Preview/Preview.tsx

+  );
+
+  const handleError = useCallback(
+    (error: FetchBaseQueryError) => {


Nvm I thought we were in a catch statement

BPierrick · 2021-10-11T16:21:36Z

app/src/features/Preview/Preview.tsx

+  );
+
+  const handleError = useCallback(
+    (error: FetchBaseQueryError) => {


Nvm I thought we were in a catch statement

simonvadee

some python files have been changes where the shouldn't: can you reset them to their version on v4 ? you can do:

git reset <commit hash> <filename>

to restore the correct version

simonvadee · 2021-10-11T16:51:13Z

tests/common/adapters/__snapshots__/test_fhir_api.ambr

+# 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',
+    },
+  }
+---


must be removed (remains from the merge I guess)

Klochette · 2021-10-12T09:21:03Z

some python files have been changes where the shouldn't: can you reset them to their version on v4 ? you can do:
git reset <commit hash> <filename>
to restore the correct version

I think I did it correctly

simonvadee

use "squash" instead of rebase to avoid conflicts

simonvadee and others added 19 commits September 22, 2021 17:08

feat(river): use HAPI FHIR $validate in preview

503a121

clean(settings): remove PYROG_API_URL

bfd46e7

clean(docker): use env variables in docker-compose

a24267f

fix(preview): update preview serializer

1fc63ba

feat(app): use OperationOutcomeIssue in preview errors

ef15842

deps: bump "requests" package to 2.26.0

e18286c

deps: add test dependency to "responses"

6a312e5

fix(fhir_api): validate() does not raise on non-2XX

480026d

tests(adapters): add tests for fhir_api adapter

81db5ef

test(preview): test preview with validation errors

084980e

refactor(tests): clean fhir_api adapter tests

ca4a42a

fix(tests): scope of api_client is function

50d0092

fix(fhir_api): differentiate _get and _post

8e4c5b7

fix(tests): scope api_client for "module"

703bd38

chore(tests): add fhir_api marker

0d0ba75

fix(tests): remove unsued snapshots

29cd41b

fix[app]: handle api errors in preview with notistack and not only al…

5635678

…ert component

refactor(app): creation of an handling error function

102de13

fix(app): add handling of server errors

06b813e

Klochette requested review from tevariou, BPierrick, simonvadee and a team September 23, 2021 14:09

Klochette linked an issue Sep 23, 2021 that may be closed by this pull request

Errors 500 are not shown on preview #622

Closed

1 task

Klochette added type/bug Something isn't working prio/high proj/new-pyrog labels Sep 23, 2021

simonvadee force-pushed the sv/use-hapi-fhir-validator branch from 29cd41b to 0ee5627 Compare September 24, 2021 12:39

BPierrick reviewed Sep 28, 2021

View reviewed changes

simonvadee force-pushed the sv/use-hapi-fhir-validator branch from fc65673 to 4f61fd4 Compare October 5, 2021 14:57

simonvadee force-pushed the sv/use-hapi-fhir-validator branch 4 times, most recently from 1c5e0e6 to 4f61fd4 Compare October 5, 2021 15:22

Base automatically changed from sv/use-hapi-fhir-validator to v4 October 5, 2021 16:01

Klochette and others added 3 commits October 11, 2021 10:36

Merge branch 'v4' into mc/app/showErrorsOnPreview

7a29917

fix(app): merging v4 in branch

43dd5d6

fix(app): corrections from reviews and add functions for different er…

84c6f14

…ror types

Klochette requested a review from BPierrick October 11, 2021 10:07

BPierrick suggested changes Oct 11, 2021

View reviewed changes

Klochette added 2 commits October 11, 2021 14:12

fix(app): fix error display on validation

b88472e

fix(app): river schema reset

1051621

BPierrick reviewed Oct 11, 2021

View reviewed changes

fix(app): resolve reviews, change api generated to what's in v4

77ee7b0

Klochette requested a review from BPierrick October 11, 2021 13:24

BPierrick suggested changes Oct 11, 2021

View reviewed changes

fix(app): delete cast for color

a6ae5ae

Klochette requested a review from BPierrick October 11, 2021 15:30

BPierrick approved these changes Oct 11, 2021

View reviewed changes

simonvadee reviewed Oct 11, 2021

View reviewed changes

Klochette added 2 commits October 12, 2021 11:19

fix(app): reset to v4

15bb1b7

fix(app): reset ambr

3483a36

Klochette requested a review from simonvadee October 12, 2021 09:20

simonvadee approved these changes Oct 12, 2021

View reviewed changes

Klochette merged commit 39476cc into v4 Oct 12, 2021

Klochette deleted the mc/app/showErrorsOnPreview branch October 12, 2021 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show errors 500 and 400 on preview #639

Show errors 500 and 400 on preview #639

Klochette commented Sep 23, 2021 •

edited

Loading

BPierrick Sep 28, 2021

BPierrick Sep 28, 2021

BPierrick Oct 11, 2021

Klochette Oct 11, 2021

BPierrick Oct 11, 2021

Klochette Oct 11, 2021

BPierrick Oct 11, 2021

BPierrick Oct 11, 2021

BPierrick Oct 11, 2021

BPierrick Oct 11, 2021 •

edited

Loading

BPierrick Oct 11, 2021

BPierrick Oct 11, 2021

BPierrick Oct 11, 2021

BPierrick Oct 11, 2021

BPierrick Oct 11, 2021

simonvadee Oct 11, 2021

BPierrick Oct 11, 2021

BPierrick left a comment

BPierrick Oct 11, 2021

BPierrick Oct 11, 2021

simonvadee left a comment

simonvadee Oct 11, 2021

Klochette commented Oct 12, 2021

simonvadee left a comment

	errors: ApiValidationError<unknown> \| undefined,
	errors?: ApiValidationError<unknown>,

Show errors 500 and 400 on preview #639

Show errors 500 and 400 on preview #639

Conversation

Klochette commented Sep 23, 2021 • edited Loading

Fixes

Description

Definition of Done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BPierrick Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BPierrick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonvadee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Klochette commented Oct 12, 2021

simonvadee left a comment

Choose a reason for hiding this comment

Klochette commented Sep 23, 2021 •

edited

Loading

BPierrick Oct 11, 2021 •

edited

Loading