Implementa a sessão by alanhoff · Pull Request #70 · nodebr/nodebr

alanhoff · 2016-11-15T21:09:31Z

No description provided.

alanhoff · 2016-11-15T21:09:43Z

codecov-io · 2016-11-15T21:19:28Z

Current coverage is 96.79% (diff: 96.07%)

Merging #70 into backend will increase coverage by 0.65%

@@            backend        #70   diff @@
==========================================
  Files            15         21     +6   
  Lines           311        468   +157   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            299        453   +154   
- Misses           12         15     +3   
  Partials          0          0

Powered by Codecov. Last update aabd929...e7edd57

brunops · 2016-11-15T22:47:47Z

circle.yml

  environment:
    DATABASE_URL: mysql://ubuntu@localhost/circle_test
    NODE_ENV: test
+    COOKIE_SECRET: here_be_dragrons


brunops

minor comments, pretty good overall 👍

brunops · 2016-11-15T22:53:58Z

lib/session.js

+  const session = cookieSession({
+    name: 'session',
+    keys: [process.env.COOKIE_SECRET],
+    maxAge: 168 * 60 * 60 * 1000, //  Uma semana


Não seria mais claro 24 * 7 ao invés de 168? (mas na real não ligo muito pra isso)

Concordo com o @brunops se ficar 24 * 7 facilita a leitura

Fixed, mas botei o dia na frente das horas :-)

brunops · 2016-11-15T22:55:59Z

lib/session.js

+      // Caso o acesso seja restrito à usuários logados precisamos verificar
+      // se a sessão foi lida com sucesso
+      if ((!req.session || !req.session.user_id) && config.restrict) {
+        res.status(401).send({ error: 'Unauthorized' })


Dá pra remover o else se adicionar um return aqui

brunops · 2016-11-15T23:01:37Z

resources/sessions/handlers.js

+
+  // Verifica se a senha está ok e seta a sessão
+  if (yield user.compare(req.body.password)) {
+    req.session.user_id = user.id


camelCase? userId?

No caso, quando são objetos é melhor usar com o _ mesmo @brunops

Quando alguma referência a uma coluna no banco de dados aí usa underscore, caso contrário camelCase mesmo.

brunops · 2016-11-15T23:40:40Z

test/fixtures/sessions.js

+exports.cookie = (username, password) => request(server)
+.post('/sessions')
+.send({ username, password })
+.then(res => {


se quiser ser fancy res => res.header['set-cookie'].map(e => e.split(';')[0]).join(';')

thebergamo · 2016-11-16T00:18:23Z

lib/error-handler.js

      type: err.details[0].type
    })
+  } else if (err.isBoom) {
+    res.status(err.output.statusCode).send(err.output.payload)


Use um return aqui no lugar do else. Nos casos acima também acredito que isso ira facilitar a leitura.

thebergamo · 2016-11-16T00:18:58Z

lib/session.js

+  const session = cookieSession({
+    name: 'session',
+    keys: [process.env.COOKIE_SECRET],
+    maxAge: 168 * 60 * 60 * 1000, //  Uma semana


Concordo com o @brunops se ficar 24 * 7 facilita a leitura

thebergamo · 2016-11-16T00:20:10Z

package.json

-    "start": "node index.js"
+    "start": "node index.js",
+    "migrate": "npm run knex migrate:latest",
+    "reset": "node -e \"require('./lib/db').dropDatabase().then(() => process.exit(0), err => { console.error(err.stack); process.exit(1) })\"; npm run migrate"


Acredito que valha a pena mover isso para um arquivo e referenciar o mesmo aqui? Talvez criar um diretório scripts o mesmo acredito que valha pro test-cov

Fixed. Não acho que o test-cov deva ir para um script visto que é um punhado de argumentos, sem lógica de programação !== script.

thebergamo · 2016-11-16T00:21:04Z

resources/sessions/handlers.js

+
+  // Verifica se a senha está ok e seta a sessão
+  if (yield user.compare(req.body.password)) {
+    req.session.user_id = user.id


No caso, quando são objetos é melhor usar com o _ mesmo @brunops

thebergamo · 2016-11-16T00:22:01Z

resources/sessions/handlers.js

+  if (yield user.compare(req.body.password)) {
+    req.session.user_id = user.id
+  } else {
+    throw Boom.badData('Sua senha está incorreta')


Acho que faz mais sentido checar se não tem password e devolver um throw. Dai estar a sessão caso tudo esteja correto e seguir a vida.

@thebergamo não entendi

if (!yeld user.compare(req.body.password)) { throw Boom.badData('Sua senha está incorreta') }

Tipo isso ^

alanhoff · 2016-11-16T01:21:18Z

@thebergamo RFR

Implementa a sessão

c262652

Adiciona a variável de ambiente para os cookies

0e25ad2

brunops reviewed Nov 15, 2016

View reviewed changes

thebergamo requested changes Nov 16, 2016

View reviewed changes

Modificações adicionais para o code review

e7edd57

alanhoff merged commit d957869 into backend Nov 16, 2016

alanhoff deleted the feat/sessions branch November 16, 2016 01:34

thebergamo mentioned this pull request Nov 16, 2016

Endpoint de Sessão #68

Closed

thebergamo added the backend label Nov 16, 2016

thebergamo added this to the Site on AIR! milestone Nov 16, 2016

thebergamo assigned alanhoff Nov 16, 2016

Conversation

alanhoff commented Nov 15, 2016

Uh oh!

alanhoff commented Nov 15, 2016

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.79% (diff: 96.07%)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Nov 15, 2016 •

edited

Loading