Backend - Users && more stuff by brunops · Pull Request #64 · nodebr/nodebr

brunops · 2016-11-15T07:34:31Z

Load environment variables from .env with dotenv module
Add steps necessary to run the app to the README
add morgan to print request logs
Add users resource
- This follows hello-world structure
- there are no 'use strict' thingies because es6 modules are strict by default :P
- email is not in the specification but probably very important (?)
- password is currently saved raw, missing bcrypt stuff

- This follows `hello-world` structure - there are no `'use strict'` thingies because commonjs is strict by default :P - `email` is not in the specification but probably very important (?) - `password` is currently saved raw, missing bcrypt stuff

brunops · 2016-11-15T07:45:23Z

Os testes não estão rodando por algum motivo, Parece ser relacionado com como aquela .truncate() query está sendo montada. Não consegui descobrir o erro, mas é isso aqui:

thebergamo · 2016-11-15T11:16:56Z

@brunops vou dar uma olhada nisso e dar merge!

alanhoff · 2016-11-15T13:37:42Z

README.md

+2. Rode as migrations
+
+```sh
+$ npm run knex migrate:latest


O lugar disto seria na documentação da wiki https://github.com/nodebr/nodebr/wiki/Backend

Depois coloco lá então 👍

alanhoff · 2016-11-15T13:39:04Z

index.js

@@ -1,5 +1,7 @@
 'use strict'

+require('dotenv').config()


Não utilizar o dotenv, os desenvolvedores podem fazer o source do .env caso seja necessário

alanhoff · 2016-11-15T13:39:10Z

lib/db/knexfile.js

+
+const path = require('path')
+
+require('dotenv').config({ path: path.resolve(__dirname, '../../.env') })


Não utilizar o dotenv, os desenvolvedores podem fazer o source do .env caso seja necessário

alanhoff · 2016-11-15T13:40:16Z

lib/db/migrations/20161114224639_users.js

+
+exports.up = knex => {
+  // Apenas roda esta migration se estivermos em um ambiente de desenvolvimento
+  if (process.env.NODE_ENV !== 'production') {


Não precisa deste if aqui, ele estava presente somente na migration do hello-world pois não queremos ela rodando em produção

alanhoff · 2016-11-15T13:40:33Z

lib/db/migrations/20161114224639_users.js

+}
+
+exports.down = knex => {
+  if (process.env.NODE_ENV !== 'production') {


Não precisa do if

alanhoff · 2016-11-15T13:46:09Z

resources/users/handlers.js

+const db = require('../../lib/db')
+const User = db.model('User')
+
+exports.findAll = (req, res) => {


Remover esta rota e handler

alanhoff · 2016-11-15T13:46:41Z

resources/users/routes.js

+
+const router = express.Router()
+
+router.get('/users', handlers.findAll)


Remover esta rota e handler

alanhoff · 2016-11-15T13:46:49Z

resources/users/routes.js

+const router = express.Router()
+
+router.get('/users', handlers.findAll)
+router.get('/users/:id', handlers.findOne)


Remover esta rota e handler

alanhoff · 2016-11-15T13:47:45Z

resources/users/schemas.js

+
+exports.create = exports.model.concat(Joi.object({
+  id: Joi.forbidden(),
+  username: Joi.string().alphanum().min(3).max(20).required(),


Utilizar .token no lugar de .alphanum, assim podemos criar usernames com underscore também

alanhoff · 2016-11-15T13:48:28Z

resources/users/schemas.js

+exports.create = exports.model.concat(Joi.object({
+  id: Joi.forbidden(),
+  username: Joi.string().alphanum().min(3).max(20).required(),
+  password: Joi.string().max(60).required()


.min(8).max(120)

Closes #64

Ajustando resource

codecov-io · 2016-11-15T16:39:59Z

Current coverage is 96.14% (diff: 96.22%)

Merging #64 into backend will increase coverage by 0.04%

@@            backend        #64   diff @@
==========================================
  Files            11         15     +4   
  Lines           256        311    +55   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            246        299    +53   
- Misses           10         12     +2   
  Partials          0          0

Powered by Codecov. Last update bc9f4cf...92833ad

Novo review

alanhoff

Faltando testes para:

Nome de usuário duplicado
Se a senha foi realmente criptografada

alanhoff · 2016-11-15T17:37:03Z

lib/db/migrations/20161114224639_users.js

@@ -0,0 +1,13 @@
+


Remover a linha em branco

alanhoff · 2016-11-15T17:37:32Z

lib/db/migrations/20161114224639_users.js

@@ -0,0 +1,13 @@
+
+exports.up = knex => {


Não precisa criar um escopo com as chaves aqui, pode simplesmente knex => knex.schema...

alanhoff · 2016-11-15T17:37:43Z

lib/db/migrations/20161114224639_users.js

+  })
+}
+
+exports.down = knex => {


Mesma questão do escopo

alanhoff · 2016-11-15T17:38:07Z

resources/users/handlers.js

@@ -0,0 +1,17 @@
+


Remover linha em branco

alanhoff · 2016-11-15T17:38:46Z

resources/users/model.js

@@ -0,0 +1,6 @@
+


Linha em branco

alanhoff · 2016-11-15T17:38:57Z

resources/users/routes.js

@@ -0,0 +1,16 @@
+


Linha em branco

alanhoff · 2016-11-15T17:39:37Z

test/fixtures/users.js

@@ -0,0 +1,27 @@
+'use strict'


Remover use strict

alanhoff · 2016-11-15T17:40:14Z

test/fixtures/users.js

+ * Insere dois registros na tabela hello_world
+ * @return {Promise} Uma promise que resolve quando os registros forem inseridos
+ */
+exports.insertMultiple = () => {


não precisa criar um escopo e um return

alanhoff · 2016-11-15T17:40:21Z

test/fixtures/users.js

+const { knex } = require('../../lib/db')
+
+/**
+ * Insere dois registros na tabela hello_world


Documentação errada

alanhoff · 2016-11-15T17:56:35Z

test/resources/users.js

@@ -0,0 +1,46 @@
+'use strict'


Remover o use strict daqui também

alanhoff · 2016-11-15T18:17:27Z

LGMT

bsanches added 6 commits November 14, 2016 22:39

Load environment variables from .env with dotenv module

3a05223

Add steps necessary to run the app to the README

8f66e19

add morgan to print request logs

ad80c85

Add users resource

62abd44

- This follows `hello-world` structure - there are no `'use strict'` thingies because commonjs is strict by default :P - `email` is not in the specification but probably very important (?) - `password` is currently saved raw, missing bcrypt stuff

Merge branch 'backend' of github.com:nodebr/nodebr into backend

c90a084

lint

5938eb9

alanhoff previously requested changes Nov 15, 2016

View reviewed changes

alanhoff added a commit that referenced this pull request Nov 15, 2016

Arruma o truncate para operar com múltiplas tabelas

7b9feea

Closes #64

alanhoff mentioned this pull request Nov 15, 2016

Arruma o truncate para operar com múltiplas tabelas #66

Merged

thebergamo and others added 7 commits November 15, 2016 12:09

Adequando às especificoes

40dbfb1

Removendo workarround baseado no PR do alan

f26c75c

Rebase nodebr/backend

1c4da67

Removendo setup do readme.md em prol da wiki

21a9df0

Trocando nome da rota para pt-br

ca8a946

Trocando nome da rota para plural

9f6f639

Merge pull request #1 from nodebr/fix/tests

c738269

Ajustando resource

alanhoff requested changes Nov 15, 2016

View reviewed changes

alanhoff reviewed Nov 15, 2016

View reviewed changes

test/resources/users.js Outdated

@@ -0,0 +1,46 @@

'use strict'

Copy link

Member

alanhoff Nov 15, 2016

Choose a reason for hiding this comment

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

Remover o use strict daqui também

bsanches added 2 commits November 15, 2016 10:02

chore stuff - linting, remove blank lines etc

1715f1f

test duplicate user insertion

92833ad

alanhoff approved these changes Nov 15, 2016

View reviewed changes

alanhoff merged commit aabd929 into nodebr:backend Nov 15, 2016

thebergamo mentioned this pull request Nov 16, 2016

Endpoint de Usuário #60

Closed


		const path = require('path')

		require('dotenv').config({ path: path.resolve(__dirname, '../../.env') })


		const router = express.Router()

		router.get('/users', handlers.findAll)

Conversation

brunops commented Nov 15, 2016

Uh oh!

brunops commented Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thebergamo commented Nov 15, 2016

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!

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!

codecov-io commented Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 96.14% (diff: 96.22%)

Uh oh!

alanhoff 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!

alanhoff commented Nov 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brunops commented Nov 15, 2016 •

edited

Loading

codecov-io commented Nov 15, 2016 •

edited

Loading