add mongoose and migrations [closes #11] by shakalee14 · Pull Request #12 · GuildCrafts/lizardboard

shakalee14 · 2016-11-01T01:22:31Z

create add_user table via migrations

shakalee14 · 2016-11-01T01:23:02Z

hey @jrob8577 - looking for feedback. wrote instructions in migrations.md

jrobcodes

@shakalee14 @Moniarchy @jamestewartjr @pllearns Changes requested. Also, could we get a PR for just the infrastructure so that I do not need to look through files that are not changing from PR to PR?

jrobcodes · 2016-11-01T16:55:29Z

.gitignore

+node_modules
+yarn-error.log
+npm-debug.log
+yarn.lock


yarn.lock should not be in the gitignore file...

jrobcodes · 2016-11-01T17:44:30Z

migrations.md

@@ -0,0 +1,11 @@
+If a contributor wants to change the schema, they must create a migration. Current migrations have been created with the following commands:


There's a lot of instruction in this file that is not specifically concerned with migrations. I would prefer to make this more concise, and place it in the CONTRIBUTING.md file, similar to the floworky approach.

Was this comment addressed? Is this file still needed?

We deleted it but it must have come back during a rebase without our realizing it. When we push up again it'll be gone.

shakalee14 · 2016-11-01T18:43:05Z

@jrob8577 we addressed your changes! thanks.

jrobcodes

@shakalee14 @jamestewartjr @pllearns @Moniarchy Comments inline

jrobcodes · 2016-11-01T19:57:39Z

README.md

+## Setting up Development Environment
+
+A mongodb database named lizardboard must be created prior to starting the application for the first time.
+- Brew install mongodb


brew should be lower case

jrobcodes · 2016-11-01T19:58:23Z

README.md

+
+A mongodb database named lizardboard must be created prior to starting the application for the first time.
+- Brew install mongodb
+- Yarn install


yarn should be lower case. We should probably also insert a command to install yarn globally (before brew install mongodb maybe?).

jrobcodes · 2016-11-01T19:59:01Z

README.md

+- Brew install mongodb
+- Yarn install
+- Run `mongod` command on one tab to open up the database server
+- Run `mongo` command on another tab to open up the MongoDB shell


Remove this line; running the mongo shell is not a necessary step to setting up the dev environment.

jrobcodes · 2016-11-01T19:59:45Z

README.md

+A mongodb database named lizardboard must be created prior to starting the application for the first time.
+- Brew install mongodb
+- Yarn install
+- Run `mongod` command on one tab to open up the database server


I think we can remove this command as well - installing the DB implies that is is running for the project to work, IMHO. If you disagree, I think I'd rather see this as Ensure mongo is running, maybe after the brew command?

jrobcodes · 2016-11-01T20:00:47Z

README.md

+
+### Database
+- [Mongodb](https://docs.mongodb.com/)
+  -[Mongoose](http://mongoosejs.com/docs/guide.html)


Have we officially decided on Mongoose? Would love to here why we decided to use this lib.

It's the most popular and supported ORM for Mongo, based on our research.

jrobcodes · 2016-11-01T20:04:39Z

routes/users.js

@@ -0,0 +1,9 @@
+const express = require('express');


Let's get rid of unused routes in the initial submission

jrobcodes · 2016-11-01T20:06:03Z

server/index.js

+// Use connect method to connect to the Server
+MongoClient.connect(url, function(err, db) {
+  assert.equal(null, err);
+  console.log("Connected correctly to server");


This is less interesting than if an error occurs - I would remove this and only output something if a failure occurs. As well, the server probably shouldn't start up unless we manage to connect to the database, so this is probably the wrong place for this code.

jrobcodes · 2016-11-01T20:06:11Z

server/index.js

+const url = 'mongodb://127.0.0.1:27017/lizardboard';
+// Use connect method to connect to the Server
+MongoClient.connect(url, function(err, db) {
+  assert.equal(null, err);


Why assert.equal?

jrobcodes · 2016-11-01T20:06:29Z

server/index.js

+  , assert = require('assert');
+
+// Connection URL
+const url = 'mongodb://127.0.0.1:27017/lizardboard';


We should move this const and db connection logic into its own module.

jrobcodes · 2016-11-01T20:06:52Z

server/index.js

+  assert.equal(null, err);
+  console.log("Connected correctly to server");
+
+  db.close();


Why are we closing the db?

- create add_user table via migrations - addressed jrobs comments, created contributing md and added info to readme

jrobcodes

@shakalee14 @Moniarchy change requests in line

jrobcodes · 2016-11-02T15:43:18Z

README.md


 ## Testing
-TBD
+<<<<<<< HEAD


There's a merge conflict in here.

jrobcodes · 2016-11-02T15:43:55Z

database.js

@@ -0,0 +1,9 @@
+const db = 'mongodb://localhost/lizardboard'


What is this const used for?

jrobcodes · 2016-11-02T15:44:18Z

database.js

@@ -0,0 +1,9 @@
+const db = 'mongodb://localhost/lizardboard'
+const url = 'mongodb://127.0.0.1:27017/lizardboard';


Let's make this upper case URL, and maybe name it to be more specific (DATABASE_URL perhaps?)

jrobcodes · 2016-11-02T15:47:15Z

database.js

+const url = 'mongodb://127.0.0.1:27017/lizardboard';
+const MongoClient = require('mongodb').MongoClient, assert = require('assert');
+
+MongoClient.connect(url, (err, res, db) => {


I think we need to make this a promise and export the creation of the promise as a function - otherwise we can't guarantee that we have successfully connected to the database when the server starts...

jrobcodes · 2016-11-02T15:49:59Z

package.json

    "test": "echo \"Error: no test specified\" && exit 1",
-    "start": "nodemon ./bin/www"
+    "start": "nodemon ./bin/www",
+    "migration:create": "node_modules/.bin/migrate -d mongodb://localhost:27017/lizardboard create",


Does create take parameters? If it does, does this work?

Yes, it does. Are there any specific changes you would like to see? We haven't had trouble with running this script. In CONTRIBUTING.md there are further instructions for using this script.

jrobcodes · 2016-11-02T15:50:12Z

server/index.js

@@ -70,4 +70,4 @@ app.use(function(err, req, res, next) {
 });



Remove extra new line

jrobcodes suggested changes Nov 1, 2016

View reviewed changes

Moniarchy mentioned this pull request Nov 1, 2016

Add CONTRIBUTING.md #3

Closed

shakalee14 and others added 2 commits November 2, 2016 08:10

add mongoose and migrations [closes #11]

a9552fc

- create add_user table via migrations - addressed jrobs comments, created contributing md and added info to readme

fixing merge conflicts

cafd992

jrobcodes suggested changes Nov 2, 2016

View reviewed changes

jrobcodes mentioned this pull request Nov 2, 2016

Create developer setup documentation #2

Closed

responded to JRob's review

3609a52

		@@ -0,0 +1,11 @@
		If a contributor wants to change the schema, they must create a migration. Current migrations have been created with the following commands:

		@@ -0,0 +1,9 @@
		const db = 'mongodb://localhost/lizardboard'
		const url = 'mongodb://127.0.0.1:27017/lizardboard';

		@@ -70,4 +70,4 @@ app.use(function(err, req, res, next) {
		});

Conversation

shakalee14 commented Nov 1, 2016

Uh oh!

shakalee14 commented Nov 1, 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!

shakalee14 commented Nov 1, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants