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

Catch missing model 404s #7560

Merged
merged 2 commits into from
Jan 31, 2024
Merged

Conversation

jrjohnson
Copy link
Member

I'm not sure if this broke or was never working but we're not displaying a nice error when a model is missing. This doesn't happen that often for us, but when something is deleted we need to do better than or standard error message because refreshing the page will just continue to be broken. Instead we now display our "Rats!" message with a link back to the dashboard.

Fixes ilios/ilios#5127

I'm not sure if this broke or was never working but we're not displaying
a nice error when a model is missing. This doesn't happen that often for
us, but when something is deleted we need to do better than or standard
error message because refreshing the page will just continue to be
broken.  Instead we now display our "Rats!" message with a link back to
the dashboard.
If we split this route then ember's default error substate system
doesn't work anymore.
@jrjohnson jrjohnson force-pushed the 5127-deleted-report branch from 5cde3d4 to 1d2bfe0 Compare January 31, 2024 05:52
@jrjohnson jrjohnson marked this pull request as ready for review January 31, 2024 06:32
@jrjohnson jrjohnson requested a review from stopfstedt January 31, 2024 06:32
Copy link
Member

@stopfstedt stopfstedt left a comment

Choose a reason for hiding this comment

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

this does not work for me, hard stop on the default route (unauthenticated). i was expecting the login form, but got an exception instead. from the browser console:

Uncaught (in promise) TypeError: this.model.errors is undefined
    get isA404 error.js:19
...    

image

@stopfstedt stopfstedt self-requested a review January 31, 2024 16:34
@stopfstedt
Copy link
Member

hmm, seems to be working fine on netlify. i'm having another go at it locally.

@stopfstedt
Copy link
Member

i figured it out - forgot that we have to provide the backend URL on startup as env variable now since dotenv support is gone. *headdesk

all is well, this checks out.

Copy link
Member

@stopfstedt stopfstedt left a comment

Choose a reason for hiding this comment

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

LGTM

@dartajax dartajax merged commit b045952 into ilios:master Jan 31, 2024
18 checks passed
@jrjohnson jrjohnson deleted the 5127-deleted-report branch January 31, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zoinks Error Received - Reports
3 participants