Skip to content

Issue #127 #144

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Jul 31, 2018
Merged

Issue #127 #144

merged 6 commits into from
Jul 31, 2018

Conversation

architectt1
Copy link
Contributor

@architectt1 architectt1 commented Jul 28, 2018 •
edited
Loading

  • Updating a milestone's completionDate/duration should update the dates of subsequent milestones.

@architectt1
- Updating a milestone's completionDate/duration should update the da…
b9fc2db 
…tes of subsequent milestones.

- Fixed bug where the transaction wasn't being included into wrapped queries.
@architectt1 architectt1 added the bug-bash To collect all tasks for a bug bash label Jul 28, 2018
@architectt1 architectt1 mentioned this pull request Jul 28, 2018
Closed
@vikasrohit
Copy link

@architectt1 I think we don't need to include transaction explicitly. It is automatically handled by continuation-local-storage.
Please remove explicit passing of transaction. I think winner of the challenge, where we got timeline/milestone developed, also added some unit tests to verify that transaction is actually working. Let me know if you are seeing otherwise.
fyi @coderReview

@architectt1
Copy link
Contributor Author

@vikasrohit it seems that if we move away from async/await and stick to promises used by sequelize (and Promise.all), everything starts working again with implicit transactions. Check 0a504f4 for a working implementation without explicitly passing the transaction.

vikasrohit
vikasrohit reviewed Jul 30, 2018
View reviewed changes
Copy link

@vikasrohit vikasrohit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @architectt1 for the investigation and knowledge about async/await.

src/routes/milestones/update.js Outdated
})
.then(() => {
// Update dates of the other milestones only if the completionDate nor the duration changed
if (((!original.completionDate && !updated.completionDate) ||
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@architectt1 would it be more readable and manageable to check if milestone's completion date or duration has changed instead of check there is no change in them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this was an attempt of applying the avoid else, return early principle, but since there's a single line next, I'm going to apply the change you recommend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 61cdff3.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And made it more readable as well :)

@vikasrohit vikasrohit mentioned this pull request Jul 30, 2018
Closed
vikasrohit
vikasrohit suggested changes Jul 30, 2018
View reviewed changes
src/routes/milestones/update.spec.js Outdated
// 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(() => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need timeout call here as it was needed only when there was a bug in the way we were handling the transactions i.e. HTTP response was being sent before transaction commit. I fixed those issues with feature/timeline-milestone branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 737c0e6.

src/routes/milestones/update.spec.js Outdated
// 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(() => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here about need of timeout

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 737c0e6.

src/routes/milestones/update.spec.js Outdated
@@ -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) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming changes for #148 in the same PR, I think now it makes more sense to just not accept the startDate and endDate in payload instead of validating only for different value in these fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 737c0e6.

@architectt1
Merge branch 'feature/timeline-milestone' into issue/127
ccf7136
@architectt1
- Removed setTimeout calls.
737c0e6 
- 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.
@architectt1
Made code more readable.
61cdff3
@architectt1
- When updating a milestone, set updatedBy to updated records.
377b070 
- 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.
@architectt1
Copy link
Contributor Author

#148 changes are present in 377b070.

@vikasrohit vikasrohit merged commit 7d7e38e into feature/timeline-milestone Jul 31, 2018
@vikasrohit
Copy link

Thanks @architectt1 .
What specific change you made for

Fixed bug where the transaction wasn't being included into wrapped queries.

I guess you mean removing async/await and using promises instead, right?

@architectt1
Copy link
Contributor Author

@vikasrohit no problem.

Regarding your question, I forgot to remove that from the description, that was from before when I thought that was needed. I just removed that.

@vikasrohit vikasrohit deleted the issue/127 branch August 29, 2018 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vikasrohit vikasrohit vikasrohit approved these changes

Assignees
No one assigned
Labels
bug-bash To collect all tasks for a bug bash
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@architectt1 @vikasrohit