feat: add notifications to lsg #1532
Conversation
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
jstarpl
added
Contribution from CBC/Radio-Canada
| const merged = new Map<string, DBNotificationObj>() | ||
|
|
||
| // Pull data from the playlist notifications handler's collection | ||
| if (this._playlistNotificationsHandler) { | ||
| const playlistDocs = this._playlistNotificationsHandler.getPublishedDocs() | ||
| for (const d of playlistDocs) { | ||
| if (d._id && !merged.has(unprotectString(d._id))) { | ||
| merged.set(unprotectString(d._id), d) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Pull data from the rundown notifications handler's collection | ||
| if (this._rundownNotificationsHandler) { | ||
| const rundownDocs = this._rundownNotificationsHandler.getPublishedDocs() | ||
| for (const d of rundownDocs) { | ||
| if (d._id && !merged.has(unprotectString(d._id))) { | ||
| merged.set(unprotectString(d._id), d) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const newNotifications = Array.from(merged.values()) |
There was a problem hiding this comment.
I'm curious why this rather complex merging logic is needed. Are id conflicts expected? Is there a particular rule that says the first one should win?
...ve-status-gateway/src/topics/helpers/notification/notificationTarget/toNotificationTarget.ts
Show resolved
Hide resolved
| }) | ||
| } | ||
|
|
||
| function toNotificationTargetType(dbTargetType: DBNotificationTargetType): NotificationTargetType { |
There was a problem hiding this comment.
Nitpick: I am not a fan of this function, it feels wordy when it could be done simpler as part of building the objects inside of toNotificationTargetRundown (and others)
| this._rundownNotificationsHandler = | ||
| handlers.rundownNotificationsHandler as unknown as RundownNotificationsHandler | ||
|
|
||
| if (this._playlistNotificationsHandler) { |
There was a problem hiding this comment.
This may be a pattern that is already in use elsewhere, but is this good behaviour? (In which case, you dont need to answer)
I suspect that if this is ever false, then this handler won't initialise and it will never initialise as there is a bug with the sequence of operations at startup?
In which case it would be good to at least log that this has failed, perhaps even throwing an error would be reasonable?
I am worried that if this does fail, it will do so silently, and leave the consumer confused as to why it doesnt work with no indication in the logs that it did fail
justandras
force-pushed
the
feat/add-notifications-to-lsg
branch
from
793368c to
f6c0dbd
Compare
Co-authored-by: Julian Waller <[email protected]>
justandras
force-pushed
the
feat/add-notifications-to-lsg
branch
from
f6c0dbd to
bc26185
Compare
5 participants
About the Contributor
This pull request is posted on behalf of CBC
Type of Contribution
This is a: Feature
Current Behavior
New Behavior
Testing
Affected areas
corelibmeteor-libthat are still available, because now they will carry over fromcorelibwebuiTime Frame
Status