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

Make DB timeout message more explicit #284

Merged
merged 15 commits into from
Jan 10, 2020

Conversation

dvvanessastoiber
Copy link
Contributor

Fixes #258

Summary

  • try/catch block in db.py handles OperationalError of type sqlalchemy.exc.OperationalError
  • HTTP error code 408 is sent with abort(408, error)
  • errorAlertHandler in notifications.ts is extended to print custom error message:

image

Since i18n is used, the error message can be further customized.

in order to avoid duplicate and redundant code
in order to print custom error message
(retrieve `statement_timeout` from _config.json_ in tdp_publicdb)
* move require statement into function
* use `===`
* add missing semicolon
* avoid object literal by using variable
and generalize error message
otherwise html tags are printed out
@dvvanessastoiber dvvanessastoiber added the type: feature New feature or request label Jan 8, 2020
@thinkh thinkh changed the title Make db timeout message explicit Make DB timeout message more explicit Jan 8, 2020
@thinkh thinkh requested a review from lehnerchristian January 8, 2020 14:52
tdp_core/db.py Show resolved Hide resolved
@dvvanessastoiber dvvanessastoiber added the release: minor PR merge results in a new minor version label Jan 8, 2020
Copy link

@lehnerchristian lehnerchristian 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 to me. how could I reproduce this issue?

@dvvanessastoiber
Copy link
Contributor Author

looks good to me. how could I reproduce this issue?

The steps to reproduce this issue are documented here

@lehnerchristian
Copy link

looks good to me. how could I reproduce this issue?

The steps to reproduce this issue are documented here

Oh, sorry. didn't see the linked issue

@dvvanessastoiber
Copy link
Contributor Author

looks good to me. how could I reproduce this issue?

The steps to reproduce this issue are documented here

Oh, sorry. didn't see the linked issue

No problem. Sorry for documenting it in an unclear way!

@lehnerchristian
Copy link

looks good to me. how could I reproduce this issue?

The steps to reproduce this issue are documented here

Oh, sorry. didn't see the linked issue

No problem. Sorry for documenting it in an unclear way!

it was clearly documented, however I didn't check the connected issue ;)

but I cannot reproduce the issue as described, because the query doesn't take 1 second to execute for me. however if it works for you, it also works for me. @thinkh , if you can reproduce the issue and the fix, we can merge

@thinkh
Copy link
Member

thinkh commented Jan 10, 2020

@lehnerchristian You could also set the statement timeout to '1ms'.

@@ -48,6 +48,9 @@ let errorAlertHandler = (error: any) => {
if (error instanceof Response || error.response instanceof Response) {
const xhr: Response = error instanceof Response ? error : error.response;
return xhr.text().then((body: string) => {
if (xhr.status === 408) {
body = i18n.t('ordino_public:core.timeoutMessage');
Copy link
Member

Choose a reason for hiding this comment

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

@dvvanessastoiber Using ordino_public in tdp_core does not work.

Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

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

After reverting the i18n namespace it works again.

grafik

@thinkh thinkh merged commit 83344f7 into develop Jan 10, 2020
@thinkh thinkh deleted the stoiber/258_make_db_timeout_message_explicit branch January 10, 2020 10:27
@thinkh thinkh mentioned this pull request Jan 13, 2020
29 tasks
@ghost ghost mentioned this pull request Mar 19, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: minor PR merge results in a new minor version type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants