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

Shopping List #11

Open
wants to merge 43 commits into
base: master
Choose a base branch
from
Open

Shopping List #11

wants to merge 43 commits into from

Conversation

gordoncm
Copy link

@gordoncm gordoncm commented Apr 6, 2016

Completed shopping list app

@gordoncm
Copy link
Author

gordoncm commented Apr 8, 2016

working on adding unit tests, should be done by early monday

@gordoncm
Copy link
Author

added unit tests and updated ui

@superchris
Copy link
Member

Cameron,

I've taken a look at your submission. After following the instructions and making sure my mysql instance was happy, I was able to get the application to run. I was able to register and create a list. However, when I went to add an item I got the following error:

Error: Can't set headers after they are sent.
    at ServerResponse.OutgoingMessage.setHeader (_http_outgoing.js:346:11)
    at ServerResponse.header (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express/lib/response.js:718:10)
    at ServerResponse.send (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express/lib/response.js:163:12)
    at done (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express/lib/response.js:957:10)
    at View.renderFile [as engine] (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express-react-views/index.js:74:5)
    at View.render (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express/lib/view.js:126:8)
    at tryRender (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express/lib/application.js:639:10)
    at EventEmitter.render (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express/lib/application.js:591:3)
    at ServerResponse.render (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express/lib/response.js:961:7)
    at /Users/chrisnelson/apprenticeship/gordoncm/app.js:331:9
Error: Can't set headers after they are sent.
    at ServerResponse.OutgoingMessage.setHeader (_http_outgoing.js:346:11)
    at ServerResponse.header (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express/lib/response.js:718:10)
    at ServerResponse.send (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express/lib/response.js:163:12)
    at done (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express/lib/response.js:957:10)
    at View.renderFile [as engine] (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express-react-views/index.js:74:5)
    at View.render (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express/lib/view.js:126:8)
    at tryRender (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express/lib/application.js:639:10)
    at EventEmitter.render (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express/lib/application.js:591:3)
    at ServerResponse.render (/Users/chrisnelson/apprenticeship/gordoncm/node_modules/express/lib/response.js:961:7)
    at /Users/chrisnelson/apprenticeship/gordoncm/app.js:331:9

I was also curious why you went the direction of using what looks like server side rendering of react. Nothing wrong with this choice, it was just interesting to me.

I'd love to see this error resolved. I'd also encourage you to break your application into multiple files. One of the most important things when writing code is that it be easy to understand by other developers, and having code organized in well structured units helps a lot with this.

If you'd like to make some changes and have me take another look, I'd be happy to do so. We've scheduled a few interviews, and pending the outcome of these we may or may not do more interviews for the first pair of apprentices. Additionally, we hope to hire a second pair of apprentices in a month or two and would be happy to consider your submission for the second pair as well.

@gordoncm
Copy link
Author

Hello,

Thanks for looking at it. I will work on getting it broken up into multiple files and should have that done by the end of this week. I chose the express-react-views library as it was an easy way to get Express / MySQL / React working together.

@gordoncm
Copy link
Author

Chris,

Could you please try again with the updated files? The issue started on the sign in page, because I was using both render and redirect methods. There was also an issue in the /additem method. I was calling the render method when I should have been redirecting the user back to the /lists route.

Thanks

@gordoncm
Copy link
Author

I split everything up into multiple files. If you could take a look at it again I would appreciate it.

@superchris
Copy link
Member

Hey Cameron,

Still getting an error when I try to add items to a list. I'm getting:

{ [Error: ER_NO_DEFAULT_FOR_FIELD: Field 'current_user' doesn't have a default value]
  code: 'ER_NO_DEFAULT_FOR_FIELD',
  errno: 1364,
  sqlState: 'HY000',
  index: 0 }

in the console, in the browser the request is not completing. Seems like maybe you need to pass along the user or make this column optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants