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

Initial db setup #27

Merged
merged 13 commits into from
Oct 3, 2017
Merged

Initial db setup #27

merged 13 commits into from
Oct 3, 2017

Conversation

Heathercoraje
Copy link
Collaborator

@Heathercoraje Heathercoraje commented Oct 3, 2017

#16

Create files database yet still needs to add codes in app.js to build db properly.

heathercoraje added 4 commits October 3, 2017 12:11
relates #16
create db_build.js, career_model.js, seed_data.js, db_connection.js
@Karyum
Copy link
Contributor

Karyum commented Oct 3, 2017

pull the latest master and merge it with this branch

heathercoraje added 3 commits October 3, 2017 15:29
relates #16
edit index.js to create connection to mongoDB, create config.env and put URI
relates #16
edit index.js to create connection to mongoDB, create config.env and put URI
@Heathercoraje Heathercoraje changed the title [WIP] Initial db setup Initial db setup Oct 3, 2017
@codecov
Copy link

codecov bot commented Oct 3, 2017

Codecov Report

Merging #27 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #27   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines           6      6           
=====================================
  Hits            6      6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61cbc8c...1fd90db. Read the comment docs.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks great

for (let i = 0; i < careers.length; i++) {
careers[i].save((error, result) => {
if (error) {
console.log(error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

error handling?

Copy link
Contributor

@Karyum Karyum left a comment

Choose a reason for hiding this comment

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

You guys are also setting up the database files as if this is postgres which is not.
also maybe rename src/index.js to something else?

career.collection.drop(); // once app is open, drop schema career

let dataLength = 0;
for (let i = 0; i < careers.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

use a forEach instead cause then you won't need the dataLength var


db.once('open', () => {
console.log('we are connected to DB');
career.collection.drop(); // once app is open, drop schema career
Copy link
Contributor

Choose a reason for hiding this comment

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

why ?

});
}

function exit () {
Copy link
Contributor

Choose a reason for hiding this comment

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

arrow function here please

src/index.js Outdated
db.once('open', function () {
console.log('connected to DB');
app.listen(app.get('port'), () => {
console.log('Magic happens on port 8181!');
Copy link
Contributor

Choose a reason for hiding this comment

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

the original one was a bit better, cause what if you wanted to change the port ???
also i would do

process.env.PORT || app.get('port')

for heroku setup

require('env2')('./congif.env');

const db = mongoose.connection;
mongoose.connect(process.env.MONGODB_URI, {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need a whole different file for this ??
why not justg add it to the src/index.js ( which should be renamed )

@Karyum Karyum merged commit b554e23 into master Oct 3, 2017
@Karyum Karyum deleted the initial-db-setup branch October 3, 2017 13:16
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.

4 participants