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

Building a backend #20 #40

Merged
merged 15 commits into from
Jun 3, 2017
Merged

Building a backend #20 #40

merged 15 commits into from
Jun 3, 2017

Conversation

asiyani
Copy link

@asiyani asiyani commented Jun 1, 2017

This PR adds Nodejs and MongoDB as the backend to this project. Current functionality for this app is not disturbed. After running yarn start website works as it is working now.

  • New Folder structure
    • Moved js, css and index.html file to the public folder.
    • Added 3 new folders. server, config and test.
    • server folder contains app.js, db and routes folder.
      • db folder contains schema/model for MongoDB database.
      • routes folder contains js files related to APIs and other web routes
    • config folder got the configuration for dotenv to load Nodejs environment variables.
    • Test folder got test files for nodejs & db. e.g. db.test.js

untitled

  • MongoDB model

    • for now, there are 3 separate schemas
      • user -> user who is registered/logged in to curiosity website. (git authentication)
      • username -> Schema to store data related to usernames
      • Repository -> schema to store data related to repos.
    • Ideally, we should combine username and repository schema for denormalization? but it depends on the type of query we will be doing. We should discuss that further.
  • Routes

    • Just for testing and to get an idea about folder structure, I have created following routes.
      • GET /api/username/all -> will send username list as jason
      • GET /user, GET /user/login, GET /user/logout -> will just print message on web.
  • Configuration

    • I have 2 separate files for dotenv settings. .dev.env and .test.env
    • MONGODB_URI is different in both files because we don't want to run lots of test on main db.
  • Test

    • I am using Mocha and expect for backend testing.
    • Just a simple test added to make sure we are able to store data to MongoDB server.
      yarn run test-node
  • Readme

    • I have updated README.md file for Getting Started Guide.

Please review this folder structure and code. let me know what you guys think?
is DB schema properties enough or we need some additional information? I think we should create another issue to discuss schema design.

Copy link
Contributor

@alejandronanez alejandronanez left a comment

Choose a reason for hiding this comment

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

@asiyani thanks for the PR, that's a ton of work!

I have a couple of thoughts/concerns though.

  • The fact that you have to install MongoDB locally, I think a better approach for this would be to suggest the user to create a DB in a service like MLab and update the right info under any .env file.
  • Correct me if I'm wrong, but we should avoid hitting the DB for testing unless there's a really strong reason to do so. Maybe we can use sinon for mocking.

An ideal scenario will be to have a Docker container that has everything installed (MongoDB) but I think that's quite some work for one PR.

test/db.test.js Outdated
accessToken: 'sadjkskajfeifhwgsjfksdfkmcoiwerkhdskfjksladjf',
});

let repo = new Repository({
Copy link
Contributor

@alejandronanez alejandronanez Jun 1, 2017

Choose a reason for hiding this comment

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

You're modifying repo anywhere else, you can change this to a const

test/db.test.js Outdated
describe('Simple DB test', () => {
describe('User', () => {
it('Should save user to db', (done) => {
user.save().then((usr) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we hitting the DB here? If yes we should change things, maybe consider using sinon for mocking.

test/db.test.js Outdated
language: 'Javascript',
});

describe('Simple DB test', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also we should try not to nest describe blocks, this can lead to unexpected behaviour if we ever need to use this

test/db.test.js Outdated
const { Username } = require('./../server/db/username');
const { User } = require('./../server/db/user');

// console.log(mongoose.connection.host);
Copy link
Contributor

Choose a reason for hiding this comment

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

This code should go away. No commented code here unless there's a super strong reason 👍

});


let Username = mongoose.model('Username', usernameSchema);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use const in here.

});


let Repository = mongoose.model('Repository', repositorySchema);
Copy link
Contributor

Choose a reason for hiding this comment

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

const instead of let

config/config.js Outdated
process.env.NODE_ENV = 'dev';
}

let dotenv = require('dotenv').config({
Copy link
Contributor

Choose a reason for hiding this comment

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

const instead of let

@@ -1 +1,2 @@
node_modules
.vscode
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

README.md Outdated
## Getting Started
##### Pre-Installation Requirements
###### Node
- Download and install latest stable version of [Node](https://nodejs.org/en/download/).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should specify what version are we using and add that to the package.json under "engines" too.

README.md Outdated
- Download and install latest stable version of [Node](https://nodejs.org/en/download/).

###### MongoDb
- Download and Install [MongoDB Community Edition](https://docs.mongodb.com/manual/installation/#mongodb-community-edition).
Copy link
Contributor

Choose a reason for hiding this comment

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

Same goes for Mongo, what version should we (everybody contributing) should use.

@alejandronanez alejandronanez mentioned this pull request Jun 1, 2017
5 tasks
Copy link
Contributor

@alejandronanez alejandronanez left a comment

Choose a reason for hiding this comment

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

Change Arrow functions to regular functions in tests.

test/db.test.js Outdated
language: 'Javascript',
});

describe('Simple DB test', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also Mocha doesn't like arrow functions at all, we should move to function() { ... }. More info here https://mochajs.org/#arrow-functions

@mubaris
Copy link
Owner

mubaris commented Jun 2, 2017

@asiyani Cool work man 👍

@alejandronanez Should I wait for this PR to merge, so that I can work on Webpack

@alejandronanez
Copy link
Contributor

@alejandronanez Should I wait for this PR to merge, so that I can work on Webpack

Yes sir!

package.json Outdated
"dependencies": {}
"dependencies": {
"dotenv": "^4.0.0",
"expect": "^1.20.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a devDependency instead of a dependency

Copy link
Contributor

@alejandronanez alejandronanez left a comment

Choose a reason for hiding this comment

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

We should discuss what's the best way to do tests + DB setup later.
Good work @asiyani just change that devpendency into devDependency

@alejandronanez
Copy link
Contributor

@asiyani fix conflicts on index.html and we should be good to go 👍

@asiyani
Copy link
Author

asiyani commented Jun 2, 2017 via email

@alejandronanez
Copy link
Contributor

I didnt change anything in index.html just moved it to public folder...

Oh well, maybe that's the issue. Anyway we can't merge until that's resolved though :( Let me know if you have any issues with those conflicts maybe I can jump in.

@asiyani
Copy link
Author

asiyani commented Jun 2, 2017

I need index.html in public folder, otherwise i need to create route for index.html
Ideally that file should be in public folder.
Do you want me too move it to root for now just to merge PR.and then we move it back to public folder?

@alejandronanez
Copy link
Contributor

alejandronanez commented Jun 2, 2017 via email

@mubaris
Copy link
Owner

mubaris commented Jun 2, 2017

@asiyani You moved the index.html to public folder. After I made a change in the original index.html. Therefore, the git mv didn't work properly. If you move the new index.html to public folder, it might work.

Thoughts @alejandronanez

@alejandronanez
Copy link
Contributor

alejandronanez commented Jun 2, 2017 via email

@asiyani
Copy link
Author

asiyani commented Jun 2, 2017

conflicts are now resolved.

@mubaris mubaris merged commit 16ca70a into mubaris:v2 Jun 3, 2017
@alejandronanez
Copy link
Contributor

Good stuff boys

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