Skip to content

improve milestone hooks and change referenceId in milestone to BIGINT #321

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.

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

Merged
merged 1 commit into from
Jun 27, 2019
Merged

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