Two types of `checkpoint-review` and `final-designs` milestones

[maxceem]

Overview

Currently, we have two different implementations for milestones types checkpoint-review and final-designs.

• The old style is when we add links inside the milestone, it looks like this:

  

• The new style, when we add links in special add-links milestones which goes before checkpoint-review or final-designs. And inside checkpoint-review and final-designs milestones we only choose the winners, but cannot add links. It looks like this:

  

Issues

1. Currently we support them both just not to ruin some already created projects for development and testing, but actually, support is not robust and there are issues with the old style checkpoint-review and final-designs. If they are following one another like here final-designs comes immediately after checkpoint-review than final-desgins will work as new style milestone and will get links from checkpoint-review instead of asking user to provide the list of links.

   

   It happens because to old style and new style milestones have the same milestone type so we detect old/new style by checking if previous milestone provides the list of marvelapp links https://github.com/appirio-tech/connect-app/blob/dev/src/projects/detail/components/timeline/Milestone/Milestone.jsx#L301
   So this is not the robust solution.

Questions

1. Do we have to support both of these types? So we have to fix the issue above, or we have to remove the support for old style milestones.

cc @RishiRajSahu

Aha! Link: https://topcoder.aha.io/features/TCCONNECT-338

[RishiRajSahu]

@vikasrohit can we update timeline for visual-design product to have add-links milestone before checkpoint-review and final designs ? if this is yes then we don't have to provide any backward support for old milestones as everything is in dev only.

if answer to above is No from vikas, then @maxceem we also need to remove the additional new milestones created for both checkpoint and final-designs which we created to support the data pushed from add-links , as its hard to maintain almost similar component. we can use logic in actual checkpoint-review and final designs milestones to render data for both old and new timelines, so that maintenance is easy for the components.

[vikasrohit]

@maxceem @RishiRajSahu We are definitely okay with not supporting data which are only in dev. However, still I would like checkpoint-review and final-designs to be robust enough to be plugged and play in any mode (with or without add-links before them). And the solution for that is to use some kind of namespace(e.g. type of the milestone) when we are saving the current milestone's data in next milestone's data. So, when add-links would save its data to next milestone as details.prevMilestoneContent.links, it would also store details.prevMilestoneContent.milestoneType as add-links and now when final-designs is going to determine that if it need to show itself in only winner selection mode, it would check for details.prevMilestoneContent.milestoneType === 'add-links' && details.prevMilestoneContent.links && details.prevMilestoneContent.links.length > 0). Basically this flag can be populated _.get(milestone, 'details.content.isInReview', false) OR the condition I just describe above and ideally it should do the trick.

[maxceem]

The main issue fixed with PR #2376

@RishiRajSahu fyi I didn't implemented this part yet, will do that tomorrow

if answer to above is No from vikas, then @maxceem we also need to remove the additional new milestones created for both checkpoint and final-designs which we created to support the data pushed from add-links , as its hard to maintain almost similar component. we can use logic in actual checkpoint-review and final designs milestones to render data for both old and new timelines, so that maintenance is easy for the components.

[maxceem]

@vikasrohit I fixed it simmilar way as you proposed, but instaed of details.prevMilestoneContent.milestoneType I've added details.prevMilestoneType so we keep prevMilestoneContent===content of the prvious milestone.

[vikasrohit]

details.prevMilestoneContent.milestoneType I've added details.prevMilestoneType so we keep prevMilestoneContent===content of the prvious milestone.

Okay, makes sense.

Also, what is Rishi is trying to say is that we should not have two separate components for rendering the review only mode of these two components. We can have single component for each mode. Essentially the check we applying here could go into the specific milestone and milestone itself can update itself to render in review mode only. Last section in my previous comment was saying that:

Basically this flag can be populated _.get(milestone, 'details.content.isInReview', false) OR the condition I just describe above and ideally it should do the trick.

However, I think now we should create a separate ticket for that refactoring as the main issue is fixed.

[vikasrohit]

@maxceem do you have a project to validate this issue? I think now we don't have visual-design product which has this kind of timeline.

[maxceem]

@vikasrohit when we create Visual Design project from the scratch we get visual-design product be default so this will be the old case. After we can add a new phase it will be the new case.

[maxceem]

However, I think now we should create a separate ticket for that refactoring as the main issue is fixed.

Yeah, I've got the idea of Rishi. Agree that we can handle it as a separate issue.

[vikasrohit]

@vikasrohit when we create Visual Design project from the scratch we get visual-design product be default so this will be the old case. After we can a new phase it will be the new case.

Oh yes, I missed that. :)

[vikasrohit]

Tested and verified. Closing.