Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

feat(client): Stop passing 400 page error messages via query parameters. #3715

Merged
merged 2 commits into from May 25, 2016
Merged

feat(client): Stop passing 400 page error messages via query parameters. #3715

merged 2 commits into from May 25, 2016

Conversation

shane-tomlinson
Copy link

@shane-tomlinson shane-tomlinson commented May 6, 2016 •
edited

Use a session cookie instead.

fixes #3649

@vladikoff and @jrgm - r?

@shane-tomlinson
Copy link
Author

Tisk tisk, I didn't test this with production resources and Circle is complaining.

@shane-tomlinson shane-tomlinson force-pushed the issue-3649-stop-sending-400-message-via-query-param branch from e0dc296 to 5d8eae8 Compare May 6, 2016 11:55
@shane-tomlinson shane-tomlinson force-pushed the issue-3649-stop-sending-400-message-via-query-param branch 2 times, most recently from d35f693 to b9d4e5b Compare May 9, 2016 10:04
@shane-tomlinson
Copy link
Author

@vladikoff - I'm calling this ready for review - hopefully the tests pass this time.

@shane-tomlinson shane-tomlinson mentioned this pull request May 9, 2016
Merged
@shane-tomlinson
Copy link
Author

@vladikoff - This approach still suffers from #2929. I could remove the session cookie clearing, or set a short expiry, users should probably only see the 400 page once anyways.

@vladikoff
Copy link
Contributor

related: #3434

@shane-tomlinson
Copy link
Author

Failing functional test, pulling back to me.

@shane-tomlinson
Copy link
Author

@vladikoff - suggestions on handling #2929 and #3434? They seem to conflict. On page reload, we could keep showing the same message, or we could remove the cookie, check for the existence of the cookie after page reload, and if it's not there send the user to /signin?

@shane-tomlinson shane-tomlinson force-pushed the issue-3649-stop-sending-400-message-via-query-param branch from d0811a6 to 326a96d Compare May 9, 2016 21:17
@vladikoff
Copy link
Contributor

On page reload, we could keep showing the same message, or we could remove the cookie

I think we should remove the cookie in the hope the refresh fixed the problem (if js error)

@vladikoff
Copy link
Contributor

from mtg: watch out for the spinner, it hides things!

@vladikoff
Copy link
Contributor

from mtg: stop redirecting, replace the current view with the whatever the 400.html shows now:

User refreshes, either it fixes itself or shows the same error. That should also fix issue: #2929 // #3434

@vladikoff
Copy link
Contributor

#2929 // #3434

wow lucky numbers. from mtg: We also should do the same for unexpected_error and 500.html

@shane-tomlinson shane-tomlinson force-pushed the issue-3649-stop-sending-400-message-via-query-param branch from 29e644a to 3f7e2e3 Compare May 16, 2016 13:45
@shane-tomlinson
Copy link
Author

@vladikoff - I have updated this with the changes we talked about on Friday. Passing over to you!

@shane-tomlinson shane-tomlinson force-pushed the issue-3649-stop-sending-400-message-via-query-param branch from 3f7e2e3 to afbaaa0 Compare May 16, 2016 13:55
@vladikoff
Copy link
Contributor

Needs rebase

@vladikoff
Copy link
Contributor

@shane-tomlinson one issue I found, if you navigate directly to http://localhost:3030/unexpected_error then it spins forever

vladikoff
vladikoff reviewed May 24, 2016
View reviewed changes
app/scripts/lib/constants.js
BAD_REQUEST_PAGE: '/400.html',
// delay before redirecting to the error page to
// ensure metrics are reported to the backend.
ERROR_REDIRECT_TIMEOUT_MS: 1000,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Shane Tomlinson added 2 commits May 25, 2016 10:28
feat(client): Stop passing 400 page error messages via query parameters.
701eea8 
Use a session cookie to pass the message instead.

fixes #3649
fix(client): No longer redirect on error.
fdb23a7 
Instead, print the 400 and 500 error pages directly in content.
500.html is left in the repo because nginx redirects to it.

Add a new module, the domWriter, which writes to the DOM. This is
used by ErrorUtils to display the correct error page.

Ditch the `/unexpected_error` page.
@shane-tomlinson shane-tomlinson force-pushed the issue-3649-stop-sending-400-message-via-query-param branch from afbaaa0 to fdb23a7 Compare May 25, 2016 09:33
@shane-tomlinson
Copy link
Author

@shane-tomlinson one issue I found, if you navigate directly to http://localhost:3030/unexpected_error then it spins forever

Did you restart your content server? This is what I see:

screen shot 2016-05-25 at 10 37 40

@shane-tomlinson
Copy link
Author

@vladikoff - rebased.

@vladikoff vladikoff merged commit 2cad043 into master May 25, 2016
This was referenced May 25, 2016
Closed
Closed
@shane-tomlinson shane-tomlinson deleted the issue-3649-stop-sending-400-message-via-query-param branch October 20, 2016 14:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@vladikoff vladikoff

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stop sending error messages to the 400/500 pages
2 participants
@shane-tomlinson @vladikoff