Add species migrations & seeds by zgudino · Pull Request #30 · adoptapanama/admin-api

zgudino · 2018-03-29T22:23:13Z

Resolves #29.

fdvj

Vas bien, solo un par de correcciones y faltaria el test para probar que el seed está bien.

fdvj · 2018-03-29T22:48:05Z

migrations/20180329203816-create-specie.js

@@ -0,0 +1,28 @@
+'use strict';


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.

fdvj · 2018-03-29T22:48:25Z

migrations/20180329203816-create-specie.js

+      id: {
+        allowNull: false,
+        primaryKey: true,
+        type: Sequelize.UUID


Agregalle defaultValue: Sequelize.UUIDV4

fdvj · 2018-03-29T22:49:51Z

migrations/20180329203816-create-specie.js

+        type: Sequelize.STRING
+      },
+      createdAt: {
+        defaultValue: Sequelize.fn('NOW'),


No tienes q colocar esto. Sequelize lo crea automatico. Ponlo solo asi
createdAt: Sequelize.DATE

fdvj · 2018-03-29T22:50:16Z

migrations/20180329203816-create-specie.js

+        type: Sequelize.DATE
+      },
+      updatedAt: {
+        defaultValue: Sequelize.fn('NOW'),


Lo mismo aqui. Esto no es necesario. Pon
updatedAt: Sequelize.DATE

fdvj · 2018-03-29T22:50:46Z

migrations/20180329203816-create-specie.js

+      name: {
+        unique: true,
+        allowNull: false,
+        type: Sequelize.STRING


Limita el nombre de la especie a 32 caracteres
type: Sequelize.STRING(32)

fdvj · 2018-03-29T22:54:30Z

migrations/20180329204153-add-initial-species.js

+    return queryInterface.bulkInsert('species', species);
+  },
+
+  down: (queryInterface, Sequelize) => {


Lo mismo, action en lugar de queryInterface

fdvj · 2018-03-29T22:56:48Z

models/specie.js

+    }
+  );
+
+  Specie.associate = function(models) {


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

fdvj · 2018-03-29T22:58:12Z

models/specie.js

@@ -0,0 +1,19 @@
+'use strict';
+module.exports = (sequelize, DataTypes) => {


En lugar de sequelize, usa la variable db para seguir el formato:
https://github.com/adoptapanama/admin-api/blob/develop/models/role.js#L4

fdvj · 2018-03-29T22:58:51Z

models/specie.js

+'use strict';
+module.exports = (sequelize, DataTypes) => {
+  const Specie = sequelize.define(
+    'Specie',


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

fdvj · 2018-03-29T22:59:51Z

models/specie.js

+    'Specie',
+    {
+      id: Sequelize.UUID,
+      name: Sequelize.STRING


Agregale el allowNull: false, y el defaultValue: Sequelize.UUIDV4, y el primaryKey: true

zgudino · 2018-04-03T13:46:14Z

@fdvj procedi a hacer los ajustes que recomendaste; muchas gracias por el review. Me falta el test.

Pending

Migration's functional test is missing

fdvj · 2018-04-19T12:47:14Z

test/functional/specie.js

+    done();
+  });
+
+  it('should have field `createdAt` as date type', (done) => {


Este test no es necesario. Solamente el de arriba

Fue removido.

fdvj · 2018-04-19T12:47:49Z

test/functional/specie.js

+
+    const { Specie } = server.models;
+
+    await Specie.bulkCreate([{ name: 'anfibio' }]); // stubbed opcional


No entiendo el proposito de estos. No son necesarios para ningun test

await Server.initialize(); si lo mantengo porque ya resuelve Sequelize constructor.

fdvj · 2018-04-19T12:49:13Z

test/functional/specie.js

+    await Specie.bulkCreate([{ name: 'anfibio' }]); // stubbed opcional
+
+    // transformar instancia Model a JSON plano
+    await Specie.findAll({ limit: 10 }).then((models) => {


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());

Segui tu sugerencia. Gracias!

fdvj requested changes Mar 29, 2018

View reviewed changes

zgudino force-pushed the develop branch 3 times, most recently from 8945c7f to a6da43e Compare April 3, 2018 13:41

zgudino force-pushed the develop branch 3 times, most recently from aee058b to 8d64dee Compare April 6, 2018 21:38

fdvj requested changes Apr 19, 2018

View reviewed changes

Zahir Gudiño added 2 commits April 19, 2018 23:47

Add species migrations & seeds

1df5b95

Add species migrations test

db81bee

zgudino force-pushed the develop branch from 9178661 to db81bee Compare April 20, 2018 04:49

		@@ -0,0 +1,19 @@
		'use strict';
		module.exports = (sequelize, DataTypes) => {


		const { Specie } = server.models;

		await Specie.bulkCreate([{ name: 'anfibio' }]); // stubbed opcional

Conversation

zgudino commented Mar 29, 2018 • edited by fdvj Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

zgudino commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pending

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zgudino commented Mar 29, 2018 •

edited by fdvj

Loading

zgudino commented Apr 3, 2018 •

edited

Loading