Timeline and Milestone REST API + Misc Fixes #90
Conversation
… API" challenge (30065695)
- (Review) Milestone: startDate, status should be required fields
- (Review) When deleting a timeline, it should be cascaded to it's milestone as well
- (Review) Milestone completionDate must not be before startDate
- (Review) Postman: DELETE should not have request body
- (Review) Postman: add at least 1 negative test case for the POST end points
- (Paulo) Don’t allow startDate and endDate of milestones out of bound of it’s parent timeline.
If startDate or endDate for timeline is updated, children must validated and changed accordingly
- (Paulo) GET /timelines, /timelines/{id} should return the associated milestones
- (Paulo) Update milestone order for PATCH endpoint
|
@coderReview @vikasrohit Please have a look. |
src/routes/milestones/update.spec.js
Outdated
| // Milestone 2: order 3 | ||
| // Milestone 3: order 4 | ||
| // Milestone 4: order 0 | ||
| setTimeout(() => { |
There was a problem hiding this comment.
this is not correct, since 0 doesn't have anyone, the other orders are not changed.
| }, 3000); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
please add a few more tests for order -> like changing order with only 1 item in list and 2 items in list (these are edge cases where we can get errors if not taken care properly. or even negative order number).
also, try this case were we have this list: 1, 3, 4 and we update 4 -> 2 (this is just update, won't have any order change since order 2 is available), and 1, 2, 4 and we update 4 -> 2 (this case we end like this: 1, 2 (it was 4), 3).
There was a problem hiding this comment.
I added new tests. Please check the latest commit:
| name: { type: DataTypes.STRING(255), allowNull: false }, | ||
| description: DataTypes.STRING(255), | ||
| duration: { type: DataTypes.INTEGER, allowNull: false }, | ||
| startDate: { type: DataTypes.DATE, allowNull: false }, |
There was a problem hiding this comment.
(Review) Milestone: startDate, status should be required fields
| // Update the deletedBy, then delete | ||
| timeline.update({ deletedBy: req.authUser.userId }) | ||
| .then(() => timeline.destroy()) | ||
| // Cascade delete the milestones |
There was a problem hiding this comment.
(Review) When deleting a timeline, it should be cascaded to it's milestone as well
| duration: Joi.number().integer().required(), | ||
| startDate: Joi.date().required(), | ||
| endDate: Joi.date().min(Joi.ref('startDate')).allow(null), | ||
| completionDate: Joi.date().min(Joi.ref('startDate')).allow(null), |
There was a problem hiding this comment.
(Review) Milestone completionDate must not be before startDate
There was a problem hiding this comment.
@ngoctay I think it should not be after endDate, wouldn't you agree?
| duration: Joi.number().integer().required(), | ||
| startDate: Joi.date().required(), | ||
| endDate: Joi.date().min(Joi.ref('startDate')).allow(null), | ||
| completionDate: Joi.date().min(Joi.ref('startDate')).allow(null), |
There was a problem hiding this comment.
(Review) Milestone completionDate must not be before startDate
There was a problem hiding this comment.
@ngoctay I think it should not be after endDate, wouldn't you agree?
There was a problem hiding this comment.
I think that it should not be after endDate.
I think startDate and endDate looks like a plan. The completionDate is the actual completed date.
Actual completed date can be before or after then planned endDate.
If startDate or endDate for timeline is updated, children must validated and changed accordingly