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

feat(error): Server error handler (DSP-710) #355

Merged
merged 17 commits into from Jan 12, 2021

Conversation

@kilchenmann
Copy link
Collaborator

@kilchenmann kilchenmann commented Jan 9, 2021

resolves DSP-710 (s. discussion)

closes #311

  • Implementation of error handler service with following job:

    • depending on error status show message from dsp-ui notification service (mat snackbar) or show full-size error page (case 500 and 503 / server error)
  • Add error handler to all components and services where we do api requests (incl. refactoring of Imports)

  • Error test page /teapot with status 418 (easteregg)

    • shows the default app error; default means it's not one of the most common error we have: 403, 404, 500 or 503
    • change the status number in app-routing.module.ts from 418 to another one (e.g. 403, 404, 500 or 503) to see the different error pages
  • Test the implementation of error handler:

    • run the app and turn off knora-stack (error 500) or at least fuseki (error 503): it should show a full-size "Service unavailable" error message
    • turn on knora-stack or fuseki again and press "reload page" link in the error message and it should load the page again
    • with turned-off fuseki the only page that works is the /help page because there we do not do db-relevant requests.

Comment: We could add a link to https://status.dasch.swiss in one of the error 5xx page. So, user will see what's going on on the server. At the moment this service is only implemented for the prod server and not yet for the test (or staging) server which would be https://status.test.dasch.swiss (https://status.staging.dasch.swiss). Btw with this status page the ops-developer team will be informed when a service doesn't run anymore. So, we don't need a contact button anymore on the server error page to send this kind of error report.

@kilchenmann kilchenmann requested review from flavens and mpro7 Jan 10, 2021
@mpro7
Copy link
Contributor

@mpro7 mpro7 commented Jan 11, 2021

image
Before you get the error page it is possible to see for milliseconds the page under it, with loading animation and snackbar initiated too I think. Here is the recording:

Screen.Recording.2021-01-11.at.15.20.51.copy.mov

Loading

mpro7
mpro7 approved these changes Jan 11, 2021
Copy link
Contributor

@mpro7 mpro7 left a comment

Seems ok for me. Only that blinking thing while loading the error page I've mentioned in previous comment. If not easy thing to fix, might be moved to another task/bug.

Loading

@@ -540,7 +540,7 @@ $gc-small: $form-width - $gc-large - 4;

.cdk-overlay-pane {
.mat-dialog-container {
max-height: 80vh !important;
// max-height: 80vh !important;
Copy link
Contributor

@mpro7 mpro7 Jan 11, 2021

Choose a reason for hiding this comment

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

why not to remove?

Loading

Copy link
Collaborator Author

@kilchenmann kilchenmann Jan 12, 2021

Choose a reason for hiding this comment

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

wasn't sure if we still need it and we have to fix the height in another way. But yes, let's delete this line 😉

Loading

Copy link
Collaborator Author

@kilchenmann kilchenmann Jan 12, 2021

Choose a reason for hiding this comment

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

done in 924fc3d

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented Jan 12, 2021

image
Before you get the error page it is possible to see for milliseconds the page under it, with loading animation and snackbar initiated too I think. Here is the recording:

Screen.Recording.2021-01-11.at.15.20.51.copy.mov

Yes, I know this issue. For the moment we have to live with it. As you written above, it would be complicated to solve it in a quick way. This full-screen error page opens only in case of server errors. I hope, that we'll not have too many server errors 😆🙈
We should solve it in a separate task...

Loading

Copy link
Collaborator

@flavens flavens left a comment

it looks good but I strongly suggest that Mike checks the error messages.

Loading

@mpro7
Copy link
Contributor

@mpro7 mpro7 commented Jan 12, 2021

image
Before you get the error page it is possible to see for milliseconds the page under it, with loading animation and snackbar initiated too I think. Here is the recording:
Screen.Recording.2021-01-11.at.15.20.51.copy.mov

Yes, I know this issue. For the moment we have to live with it. As you written above, it would be complicated to solve it in a quick way. This full-screen error page opens only in case of server errors. I hope, that we'll not have too many server errors 😆🙈
We should solve it in a separate task...

Ok, could you please create the task if not already did.

Loading

@kilchenmann kilchenmann requested a review from mdelez Jan 12, 2021
@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented Jan 12, 2021

it looks good but I strongly suggest that Mike checks the error messages.

As @flavens suggested: @mdelez can you check the error messages? Thanks!
Btw. the messages:
500: "An error has occured in a server side script, a no more specific message is suitable"
and
503: "The server is currently unavailable (overloaded or down)"
are taken from our http status codes from dsp-ui-lib. I downloaded this list a few years ago from Wikipedia. Maybe using this description doesn't make much sense.

Loading

errorMessages: ErrorMsg[] = [
{
status: 0,
message: "Undefined Error",
description: `Maybe I'm a teapot, maybe not.<br>
But anyway, something undefined went wrong.`,
action: 'goback',
image: 'dsp-error.svg'
},
{
status: 403,
message: "Forbidden",
description: `This is not the content you were looking for.<br>
Your request was valid but you do not have the<br>
necessary permissions to access it.`,
action: 'goback',
image: 'dsp-error-403.svg'
},
{
status: 404,
message: "Not found",
description: `This is not the content you were looking for.<br>
But we couldn't find anything with this request.`,
action: 'goback',
image: 'dsp-error-404.svg'
},
{
status: 500,
message: "Internal Server Error",
description: `The DaSCH Service Platform is not available at the moment.<br>
An error has occured in a server side script, a no more specific message is suitable.`,
action: 'reload',
image: 'dsp-error-500.svg'
},
{
status: 503,
message: "Service unavailable",
description: `The DaSCH Service Platform is not available at the moment.<br>
The server is currently unavailable (overloaded or down).`,
action: 'reload',
image: 'dsp-error-503.svg'
}
];
Copy link
Collaborator Author

@kilchenmann kilchenmann Jan 12, 2021

Choose a reason for hiding this comment

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

@mdelez Here are the error messages defined...

Loading

Copy link
Contributor

@mdelez mdelez left a comment

Wording improvements :)

Loading

src/app/main/error/error.component.ts Outdated Show resolved Hide resolved
Loading
src/app/main/error/error.component.ts Outdated Show resolved Hide resolved
Loading
src/app/main/error/error.component.ts Outdated Show resolved Hide resolved
Loading
@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented Jan 12, 2021

It seems that the tests failing from time to time. The problem is the dialog box:
Error: Found the synthetic listener @dialogContainer.start. Please include either "BrowserAnimationsModule" or "NoopAnimationsModule" in your application. I'll add the suggested module now.

Loading

@kilchenmann kilchenmann requested review from mdelez and mpro7 Jan 12, 2021
@mpro7
Copy link
Contributor

@mpro7 mpro7 commented Jan 12, 2021

@kilchenmann did adding the module in a49b63f help? Because it seems test failed again.
EDIT:
Ok. I see new task to solve this issue: DSP-1202. Thanks!

Loading

mpro7
mpro7 approved these changes Jan 12, 2021
mdelez
mdelez approved these changes Jan 12, 2021
@kilchenmann kilchenmann merged commit d5b77bf into main Jan 12, 2021
4 checks passed
Loading
@kilchenmann kilchenmann deleted the wip/dsp-710-handle-internal-server-error branch Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants