[$60] Updating a milestone's date/duration should update the dates of subsequent milestones

[vikasrohit]

As of now, when startDate/endDate/completionDate or duration is updated, we are not cascading the effect to upcoming milestones. Purpose of this ticket is to update the startDate and endDate of subsequent milestones when we receive call to update startDate/endDate/completionDate/duration of any milestone.

fyi @RishiRajSahu @coderReview

UPDATE
It should throw error (422), when the milestone's startDate or endDate is updated. We won't allow updating startDate and endDate of milestones.

[architectt1]

My understanding of this is that we need to update the following milestones in order after a given milestone gets either its completion date or duration changed. For example, a milestone gets its completion date set on day 6, then next milestone startDate should be updated to day 7, endDate = (7 + milestone duration - 1), and so on. Please confirm this assumption.

Assuming that's correct, there's an edge case that will break the continuity of milestones (as in, their start date, duration, end date, etc). The current implementation allows a user to update the order of a milestone. Right now that only handles the update of the affected milestones' order, but not the other attributes.

I wonder if this issue (#127) should turn into a different requirement/price?

[vikasrohit]

@architectt1 Yes, your assumption is correct in the first para. Thanks for catching that. I missed to consider the scenario when completionDate is set for a milestone.

Your point is valid for order change of milestone as well. I am not sure if we are going to use the order change feature ever but we kept it for the sake of comprehensiveness. If we can update the dates of the milestones when their order is changed and that does not leave any use case, we should be good.

For the price point, we can keep the last requirement (about updating dates when order is changed) in separate issue to keep the scope of the issue as per original plan.

[architectt1]

@vikasrohit Could we add that to the scope of this issue and increase the price? I ask this because it's hard for me to come up with a half solution that doesn't take the order into account. The scenario where I'm wondering what to do is the following:

Milestone 1, order 1: 1-3, duration 3, completion date: 4.
Milestone 2, order 2: 5-7, duration 3, completion date: 7.
Milestone 3, order 3: 8-11, duration 4, no completion date.

The input, although maybe wrong, we still need to handle it:

PATCH milestone 3, set order to 2, and completion date to 10.

Milestone 1, order 1: 1-3, duration 3, completion date: 4.
Milestone 3, order 2 (previously 3): 8-11, duration 4, completion date: 10.
Milestone 2, order 3 (previously 2): 11-13, duration 3, completion date: (previously 7) ???

Does that make sense? We can update the start date and end date of the milestone being updated and following ones by code, but if a completion date was already set for later milestones, it doesn't feel ok to change that, nor it does if leaving it in an inconsistent state (i.e., leaving it at 7 when the previous milestone was updated to 10).

When writing this I see that it could also apply to a scenario where order didn't change, but I still think that as I said before, it's hard for me to come up with a half solution that doesn't take the order into account.

[coderReview]

@architectt1 I think order can't be changed if completion date is already set, like you said. Leave order update out of the scope of this issue as @vikasrohit has said.

For the sake of this issue, assume order can't be changed.

[vikasrohit]

I can understand. However, I am trying to keep order thing as separate issue or scope because we might want to stop supporting changing order of milestones altogether to avoid inconsistent state. At the very least we can and we should stop changing order when a milestone is marked as complete. User has to mark it non complete (only admin would be able to do that) and then change the order if required. We need to think about that. I think we need to put more validation of business rules e.g. not allowing setting completion date of a milestone when any of its previous milestones is not yet complete. For this issue can we assume that there is no change in order when user is updating dates of the milestone? How about separating order change update as separate endpoint and stop allowing order change in milestone update.

…

On Sat, Jul 28, 2018, 9:30 PM architectt1 ***@***.***> wrote: @vikasrohit <https://github.com/vikasrohit> Could we add that to the scope of this issue and increase the price? I ask this because it's hard for me to come up with a *half solution* that doesn't take the order into account. The scenario where I'm wondering what to do is the following: Milestone 1, order 1: 1-3, duration 3, completion date: 4. Milestone 2, order 2: 5-7, duration 3, completion date: 7. Milestone 3, order 3: 8-11, duration 4, no completion date. The input, although maybe wrong, we still need to handle it: PATCH milestone 3, set order to 2, and completion date to 10. Milestone 1, order 1: 1-3, duration 3, completion date: 4. Milestone 3, order 2 (previously 3): 8-11, duration 4, completion date: 10. Milestone 2, order 3 (previously 2): 11-13, duration 3, completion date: (previously 7) ??? Does that make sense? We can update the start date and end date of the milestone being updated and following ones by code, but if a completion date was already set for later milestones, it doesn't feel ok to change that, nor it doesn't leaving it in an inconsistent state (i.e., leaving it at 7 when the previous milestone was updated to 10). When writing this I see that it could also apply to a scenario where order didn't change, but I still think that as I said before, *it's hard for me to come up with a half solution that doesn't take the order into account*. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#127 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACTj4LC0jWkO1oy1W2AdVCpwYpMOijRSks5uLIq3gaJpZM4Vf-5V> .

[architectt1]

Ok, so indeed there are some more edge cases. For now, I kept myself making changes only about the issue's scope. I also fixed an issue where most queries in that endpoint were being run without including the wrapping transaction. This in tests made updates not work, and basically hang. Maybe you had some issues like this in production? anyway, changes are in #144.

@vikasrohit I'm not sure adding a new endpoint is the best path to take, as in my opinion it wouldn't follow REST principles. I think more logic should be added to this endpoint.

[vikasrohit]

I'm not sure adding a new endpoint is the best path to take, as in my opinion it wouldn't follow REST principles. I think more logic should be added to this endpoint.

Agree. But we might need to restrict that order change endpoint to only admins and may be in future we just want to deprecate it. So, having separate endpoint would be better in that case as we don't have to validate the other endpoints if it is modified/deleted.

For transaction thing, I have commented on your PR. IMO, it should be passing the transaction automatically through usage of continuation-local-storage. If it is not working as expected, we might need to dig into it more. Thanks for the heads up.