6 widget routes (fixes issue #6) (Replaces Widget Routes #33) by pllearns · Pull Request #34 · GuildCrafts/lizardboard

pllearns · 2016-11-09T22:56:16Z

-Updated RESTful APIs
-Updated naming conventions for schemas
-Refactored code and cleaned out comments

jrobcodes

@pllearns comments inline

jrobcodes · 2016-11-09T23:09:22Z

database/DashboardSchema.js

+const WidgetSchema = require('./WidgetSchema')
+
+const DashboardSchema = mongoose.Schema ({
+  _user: {type: Number, ref: 'User'},


Shouldn't this be an ObjectID? Or is this some functionality provided by mongoose? Why is the name _user instead of user?

jrobcodes · 2016-11-09T23:09:52Z

database/DashboardSchema.js

@@ -0,0 +1,12 @@
+const mongoose = require('mongoose')
+const Schema = mongoose.Schema


const { Schema } = mongoose

jrobcodes · 2016-11-09T23:10:05Z

database/User.js

@@ -0,0 +1,16 @@
+const mongoose = require('mongoose')
+mongoose.connect('mongodb://localhost/lizardboard')


Why do we connect here, but not in the other model file?

jrobcodes · 2016-11-09T23:10:30Z

database/User.js

+const db= mongoose.connection
+
+const UserSchema = require('./UserSchema')
+db.on('error', console.error.bind(console,'connection error:'))


We need to separate the setup of the database connection from the definition of the models...

jrobcodes · 2016-11-09T23:11:41Z

database/UserSchema.js

+
+const UserSchema = mongoose.Schema ({
+  username: String,
+  dashboards: [{type: Schema.Types.ObjectId, ref:'Dashboard'}]


dashboards are not a property of user - the relationship should be stored in the Dashboard object, unless this provides us with some needed functionality?

jrobcodes · 2016-11-09T23:16:05Z

routes/users.js

+  })
+})
+
+router.get('/widgets/:widgetId', ( request, response, next ) => {


Please name the variable :id instead of :widgetId - given the name of the route, it is understood that this is a widget id. Also, why is this route declared in the user routes file? It should be in its own module for widgets route.

jrobcodes · 2016-11-09T23:17:07Z

routes/users.js

+  .then( data => console.log('finished') )
+})
+
+router.post('/:userId/widgets', (request, response, next ) => {


Widgets do not belong to a user, so it does not seem to make sense to include them here. I could see an argument being made for /dashboards/:id/widgets, but we would probably just return the widgets along with the dashboard - it wouldn't make sense to have a set of widgets outside of a dashboard. I think this route will also be removed when we clean up the User model, as requested above (widgets do not belong to users).

jrobcodes · 2016-11-09T23:17:40Z

routes/users.js

+router.post('/:userId/widgets', (request, response, next ) => {
+  const { userId } = request.params
+  const widget = { type, title, size, contents }
+  widget = request.body


This is not a legal statement - you can not reassign a constant.

jrobcodes · 2016-11-09T23:18:59Z

routes/users.js

+  )
+})
+
+router.put('/:userId/widgets/:widgetId', ( request, response, next ) => {


Omit route; it does not make sense in the context of our domain (users do not contain widgets).

jrobcodes · 2016-11-09T23:19:07Z

routes/users.js

+  )
+})
+
+router.delete( '/:userId/widgets/:widgetId', ( request, response, next ) => {


Same; remove route.

jrobcodes

@pllearns @jessehall3 @harmanlearns Comments inline

jrobcodes · 2016-11-10T00:48:20Z

.gitignore

 *.log
 .env
 /build
+./userRouteNotes.js


How will the other team get this if it's in gitignore? May as well remove, and remove the file, and send the file to them using conventional means, instead of polluting the gitignore file.

jrobcodes · 2016-11-10T00:48:41Z

bin/www

+
+mongoose.connect(connection)
+  .then(() => console.log('connection successful'))
+  .catch((error) => console.error(error))


If we fail to connect, we should probably not start the server...

jrobcodes · 2016-11-10T00:49:06Z

database/DashboardSchema.js

@@ -0,0 +1,12 @@
+const mongoose = require('mongoose')
+const { Schema } = mongoose
+const WidgetSchema = require('./WidgetSchema')


WidgetSchema is not used in this file?

jrobcodes · 2016-11-10T00:49:35Z

database/UserSchema.js

@@ -0,0 +1,10 @@
+const mongoose = require('mongoose')
+const Schema = mongoose.Schema


Update as in other files.

jrobcodes · 2016-11-10T00:50:32Z

package.json

    "ejs": "^2.5.2",
    "express": "~4.13.4",
-    "mongoose": "^4.6.5",
+    "mongodb": "^2.2.11",


Are we using this import, and if so, for what? (since we have mongoose...)

jrobcodes · 2016-11-10T00:53:32Z

routes/userRouteNotes.js

+})
+
+router.post('/dashboards/:id/widgets', (request, response, next ) => {
+  Widget.create(request.body, (error, post) => {


Please consider a different name for the post parameter - this is not an intention revealing name

jrobcodes · 2016-11-10T00:53:56Z

routes/userRouteNotes.js

+  })
+})
+
+router.put('/:userId/widgets/:widgetId', ( request, response, next ) => {


Remove this route, widgets do not belong to users.

jrobcodes · 2016-11-10T00:54:13Z

routes/userRouteNotes.js

+  )
+})
+
+router.delete( '/:userId/widgets/:widgetId', ( request, response, next ) => {


Remove this route, widgets do not belong to users.

jrobcodes · 2016-11-10T00:55:30Z

routes/users.js

+const User = require('../database/UserSchema.js')
+const router = express.Router()
+const mongoose = require('mongoose')
+const Widget = require('../database/WidgetSchema.js')


This import is not used. Also, the filename WidgetSchema is very misleading if this module exports a model - I would move model definition modules into a models folder, instead of database, and remove the Schema part of the file name. As well, files should be named with lower snake case.

jrobcodes · 2016-11-10T00:55:51Z

routes/users.js

-});
+router.post('/', ( request, response, next ) => {
+  User.create(request.body, (error, post) => {
+    if ( error ) return next ( error )


See above - multi line if, change variable name, formatting issues.

jrobcodes

@pllearns Mostly formatting changes; a few functional and naming changes

jrobcodes · 2016-11-10T17:03:31Z

bin/www

+mongoose.Promise = global.Promise
+const connection = 'mongodb://localhost/lizardboard'
+
+mongoose.connect(connection)


This needs to be moved such that it happens prior to server startup. Please move imports to the top of the module, with other imports. We will also need the connection string to be added in a .env file, with instructions added to README on how to set this up.

server.on('error', onError); server.on('listening', onListening); mongoose.connect( process.env. MONGODB_URI ) .then( () => server.listen( port ) ) server.listen(port);

jrobcodes · 2016-11-10T17:04:47Z

routes/users.js

-const express = require('express');
-const router = express.Router();
+const express = require('express')
+const User = require('../models/user.js')


Let's not change the order of existing imports - it introduces changes in the diff that aren't really changes. :) Just shift the User import down below the instantiation of router and it should be good

jrobcodes · 2016-11-10T17:05:25Z

routes/users.js

-router.get('/', (request, response, next) => {
-  response.send('respond with a resource');
-});
+router.post('/', ( request, response, next ) => {


Add space between ( and '/'

jrobcodes · 2016-11-10T17:06:34Z

routes/users.js

-  response.send('respond with a resource');
-});
+router.post('/', ( request, response, next ) => {
+  User.create(request.body, (error, post) => {


Add space between ( and request.body, as well as between (, ) and parameters in callback function. Please consider a different name for the parameter post - this does not reveal the intent of this parameter (I'm not sure what it does, is intended to represent, or what data it should hold)

jrobcodes · 2016-11-10T17:06:41Z

routes/users.js

+router.post('/', ( request, response, next ) => {
+  User.create(request.body, (error, post) => {
+    if ( error ) {
+    return next ( error )


Indentation off.

jrobcodes · 2016-11-10T17:06:54Z

routes/users.js

+    if ( error ) {
+    return next ( error )
+  }
+  response.json( post )


Add whitespace above this line

jrobcodes · 2016-11-10T17:07:38Z

views/layout.pug

    div.footer-nav

    include footer
-    script(src="/javascripts/index.js")


Was this change intentional? Wanted to verify since I'm not sure why we're removing sripts from the layout when working on the API.

This was intentional since it isn't needed. Harman pointed this out to me as we were refactoring yesterday.

It causes 304 errors because these files do not exist.

jrobcodes

@pllearns still a few changes needed

jrobcodes · 2016-11-10T18:06:16Z

README.md

@@ -12,7 +12,10 @@ A mongodb database named lizardboard must be created prior to starting the appli
 - brew install yarn


Please revert changes in this file - these are recommendations, not instructions for setting up the application. See the floworky README for guidance adding instructions for setting up .env

jrobcodes · 2016-11-10T18:07:00Z

bin/www

+
+mongoose.connect( process.env. MONGODB_URI )
+  .then( () => server.listen( port ) )
+server.listen( port );


Remove this line - no need to start the server twice. I think the server.on lines also need to be above this to make sure set up occurs properly

jrobcodes · 2016-11-10T18:07:50Z

routes/users.js

+  User.create( request.body, ( error, user ) => {

-module.exports = router;
+   if ( error ) {


Indentation is still messed up in this router. Please correct.

jrobcodes

@pllearns Moar changes

jrobcodes · 2016-11-10T21:23:45Z

README.md

+``` echo MONGODB_URI=mongodb://`whoami`@localhost:3000/lizardboard >> .env
+```
+
+## (Optional) Configuration for test RESTful APIs


Configuration for test_ing_

jrobcodes · 2016-11-10T21:24:21Z

README.md

+- yarn start (start the server)
+
+Lizardboard needs an .env file to specify environment variables that are required to run the application.
+``` echo MONGODB_URI=mongodb://`whoami`@localhost:3000/lizardboard >> .env


Shouldn't this just be mongodb://localhost/lizardboard?

jrobcodes · 2016-11-10T21:24:39Z

bin/www

 app.set('port', port);

+mongoose.connect( connection )
+.then( () => server.listen( port ) )


Indentation off

jrobcodes · 2016-11-10T21:25:18Z

routes/users.js

-  response.send('respond with a resource');
-});
+router.post( '/', ( request, response, next ) => {
+  User.create( request.body, ( error, user ) => {


Missed this in previous commits; sorry `bout that - these should be promisified. See harman and jesse, they've got an example in their PR

… definitions.

jrobcodes suggested changes Nov 9, 2016

View reviewed changes

jrobcodes suggested changes Nov 10, 2016

View reviewed changes

Phillip Lorenzo added 2 commits November 10, 2016 15:04

Updated naming conventions for widget models and updated API endpoint…

384c200

… definitions.

Cleaned up spacing in users.js file

2448a14

		@@ -0,0 +1,12 @@
		const mongoose = require('mongoose')
		const Schema = mongoose.Schema

		@@ -0,0 +1,16 @@
		const mongoose = require('mongoose')
		mongoose.connect('mongodb://localhost/lizardboard')

		@@ -0,0 +1,10 @@
		const mongoose = require('mongoose')
		const Schema = mongoose.Schema

		@@ -12,7 +12,10 @@ A mongodb database named lizardboard must be created prior to starting the appli
		- brew install yarn

Conversation

pllearns commented Nov 9, 2016

Uh oh!

jrobcodes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrobcodes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrobcodes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrobcodes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrobcodes left a comment