Initialmodels #5

NadiaMouldi · 2018-01-04T12:40:01Z

No description provided.

erichkauffman · 2018-01-05T02:36:47Z

models/categories.js

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


use const rather than var on all model files

erichkauffman · 2018-01-05T02:38:02Z

models/categories.js

+  var categories = sequelize.define('categories', {
+    name: DataTypes.STRING
+  }, {
+    classMethods: {


I don't think sequelize is using classMethods anymore. If you checkout voting, the association is done outside of the sequelize.define parenthesis.

erichkauffman · 2018-01-05T02:39:36Z

models/item.js

+    name: DataTypes.STRING
+  }, {
+    classMethods: {
+      item.associate: function(models) {


Since this should be outside of the parenthesis, the association statement should look like item.associate = (models) => { Here we are using a lambda instead of function.

erichkauffman · 2018-01-05T02:40:09Z

models/menuitem.js

+    size: DataTypes.INTEGER
+    price: DataTypes.INTEGER
+  }, {
+    classMethods: {


Same as others, not using classMethods.

erichkauffman · 2018-01-05T02:40:22Z

models/menuitem.js

+    price: DataTypes.INTEGER
+  }, {
+    classMethods: {
+      menuitem.associate: function(models) {


Same as others, use lambda

erichkauffman · 2018-01-05T02:40:36Z

models/orders.js

+    email: DataTypes.STRING,
+    total_cost: DataTypes.FLOAT
+  }, {
+    classMethods: {


Same as others

erichkauffman · 2018-01-05T02:40:51Z

models/toppings.js

+    name: DataTypes.STRING,
+    price_extra: DataTypes.FLOAT
+  }, {
+    classMethods: {


Same as others

…ering into initialmodels

NadiaMouldi · 2018-01-05T15:36:07Z

Made the lambda changes.

elijahverdoorn

I didn't download and run this, but there are lots of issues with code style that need to be addressed before going over the actual logic. Make some changes, and poke me to take another look at it.

elijahverdoorn · 2018-01-06T19:33:13Z

migrations/1-create-order.js

+'use strict'
+module.exports = {
+  up: (queryInterface, Sequelize) => {
+    return queryInterface.createTable('order', {


if you're standardizing on lowercase for table names, that's fine, just be consistent. Some people prefer to caps the first letter, called PascalCase, but if you'd rather do it this way it's ok. Just want to point out the implicit decision being made here.

elijahverdoorn · 2018-01-06T19:33:56Z

migrations/2-create-category.js

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


Please add a newline at the end of this file.

elijahverdoorn · 2018-01-06T19:34:54Z

migrations/3-create-menuitem.js

+        type: Sequelize.STRING
+      },
+      latenight:{
+        type: Sequelize.STRING


Should these be STRING? To me they make more sense as BOOLEAN.

elijahverdoorn · 2018-01-06T19:35:05Z

migrations/3-create-menuitem.js

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


elijahverdoorn · 2018-01-06T19:35:59Z

migrations/3-create-menuitem.js

+'use strict';
+module.exports = {
+  up: (queryInterface, Sequelize) => {
+    return queryInterface.createTable('menuitem', {


This should either be camelCase, or use under_scores. It's hard to read with alllowercase.

elijahverdoorn · 2018-01-06T19:38:43Z

migrations/6-associate-item-order.js

+module.exports = {
+  up: (queryInterface, Sequelize) => {
+    return queryInterface.createTable(
+      'item-order',


Standardize on a naming convention please.

elijahverdoorn · 2018-01-06T19:39:31Z

migrations/6-associate-item-order.js

+          allowNull: false,
+          type: Sequelize.DATE,
+        },
+        ItemId: {


PascalCase?

elijahverdoorn · 2018-01-06T19:39:57Z

migrations/6-associate-item-order.js

+        },
+        ItemId: {
+          type: Sequelize.INTEGER,
+          primaryKey: true,


There should only be one primary key on a table.

elijahverdoorn · 2018-01-06T19:40:20Z

models/category.js

+module.exports = (sequelize, DataTypes) => {
+  const category = sequelize.define('category', {
+    name: DataTypes.STRING
+  }, {


Fix indentation

elijahverdoorn · 2018-01-06T19:40:28Z

models/category.js

+        // associations can be defined here
+      }
+  return category;
+}


elijahverdoorn · 2018-01-07T21:06:03Z

config/config.json

  "development": {
    "username": "root",
-    "password": null,
+    "password": "mysqlpassword",


Is there a reason for this? It is better to leave it to null, since most developers do not put a password on their development database.

no particular reason for this, I'll change it to null

elijahverdoorn · 2018-01-07T21:08:21Z

routes/orders.js

+ * @apiSuccess {String} description A description of the action
+ * @apiSuccess {Number} duration Duration, in seconds, of the action
+ * @apiSuccess {Number} assetId The asset that this action is associated with, if any
+ * @apiSuccess {Number} redValue If this is a background color changing action, the value of red that should be used. Should be between 0-255.


This should not be here, copy/paste leftovers.

I already updated the routes. Can you see the new changes?

Erich Kauffman and others added 7 commits November 4, 2017 15:15

Add sequelize-cli

7b58bf2

Create migrations for models

dea7aff

create models

fdc549a

Merge branch 'master' of https://github.com/oleville/pause-ordering

4ec26ec

create migrations for models

d4f8626

Delete 20171104204237-create-orders.js

cc85765

Delete 20171104204523-create-toppings.js

e3694b0

erichkauffman suggested changes Jan 5, 2018

View reviewed changes

NedjaEM added 2 commits January 5, 2018 09:24

Changed to use lambda and made table names singular

5dbb03d

Merge branch 'initialmodels' of https://github.com/oleville/pause-ord…

2eda9d3

…ering into initialmodels

erichkauffman requested review from elijahverdoorn and nicholascain January 5, 2018 21:08

elijahverdoorn requested changes Jan 6, 2018

View reviewed changes

NedjaEM added 3 commits January 6, 2018 15:54

changed all names to uder_score

51c5e57

changed some types to boolean in models

a15669e

added routes

e9cccee

elijahverdoorn requested changes Jan 7, 2018

View reviewed changes

NedjaEM added 3 commits January 17, 2018 05:26

fixing routes

1123dc6

changed password to null

8954187

removed menuitem

c44a016

Uh oh!

Initialmodels #5

Are you sure you want to change the base?

Initialmodels #5

Uh oh!

Conversation

NadiaMouldi commented Jan 4, 2018

Uh oh!

erichkauffman Jan 5, 2018 • 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!

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!

NadiaMouldi commented Jan 5, 2018

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

erichkauffman Jan 5, 2018 •

edited

Loading