-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat(cb2-12694): Implement types-definition into scheduled-ops service #78
Conversation
@@ -46,7 +48,7 @@ export class CleanupService { | |||
if (moment(new Date(visit.startTime)).isAfter(recentActivityWindowTime)) { | |||
// Visit open less than 3 hours, no action needed | |||
this.addToLogVisits( | |||
visit.id, | |||
visit.id!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the ! required here, won't there always be a visit id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We know at this point it will always be populated, as it has come from the table, however id is optional in the new schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that the schema is correct in that case. FAIK the activity id is mandatory after they've been created as it gets automatically generated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we use the activity schema before the ID is generated, meaning it would have to be optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly, that was my understanding too.
I know it was previously optional, and the moving of the interfaces was like-to-like from the owner-repo to the definitions repo.
Maybe there is a bit of work required to re-look at what was previously there across the interfaces. If I make that mandatory now, I don't know what the impact on anything that currently doesn't provide it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there might be an argument for using something else until its generated, then changing the schema so its required (assuming its always required once generated). Theres quite a lot of validation before its generated though.
Implement types-definition into scheduled-ops service
scheduled-ops service does not currently use the type-definitions package and instead creates its own object for test-results and activities
CB2-12694
Checklist