Add paused status for the milestones

[vikasrohit]

Continuing from appirio-tech/connect-app#2968

This issue is for handling the backend changes required for the new status. We would need to keep history of the milestone status changes so that we can enable caller of the API to calculate the time spent in the each of the state of the milestone and move the status back to previous status when user wants to resume the milestone.

fyi @maxceem @RishiRajSahu

[maxceem]

@vikasrohit there is a couple of ways to go, would be great to have your view on them.

Does it make sense to make this new model universal so it could be reused by any other model not only the milestones in the future?

1. So we can refer to the rows in this model by the milestoneId. In this case we can use it only for milestones. And if later we want to have the same functionality for other model we would need to create another model for that.

   We can call model MilestonePause

   - id: Number, primary, autoincerement
   - milestoneId: Number (required)
   - startTime: Datatime (required)
   - endTime: Datetime
   - comment: Text (required)
   - createdBy
   - createdAt (will duplicate startTime)
   - updatedBy
   - updatedAt (will duplicate endTime)      
   

2. Alternatively, the tracking model can refer other entities by two fields: reference and referenceId. So for the milestones reference would be always milestones and referenceId would be equal to the particular milestoneId. In such case we could reuse our tracking model for any other model by defining new reference values. As a disadvantage, theoretically, this tracking table could grow big if tracks pauses for too many models. Also, if we are not going to use it for any other models, this is not a necessary abstraction.

   We can call model Pause

   - id: Number, primary, autoincerement
   - reference: String (required)
   - referenceId: String (required)
   - startTime: Datatime (required)
   - endTime: Datetime
   - comment: Text (required)
   - createdBy
   - createdAt (will duplicate startTime)
   - updatedBy 
   - updatedAt (will duplicate endTime)
   

3. I guess we should make restictions for this model.

   1. We can only define startTime when we create the records, and we cannot update startTime.
   2. We can only define endTime once. If it's already defined we cannot update it.
   3. We can update the comment though.

4. If we make the restrictions above for all the users, then createdAt will always duplicate startTime. And updatedAt will always duplicate endTime. Unless admins can make changes to this model bypassing restrictions above.

[vikasrohit]

Thanks @maxceem for the updates and options. I would definitely like to make it bit generic if not fully generic. By that I mean is that we should think only about pause status for milestone (or may be a generic entity), rather we should keep history of all status changes of the target entity, some thing we are doing for project status changes already. So, my proposed schema would be:

- id: Number, primary, autoincerement
- milestoneId: String (required)
- status: String (required)
- comment: Text (required)
- createdBy
- createdAt (will duplicate startTime)
- updatedBy 
- updatedAt (will duplicate endTime)

Now, for every change in the status of the target entity, we would make an entry in this table. So, when a milestone is paused, it would have one entry with timestamp of pause as createdAt and later when user resumes the milestone, we would retrieve the record from this table which was previous (by order of createdAt) to the current paused status record and use the status from that record to move the milestone out of paused status. And it would cause one more entry in this table with the status retrieved. Now, when we need to calculate the pause time for the milestone, we can take difference of the createdAt field for paused status record and its next(again by order of createdAt) record.
Also, to make this table generic, we can keep reference and referenceId instead of milestoneId, if we agree on that.

[maxceem]

Got the idea, thanks @vikasrohit. Just one thing to confirm. In the next comment, you are saying, that if we have milestone in pause status, we have one entry in the status table - it's clear.
After you saying:

when user resumes the milestone, we would retrieve the record from this table which was previous (by order of createdAt) to the current paused status record and use the status from that record to move the milestone out of paused status.

Do I understand that when a user updates the status of the milestone, we should retrieve the latest status as you say, just to check if a new status is different from the previous one. And if a new status is different, then we are creating a new record in the status table. If the new status is the same like before, it means status is not changed, so we shouldn't create a new status record.

[vikasrohit]

Do I understand that when a user updates the status of the milestone, we should retrieve the latest status as you say, just to check if a new status is different from the previous one. And if a new status is different, then we are creating a new record in the status table. If the new status is the same like before, it means status is not changed, so we shouldn't create a new status record.

Nopes, that is not the idea. I think we can easily achieve this from the front end only by not allowing user to change the status to the same status again, right?

Let me give some practical example to make it more understandable. Lets say, we have a milestone with id 123 created, so there would be following entry in the new proposed table:

1, 123, draft, , vikas, 2019-04-26T15:07:10, vikas, 2019-04-26T15:07:10

Now, lets say the milestone is moved to the reviewed status, now entries would be:

1, 123, draft, , vikas, 2019-04-26T15:07:10, vikas, 2019-04-26T15:07:10
2, 123, reviewed, , vikas, 2019-04-26T15:08:30, vikas, 2019-04-26T15:08:30

Now, assume that it is moved to paused status:

1, 123, draft, , vikas, 2019-04-26T15:07:10, vikas, 2019-04-26T15:07:10
2, 123, reviewed, , vikas, 2019-04-26T15:08:30, vikas, 2019-04-26T15:08:30
3, 123, paused, , vikas, 2019-04-26T15:10:30, vikas, 2019-04-26T15:10:30

Now, when user tries to resume the milestone, we would check from the entries above that it was in reviewed status before moving to the paused status, so we would move it to reviewed status when user performs a resume operation and now entries would be:

1, 123, draft, , vikas, 2019-04-26T15:07:10, vikas, 2019-04-26T15:07:10
2, 123, reviewed, , vikas, 2019-04-26T15:08:30, vikas, 2019-04-26T15:08:30
3, 123, paused, , vikas, 2019-04-26T15:10:30, vikas, 2019-04-26T15:10:30
4, 123, reviewed, , vikas, 2019-04-26T15:16:30, vikas, 2019-04-26T15:16:30

And total time the milestone was in paused state is the difference between createdAt of the last two entries i.e. 2019-04-26T15:16:30 - 2019-04-26T15:10:30 which is 6 minutes.

[maxceem]

Thanks, @vikasrohit. Got your idea.

1. As I understand, every time we change the status of milestones, we should "log" this event in the "status_history" table as you described above.
   We change milestone statuses in several ways:

   • using create/update milestone endpoints
   • when we create/update timelines, we make a cascade update of milestones of the timeline

   I think the best way to handle it would be using sequalize hooks and every time when we detect changing of the milestone status, we should log this into statusHistory.

2. We should create additional endpoints for milestones:

   • PATCH /v4/timelines/:timelineId(\\d+)/milestones/:milestoneId(\\d+)/status/pause - which would move milestone and phase into the paused status
   • PATCH /v4/timelines/:timelineId(\\d+)/milestones/:milestoneId(\\d+)/status/resume - which would move milestone and phase into the previous status

[vikasrohit]

1. I am okay with sequlize hooks if they don't pose any performance issue. I checked the docs, and it says it loads all the affected records in memory to perform the hook operation. It does not seem bad to me though as we are not going to have batch operation to update the status, but still it might need some monitoring after we implement it.

2. Agreed.

[maxceem]

Thanks @vikasrohit

It looks like we also should add some read-only virtual column actualDuration to the milestones model. Which wouldn't be stored in the DB and would be calculated as you described above. This property should be stored in the ElasticSearch index though.

[maxceem]

@vikasrohit note, if we add a new virtual property actualDuration to milestones and store it in ES index, we would have to reindex ES or create some fallback code in Connect app, to support the case where we didn't have actualDuration yet. As for such cases actualDuration === duration.

[vikasrohit]

I think it would be good to have that virtual column and have it indexed in ES. Only question I have is that, how much would it cost if we don't have such field and calculate the actualDuration based on the actual data. I know we have to pass the status_history records for each milestone in our responses to milestone endpoints and I think that is the only possible thing to slow down the response, right?

create some fallback code in Connect app

I would like to go with this rather than reindexing because we would always have actualDuration === duration for all milestones before the time we roll out the pause feature to the production and I would like to avoid reindexing for them. Also, logically, we can say that for the milestones where we don't have the actualDuration field, duration is the actualDuration. One more way just came writing the last line, we can add this logic of defaulting to duration, to the project service as well. Whenever we don't get any record of the milestones from status history table, we can send actualDuration field with value of duration field. Please check if it does not cause any performance cost otherwise go with the front end changes as you suggested.

[maxceem]

I know we have to pass the status_history records for each milestone in our responses to milestone endpoints and I think that is the only possible thing to slow down the response, right?

Right, only this.

One more way just came writing the last line, we can add this logic of defaulting to duration, to the project service as well.

Good idea. I don't see performance issues if we do it server-side.

[maxceem]

@vikasrohit the last thing. In this comment you've mentioned that we should calculate the actual time since milestone comes to the reviewed status. I think we should only calculate actual duration after milestone comes to the active status. Because reviewed status means that milestone is planned, but didn't start yet. And since we put milestone to active status, we cannot put to any other status except of paused. Let me know if my understanding is correct here.

[maxceem]

One more way just came writing the last line, we can add this logic of defaulting to duration, to the project service as well.

Regarding this, I think it would be better to keep it client-side. If in the future if we reindex ES in project service, this code in project service wouldn't be used anymore. And we would forget to delete it :-). While in client side we already have a lot of code to support several backend versions, so it's ok to have such code there. Also, most likely we can make this fix fallback code in one place client side, while in project service we would need to patch several endpoints (at least 2).

[vikasrohit]

I was trying to be as much generic as we can be with ability to pause the milestone in any state. However, I think it would create confusion for now so, lets allow paused status only from active status and hence calculation of actualDuration would also be correct as per the understanding you shared above.
I am okay with client side as well. Go with it.

[maxceem]

@vikasrohit I've got one question regarding sending BUS events.

When we pause/resume milestones we should send connect.action.timeline.milestone.transition.paused/connect.action.timeline.milestone.transition.active events. Should we also send connect.action.timeline.milestone.updated in such case?

When we update milestones we send both events, if we change status.

[vikasrohit]

Yes. connect.action.timeline.milestone.updated is a generic event which is sent after every update in a milestone while the other two are derived/specialized events of the the raw/generic event. This is to facilitate easy consumption of such specialized events while keeping a common event which is always raised for a consumer who wants to consume every milestone update.