Create applications #8

seanjparker · 2019-09-03T15:02:16Z

For issues #1
This pull request implements applications along with a basic error handler and dashboard

Features:

Applications form (front-end)
Application router + controller
Basic dashboard (front-end)
Caching of application questions
Dependency injection for controllers, services, cache
Testing using Ava and ts-mockito
CV uploads to Dropbox
Storage of applications in MySQL DB

It it quite simple to add new hackathon questions, only a few files need to be modified but with some more work it could be better (this can be for future work)

Routes:

GET /apply
POST /apply
GET /

You can test out the routes using Postman and import the Postman Collection

Add meta tag for setting width to device width on load Fix padding for form progress

Start adding db entities

Also, bump target of ts compiler to es2018

Add application routes, pages, controllers and services Add initial dashboard and route (only basic page for now) Add basic error handler

kzalys · 2019-09-03T20:45:43Z

I can't see any tests. Will they be coming later?

seanjparker · 2019-09-03T23:20:40Z

I can't see any tests. Will they be coming later?

There are a couple of things I need to add, like CV support
And yes, the tests will be coming in this PR

kzalys · 2019-09-04T08:59:26Z

I can't see any tests. Will they be coming later?

There are a couple of things I need to add, like CV support
And yes, the tests will be coming in this PR

Alright, do you want me to wait for the changes or should review the PR now?

seanjparker · 2019-09-04T16:29:50Z

I can't see any tests. Will they be coming later?

There are a couple of things I need to add, like CV support
And yes, the tests will be coming in this PR

Alright, do you want me to wait for the changes or should review the PR now?

Wait for the changes, hopefully it will be done by this weekend

Use nyc for code coverage

Add HTML coverage report

@kzalys

Thanks to @kzalys

kzalys

Nice! The application form looks really cool and I love how smart the questions system is!

I left a few suggestions below.

.env.example

kzalys · 2019-09-14T14:55:19Z

Makefile

@@ -25,12 +25,12 @@ setup-network:

 # starts the app and MySQL in docker containers
 up: build-docker setup-network
-	@echo "=============starting hs_hub============="
+	@echo "=============starting hs_application============="
 	docker-compose up -d

 # starts the app and MySQL in docker containers for dev environment
 up-dev: build-docker-dev setup-network


This is not a big deal but I'd say that it's better to just start the app on the host for a dev environment. Based on my experience working on hs_auth, it's not fun working on an app that is running in a container since you need to restart the container (and rebuild the image in this case) whenever you add any new dependencies or change any env variables.

I have found the same 😂I will include a separate docker-compose for dev environments that will launch only the DB

kzalys · 2019-09-14T14:57:24Z

docker-compose.yml

@@ -19,7 +19,6 @@ services:
    command: --default-authentication-plugin=mysql_native_password --log_error_verbosity=1
    environment:
      MYSQL_ROOT_PASSWORD: root


I'd say this should be set in a .env file because people might forget to change the root password if it's hidden inside the docker-compose file

Hmm, true. I should write a script that creates a new user since having the root user is bad practice anyway

Could you create an issue for this if you're not going to do it in this diff?

It might be in this PR, I think I have a fix for the problem I was having

kzalys · 2019-09-14T15:03:24Z

src/app.ts

+   */
+  private devMiddlewareSetup = (app: Express): void => {
+    // Development environment set up
+    if (app.get("env") === "dev") {


I think it would be cleaner if this check was done prior to calling devMiddlewareSetup because it's not evident from the method's name or the comment that env has to be set to dev for this method to do anything

Yep, I agree. Good catch

kzalys · 2019-09-14T15:09:02Z

src/app.ts

+
+  /**
+   * Loads the applications settings into Cache
+   * Questions are stored under the name `Questions`


Are the questions not stored under Sections? That's the name of the class right?

I forgot to change this! Initially it was called Questions but then I changed to storing the questions in sections

kzalys · 2019-09-14T16:05:30Z

src/util/errorHandling/apiError.ts

+    this.statusCode = _statusCode;
+    this.message = _message;
+
+    switch (this.statusCode) {


Could replace this switch by defining a map where the key is the response code and the value is the error message and then just call the map.

Though you would need to also add an if for when the response code is not in the map but it should still be cleaner.

Yeah, it would be cleaner. I just stole this from hs_hub for now. I will rework this when I get round to it. I've created another issue for improving the error handling (see #10)

kzalys · 2019-09-14T16:06:42Z

src/util/errorHandling/errorHandler.ts

+    }
+
+    console.error(err.stack);
+    res.status(HttpResponseCode.INTERNAL_ERROR).send(new ApiError(HttpResponseCode.INTERNAL_ERROR, err.stack));


Might not be a great idea to send the entire error stack trace to the user in prod as it might contain some sensitive information.

Good catch, I'll change this

kzalys · 2019-09-14T16:18:48Z

README.md

@@ -2,6 +2,7 @@



This file still contains instructions for hs_hub but fixing this isn't urgent tbh

kzalys · 2019-09-14T16:20:13Z

docker-compose.yml

@@ -19,7 +19,6 @@ services:
    command: --default-authentication-plugin=mysql_native_password --log_error_verbosity=1
    environment:
      MYSQL_ROOT_PASSWORD: root
-      MYSQL_DATABASE: hub
    ports:
      - "${DB_PORT}:${DB_PORT}"
    # The healthcheck is used to ensure the database is running before the hub starts


It says: before the hub starts

Thanks 😊

kzalys · 2019-09-14T16:31:26Z

src/views/pages/apply.ejs

@@ -3,6 +3,8 @@



Would be great to somehow indicate which fields are required but I'm guessing that is not a trivial change since the form is generated dynamically

I am working on this, just thinking about how to design it. The questions all have a setting called required so it shouldn’t be too difficult.

Right now the required inputs turn red when you click and purple otherwise but it should be made clearer

Update applicant age req Run dev hs_applications on host Remove dev dockerfile

Add init script for db container

kzalys

Awesome

seanjparker added 14 commits August 25, 2019 13:30

Add cache from hs_hub

5518c80

Fix makefile for hs_application

e61376e

Add application sections and form flow

58e15a3

Add dropdown support in application

07da259

Fix country spelling in questions

e51a1ff

Add support for radio inputs

4cc081a

Move material-dashboard.js to external js

dcb7a38

Add required attribute in form

80db993

Add name to each input for form submission

885cc88

Add meta tag for setting width to device width on load Fix padding for form progress

Add inversify and typeorm dependency

9287973

Add dependency injection for routes + controllers

691cf68

Start adding db entities

Rename route and repository files for applications

dbcf8bd

Add class-validation, typeorm

a095a29

Also, bump target of ts compiler to es2018

Add di for routes, controllers + services

3977f47

Add application routes, pages, controllers and services Add initial dashboard and route (only basic page for now) Add basic error handler

seanjparker added the feature New feature or request label Sep 3, 2019

seanjparker added this to the GUH19 Applications Open milestone Sep 3, 2019

seanjparker requested a review from a team September 3, 2019 15:02

seanjparker self-assigned this Sep 3, 2019

seanjparker added 7 commits September 5, 2019 21:26

Add ava for tests instead of jest

37d9764

Use nyc for code coverage

Make repositories easier to mock

1207739

Add application service test

e2455b6

Remove postgres dependency from travis CI

89124f7

Move nyc config into package.json

58147de

Add HTML coverage report

Add adapted cache tests from hs_hub

676fa18

Thanks to @kzalys

Add codecov to travis CI script

dec4fd9

seanjparker added 7 commits September 8, 2019 18:29

Move controller tests to integration test folder

9983bdd

Add e2e tests, fix imports in integration tests

6d6f709

Update applicant model with optional annotations

a7469fd

Add file upload support on front-end

5c604c0

Add support for file uploads and dropbox storage

5545608

Add controller and service tests for file upload

7deb308

Move comment in .env.example

f7be913

kzalys suggested changes Sep 14, 2019

View reviewed changes

seanjparker mentioned this pull request Sep 15, 2019

Improved error handling #10

Open

seanjparker added 4 commits September 15, 2019 21:55

Rename applicationService/Repo to applicantService

4aed7c9

Update inverify with new applicant naming

c309ed2

Add gulp to build process

81610d7

Update applicant age req Run dev hs_applications on host Remove dev dockerfile

Update tests to use new applicant service name

184574b

seanjparker force-pushed the create-applications branch from 042d8a9 to 184574b Compare September 16, 2019 14:44

seanjparker added 4 commits September 16, 2019 16:23

Update error handler to print simple message

9683a97

Fix display bug for long input boxes

3231f66

Add comment to clarify form validation todo

7af45e8

Update to new ports

d385409

Add init script for db container

kzalys previously approved these changes Sep 17, 2019

View reviewed changes

Fix make up command for production

b191c32

seanjparker dismissed kzalys’s stale review via b191c32 September 17, 2019 19:45

seanjparker requested a review from kzalys September 17, 2019 19:51

kzalys approved these changes Sep 17, 2019

View reviewed changes

seanjparker merged commit cdb9bbc into master Sep 17, 2019

seanjparker deleted the create-applications branch September 17, 2019 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create applications #8

Create applications #8

seanjparker commented Sep 3, 2019 •

edited

Loading

kzalys commented Sep 3, 2019

seanjparker commented Sep 3, 2019

kzalys commented Sep 4, 2019

seanjparker commented Sep 4, 2019

kzalys left a comment

kzalys Sep 14, 2019

seanjparker Sep 15, 2019

kzalys Sep 14, 2019

seanjparker Sep 15, 2019

kzalys Sep 17, 2019

seanjparker Sep 17, 2019

kzalys Sep 14, 2019

seanjparker Sep 15, 2019

kzalys Sep 14, 2019

seanjparker Sep 15, 2019

kzalys Sep 14, 2019

seanjparker Sep 15, 2019

kzalys Sep 14, 2019

seanjparker Sep 15, 2019

kzalys Sep 14, 2019

kzalys Sep 14, 2019

seanjparker Sep 15, 2019

kzalys Sep 14, 2019

seanjparker Sep 15, 2019

kzalys left a comment

Create applications #8

Create applications #8

Conversation

seanjparker commented Sep 3, 2019 • edited Loading

kzalys commented Sep 3, 2019

seanjparker commented Sep 3, 2019

kzalys commented Sep 4, 2019

seanjparker commented Sep 4, 2019

kzalys 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

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

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

kzalys left a comment

Choose a reason for hiding this comment

seanjparker commented Sep 3, 2019 •

edited

Loading