Add species migrations & seeds #30
Conversation
| @@ -0,0 +1,28 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Los nombres solo tienen que llegar hasta minuto, no segundo. Seria 201803292038_create_species.js.
También, une las dos migraciones en una sola. No es necesario tener dos migraciones separadas.
| id: { | ||
| allowNull: false, | ||
| primaryKey: true, | ||
| type: Sequelize.UUID |
There was a problem hiding this comment.
Agregalle defaultValue: Sequelize.UUIDV4
| type: Sequelize.STRING | ||
| }, | ||
| createdAt: { | ||
| defaultValue: Sequelize.fn('NOW'), |
There was a problem hiding this comment.
No tienes q colocar esto. Sequelize lo crea automatico. Ponlo solo asi
createdAt: Sequelize.DATE
| type: Sequelize.DATE | ||
| }, | ||
| updatedAt: { | ||
| defaultValue: Sequelize.fn('NOW'), |
There was a problem hiding this comment.
Lo mismo aqui. Esto no es necesario. Pon
updatedAt: Sequelize.DATE
| name: { | ||
| unique: true, | ||
| allowNull: false, | ||
| type: Sequelize.STRING |
There was a problem hiding this comment.
Limita el nombre de la especie a 32 caracteres
type: Sequelize.STRING(32)
| return queryInterface.bulkInsert('species', species); | ||
| }, | ||
|
|
||
| down: (queryInterface, Sequelize) => { |
There was a problem hiding this comment.
Lo mismo, action en lugar de queryInterface
models/specie.js
Outdated
| } | ||
| ); | ||
|
|
||
| Specie.associate = function(models) { |
There was a problem hiding this comment.
Elimina esto. Las asociaciones se hacen a través de una opción que le agregamos al framework:
https://github.com/adoptapanama/admin-api/blob/develop/models/role.js#L44
models/specie.js
Outdated
| @@ -0,0 +1,19 @@ | |||
| 'use strict'; | |||
| module.exports = (sequelize, DataTypes) => { | |||
There was a problem hiding this comment.
En lugar de sequelize, usa la variable db para seguir el formato:
https://github.com/adoptapanama/admin-api/blob/develop/models/role.js#L4
models/specie.js
Outdated
| 'use strict'; | ||
| module.exports = (sequelize, DataTypes) => { | ||
| const Specie = sequelize.define( | ||
| 'Specie', |
There was a problem hiding this comment.
Sube specie a la primera linea, y luego abre el bracket como se ve aqui:
https://github.com/adoptapanama/admin-api/blob/develop/models/role.js#L5
models/specie.js
Outdated
| 'Specie', | ||
| { | ||
| id: Sequelize.UUID, | ||
| name: Sequelize.STRING |
There was a problem hiding this comment.
Agregale el allowNull: false, y el defaultValue: Sequelize.UUIDV4, y el primaryKey: true
zgudino
force-pushed
the
develop
branch
3 times, most recently
from
8945c7f
to
a6da43e
Compare
|
@fdvj procedi a hacer los ajustes que recomendaste; muchas gracias por el review. Me falta el test. Pending
|
zgudino
force-pushed
the
develop
branch
3 times, most recently
from
aee058b
to
8d64dee
Compare
test/functional/specie.js
Outdated
| done(); | ||
| }); | ||
|
|
||
| it('should have field `createdAt` as date type', (done) => { |
There was a problem hiding this comment.
Este test no es necesario. Solamente el de arriba
test/functional/specie.js
Outdated
|
|
||
| const { Specie } = server.models; | ||
|
|
||
| await Specie.bulkCreate([{ name: 'anfibio' }]); // stubbed opcional |
There was a problem hiding this comment.
No entiendo el proposito de estos. No son necesarios para ningun test
There was a problem hiding this comment.
await Server.initialize(); si lo mantengo porque ya resuelve Sequelize constructor.
test/functional/specie.js
Outdated
| await Specie.bulkCreate([{ name: 'anfibio' }]); // stubbed opcional | ||
|
|
||
| // transformar instancia Model a JSON plano | ||
| await Specie.findAll({ limit: 10 }).then((models) => { |
There was a problem hiding this comment.
Igual este. Otra observación es el await con el then. La idea del await es deshacer los promises (el then). Esto no va, pero de ir, lo escribirías así:
const models = await Specie.findAll({ limit: 10 });
species = models.map((model) => model.toJSON());There was a problem hiding this comment.
Segui tu sugerencia. Gracias!
Resolves #29.