Using middleware for error pages break the response structure our app is expecting

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mjschranz, Assigned: mjschranz)

Tracking

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
For example, let's look at the steps taken when loading in data from a url such as https://popcorn.webmaker.org/editor/37199/edit.

What we expect to happen is:
1. We grab the email on session and look to see if the project not only exists but if it belongs to this person. If either happen, we throw either a 500 or 404 depending on if it existed and if they owned this project.
2. We then try to remix that project. If it didn't exist then it will fail again and then fallback to loading our base empty project.

Step two doesn't happen. Previous, we returned a nice JSON object. Now we just return an HTML page in one gigantic string.

Basically, we never hit https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/src/butter.js#L887-L909 and go straight to https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/src/butter.js#L911-L912. Thankfully our code is robust enough that the app doesn't implode :)

Comment 1

5 years ago
If we wanted to be smart about this, could we send a "Accept: application/json" header with the XHR request, and then inside our global error handler check the Accept header so we could return the error as a JSON response instead of an HTML page.
(Assignee)

Comment 2

5 years ago
Created attachment 766661 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/74

I think 1 review is fine here. Whoever can first works for me.
Attachment #766661 - Flags: review?(jon)
Attachment #766661 - Flags: review?(david.humphrey)
(Assignee)

Comment 3

5 years ago
Did Jon's review comments.
Comment on attachment 766661 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/74

Flag me again once Jon has signed off on this.
Attachment #766661 - Flags: review?(david.humphrey)

Comment 5

5 years ago
Comment on attachment 766661 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/74

r+ with nits, add the res.json bits to the 404 handler in server.js as well.
Attachment #766661 - Flags: review?(jon) → review+
(Assignee)

Comment 6

5 years ago
Comment on attachment 766661 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/74

Something changed that caused my original intent to break.

Sorry Jon! Need another quick once over. Or Scott if you have time.
Attachment #766661 - Flags: review?(scott)
Attachment #766661 - Flags: review?(jon)
Attachment #766661 - Flags: review+
Comment on attachment 766661 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/74

It is working when I try to edit a project while not logged in, it forces a remix.

If I am logged in, it instead loads a blank page.
Attachment #766661 - Flags: review?(scott) → review-
Comment on attachment 766661 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/74

Yup, now working with both remix and edit while not signed in, and while signed in via a different account.
Attachment #766661 - Flags: review?(scott)
Attachment #766661 - Flags: review?(jon)
Attachment #766661 - Flags: review+

Comment 10

5 years ago
Commit pushed to master at https://github.com/mozilla/popcorn.webmaker.org

https://github.com/mozilla/popcorn.webmaker.org/commit/196f2ace8b1fe9f416a1d4308dd3f1e03d24747b
Fix Bug 885883 - Return JSON objects for API request fails rather than hitting our error page when request in headers

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.