#86, #87 #88

ngoctay · 2018-05-31T19:54:19Z

(code, unit tests, Postman updates)

#87 - updated Swagger

vikasrohit

@coderReview I have added few comments which we need to be fixed before getting this merged.
Overall it is looking good @ngoctay

vikasrohit · 2018-06-04T05:29:08Z

config/default.json

-  "AUTH0_CLIENT_SECRET": "",
-  "AUTH0_AUDIENCE": "",
-  "AUTH0_URL": "",
+  "AUTH0_CLIENT_ID": "5fctfjaLJHdvM04kSrCcC8yn0I4t1JTd",


Please remove this complete file from the commit. Where did you find these values?

OK. Will remove the AUTH_*** values from this file.
I got it from forum. It seems that we can't deploy the service without these values.

@ngoctay Can you please point me to the forum?

@vikasrohit I found those in another topcoder-platform github project.

here is the link: https://github.com/appirio-tech/tc-core-library-js/blob/feature/m2mtoken/config/test.json

vikasrohit · 2018-06-04T05:30:12Z

src/models/project.js

@@ -39,6 +36,7 @@ module.exports = function defineProject(sequelize, DataTypes) {
    cancelReason: DataTypes.STRING,
    version: DataTypes.STRING(15),
    templateId: DataTypes.BIGINT,
+    projectTemplateId: DataTypes.BIGINT,


Please use the templateId instead, which is already added to the project schema.

vikasrohit · 2018-06-04T05:53:02Z

src/routes/projectTypes/get.js

+  permissions('projectType.view'),
+  (req, res, next) => models.ProjectType.findOne({
+    where: {
+      key: req.params.key,


we should not retrieve records which are deleted i.e. deletedAt is not null.

We don't need it.
Sequelize will add the "deletedAt IS NOT NULL" condition automatically because the ProjectType model is paranoid.

tableName: 'project_types', paranoid: true, timestamps: true,

I also have a test case to verify it in get.spec.js:

it('should return 404 for deleted type', (done) => { models.ProjectType.destroy({ where: { key } }) .then(() => { request(server) .get(`/v4/projectTypes/${key}`) .set({ Authorization: `Bearer ${testUtil.jwts.admin}`, }) .expect(404, done); }); });

vikasrohit · 2018-06-04T05:53:39Z

src/routes/projectTypes/list.js

+module.exports = [
+  permissions('projectType.view'),
+  (req, res, next) => models.ProjectType.findAll({
+    attributes: { exclude: ['deletedAt', 'deletedBy'] },


We should not retrieve the deleted records i.e. where deletedAt is not null.

Same with get.js

vikasrohit · 2018-06-04T05:57:14Z

src/routes/projectTypes/update.js

+
+    return models.ProjectType.findOne({
+      where: {
+        key: req.params.key,


We should not update a deleted record i.e. where deletedAt is not null.

Same with get.js

vikasrohit · 2018-06-04T05:59:26Z

src/routes/projects/create.js

      details: Joi.any(),
      challengeEligibility: Joi.array().items(Joi.object().keys({
        role: Joi.string().valid('submitter', 'reviewer', 'copilot'),
        users: Joi.array().items(Joi.number().positive()),
        groups: Joi.array().items(Joi.number().positive()),
      })).allow(null),
      templateId: Joi.number().positive(),
+      projectTemplateId: Joi.number().integer().positive(),


Use templateId instead of new field.

vikasrohit · 2018-06-04T06:12:34Z

src/routes/projects/create.js

+        })
+        // Validate the projectTemplateId
+        .then(() => {
+          if (project.projectTemplateId) {


Please use templateId instead of projectTemplateId everywhere in the new code.

vikasrohit · 2018-06-04T06:51:46Z

src/routes/projects/create.js

+          }
+
+          const phases = _.values(projectTemplate.phases);
+          return Promise.all(_.map(phases, phase =>


We have to create phases before creating direct project. Please try to wrap (may be in the projects model) all project child entities creation in single promise so that projects.create does not look too much nested.

We have to create phases before creating direct project. Please try to wrap (may be in the projects model) all project child entities creation in single promise so that projects.create does not look too much nested.

OK. Will move it to a then() right after creating the project

Can we wrap it in a single method which creates project,phase and products and use that in project.create route?

i will wrap a single method which creates phases and products. Cannot wrap creating project because we need to return the project instance for creating direct project later.

Didn't get it. Even if we wrap phase/product creation in project creation method, we can still return project object for creating direct project later. Wrapping all 3 in single method would ease having a single database transaction as well.

I will wrap all 3 into a single method.
Regarding to the transaction, we don't need to specify it explicitly since we use continuation-local-storage.

vikasrohit · 2018-06-04T06:56:50Z

src/routes/projects/update.js

      details: Joi.any(),
      memers: Joi.any(),
+      projectTemplateId: Joi.any().strip(), // ignore the project template id


Please use templateId field.

vikasrohit · 2018-06-05T10:39:46Z

src/routes/projects/create.js

-                    return Promise.resolve();
-                  });
-              })));
+          return createPhases(req, newProject, projectTemplate);


Thanks @ngoctay for the changes, however, can we use the same database transaction, for creating phase/product, which project creation is using? I mean if project creation is not using transaction specifically, can we create a new transaction and pass that to both project and phase/product creation calls?

Sorry for late reply, @vikasrohit and @coderReview

Please don't worry about the transaction.
We have the transaction wrapper already:
models.sequelize.transaction(() => {

And we don't need the option { transaction: tx } because we use continuation-local-storage.
All db operations inside the models.sequelize.transaction(() => { are automatically wrapped in the transaction.

Reference: http://docs.sequelizejs.com/manual/tutorial/transactions.html

Thanks @ngoctay for the update. Understood.

vikasrohit · 2018-06-05T12:34:11Z

src/routes/projects/create.js

+          }
+
+          const phases = _.values(projectTemplate.phases);
+          return Promise.all(_.map(phases, phase =>


Didn't get it. Even if we wrap phase/product creation in project creation method, we can still return project object for creating direct project later. Wrapping all 3 in single method would ease having a single database transaction as well.

vikasrohit · 2018-06-06T05:08:08Z

Thanks @ngoctay for the hard work.

ngoctay added 2 commits May 31, 2018 10:49

#87 - Added /projectTypes endpoints

d1c284d

(code, unit tests, Postman updates)

#86 - code, unit tests, updated Postman and Swagger

7c10f7e

#87 - updated Swagger

ngoctay mentioned this pull request May 31, 2018

New model for storing project types in the database #87

Closed

vikasrohit self-requested a review June 4, 2018 07:01

vikasrohit suggested changes Jun 4, 2018

View reviewed changes

ngoctay added 2 commits June 4, 2018 16:18

Additiona fix for #86 #87 according to reviewer comments

5a8ab3c

#86 #87 - wrap a single method which creates phases and products

4203102

vikasrohit reviewed Jun 5, 2018

View reviewed changes

Wrap creating project, phases, and products in a single method

2fc571c

vikasrohit approved these changes Jun 6, 2018

View reviewed changes

vikasrohit merged commit 47c3667 into topcoder-platform:feature/dev-challenges Jun 6, 2018

#86, #87 #88

#86, #87 #88

Uh oh!

Conversation

ngoctay commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

vikasrohit Jun 4, 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!

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!

ngoctay Jun 6, 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!

vikasrohit commented Jun 6, 2018

Uh oh!

ngoctay commented May 31, 2018 •

edited

Loading

vikasrohit Jun 4, 2018 •

edited

Loading

ngoctay Jun 6, 2018 •

edited

Loading