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

Bugfix: ensure backend api routes are configured and respond to request #30

Merged
merged 3 commits into from
Apr 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
.DS_STORE

/build
/node_modules
/be/.env
/be/**/logs/*
be/node_modules/
fe/node_modules/

# e2e test output
# e2e test output and artifacts
cypress/screenshots/
Comment on lines +1 to +11
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noticed these files crop up in the diff when running cypress locally

nohup.out
/tmp/**/*
5 changes: 3 additions & 2 deletions be/server.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@


import { App } from './app.js';
import { HealthRoute } from './src/routes/health.routes.js';
import { UserRoute } from './src/routes/user.routes.js';

// ValidateEnv();

const app = new App([new UserRoute()]);
const app = new App([new HealthRoute(), new UserRoute()]);

app.listen();
app.listen();
29 changes: 29 additions & 0 deletions be/src/routes/health.routes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// TODO: add swagger docs. get health and E2E test working first.

import { Router } from 'express'

// memoize router so that when the consumers retrieve it through getter,
// it's not a fresh instance, as that essentially wipes out the routes
const router = Router()

export class HealthRoute {
Copy link
Collaborator Author

@vanderhoop vanderhoop Apr 12, 2023

Choose a reason for hiding this comment

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

I added this as a distinct route, as it was the lowest complexity setup that I was sure would be immediately viable in CI, as there's no dependency on the database for this route (and the DB isn't set up in TravisCI, which we'll roll with until we get AWS + Jenkins set up).

get path() {
return '/health'
}

get router() {
return router
}

constructor() {
this.initializeRoutes()
}

buildResponse(req, res) {
res.status(200).json({ data: { attributes: { healthy: true } } })
Copy link
Collaborator Author

@vanderhoop vanderhoop Apr 12, 2023

Choose a reason for hiding this comment

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

I initially used a smaller payload here, but the front-end api client was confusingly responding with undefined due to the successDataPath config we had setup, which assumes the data is nested under data.attributes.

}

initializeRoutes() {
this.router.get(this.path, this.buildResponse)
}
}
8 changes: 6 additions & 2 deletions be/src/routes/user.routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,16 @@
import { Router } from 'express'
import { UsersController } from '../controllers/users.controller.js'

// memoize router so that when the consumers retrieve it via getter,
// it's not a fresh instance, as that essentially wipes out the routes
const router = Router()

export class UserRoute {
static path() {
get path() {
return '/users'
}
get router() {
return Router()
return router
}
get user() {
return new UsersController()
Expand Down
5 changes: 3 additions & 2 deletions cypress/e2e/home/home.cy.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ describe('Home Page', () => {
beforeEach(() => {
cy.visit(URL)
})
it('renders a main section', () => {
cy.get('main').should('exist')

it('renders a main section that successfully interacts with the API', () => {
cy.get('.backend-healthy main').should('exist')
Comment on lines +8 to +9
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This updated assertion, in combination with implementation code below, will protect us against the regression of the API routes being unavailable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does point out to me that we should probably set up some lower level integration tests for our API endpoints, as faster tests could have caught this.

We'll just need to set up the non-trivial boilerplate of setting these HTTP tests up, which I'm happy to do later this week or start of next.

})
})
1 change: 1 addition & 0 deletions fe/config/application.template.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ development:
PROXIED_API_TOKEN: ''

# === FE ===
REACT_APP_API_URL: http://localhost:8080
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The API client code assumes this variable is set, and it's vital in development, as the FE and BE are served via different ports. In other envs, it's not actually necessary, as the API client then just uses the path, as it doesn't need a fully qualified URL due to the api being served by the same server as the frontend.

It does feel weird to me that we have so many ENV VARS declaring similar values/configs, so I've created #31.

API_URL: http://localhost:8080/api/v1
FE_PORT: 3000

Expand Down
11 changes: 10 additions & 1 deletion fe/src/js/main/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
import React from 'react'
import App from './App'
import ReactDOM from 'react-dom'
import { api } from '../services/api'

ReactDOM.render(<App />, document.getElementById('root'))
const root = document.getElementById('root')

api.get('/api/v1/health').then((response) => {
if (response.healthy) {
root.classList.add('backend-healthy')
}
})

ReactDOM.render(<App />, root)