Hello 👋(Code-Review)

[jamiecoe]

Hey people 👋

Great work so far 🎉 You've been able to keep a good balance of front-end / back-end / documentation / tests. I know you've decided to switch DBs to Airtable which has set you back a little, but you're doing an excellent job so keep it up 👍

I'm aware you've only got a week left, so feel free to ignore my suggestions if you've got more urgent things to focus on. Hopefully they will also be useful for future projects.

Also please do let me know if I've said something which doesn't make sense, and apologies if you were already aware of an issue, or you had a good reason for doing something a certain way.

Best of luck for the final MVP 🥇

[harryyy27]

Cheers man.

I am a little confused about the 500 error still, are you saying it should be on the front end? Rather than the Backend? Or should I have one in both? Your suggestions have definitely helped! Really useful for fixing certain glaring errors.

Also, have you looked at our routes tests on the front end? Do you think I should test every route? Also do you think it might be a good idea to somehow test the URL or status code on the front end rather than the text in the first element?

[jamiecoe]

Hi @harryyy27

You want both for a 500 error.

A 500 error means an Internal Server Error, so you definitely want to catch it first from the backend. You've almost setup the 500 error route (don't forget about the bug mentioned in #74):

app.use((err, req, res, next) => {
    res.status(500)
    res.render('500') // res.render() will throw an error because you're not using a rendering engine
    // you want res.send(); instead 
})

Once you send the 500 response back to the client - one option would be to route the user to an error page. For example, in contactUs.js lets say when you do the api call, it returns a 500 error. It will go to the catch(), which we can use to redirect the user:

fetch('/api/contactUs', {
            method: 'POST',
            headers: {
                "Content-Type": "application/json"
            },
            body: data
        })
            .then(res => console.log(res))
            .catch(err => {
                console.log('ERROR IS', err)                
                this.props.history.push('/server-error');
            })

Then you could have a /server-error route set up which renders a ServerError component (ie: a message telling the user there has been a server error):
<Route path ='/server-error' component = {ServerError} />

You don't have to do this exact approach, but the general idea is:

• Pick up error on express server - the express docs say synchronous errors are automatically sent to your error handling route, but for asynchronous errors you need to call next(err)
• Send a response to front end with a status of 500, and optionally a message with more detail
• Pick this up in the front-end, and do something useful with it (eg: reroute user, show them a message)

Does that make sense?

[jamiecoe]

Your front end tests look good. I haven't used react-testing-library, but I've heard good things! Might be worth talking to someone with more experience of it.

What I would say is try not to test things about the implementation that need to be flexible. For example, if the text-content is going to be changed and updated, it's probably not going to helpful to test (your tests will keep breaking).

Testing things that should remain consistent will be useful. For example:

• URLs probably would be helpful as they should be consistent.
• The behaviour of a menu bar opening and closing.
• An error message/page rendering if your api responds with an error