adds: error page front-end concept #925

raygervais · 2020-04-03T04:46:30Z

Issue This PR Addresses

Fixes #890

Type of Change

Bugfix: Change which fixes an issue
New Feature: Change which adds functionality
Documentation Update: Change which improves documentation
UI: Change which improves UI

Description

This adds a catch-all route at the end of the routes declaration which redirects to our (WIP) 40X page component. Will add appropriate 50x page as well.

How To Access

From a fully deployed Docker-compose environment, attempt any url which you'd fat-finger by accident at 3am. This will redirect back to the front-end with the appropriate error response. Example, entering localhost:3000/AWorldWhereWindowsWins should redirect straight to localhost:3000/error?status=404 as seen below.

Checklist

Quality: This PR builds and passes our npm test and works locally
Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

raygervais · 2020-04-03T04:47:55Z

@cindyledev, @humphd, @agarcia-caicedo, looking for your input on how we should style and layout the 40X / 50X error pages. For the above, just to prove the concept I reused Banner layout styling. Was contemplating adding the banner (sans banner text) in the background.

PR is still WIP, looking for input and feedback.

raygervais · 2020-04-03T05:09:20Z

Because of the Zeit setup, you won't be able to follow the instructions first step (attempting to access any non-standard URL) and instead can test with this URL (which is the redirect): https://telescope-mxr4bagfx.now.sh/error?status=<PUT_WHATEVER_STATUS_YOUD_LIKE>. Locally, this works end-to-end.

cindyorangis · 2020-04-03T13:52:39Z

Tested on Zeit, I like where this is going. Unfortunately, this seems to be suffering from the css bug #841 #741 . We have at least 3 PRs open that are affected by it

cindyorangis · 2020-04-04T02:19:34Z

Also tested on Linux

humphd · 2020-04-04T15:30:38Z

src/backend/web/routes/index.js

+ * 404 Handler, Pass to front-end
+ * Leverage .status because adding the `404` status in redirect causes "Not Found. Redirecting to /404?search=" to display.
+ */
+router.use('*', (req, res) => {


The way this is usually done is as error handling middleware at the end of the routing chain, with an extra error handler at the end:

// Last in the chain app.use(function (req, res, next) { res.status(404).send('...'); }); // Error handling middleware app.use(function (err, req, res, next) { logger.error({ error: err }, 'express web server error'); res.status(500).send('...') });

humphd · 2020-04-04T15:31:11Z

src/backend/web/routes/index.js

+  const originalUrl = req.originalUrl.trim();
+  res
+    .status(404)
+    .redirect(`/404?search=${originalUrl}`)


You better encodeURIComponent() this originalUrl

humphd · 2020-04-04T15:31:47Z

src/backend/web/routes/index.js

+  res
+    .status(404)
+    .redirect(`/404?search=${originalUrl}`)
+    .sendFile(path.join(__dirname, '../../../frontend/public/index.html'));


Why redirect and send this file? Would you not just redirect?

humphd · 2020-04-04T15:32:10Z

src/frontend/src/pages/404.jsx

+  const params = new URLSearchParams(props.location.search);
+  let originalUrl = params
+    .get('search')
+    ?.replace('/', '')


I don't understand what this is doing. I'm also not sure that we should introduce ?. to our code base without discussing implications.

The ?. is the new optional chaining operator, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining

It's essentially a replacement of the following semantic:

const a= params.get('search'); if (a) { const b = a.replace('/', ''); if (b) { const c = b.match(....); ... and so on } }

The reason for the actual functions is removing the / at the start of the URL search param, which I may be able to remove with the urlEncode that you suggested above. Furthermore, it formats the param client-side to Capital Spaced if capital letters exist. Can remove, thought it was a nice little addon.

src/frontend/src/pages/404.jsx

humphd

This is looking amazing. I'm not sure about the purple colour you're introducing (cc @agarcia-caicedo). I left some initial comments in the code.

humphd · 2020-04-06T15:55:09Z

src/frontend/src/pages/404.jsx

+  },
+}));
+
+function RetrieveBackgroundAsset() {


Isn't this repeated code? Can we refactor this out into a separate component?

I'd like to refactor into a separate component in this PR, is that alright? Or separate follow-up?

Here is fine, nothing is depending on this for timing.

humphd · 2020-04-06T15:55:59Z

src/frontend/src/pages/404.jsx

+  const classes = useStyles();
+
+  return (
+    <PageBase title="404">


Either here or in follow-up, I'd love to generalize this page so that it works for 403s (forbidden) and 500s (errors).

I can do that here, keep all the scope around 40X and 50X pages starting here.

src/backend/web/app.js

agarcia-caicedo · 2020-04-08T02:54:20Z

This is looking amazing. I'm not sure about the purple colour you're introducing (cc @agarcia-caicedo). I left some initial comments in the code.

Can't say I'm a fan of the purple. I don't think it works aesthetically, and since it's the only place we've used it, it looks out of place

cindyorangis · 2020-04-11T03:43:50Z

I'm digging this so far but I'm not too sure about the purple back button. @agarcia-caicedo , what are your thoughts?

fixes: rebase and component cleanup reverts: colors back to secondary base adds: authenticaton 403 concept and 500 wip

Silvyre

Navigating to /404 (which is currently where My Feeds sends unauthorized users to) is still rendering the "Gatsby.js development 404 page" for me:

(HTML of Gatsby.js 404 page)

Gatsby.js development 404 page

There's not a page yet at /404

Preview custom 404 page

Create a React.js component in your site directory at src/pages/404.js and this page will automatically refresh to show the new page component you created.

If you were trying to reach another page, perhaps you can find it below.

Pages (6)

Search:

Is this intended behaviour; should I update the My Feeds to redirect users to somewhere else?

raygervais · 2020-04-15T19:23:53Z

This is because we don't have a custom 404 page, instead I'm using /error as a wrapper for both. I'd redirect there, or in a follow up PR I create a 404 which is a wrapper around error.

raygervais · 2020-04-15T19:24:30Z

You should be sending to 403, since they are not authorized @Silvyre. /error?status=403

Silvyre · 2020-04-15T19:57:26Z

I'm using /error as a wrapper for both. I'd redirect there, or in a follow up PR I create a 404 which is a wrapper around error.

Okay, cool: redirecting to /error?status=403 seems to work well. We could alternatively have the My Feeds page redirect logged-out users to the login page (which I recall is what @humphd may have suggested this morning). I filed an issue (#1016) to implement this.

A few other things...

Navigating to http://localhost:3000/error (without a query string) presents:

Do you think it would be worth populating the blank card here with the "Something went wrong!" text?

Also, I noticed that navigating to http://localhost:3000/error?status=40X renders the numerical status code (e.g. "40X") within the page title, which is really cool:

Do you think it would be worthwhile to add a switch that replaces some numerical status codes with their common names? e.g. being redirected to http://localhost:3000/error?status=404 would result in a page title of "Not Found | Telescope" instead of "404 | Telescope", etc. Ultimately up to you!

Lastly, I'm not sure whether this next issue affects only my own machine/browser, but when I navigate to a page like http://localhost:3000/error?status=404, the card/button component appears to have some margin issues:

This does not happen when I decrease the size of my browser window to a size breakpoint smaller:

Thanks for all of your hard work on this! 👍

humphd

One more thing I notice, then this is good from my end.

src/backend/web/authentication.js

raygervais · 2020-04-15T23:05:04Z

Thanks for testing so vigorously @Silvyre, that edge (where the user stumbles onto the page) I never thought of. Corrected in upcoming commit.

raygervais · 2020-04-16T02:31:39Z

@Silvyre can I get a re-review?

cindyorangis

I have some small nits in error.jsx:

instead of importing on multiple lines

import Typography from '@material-ui/core/Typography';
import { Fab } from '@material-ui/core';
import Grid from '@material-ui/core/Grid';
import Card from '@material-ui/core/Card';
import CardActions from '@material-ui/core/CardActions';
import CardContent from '@material-ui/core/CardContent';

write it in one line import { Card, CardActions, CardContent, Fab, Grid, Typography } from '@material-ui/core';

eslint doesn't seem to like that you import PageBase from './PageBase' before all the @material-ui stuff
import DynamicBackgroundContainer from '../components/DynamicBackgroundContainer';, eslint wants the .jsx extension (also used in Banner.jsx)
In Fixes #1002: fixes JSX-related issues in Banner.jsx, Posts.jsx #1006 we removed <ThemeProvider> since it didn't really do anything. Should be done here as well.

Other than these little things, the rest look great :D

Silvyre

Quite happy to push this on through! 👍

adds: corrections based on circleCI and SSR requirements

raygervais · 2020-04-16T14:13:31Z

Thanks for the corrections @cindyledev, can I get a re-review when you have a moment? So Happy to have the eslint warnings gone! 🎆

vercel bot had a problem deploying to Preview April 3, 2020 04:46 Failure

raygervais force-pushed the issue/890 branch from e54bafa to 663516b Compare April 3, 2020 04:52

vercel bot had a problem deploying to Preview April 3, 2020 04:52 Failure

vercel bot deployed to Preview April 3, 2020 05:02 View deployment

humphd requested changes Apr 4, 2020

View reviewed changes

Silvyre suggested changes Apr 4, 2020

View reviewed changes

src/frontend/src/pages/404.jsx Outdated Show resolved Hide resolved

src/frontend/src/pages/404.jsx Outdated Show resolved Hide resolved

raygervais force-pushed the issue/890 branch from f9356f2 to 110c089 Compare April 5, 2020 18:44

vercel bot deployed to Preview April 5, 2020 18:44 View deployment

vercel bot deployed to Preview April 5, 2020 22:45 View deployment

raygervais force-pushed the issue/890 branch from 7f1d3aa to cc3c1fa Compare April 5, 2020 22:55

vercel bot deployed to Preview April 5, 2020 22:55 View deployment

vercel bot deployed to Preview April 6, 2020 02:57 View deployment

raygervais changed the title ~~[WIP] adds: 404 page front-end concept~~ adds: 404 page front-end concept Apr 6, 2020

raygervais marked this pull request as ready for review April 6, 2020 12:38

raygervais requested a review from agarcia-caicedo April 6, 2020 12:38

raygervais force-pushed the issue/890 branch from 01d16be to 6f5627a Compare April 6, 2020 12:45

vercel bot deployed to Preview April 6, 2020 12:45 View deployment

humphd mentioned this pull request Apr 6, 2020

Restrict access to My Feeds to logged-in users #760

Closed

humphd requested changes Apr 6, 2020

View reviewed changes

Silvyre self-requested a review April 6, 2020 16:47

vercel bot had a problem deploying to Preview April 8, 2020 03:31 Failure

raygervais force-pushed the issue/890 branch from d978168 to 66f99fd Compare April 8, 2020 04:06

vercel bot deployed to Preview April 8, 2020 04:06 View deployment

humphd requested a review from cindyorangis April 8, 2020 15:09

refactors: background loading concept to dedicated component

b427130

fixes: rebase and component cleanup reverts: colors back to secondary base adds: authenticaton 403 concept and 500 wip

raygervais force-pushed the issue/890 branch from 7d5d013 to af5303a Compare April 15, 2020 15:15

vercel bot deployed to Preview April 15, 2020 15:15 View deployment

humphd self-requested a review April 15, 2020 15:24

Silvyre suggested changes Apr 15, 2020

View reviewed changes

Silvyre mentioned this pull request Apr 15, 2020

My Feeds should redirect unauthorized users to our new 403 page #1016

Closed

humphd requested changes Apr 15, 2020

View reviewed changes

src/backend/web/authentication.js Show resolved Hide resolved

raygervais force-pushed the issue/890 branch from af5303a to 4c964df Compare April 15, 2020 23:05

vercel bot deployed to Preview April 15, 2020 23:05 View deployment

humphd previously approved these changes Apr 16, 2020

View reviewed changes

Grommers00 previously approved these changes Apr 16, 2020

View reviewed changes

Silvyre self-requested a review April 16, 2020 04:49

cindyorangis requested changes Apr 16, 2020

View reviewed changes

Silvyre previously approved these changes Apr 16, 2020

View reviewed changes

removes: 404.jsx, we'll use error.jsx

3f98db7

adds: corrections based on circleCI and SSR requirements

raygervais dismissed stale reviews from Silvyre, Grommers00, and humphd via 3f98db7 April 16, 2020 13:46

raygervais force-pushed the issue/890 branch from 4c964df to 3f98db7 Compare April 16, 2020 13:46

vercel bot deployed to Preview April 16, 2020 13:46 View deployment

humphd approved these changes Apr 16, 2020

View reviewed changes

raygervais removed the request for review from agarcia-caicedo April 16, 2020 15:31

cindyorangis approved these changes Apr 16, 2020

View reviewed changes

raygervais merged commit 46441e0 into Seneca-CDOT:master Apr 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds: error page front-end concept #925

adds: error page front-end concept #925

raygervais commented Apr 3, 2020 •

edited

Loading

raygervais commented Apr 3, 2020

raygervais commented Apr 3, 2020 •

edited

Loading

cindyorangis commented Apr 3, 2020 •

edited

Loading

cindyorangis commented Apr 4, 2020

humphd Apr 4, 2020

humphd Apr 4, 2020

humphd Apr 4, 2020

humphd Apr 4, 2020

raygervais Apr 5, 2020

humphd left a comment

humphd Apr 6, 2020

raygervais Apr 6, 2020

humphd Apr 6, 2020

humphd Apr 6, 2020

raygervais Apr 6, 2020

humphd Apr 6, 2020

agarcia-caicedo commented Apr 8, 2020

cindyorangis commented Apr 11, 2020

Silvyre left a comment

raygervais commented Apr 15, 2020

raygervais commented Apr 15, 2020

Silvyre commented Apr 15, 2020 •

edited

Loading

humphd left a comment

raygervais commented Apr 15, 2020

raygervais commented Apr 16, 2020 •

edited

Loading

cindyorangis left a comment

Silvyre left a comment

raygervais commented Apr 16, 2020

adds: error page front-end concept #925

adds: error page front-end concept #925

Conversation

raygervais commented Apr 3, 2020 • edited Loading

Issue This PR Addresses

Type of Change

Description

How To Access

Checklist

raygervais commented Apr 3, 2020

raygervais commented Apr 3, 2020 • edited Loading

cindyorangis commented Apr 3, 2020 • edited Loading

cindyorangis commented Apr 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarcia-caicedo commented Apr 8, 2020

cindyorangis commented Apr 11, 2020

Silvyre left a comment

Choose a reason for hiding this comment

Gatsby.js development 404 page

Pages (6)

raygervais commented Apr 15, 2020

raygervais commented Apr 15, 2020

Silvyre commented Apr 15, 2020 • edited Loading

humphd left a comment

Choose a reason for hiding this comment

raygervais commented Apr 15, 2020

raygervais commented Apr 16, 2020 • edited Loading

cindyorangis left a comment

Choose a reason for hiding this comment

Silvyre left a comment

Choose a reason for hiding this comment

raygervais commented Apr 16, 2020

raygervais commented Apr 3, 2020 •

edited

Loading

raygervais commented Apr 3, 2020 •

edited

Loading

cindyorangis commented Apr 3, 2020 •

edited

Loading

Silvyre commented Apr 15, 2020 •

edited

Loading

raygervais commented Apr 16, 2020 •

edited

Loading