Backend - Shares by FelipeMonobe · Pull Request #69 · nodebr/nodebr

FelipeMonobe · 2016-11-15T18:46:21Z

Shares

Handlers
Model
Routes
Schemas
Fixtures
Tests

alanhoff · 2016-11-15T18:47:39Z

package.json

    "test": "standard && lab --verbose --colors --assert code --ignore __core-js_shared__",
    "test-cov": "npm test -- -r console -o stdout -r html -o coverage/coverage.html -r lcov -o coverage/lcov.info",
    "knex": "knex --knexfile ./lib/db/knexfile.js",
+    "fix": "standard --fix",


Não tenho um motivo muito forte para retirar este comando daqui, mas precisa ter cuidado com ele.

alanhoff · 2016-11-15T18:48:16Z

resources/shares/handlers.js

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


mudar o nome para findAll

alanhoff · 2016-11-15T18:48:35Z

resources/shares/handlers.js

+const Share = db.model('Share')
+
+exports.findLimitedByPage = (req, res) => {
+  const offset = req.query.page * req.query.limit


Achei que o offset e o limit estariam na query, está correto?

O que vem na query é o limit e a page.
Ai dessa forma o offset teria que ser calculado, vai trocar pra vir o offset direto do front?

Isso, vamos fazer com que o offset também venha, não faz muito sentido ter page e limit, mas offset e limit sim :-)

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

resources/shares/handlers.js

+}
+
+exports.findOne = (req, res) => {
+  return Share


Acho que vai dar problema se o share não existir

alanhoff · 2016-11-15T18:50:13Z

resources/shares/handlers.js

+}
+
+exports.remove = (req, res) => {
+  return Share


Quando implementar a sessão precisamos ter certeza que apenas o usuário que criou o share poderá excluir o mesmo

alanhoff · 2016-11-15T18:58:39Z

test/resources/shares.js

+  })
+
+  lab.describe(`GET ${ENDPOINT}/:id`, () => {
+    lab.test('deve retornar a linha selecionada', co.wrap(function * () {


Faltando um teste para quando não achar o share

alanhoff · 2016-11-15T18:59:03Z

test/resources/shares.js

+  })
+
+  lab.describe(`DELETE ${ENDPOINT}/:id`, () => {
+    lab.test('deve remover do banco de dados a linha selecionada', co.wrap(function * () {


Faltando o teste para remover apenas os shares que pertencem ao usuário

alanhoff · 2016-11-15T18:59:39Z

resources/shares/model.js

+ * @param {Function} bookshelf Uma instância do Bookshelf
+ */
+module.exports = bookshelf => bookshelf.model('Share', {
+  tableName: 'shares'


Faltando o relacionamento para a table users.

alanhoff · 2016-11-15T19:01:10Z

resources/shares/handlers.js

+exports.findLimitedByPage = (req, res) => {
+  const offset = req.query.page * req.query.limit
+
+  return Share


Deve também retornar o usuário:

{ title: .., link: .., user: { id: ..., username: ... } }

alanhoff · 2016-11-15T19:01:28Z

resources/shares/handlers.js

+
+exports.findOne = (req, res) => {
+  return Share
+    .findById(req.params.id)


Deve retornar o usuário também

- 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

Closes #64

Implementa a sessão

thebergamo · 2016-11-24T13:40:50Z

@alanhoff @FelipeMonobe como está esse cara?

arku · 2016-12-17T14:49:54Z

test/resources/shares.js

+      yield fixtures.users.insertMultiple()
+      yield fixtures.shares.insertMultipleWithId([id])
+
+      const req = yield request(server)


@FelipeMonobe

From MDN docs,

A constant cannot share its name with a function or a variable in the same scope.

The constant variable req is used in the same scope, again in line 174. And that is why, CI build fails. Can you rename the variable to something else?

@arun1595 well spotted man. I'm switching the first req to a simple yield, it should fix this issue

…(3 last tests)

FelipeMonobe · 2016-12-18T18:16:15Z

#69

The last three tests are still failing. I couldn't find out where's the issue in my code.

GET /compartilhamentos/:id with a different id should expect status code 404 but the request is timing out with Unhandled rejection CustomError: EmptyResponse, maybe it has something to do with error handling;
DELETE /compartilhamentos/:id both tests there's something wrong going on with sessions resource, it fails with Cannot read property 'map' of undefined at /test/fixtures/sessions.js:14:3

thebergamo · 2016-12-19T17:56:40Z

resources/shares/handlers.js

+  return Shares
+  .forge({ id: req.params.id })
+  .destroy()
+  .then(() => res.send({ success: true }))


Check this, you must implement an catch for this and other promises. Probably the error is generated for an error in constraint.

Dude, I'm not sure that's the problem, since the sessions handler also has an uncaught function:

exports.findOne = (req, res) => { User.findById(req.session.user_id) .then((user) => res.send(user)) }

I've put catches to all my functions and they didn't work

I need to look more closely. Will do some tests in the week

codecov-io · 2016-12-30T16:44:35Z

Current coverage is 96.60% (diff: 95.85%)

Merging #69 into backend will increase coverage by 0.46%

@@            backend        #69   diff @@
==========================================
  Files            15         25    +10   
  Lines           311        560   +249   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            299        541   +242   
- Misses           12         19     +7   
  Partials          0          0

Powered by Codecov. Last update aabd929...ded0f5c

FelipeMonobe added 3 commits November 15, 2016 15:09

shares handlers, model, routes, schemas, fixtures and tests

fb43672

user_id index

73fdb89

standard fix & minor fixes

2abea76

alanhoff requested changes Nov 15, 2016

View reviewed changes

alanhoff and others added 26 commits November 15, 2016 19:08

Implementa a sessão

c262652

Adiciona a variável de ambiente para os cookies

0e25ad2

migrations & first review fixes

c6df11f

Load environment variables from .env with dotenv module

416c37f

Add steps necessary to run the app to the README

46087d8

add morgan to print request logs

f2e1cbb

Add users resource

a533583

- 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

lint

2e44a70

Arruma o truncate para operar com múltiplas tabelas

3293096

Closes #64

Arruma as configurações do CircleCI

447c7e3

Adequando às especificoes

f1d1cab

Removendo workarround baseado no PR do alan

471ff27

Removendo setup do readme.md em prol da wiki

c1f1caf

Trocando nome da rota para pt-br

6a2f0f1

Trocando nome da rota para plural

e64bbbf

chore stuff - linting, remove blank lines etc

73be47a

test duplicate user insertion

0c6bb3d

rename migration

98918a2

standard fix

f262253

standard fix #2

6714d7b

Modificações adicionais para o code review

e7edd57

Merge pull request #70 from nodebr/feat/sessions

d957869

Implementa a sessão

shares handlers, model, routes, schemas, fixtures and tests

db0270e

user_id index

c11e80e

standard fix & minor fixes

fc53d00

migrations & first review fixes

a47c0bd

bsanches and others added 7 commits November 15, 2016 23:46

Add steps necessary to run the app to the README

c2c1113

Removendo setup do readme.md em prol da wiki

3bb678e

rename migration

504f39d

standard fix

4f656bb

standard fix #2

c344fbe

share - tests failing

6be9fa1

merge

8eac3a9

thebergamo mentioned this pull request Nov 16, 2016

Endpoint de compartilhamento #61

Open

thebergamo added the backend label Nov 16, 2016

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

arku suggested changes Dec 17, 2016

View reviewed changes

1 test removed, some tests fixed, pending timeout & session failures …

ded0f5c

…(3 last tests)

thebergamo reviewed Dec 19, 2016

View reviewed changes

catches

f50970c

Conversation

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

thebergamo commented Nov 24, 2016

Uh oh!

arku Dec 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

FelipeMonobe commented Dec 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FelipeMonobe Dec 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 30, 2016

Current coverage is 96.60% (diff: 95.85%)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arku Dec 17, 2016 •

edited

Loading

FelipeMonobe Dec 30, 2016 •

edited

Loading