[risk=no][RW-8242] Use fetchWithErrorModal() for workspace-about API calls #7082

jmthibault79 · 2022-10-20T19:51:55Z

Add a new fetchWithErrorModal() function which wraps API calls, and use this function for all workspace-about API calls.

By default, it pops up an error modal when the API returns an error (like 404). It takes an optional options object with the following parameters:

onSuccess - a function to execute on the successful result
expectedResponseMatcher - whether to silence specific errors when appropriate (such as 404 for runtime polling)
customErrorResponseFormatter - custom error modal text and an optional onDismiss handler

The error modal behavior is powered by the existing notificationStore / NotificationModal

An example of the default handler

A custom handler which is only applied for 404 (otherwise the default is used)

          customErrorResponseFormatter: (er: ErrorResponse) =>
            er.statusCode === 404 && {
              title: 'Foobar not found',
              message:
                'The Foobar may not have been created yet - please try again in a few minutes',
            },

PR checklist

jmthibault79 · 2022-10-20T19:53:26Z

ui/src/app/pages/workspace/workspace-about.tsx

-        .catch((error) => {
-          console.error(error);
-        });
+      fetchWithErrorModal(() =>


the simplest way to use the new fetchWithErrorModal: a default popup will appear with info if there's an error. See errors and errors.spec for more examples

jmthibault79 · 2022-10-20T19:57:48Z

ui/src/app/utils/errors.tsx

+    () => ''
+  );
+
+  return `An API error${errorCodeStr} occurred${detailsStr}${messageStr}`;


See errors.spec for examples of how the final result looks

yonghaoy · 2022-10-21T14:55:03Z

ui/src/app/utils/errors.tsx

+    errorCode && errorCode !== ErrorCode.PARSEERROR
+      ? ` of type ${errorCode.toString()}`
+      : '';
+  const messageStr = message ? `: ${message}` : '.';


Encode message to remove HTML escaping?

Do you have an example of where this would be necessary?

if I create a corhort/dataset/something else with ‘<’, ‘>’, ‘alert’. inside.
Would that lead to XSS attack if we want to prin balbla not found

Text in React components is escaped under all but the most unusual circumstances.

ui/src/app/utils/errors.tsx

dmohs · 2022-10-24T16:29:12Z

ui/src/app/pages/workspace/workspace-about.tsx

+        fetchWithErrorModal(() =>
+          profileApi().updatePageVisits({ page: pageId })
+        );


I would probably return the promises for these so the caller can make the decision about whether it makes sense to await them.

Suggested change

fetchWithErrorModal(() =>

profileApi().updatePageVisits({ page: pageId })

);

return fetchWithErrorModal(() =>

profileApi().updatePageVisits({ page: pageId })

);

dmohs · 2022-10-24T16:33:46Z

ui/src/app/utils/errors.tsx

+    errorCode && errorCode !== ErrorCode.PARSEERROR
+      ? ` of type ${errorCode.toString()}`
+      : '';
+  const messageStr = message ? `: ${message}` : '.';


Text in React components is escaped under all but the most unusual circumstances.

dmohs · 2022-10-24T16:35:22Z

ui/src/app/utils/errors.tsx

+ * await responseP.then(resp =>
+ *  errorHandlerWithFallback(
+ *    resp,
+ *    (er) => er.statusCode === 404,


nit: err is more idiomatic for an error variable name.

ui/src/app/pages/workspace/workspace-about.tsx

dmohs · 2022-10-24T19:10:07Z

ui/src/app/utils/errors.tsx

+  return apiCall()
+    .then(onSuccess)
+    .catch(async (apiError) => {


Suggested change

return apiCall()

.then(onSuccess)

.catch(async (apiError) => {

return apiCall()

.catch(async (apiError) => {

ui/src/app/utils/errors.tsx

dmohs · 2022-10-24T19:46:39Z

ui/src/app/utils/errors.tsx

+  expectedResponseMatcher?: (ErrorResponse) => boolean,
+  customErrorResponseFormatter?: (ErrorResponse) => NotificationStore
+): Promise<NotificationStore> =>
+  convertAPIError(anyApiErrorResponse).then((errorResponse) => {


This function (convertAPIError) perhaps should be avoided. It takes an ErrorResponse, calls .json() on it, and then throws it away if that method fails.

a new version of this is toApiErrorResponse() which retains all fields in the original and the json()

TEMP JOEL HACK force 404 in getFirecloudWorkspaceUserRoles() callWithFallbackErrorModal pocErrorHandler

jmthibault79 · 2022-10-25T19:35:11Z

ui/src/app/pages/workspace/workspace-about.tsx

      this.loadUserRoles(workspace);
    }

-    async loadInitialCreditsUsage(workspace: WorkspaceData) {


we don't need to await these, so they don't need to be async

jmthibault79 commented Oct 20, 2022

View reviewed changes

jmthibault79 requested review from NehaBroad and dmohs October 20, 2022 21:22

jmthibault79 force-pushed the joel/RW-8242 branch from e8656db to 57df2e4 Compare October 21, 2022 14:16

jmthibault79 marked this pull request as ready for review October 21, 2022 14:39

jmthibault79 changed the title ~~[risk=no][RW-8242] Error handling POC~~ [risk=no][RW-8242] Use fetchWithErrorModal() for workspace-about API calls Oct 21, 2022

yonghaoy reviewed Oct 21, 2022

View reviewed changes

jmthibault79 commented Oct 21, 2022

View reviewed changes

ui/src/app/utils/errors.tsx Outdated Show resolved Hide resolved

dmohs approved these changes Oct 24, 2022

View reviewed changes

jmthibault79 added 20 commits October 25, 2022 11:29

WIP generic error handling

9793d43

TEMP JOEL HACK force 404 in getFirecloudWorkspaceUserRoles() callWithFallbackErrorModal pocErrorHandler

post rebase lint

43fa31c

WIP cleaner

637bd06

works

9703aeb

begin unit testing

9b273e9

more unit testing

6667854

lint

09ed1ed

revert

0781301

onSuccess

90eac63

optional onSuccess

29213fd

FetchOptions

cf44070

fix

46037af

WIP new defaultErrorResponseFormatter

abc8290

restructuring WIP

edcd22c

works

a756200

test fixes

266056a

cleanup

b2c1e3f

again

b6bd25d

DONE

d0cf43a

api done

3d53b85

jmthibault79 force-pushed the joel/RW-8242 branch from 57df2e4 to 3d53b85 Compare October 25, 2022 19:28

jmthibault79 commented Oct 25, 2022

View reviewed changes

jmthibault79 added 2 commits October 25, 2022 15:39

defaultApiErrorFormatter and rm PARSE_ERROR check

97b7be6

parsedResponse

0b0adc1

jmthibault79 merged commit 9e8fcb1 into main Oct 25, 2022

jmthibault79 deleted the joel/RW-8242 branch October 25, 2022 20:11

[risk=no][RW-8242] Use fetchWithErrorModal() for workspace-about API calls #7082

[risk=no][RW-8242] Use fetchWithErrorModal() for workspace-about API calls #7082

Uh oh!

Conversation

jmthibault79 commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmthibault79 Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmthibault79 commented Oct 20, 2022 •

edited

Loading

jmthibault79 Oct 20, 2022 •

edited

Loading