Skip to content

Conversation

mfikria
Copy link
Contributor

@mfikria mfikria commented Jun 25, 2019

No description provided.

@maxceem maxceem self-requested a review June 26, 2019 03:56
Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Thanks for quick PR @mfikria

I found some issue in the current implementation. If we create a timeline with defined templateId, it would trigger creating the list of milestones for the template using bulkCreate https://github.com/topcoder-platform/tc-project-service/blob/feature/milestone-pausing/src/routes/timelines/create.js#L61-L109

In the current implementation if I define templateId when creating a timeline it would look like the timeline and milestone is created - as the POST /timelines endpoint would return a successful response. But in reality, it won't as I can see in the DB.

So there are two issues:

  • I guess timeline and milestones are not created because of some issue in hook logic, if such issues happen the request should fail, instead of returning a successful result
  • in out particular case the request should be succesfull, timeline and milestones should be created when providing templateId together with the statuHistory records for milestones.

For testing I'm using the next body for POST /timelines:

{
  "param":{
    "name":"new name",
    "description":"new description",
    "startDate":"2018-05-29T00:00:00.000Z",
    "endDate": "2018-05-30T00:00:00.000Z",
    "reference": "project",
    "referenceId": 1,
    "templateId": 29
  }
}

Make sure that you put to referenceId some existent projects.id. And to templateId some existent milestone_templates.referenceId.

@mfikria
Copy link
Contributor Author

mfikria commented Jun 26, 2019

@maxceem I cannot reproduce the issue since I've got success response and timeline, milestone, and statusHistory are created correctly in the DB. My steps to check the issue:
0. Kafka, rabbitmq, postgres, etc are run

  1. npm i (with node 8.9)
  2. npm run sync:db
  3. npm run sync:es
  4. CONNECT_USER_TOKEN=yourtoken npm run demo-data
  5. Test POST /timelines with postman

Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Thanks for the guidance @mfikria. Cannot reproduce the issues after recreating the DB and reindexing ES. Probably was some data inconsistency.

@maxceem maxceem merged commit dcfe7da into topcoder-platform:feature/milestone-pausing Jun 27, 2019
@maxceem
Copy link
Contributor

maxceem commented Jun 27, 2019

For reference, here was the task:

Issue:

When we use bulkCreate method to create milestones the hook afterCreate is not called which leads to a couple of issues:

  • when milestones are created in a bulk using POST /timelines endpoint, the StatusHistory records are not created. Also, statusHistory records are not populated by the POST /timelines endpoint even if they would be created, as afterCreate hook is not called, thus mapWithStatusHistory is not called either.
  • I saw that you've updated unit tests in your submission and create statusHistory records manually, as in unit tests we also use bulkCreate which doesn't trigger afterCreate hook

Task:

  1. fix model StatusHistory: referenceId should be BIGINT instead of STRING. Note, that migration script already uses bigint, so only the model should be fixed and any place in the code which uses string for referenceId instead of an integer like this one https://github.com/topcoder-platform/tc-project-service/blob/feature/milestone-pausing/src/models/milestone.js#L15. Update unit tests and swagger if they use string instead of bigint.
  2. add afterBulkCreate hook which would create statusHistory records. Preferable, create statusHistory records in a bulk in this method if possible. Also, populate status history in this hook.
  3. Improve method mapWithStatusHistory to support array of milestones. So instead of querying DB several times for each milestone, we query DB once, find statusHistory records for all milestones, and after populate each milestone with statusHistory records which belong to that milestone. This would be useful inside afterBulkCreate hook and also inside afterFind hook in case when the "milestone" is an array.
  4. Update unit tests: don't create StatusHisotry records manually as you've implemented in your last submission. After the fixes above they should be created automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants