From b9fc2dbb98696710c8d6024f65770c1b0f9c4a5b Mon Sep 17 00:00:00 2001 From: Gian Franco Zabarino Date: Sat, 28 Jul 2018 19:17:48 -0300 Subject: [PATCH 1/5] - Updating a milestone's completionDate/duration should update the dates of subsequent milestones. - Fixed bug where the transaction wasn't being included into wrapped queries. --- src/routes/milestones/update.js | 90 ++++++++++++++++++------- src/routes/milestones/update.spec.js | 98 +++++++++++++++++++++------- 2 files changed, 141 insertions(+), 47 deletions(-) diff --git a/src/routes/milestones/update.js b/src/routes/milestones/update.js index 9f4e19b9..85542b53 100644 --- a/src/routes/milestones/update.js +++ b/src/routes/milestones/update.js @@ -3,6 +3,7 @@ */ import validate from 'express-validation'; import _ from 'lodash'; +import moment from 'moment'; import Joi from 'joi'; import Sequelize from 'sequelize'; import { middleware as tcMiddleware } from 'tc-core-library-js'; @@ -13,6 +14,37 @@ import models from '../../models'; const permissions = tcMiddleware.permissions; +/** + * Cascades endDate/completionDate changes to all milestones with a greater order than the given one. + * @param {Object} updatedMilestone the milestone that was updated + * @param {Object} transaction the wrapping transaction + * @returns {Promise} a promise + */ +async function updateComingMilestones(updatedMilestone, transaction) { + const comingMilestones = _.sortBy(await models.Milestone.findAll({ + where: { + timelineId: updatedMilestone.timelineId, + order: { $gt: updatedMilestone.order }, + }, + transaction, + }), 'order'); + let startDate = moment.utc(updatedMilestone.completionDate + ? updatedMilestone.completionDate + : updatedMilestone.endDate).add(1, 'days').toDate(); + const promises = _.map(comingMilestones, (_milestone) => { + const milestone = _milestone; + if (milestone.startDate.getTime() !== startDate.getTime()) { + milestone.startDate = startDate; + milestone.endDate = moment.utc(startDate).add(milestone.duration - 1, 'days').toDate(); + } + startDate = moment.utc(milestone.completionDate + ? milestone.completionDate + : milestone.endDate).add(1, 'days').toDate(); + return milestone.save({ transaction }); + }); + await Promise.all(promises); +} + const schema = { params: { timelineId: Joi.number().integer().positive().required(), @@ -23,7 +55,7 @@ const schema = { id: Joi.any().strip(), name: Joi.string().max(255).optional(), description: Joi.string().max(255), - duration: Joi.number().integer().optional(), + duration: Joi.number().integer().min(1).optional(), startDate: Joi.date().optional(), endDate: Joi.date().allow(null), completionDate: Joi.date().allow(null), @@ -62,29 +94,10 @@ module.exports = [ timelineId: req.params.timelineId, }); - // Validate startDate and endDate to be within the timeline startDate and endDate - let error; - if (req.body.param.startDate < req.timeline.startDate) { - error = 'Milestone startDate must not be before the timeline startDate'; - } else if (req.body.param.endDate && req.timeline.endDate && req.body.param.endDate > req.timeline.endDate) { - error = 'Milestone endDate must not be after the timeline endDate'; - } - if (entityToUpdate.endDate && entityToUpdate.endDate < entityToUpdate.startDate) { - error = 'Milestone endDate must not be before startDate'; - } - if (entityToUpdate.completionDate && entityToUpdate.completionDate < entityToUpdate.startDate) { - error = 'Milestone endDate must not be before startDate'; - } - if (error) { - const apiErr = new Error(error); - apiErr.status = 422; - return next(apiErr); - } - let original; let updated; - return models.sequelize.transaction(() => + return models.sequelize.transaction(transaction => // Find the milestone models.Milestone.findOne({ where }) .then((milestone) => { @@ -94,14 +107,34 @@ module.exports = [ apiErr.status = 404; return Promise.reject(apiErr); } + // if any of these keys was provided and is different from what's in the database, error + if (['startDate', 'endDate'] + .some(key => entityToUpdate[key] && ( + !milestone[key] || + (milestone[key] && entityToUpdate[key].getTime() !== milestone[key].getTime()) + ))) { + const apiErr = new Error('Updating a milestone startDate or endDate is not allowed'); + apiErr.status = 422; + return Promise.reject(apiErr); + } + + if (entityToUpdate.completionDate && entityToUpdate.completionDate < milestone.startDate) { + const apiErr = new Error('The milestone completionDate should be greater or equal than the startDate.'); + apiErr.status = 422; + return Promise.reject(apiErr); + } original = _.omit(milestone.toJSON(), ['deletedAt', 'deletedBy']); // Merge JSON fields entityToUpdate.details = util.mergeJsonObjects(milestone.details, entityToUpdate.details); + if (entityToUpdate.duration && entityToUpdate.duration !== milestone.duration) { + entityToUpdate.endDate = moment.utc(milestone.startDate).add(entityToUpdate.duration - 1, 'days').toDate(); + } + // Update - return milestone.update(entityToUpdate); + return milestone.update(entityToUpdate, { transaction }); }) .then((updatedMilestone) => { // Omit deletedAt, deletedBy @@ -118,6 +151,7 @@ module.exports = [ id: { $ne: updated.id }, order: updated.order, }, + transaction, }) .then((count) => { if (count === 0) { @@ -133,6 +167,7 @@ module.exports = [ id: { $ne: updated.id }, order: { $between: [original.order + 1, updated.order] }, }, + transaction, }); } @@ -144,8 +179,19 @@ module.exports = [ id: { $ne: updated.id }, order: { $between: [updated.order, original.order - 1] }, }, + transaction, }); }); + }) + .then(() => { + // Update dates of the other milestones only if the completionDate nor the duration changed + if (((!original.completionDate && !updated.completionDate) || + (original.completionDate && updated.completionDate && + original.completionDate.getTime() === updated.completionDate.getTime())) && + original.duration === updated.duration) { + return Promise.resolve(); + } + return updateComingMilestones(updated, transaction); }), ) .then(() => { diff --git a/src/routes/milestones/update.spec.js b/src/routes/milestones/update.spec.js index 1d5edcc1..99fbc516 100644 --- a/src/routes/milestones/update.spec.js +++ b/src/routes/milestones/update.spec.js @@ -256,8 +256,6 @@ describe('UPDATE Milestone', () => { param: { name: 'Milestone 1-updated', duration: 3, - startDate: '2018-05-14T00:00:00.000Z', - endDate: '2018-05-15T00:00:00.000Z', completionDate: '2018-05-16T00:00:00.000Z', description: 'description-updated', status: 'closed', @@ -482,28 +480,10 @@ describe('UPDATE Milestone', () => { .expect(422, done); }); - it('should return 422 if startDate is after completionDate', (done) => { + it('should return 422 if startDate is different than the original startDate', (done) => { const invalidBody = { param: _.assign({}, body.param, { - startDate: '2018-05-29T00:00:00.000Z', - completionDate: '2018-05-28T00:00:00.000Z', - }), - }; - - request(server) - .patch('/v4/timelines/1/milestones/1') - .set({ - Authorization: `Bearer ${testUtil.jwts.admin}`, - }) - .send(invalidBody) - .expect('Content-Type', /json/) - .expect(422, done); - }); - - it('should return 422 if startDate is before timeline startDate', (done) => { - const invalidBody = { - param: _.assign({}, body.param, { - startDate: '2018-05-01T00:00:00.000Z', + startDate: '2018-07-01T00:00:00.000Z', }), }; @@ -517,7 +497,7 @@ describe('UPDATE Milestone', () => { .expect(422, done); }); - it('should return 422 if endDate is after timeline endDate', (done) => { + it('should return 422 if endDate is different than the original endDate', (done) => { const invalidBody = { param: _.assign({}, body.param, { endDate: '2018-07-01T00:00:00.000Z', @@ -548,8 +528,6 @@ describe('UPDATE Milestone', () => { resJson.name.should.be.eql(body.param.name); resJson.description.should.be.eql(body.param.description); resJson.duration.should.be.eql(body.param.duration); - resJson.startDate.should.be.eql(body.param.startDate); - resJson.endDate.should.be.eql(body.param.endDate); resJson.completionDate.should.be.eql(body.param.completionDate); resJson.status.should.be.eql(body.param.status); resJson.type.should.be.eql(body.param.type); @@ -898,6 +876,76 @@ describe('UPDATE Milestone', () => { }); }); + it('should return 200 for admin - changing completionDate will cascade changes to coming ' + + // eslint-disable-next-line func-names + 'milestones', function (done) { + this.timeout(10000); + + request(server) + .patch('/v4/timelines/1/milestones/2') + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send({ param: _.assign({}, body.param, { + completionDate: '2018-05-18T00:00:00.000Z', order: undefined, duration: undefined, + }) }) + .expect(200) + .end(() => { + // Milestone 3: startDate: '2018-05-14T00:00:00.000Z' to '2018-05-19T00:00:00.000Z' + // endDate: null to '2018-05-21T00:00:00.000Z' + // Milestone 4: startDate: '2018-05-14T00:00:00.000Z' to '2018-05-22T00:00:00.000Z' + // endDate: null to '2018-05-24T00:00:00.000Z' + setTimeout(() => { + models.Milestone.findById(3) + .then((milestone) => { + milestone.startDate.should.be.eql(new Date('2018-05-19T00:00:00.000Z')); + milestone.endDate.should.be.eql(new Date('2018-05-21T00:00:00.000Z')); + return models.Milestone.findById(4); + }) + .then((milestone) => { + milestone.startDate.should.be.eql(new Date('2018-05-22T00:00:00.000Z')); + milestone.endDate.should.be.eql(new Date('2018-05-24T00:00:00.000Z')); + done(); + }) + .catch(done); + }, 3000); + }); + }); + + it('should return 200 for admin - changing duration will cascade changes to coming ' + + // eslint-disable-next-line func-names + 'milestones', function (done) { + this.timeout(10000); + + request(server) + .patch('/v4/timelines/1/milestones/2') + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send({ param: _.assign({}, body.param, { duration: 5, order: undefined, completionDate: undefined }) }) + .expect(200) + .end(() => { + // Milestone 3: startDate: '2018-05-14T00:00:00.000Z' to '2018-05-19T00:00:00.000Z' + // endDate: null to '2018-05-21T00:00:00.000Z' + // Milestone 4: startDate: '2018-05-14T00:00:00.000Z' to '2018-05-22T00:00:00.000Z' + // endDate: null to '2018-05-24T00:00:00.000Z' + setTimeout(() => { + models.Milestone.findById(3) + .then((milestone) => { + milestone.startDate.should.be.eql(new Date('2018-05-19T00:00:00.000Z')); + milestone.endDate.should.be.eql(new Date('2018-05-21T00:00:00.000Z')); + return models.Milestone.findById(4); + }) + .then((milestone) => { + milestone.startDate.should.be.eql(new Date('2018-05-22T00:00:00.000Z')); + milestone.endDate.should.be.eql(new Date('2018-05-24T00:00:00.000Z')); + done(); + }) + .catch(done); + }, 3000); + }); + }); + it('should return 200 for connect admin', (done) => { request(server) .patch('/v4/timelines/1/milestones/1') From 0a504f4675e246f6c61698d43030d4e4cbc38d20 Mon Sep 17 00:00:00 2001 From: Gian Franco Zabarino Date: Sun, 29 Jul 2018 12:22:15 -0300 Subject: [PATCH 2/5] Moved back to implicit transactions. --- src/routes/milestones/update.js | 47 +++++++++++++++------------------ 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/src/routes/milestones/update.js b/src/routes/milestones/update.js index 85542b53..4c3df09c 100644 --- a/src/routes/milestones/update.js +++ b/src/routes/milestones/update.js @@ -17,32 +17,32 @@ const permissions = tcMiddleware.permissions; /** * Cascades endDate/completionDate changes to all milestones with a greater order than the given one. * @param {Object} updatedMilestone the milestone that was updated - * @param {Object} transaction the wrapping transaction * @returns {Promise} a promise */ -async function updateComingMilestones(updatedMilestone, transaction) { - const comingMilestones = _.sortBy(await models.Milestone.findAll({ +function updateComingMilestones(updatedMilestone) { + return models.Milestone.findAll({ where: { timelineId: updatedMilestone.timelineId, order: { $gt: updatedMilestone.order }, }, - transaction, - }), 'order'); - let startDate = moment.utc(updatedMilestone.completionDate - ? updatedMilestone.completionDate - : updatedMilestone.endDate).add(1, 'days').toDate(); - const promises = _.map(comingMilestones, (_milestone) => { - const milestone = _milestone; - if (milestone.startDate.getTime() !== startDate.getTime()) { - milestone.startDate = startDate; - milestone.endDate = moment.utc(startDate).add(milestone.duration - 1, 'days').toDate(); - } - startDate = moment.utc(milestone.completionDate - ? milestone.completionDate - : milestone.endDate).add(1, 'days').toDate(); - return milestone.save({ transaction }); + }).then((affectedMilestones) => { + const comingMilestones = _.sortBy(affectedMilestones, 'order'); + let startDate = moment.utc(updatedMilestone.completionDate + ? updatedMilestone.completionDate + : updatedMilestone.endDate).add(1, 'days').toDate(); + const promises = _.map(comingMilestones, (_milestone) => { + const milestone = _milestone; + if (milestone.startDate.getTime() !== startDate.getTime()) { + milestone.startDate = startDate; + milestone.endDate = moment.utc(startDate).add(milestone.duration - 1, 'days').toDate(); + } + startDate = moment.utc(milestone.completionDate + ? milestone.completionDate + : milestone.endDate).add(1, 'days').toDate(); + return milestone.save(); + }); + return Promise.all(promises); }); - await Promise.all(promises); } const schema = { @@ -97,7 +97,7 @@ module.exports = [ let original; let updated; - return models.sequelize.transaction(transaction => + return models.sequelize.transaction(() => // Find the milestone models.Milestone.findOne({ where }) .then((milestone) => { @@ -134,7 +134,7 @@ module.exports = [ } // Update - return milestone.update(entityToUpdate, { transaction }); + return milestone.update(entityToUpdate); }) .then((updatedMilestone) => { // Omit deletedAt, deletedBy @@ -151,7 +151,6 @@ module.exports = [ id: { $ne: updated.id }, order: updated.order, }, - transaction, }) .then((count) => { if (count === 0) { @@ -167,7 +166,6 @@ module.exports = [ id: { $ne: updated.id }, order: { $between: [original.order + 1, updated.order] }, }, - transaction, }); } @@ -179,7 +177,6 @@ module.exports = [ id: { $ne: updated.id }, order: { $between: [updated.order, original.order - 1] }, }, - transaction, }); }); }) @@ -191,7 +188,7 @@ module.exports = [ original.duration === updated.duration) { return Promise.resolve(); } - return updateComingMilestones(updated, transaction); + return updateComingMilestones(updated); }), ) .then(() => { From 737c0e6aca3043d43d6fe480ae106d3ae670ca81 Mon Sep 17 00:00:00 2001 From: Gian Franco Zabarino Date: Mon, 30 Jul 2018 21:28:13 -0300 Subject: [PATCH 3/5] - Removed setTimeout calls. - startDate and endDate are now forbidden from being present in the payload. - Removed explicit check against startDate and endDate parameters, since now they are forbidden. --- src/routes/milestones/update.js | 14 +- src/routes/milestones/update.spec.js | 196 +++++++++++---------------- 2 files changed, 80 insertions(+), 130 deletions(-) diff --git a/src/routes/milestones/update.js b/src/routes/milestones/update.js index 4c3df09c..41872634 100644 --- a/src/routes/milestones/update.js +++ b/src/routes/milestones/update.js @@ -56,8 +56,8 @@ const schema = { name: Joi.string().max(255).optional(), description: Joi.string().max(255), duration: Joi.number().integer().min(1).optional(), - startDate: Joi.date().optional(), - endDate: Joi.date().allow(null), + startDate: Joi.any().forbidden(), + endDate: Joi.any().forbidden(), completionDate: Joi.date().allow(null), status: Joi.string().max(45).optional(), type: Joi.string().max(45).optional(), @@ -107,16 +107,6 @@ module.exports = [ apiErr.status = 404; return Promise.reject(apiErr); } - // if any of these keys was provided and is different from what's in the database, error - if (['startDate', 'endDate'] - .some(key => entityToUpdate[key] && ( - !milestone[key] || - (milestone[key] && entityToUpdate[key].getTime() !== milestone[key].getTime()) - ))) { - const apiErr = new Error('Updating a milestone startDate or endDate is not allowed'); - apiErr.status = 422; - return Promise.reject(apiErr); - } if (entityToUpdate.completionDate && entityToUpdate.completionDate < milestone.startDate) { const apiErr = new Error('The milestone completionDate should be greater or equal than the startDate.'); diff --git a/src/routes/milestones/update.spec.js b/src/routes/milestones/update.spec.js index 99fbc516..1c066efe 100644 --- a/src/routes/milestones/update.spec.js +++ b/src/routes/milestones/update.spec.js @@ -462,56 +462,23 @@ describe('UPDATE Milestone', () => { .expect(200, done); }); - it('should return 422 if startDate is after endDate', (done) => { - const invalidBody = { - param: _.assign({}, body.param, { - startDate: '2018-05-29T00:00:00.000Z', - endDate: '2018-05-28T00:00:00.000Z', - }), - }; - - request(server) - .patch('/v4/timelines/1/milestones/1') - .set({ - Authorization: `Bearer ${testUtil.jwts.admin}`, - }) - .send(invalidBody) - .expect('Content-Type', /json/) - .expect(422, done); - }); - - it('should return 422 if startDate is different than the original startDate', (done) => { - const invalidBody = { - param: _.assign({}, body.param, { - startDate: '2018-07-01T00:00:00.000Z', - }), - }; - - request(server) - .patch('/v4/timelines/1/milestones/1') - .set({ - Authorization: `Bearer ${testUtil.jwts.admin}`, - }) - .send(invalidBody) - .expect('Content-Type', /json/) - .expect(422, done); - }); - - it('should return 422 if endDate is different than the original endDate', (done) => { - const invalidBody = { - param: _.assign({}, body.param, { - endDate: '2018-07-01T00:00:00.000Z', - }), - }; - - request(server) - .patch('/v4/timelines/1/milestones/1') - .set({ - Authorization: `Bearer ${testUtil.jwts.admin}`, - }) - .send(invalidBody) - .expect('Content-Type', /json/) - .expect(422, done); + ['startDate', 'endDate'].forEach((field) => { + it(`should return 422 if ${field} is present in the payload`, (done) => { + const invalidBody = { + param: _.assign({}, body.param, { + [field]: '2018-07-01T00:00:00.000Z', + }), + }; + + request(server) + .patch('/v4/timelines/1/milestones/1') + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send(invalidBody) + .expect('Content-Type', /json/) + .expect(422, done); + }); }); it('should return 200 for admin', (done) => { @@ -717,14 +684,13 @@ describe('UPDATE Milestone', () => { .expect(200) .end(() => { // Milestone 6: order 0 - setTimeout(() => { - models.Milestone.findById(6) - .then((milestone) => { - milestone.order.should.be.eql(0); - - done(); - }); - }, 3000); + models.Milestone.findById(6) + .then((milestone) => { + milestone.order.should.be.eql(0); + + done(); + }) + .catch(done); }); }); @@ -782,22 +748,21 @@ describe('UPDATE Milestone', () => { // Milestone 6: order 1 => 1 // Milestone 7: order 3 => 3 // Milestone 8: order 4 => 2 - setTimeout(() => { - models.Milestone.findById(6) - .then((milestone) => { - milestone.order.should.be.eql(1); - }) - .then(() => models.Milestone.findById(7)) - .then((milestone) => { - milestone.order.should.be.eql(3); - }) - .then(() => models.Milestone.findById(8)) - .then((milestone) => { - milestone.order.should.be.eql(2); - - done(); - }); - }, 3000); + models.Milestone.findById(6) + .then((milestone) => { + milestone.order.should.be.eql(1); + }) + .then(() => models.Milestone.findById(7)) + .then((milestone) => { + milestone.order.should.be.eql(3); + }) + .then(() => models.Milestone.findById(8)) + .then((milestone) => { + milestone.order.should.be.eql(2); + + done(); + }) + .catch(done); }); }); }); @@ -856,22 +821,21 @@ describe('UPDATE Milestone', () => { // Milestone 6: order 1 => 1 // Milestone 7: order 2 => 3 // Milestone 8: order 4 => 2 - setTimeout(() => { - models.Milestone.findById(6) - .then((milestone) => { - milestone.order.should.be.eql(1); - }) - .then(() => models.Milestone.findById(7)) - .then((milestone) => { - milestone.order.should.be.eql(3); - }) - .then(() => models.Milestone.findById(8)) - .then((milestone) => { - milestone.order.should.be.eql(2); - - done(); - }); - }, 3000); + models.Milestone.findById(6) + .then((milestone) => { + milestone.order.should.be.eql(1); + }) + .then(() => models.Milestone.findById(7)) + .then((milestone) => { + milestone.order.should.be.eql(3); + }) + .then(() => models.Milestone.findById(8)) + .then((milestone) => { + milestone.order.should.be.eql(2); + + done(); + }) + .catch(done); }); }); }); @@ -895,20 +859,18 @@ describe('UPDATE Milestone', () => { // endDate: null to '2018-05-21T00:00:00.000Z' // Milestone 4: startDate: '2018-05-14T00:00:00.000Z' to '2018-05-22T00:00:00.000Z' // endDate: null to '2018-05-24T00:00:00.000Z' - setTimeout(() => { - models.Milestone.findById(3) - .then((milestone) => { - milestone.startDate.should.be.eql(new Date('2018-05-19T00:00:00.000Z')); - milestone.endDate.should.be.eql(new Date('2018-05-21T00:00:00.000Z')); - return models.Milestone.findById(4); - }) - .then((milestone) => { - milestone.startDate.should.be.eql(new Date('2018-05-22T00:00:00.000Z')); - milestone.endDate.should.be.eql(new Date('2018-05-24T00:00:00.000Z')); - done(); - }) - .catch(done); - }, 3000); + models.Milestone.findById(3) + .then((milestone) => { + milestone.startDate.should.be.eql(new Date('2018-05-19T00:00:00.000Z')); + milestone.endDate.should.be.eql(new Date('2018-05-21T00:00:00.000Z')); + return models.Milestone.findById(4); + }) + .then((milestone) => { + milestone.startDate.should.be.eql(new Date('2018-05-22T00:00:00.000Z')); + milestone.endDate.should.be.eql(new Date('2018-05-24T00:00:00.000Z')); + done(); + }) + .catch(done); }); }); @@ -929,20 +891,18 @@ describe('UPDATE Milestone', () => { // endDate: null to '2018-05-21T00:00:00.000Z' // Milestone 4: startDate: '2018-05-14T00:00:00.000Z' to '2018-05-22T00:00:00.000Z' // endDate: null to '2018-05-24T00:00:00.000Z' - setTimeout(() => { - models.Milestone.findById(3) - .then((milestone) => { - milestone.startDate.should.be.eql(new Date('2018-05-19T00:00:00.000Z')); - milestone.endDate.should.be.eql(new Date('2018-05-21T00:00:00.000Z')); - return models.Milestone.findById(4); - }) - .then((milestone) => { - milestone.startDate.should.be.eql(new Date('2018-05-22T00:00:00.000Z')); - milestone.endDate.should.be.eql(new Date('2018-05-24T00:00:00.000Z')); - done(); - }) - .catch(done); - }, 3000); + models.Milestone.findById(3) + .then((milestone) => { + milestone.startDate.should.be.eql(new Date('2018-05-19T00:00:00.000Z')); + milestone.endDate.should.be.eql(new Date('2018-05-21T00:00:00.000Z')); + return models.Milestone.findById(4); + }) + .then((milestone) => { + milestone.startDate.should.be.eql(new Date('2018-05-22T00:00:00.000Z')); + milestone.endDate.should.be.eql(new Date('2018-05-24T00:00:00.000Z')); + done(); + }) + .catch(done); }); }); From 61cdff3025f43533b6d7327588bcc66f4c4ed44e Mon Sep 17 00:00:00 2001 From: Gian Franco Zabarino Date: Mon, 30 Jul 2018 21:49:03 -0300 Subject: [PATCH 4/5] Made code more readable. --- src/routes/milestones/update.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/routes/milestones/update.js b/src/routes/milestones/update.js index 41872634..03534061 100644 --- a/src/routes/milestones/update.js +++ b/src/routes/milestones/update.js @@ -171,14 +171,11 @@ module.exports = [ }); }) .then(() => { - // Update dates of the other milestones only if the completionDate nor the duration changed - if (((!original.completionDate && !updated.completionDate) || - (original.completionDate && updated.completionDate && - original.completionDate.getTime() === updated.completionDate.getTime())) && - original.duration === updated.duration) { - return Promise.resolve(); + // Update dates of the other milestones only if the completionDate or the duration changed + if (!_.isEqual(original.completionDate, updated.completionDate) || original.duration !== updated.duration) { + return updateComingMilestones(updated); } - return updateComingMilestones(updated); + return Promise.resolve(); }), ) .then(() => { From 377b0706b4c0da305db9398250c98ff171d702b5 Mon Sep 17 00:00:00 2001 From: Gian Franco Zabarino Date: Mon, 30 Jul 2018 23:25:14 -0300 Subject: [PATCH 5/5] - When updating a milestone, set updatedBy to updated records. - Handled scenario where the milestone's endDate was null and the startDate didn't change. Now the endDate gets updated. - When the last timeline's (ordered by order) endDate changes, the timeline gets set that endDate as its endDate. Added unit tests about this. - Updated Swagger documentation. - Removed startDate/endDate fields from Postman's milestone's PATCH requests, since now they are not accepted by the server. --- postman.json | 10 ++-- src/routes/milestones/update.js | 35 +++++++++++-- src/routes/milestones/update.spec.js | 66 ++++++++++++++++++++++++ swagger.yaml | 75 +++++++++++++++++++++++++--- 4 files changed, 170 insertions(+), 16 deletions(-) diff --git a/postman.json b/postman.json index 145003ff..a3336ae5 100644 --- a/postman.json +++ b/postman.json @@ -3802,7 +3802,7 @@ ], "body": { "mode": "raw", - "raw": "{\r\n \"param\":{\r\n \"name\": \"milestone 1-updated\",\r\n \"description\": \"description-updated\",\r\n \"duration\": 3,\r\n \"startDate\": \"2018-05-04T00:00:00.000Z\",\r\n \"endDate\": \"2018-05-06T00:00:00.000Z\",\r\n \"completionDate\": \"2018-05-07T00:00:00.000Z\",\r\n \"status\": \"closed\",\r\n \"type\": \"type2\",\r\n \"details\": {\r\n \"detail1\": {\r\n \"subDetail1C\": 3\r\n },\r\n \"detail2\": [\r\n 4\r\n ]\r\n },\r\n \"order\": 1,\r\n \"plannedText\": \"plannedText 1-updated\",\r\n \"activeText\": \"activeText 1-updated\",\r\n \"completedText\": \"completedText 1-updated\",\r\n \"blockedText\": \"blockedText 1-updated\"\r\n }\r\n}" + "raw": "{\r\n \"param\":{\r\n \"name\": \"milestone 1-updated\",\r\n \"description\": \"description-updated\",\r\n \"duration\": 3,\r\n \"completionDate\": \"2018-05-07T00:00:00.000Z\",\r\n \"status\": \"closed\",\r\n \"type\": \"type2\",\r\n \"details\": {\r\n \"detail1\": {\r\n \"subDetail1C\": 3\r\n },\r\n \"detail2\": [\r\n 4\r\n ]\r\n },\r\n \"order\": 1,\r\n \"plannedText\": \"plannedText 1-updated\",\r\n \"activeText\": \"activeText 1-updated\",\r\n \"completedText\": \"completedText 1-updated\",\r\n \"blockedText\": \"blockedText 1-updated\"\r\n }\r\n}" }, "url": { "raw": "{{api-url}}/v4/timelines/1/milestones/1", @@ -3836,7 +3836,7 @@ ], "body": { "mode": "raw", - "raw": "{\r\n \"param\":{\r\n \"name\": \"milestone 1-updated\",\r\n \"description\": \"description-updated\",\r\n \"duration\": 3,\r\n \"startDate\": \"2018-05-04T00:00:00.000Z\",\r\n \"endDate\": \"2018-05-06T00:00:00.000Z\",\r\n \"completionDate\": \"2018-05-07T00:00:00.000Z\",\r\n \"status\": \"closed\",\r\n \"type\": \"type2\",\r\n \"details\": {\r\n \"detail1\": {\r\n \"subDetail1C\": 3\r\n },\r\n \"detail2\": [\r\n 4\r\n ]\r\n },\r\n \"order\": 2,\r\n \"plannedText\": \"plannedText 1-updated\",\r\n \"activeText\": \"activeText 1-updated\",\r\n \"completedText\": \"completedText 1-updated\",\r\n \"blockedText\": \"blockedText 1-updated\"\r\n }\r\n}" + "raw": "{\r\n \"param\":{\r\n \"name\": \"milestone 1-updated\",\r\n \"description\": \"description-updated\",\r\n \"duration\": 3,\r\n \"completionDate\": \"2018-05-07T00:00:00.000Z\",\r\n \"status\": \"closed\",\r\n \"type\": \"type2\",\r\n \"details\": {\r\n \"detail1\": {\r\n \"subDetail1C\": 3\r\n },\r\n \"detail2\": [\r\n 4\r\n ]\r\n },\r\n \"order\": 2,\r\n \"plannedText\": \"plannedText 1-updated\",\r\n \"activeText\": \"activeText 1-updated\",\r\n \"completedText\": \"completedText 1-updated\",\r\n \"blockedText\": \"blockedText 1-updated\"\r\n }\r\n}" }, "url": { "raw": "{{api-url}}/v4/timelines/1/milestones/1", @@ -3870,7 +3870,7 @@ ], "body": { "mode": "raw", - "raw": "{\r\n \"param\":{\r\n \"name\": \"milestone 1-updated\",\r\n \"description\": \"description-updated\",\r\n \"duration\": 3,\r\n \"startDate\": \"2018-05-04T00:00:00.000Z\",\r\n \"endDate\": \"2018-05-06T00:00:00.000Z\",\r\n \"completionDate\": \"2018-05-07T00:00:00.000Z\",\r\n \"status\": \"closed\",\r\n \"type\": \"type2\",\r\n \"details\": {\r\n \"detail1\": {\r\n \"subDetail1C\": 3\r\n },\r\n \"detail2\": [\r\n 4\r\n ]\r\n },\r\n \"order\": 1,\r\n \"plannedText\": \"plannedText 1-updated\",\r\n \"activeText\": \"activeText 1-updated\",\r\n \"completedText\": \"completedText 1-updated\",\r\n \"blockedText\": \"blockedText 1-updated\"\r\n }\r\n}" + "raw": "{\r\n \"param\":{\r\n \"name\": \"milestone 1-updated\",\r\n \"description\": \"description-updated\",\r\n \"duration\": 3,\r\n \"completionDate\": \"2018-05-07T00:00:00.000Z\",\r\n \"status\": \"closed\",\r\n \"type\": \"type2\",\r\n \"details\": {\r\n \"detail1\": {\r\n \"subDetail1C\": 3\r\n },\r\n \"detail2\": [\r\n 4\r\n ]\r\n },\r\n \"order\": 1,\r\n \"plannedText\": \"plannedText 1-updated\",\r\n \"activeText\": \"activeText 1-updated\",\r\n \"completedText\": \"completedText 1-updated\",\r\n \"blockedText\": \"blockedText 1-updated\"\r\n }\r\n}" }, "url": { "raw": "{{api-url}}/v4/timelines/1/milestones/1", @@ -3904,7 +3904,7 @@ ], "body": { "mode": "raw", - "raw": "{\r\n \"param\":{\r\n \"name\": \"milestone 1-updated\",\r\n \"description\": \"description-updated\",\r\n \"duration\": 3,\r\n \"startDate\": \"2018-05-04T00:00:00.000Z\",\r\n \"endDate\": \"2018-05-06T00:00:00.000Z\",\r\n \"completionDate\": \"2018-05-07T00:00:00.000Z\",\r\n \"status\": \"closed\",\r\n \"type\": \"type2\",\r\n \"details\": {\r\n \"detail1\": {\r\n \"subDetail1C\": 3\r\n },\r\n \"detail2\": [\r\n 4\r\n ]\r\n },\r\n \"order\": 3,\r\n \"plannedText\": \"plannedText 1-updated\",\r\n \"activeText\": \"activeText 1-updated\",\r\n \"completedText\": \"completedText 1-updated\",\r\n \"blockedText\": \"blockedText 1-updated\"\r\n }\r\n}" + "raw": "{\r\n \"param\":{\r\n \"name\": \"milestone 1-updated\",\r\n \"description\": \"description-updated\",\r\n \"duration\": 3,\r\n \"completionDate\": \"2018-05-07T00:00:00.000Z\",\r\n \"status\": \"closed\",\r\n \"type\": \"type2\",\r\n \"details\": {\r\n \"detail1\": {\r\n \"subDetail1C\": 3\r\n },\r\n \"detail2\": [\r\n 4\r\n ]\r\n },\r\n \"order\": 3,\r\n \"plannedText\": \"plannedText 1-updated\",\r\n \"activeText\": \"activeText 1-updated\",\r\n \"completedText\": \"completedText 1-updated\",\r\n \"blockedText\": \"blockedText 1-updated\"\r\n }\r\n}" }, "url": { "raw": "{{api-url}}/v4/timelines/1/milestones/1", @@ -3938,7 +3938,7 @@ ], "body": { "mode": "raw", - "raw": "{\r\n \"param\":{\r\n \"name\": \"milestone 1-updated\",\r\n \"description\": \"description-updated\",\r\n \"duration\": 3,\r\n \"startDate\": \"2018-05-04T00:00:00.000Z\",\r\n \"endDate\": \"2018-05-06T00:00:00.000Z\",\r\n \"completionDate\": \"2018-05-07T00:00:00.000Z\",\r\n \"status\": \"closed\",\r\n \"type\": \"type2\",\r\n \"details\": {\r\n \"detail1\": {\r\n \"subDetail1C\": 3\r\n },\r\n \"detail2\": [\r\n 4\r\n ]\r\n },\r\n \"order\": 1,\r\n \"plannedText\": \"plannedText 1-updated\",\r\n \"activeText\": \"activeText 1-updated\",\r\n \"completedText\": \"completedText 1-updated\",\r\n \"blockedText\": \"blockedText 1-updated\"\r\n }\r\n}" + "raw": "{\r\n \"param\":{\r\n \"name\": \"milestone 1-updated\",\r\n \"description\": \"description-updated\",\r\n \"duration\": 3,\r\n \"completionDate\": \"2018-05-07T00:00:00.000Z\",\r\n \"status\": \"closed\",\r\n \"type\": \"type2\",\r\n \"details\": {\r\n \"detail1\": {\r\n \"subDetail1C\": 3\r\n },\r\n \"detail2\": [\r\n 4\r\n ]\r\n },\r\n \"order\": 1,\r\n \"plannedText\": \"plannedText 1-updated\",\r\n \"activeText\": \"activeText 1-updated\",\r\n \"completedText\": \"completedText 1-updated\",\r\n \"blockedText\": \"blockedText 1-updated\"\r\n }\r\n}" }, "url": { "raw": "{{api-url}}/v4/timelines/1/milestones/1", diff --git a/src/routes/milestones/update.js b/src/routes/milestones/update.js index 03534061..9cb2c698 100644 --- a/src/routes/milestones/update.js +++ b/src/routes/milestones/update.js @@ -17,7 +17,9 @@ const permissions = tcMiddleware.permissions; /** * Cascades endDate/completionDate changes to all milestones with a greater order than the given one. * @param {Object} updatedMilestone the milestone that was updated - * @returns {Promise} a promise + * @returns {Promise} a promise that resolves to the last found milestone. If no milestone exists with an + * order greater than the passed updatedMilestone, the promise will resolve to the passed + * updatedMilestone */ function updateComingMilestones(updatedMilestone) { return models.Milestone.findAll({ @@ -32,16 +34,29 @@ function updateComingMilestones(updatedMilestone) { : updatedMilestone.endDate).add(1, 'days').toDate(); const promises = _.map(comingMilestones, (_milestone) => { const milestone = _milestone; - if (milestone.startDate.getTime() !== startDate.getTime()) { + + // Update the milestone startDate if different than the iterated startDate + if (!_.isEqual(milestone.startDate, startDate)) { milestone.startDate = startDate; - milestone.endDate = moment.utc(startDate).add(milestone.duration - 1, 'days').toDate(); + milestone.updatedBy = updatedMilestone.updatedBy; + } + + // Calculate the endDate, and update it if different + const endDate = moment.utc(startDate).add(milestone.duration - 1, 'days').toDate(); + if (!_.isEqual(milestone.endDate, endDate)) { + milestone.endDate = endDate; + milestone.updatedBy = updatedMilestone.updatedBy; } + + // Set the next startDate value to the next day after completionDate if present or the endDate startDate = moment.utc(milestone.completionDate ? milestone.completionDate : milestone.endDate).add(1, 'days').toDate(); return milestone.save(); }); - return Promise.all(promises); + + // Resolve promise to the last updated milestone, or to the passed in updatedMilestone + return Promise.all(promises).then(updatedMilestones => updatedMilestones.pop() || updatedMilestone); }); } @@ -94,6 +109,8 @@ module.exports = [ timelineId: req.params.timelineId, }); + const timeline = req.timeline; + let original; let updated; @@ -173,7 +190,15 @@ module.exports = [ .then(() => { // Update dates of the other milestones only if the completionDate or the duration changed if (!_.isEqual(original.completionDate, updated.completionDate) || original.duration !== updated.duration) { - return updateComingMilestones(updated); + return updateComingMilestones(updated) + .then((lastTimelineMilestone) => { + if (!_.isEqual(lastTimelineMilestone.endDate, timeline.endDate)) { + timeline.endDate = lastTimelineMilestone.endDate; + timeline.updatedBy = lastTimelineMilestone.updatedBy; + return timeline.save(); + } + return Promise.resolve(); + }); } return Promise.resolve(); }), diff --git a/src/routes/milestones/update.spec.js b/src/routes/milestones/update.spec.js index 1c066efe..b3fde3fe 100644 --- a/src/routes/milestones/update.spec.js +++ b/src/routes/milestones/update.spec.js @@ -874,6 +874,40 @@ describe('UPDATE Milestone', () => { }); }); + it('should return 200 for admin - changing completionDate will change the timeline\'s ' + + // eslint-disable-next-line func-names + 'endDate', function (done) { + this.timeout(10000); + + request(server) + .patch('/v4/timelines/1/milestones/2') + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send({ param: _.assign({}, body.param, { + completionDate: '2018-05-18T00:00:00.000Z', order: undefined, duration: undefined, + }) }) + .expect(200) + .end(() => { + // Milestone 3: startDate: '2018-05-14T00:00:00.000Z' to '2018-05-19T00:00:00.000Z' + // endDate: null to '2018-05-21T00:00:00.000Z' + // Milestone 4: startDate: '2018-05-14T00:00:00.000Z' to '2018-05-22T00:00:00.000Z' + // BELOW will be the new timeline's endDate + // endDate: null to '2018-05-24T00:00:00.000Z' + models.Timeline.findById(1) + .then((timeline) => { + // timeline start shouldn't change + timeline.startDate.should.be.eql(new Date('2018-05-02T00:00:00.000Z')); + + // timeline end should change + timeline.endDate.should.be.eql(new Date('2018-05-24T00:00:00.000Z')); + + done(); + }) + .catch(done); + }); + }); + it('should return 200 for admin - changing duration will cascade changes to coming ' + // eslint-disable-next-line func-names 'milestones', function (done) { @@ -906,6 +940,38 @@ describe('UPDATE Milestone', () => { }); }); + it('should return 200 for admin - changing duration will change the timeline\'s ' + + // eslint-disable-next-line func-names + 'endDate', function (done) { + this.timeout(10000); + + request(server) + .patch('/v4/timelines/1/milestones/2') + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send({ param: _.assign({}, body.param, { duration: 5, order: undefined, completionDate: undefined }) }) + .expect(200) + .end(() => { + // Milestone 3: startDate: '2018-05-14T00:00:00.000Z' to '2018-05-19T00:00:00.000Z' + // endDate: null to '2018-05-21T00:00:00.000Z' + // Milestone 4: startDate: '2018-05-14T00:00:00.000Z' to '2018-05-22T00:00:00.000Z' + // BELOW will be the new timeline's endDate + // endDate: null to '2018-05-24T00:00:00.000Z' + models.Timeline.findById(1) + .then((timeline) => { + // timeline start shouldn't change + timeline.startDate.should.be.eql(new Date('2018-05-02T00:00:00.000Z')); + + // timeline end should change + timeline.endDate.should.be.eql(new Date('2018-05-24T00:00:00.000Z')); + + done(); + }) + .catch(done); + }); + }); + it('should return 200 for connect admin', (done) => { request(server) .patch('/v4/timelines/1/milestones/1') diff --git a/swagger.yaml b/swagger.yaml index d2799dee..b95bbe94 100755 --- a/swagger.yaml +++ b/swagger.yaml @@ -1335,7 +1335,7 @@ paths: name: body required: true schema: - $ref: '#/definitions/MilestoneBodyParam' + $ref: '#/definitions/MilestonePostBodyParam' responses: '403': description: No permission or wrong token @@ -1413,7 +1413,7 @@ paths: in: body required: true schema: - $ref: "#/definitions/MilestoneBodyParam" + $ref: "#/definitions/MilestonePatchBodyParam" delete: tags: @@ -3201,7 +3201,7 @@ definitions: items: $ref: "#/definitions/Timeline" - MilestoneRequest: + MilestonePostRequest: title: Milestone request object type: object required: @@ -3264,14 +3264,77 @@ definitions: type: string description: the milestone blocked text - MilestoneBodyParam: + MilestonePatchRequest: + title: Milestone request object + type: object + required: + - name + - duration + - status + - type + - order + - plannedText + - activeText + - completedText + - blockedText + properties: + name: + type: string + description: the milestone name + description: + type: string + description: the milestone description + duration: + type: number + format: integer + description: the milestone duration + completionDate: + type: string + format: date + description: the milestone completion date + status: + type: string + description: the milestone status + type: + type: string + description: the milestone type + details: + type: object + description: the milestone details + order: + type: number + format: integer + description: the milestone order + plannedText: + type: string + description: the milestone planned text + activeText: + type: string + description: the milestone active text + completedText: + type: string + description: the milestone completed text + blockedText: + type: string + description: the milestone blocked text + + MilestonePostBodyParam: + title: Milestone body param + type: object + required: + - param + properties: + param: + $ref: "#/definitions/MilestonePostRequest" + + MilestonePatchBodyParam: title: Milestone body param type: object required: - param properties: param: - $ref: "#/definitions/MilestoneRequest" + $ref: "#/definitions/MilestonePatchRequest" Milestone: title: Milestone object @@ -3306,7 +3369,7 @@ definitions: format: int64 description: READ-ONLY. User that last updated this object readOnly: true - - $ref: "#/definitions/MilestoneRequest" + - $ref: "#/definitions/MilestonePostRequest" MilestoneResponse: title: Single milestone response object