Notifications Service #696
Conversation
…nz-site into notification-service
There was a problem hiding this comment.
Looking good so far.
My main comments below aim to refactor and consolidate as much code as possible so that we only have to make change in one place if the code is the same or similar enough.
I'll do another round of more thorough review of the SQL bits, since I'm not super well versed in SQL it takes me extra time to make sure I'm not missing anything, although I think there won't be much to change anyway, it looks good already!
src/server/notificationService.ts
Outdated
| ); | ||
| } | ||
| }); | ||
| await Promise.all(notificationPromiseArray); |
There was a problem hiding this comment.
We should probably wrap the entirety of this message handler in a try-catch block
src/server/notificationService.ts
Outdated
| .where('collection_id', collectionId) | ||
| .fetchAll({ | ||
| required: false, | ||
| withRelated: ['collection'] |
There was a problem hiding this comment.
Does this load the same collection on each CollectionSubscription model?
If so it would probably be better to pass the required collection info (I guess only the collection name is missing?) along in the 'send-notifications-for-collection' message instead.
src/server/notificationService.ts
Outdated
| const editor = await new Editor({id: editorId}).fetch(); | ||
| const editorJSON = editor.toJSON(); |
There was a problem hiding this comment.
Same comment here, we have access to req.user.name when we emit the 'send-notifications-for-collection' message, so better to pass it along in the arguments rather than have to re-fetch and serialize the editor just to get the name.
src/server/routes/editor.js
Outdated
| try { | ||
| const editorId = req.params.id; | ||
| const {Notification} = req.app.locals.orm; | ||
| const allNotifications = await new Notification().where('subscriber_id', editorId).fetchAll({requried: false}); |
There was a problem hiding this comment.
| const allNotifications = await new Notification().where('subscriber_id', editorId).fetchAll({requried: false}); | |
| const allNotifications = await new Notification().where('subscriber_id', editorId).fetchAll({required: false}); |
| @@ -224,6 +232,59 @@ class CollectionPage extends React.Component { | |||
| }, this.pagerElementRef.triggerSearch); | |||
| } | |||
|
|
|||
| handleSubscribe() { | |||
There was a problem hiding this comment.
As far as I can tell, all this code (handleSubscribe, handleUnsubscribe, setIsSubscribed and the buttons that go with it) is duplicated for each display page. Instead it would be better if the whole subscription part was a separate component that can then be added to the relevant pages.
You'll probably want to pass the submissionUrl as a prop to make it more flexible.
We might end up with separate CollectionSubscription and an EntitySubscription components if they require very different implementations.
| }); | ||
| } | ||
| function handleSubscribe(bbid) { | ||
| const submissionUrl = '/subscription/subscribe/entity'; |
There was a problem hiding this comment.
I think this URL structure would be more appropriate and "RESTy":
/${entityType}/${bbid}/subscribe and associated /unsubscribe and /subscribed
I guess that means we don't need to send any information with that request anymore: BBID is in the route and the user needs to be logged in to access the route so we have their info in the route handler.
That would also work for collections: /collection/${collectionId}/subscribe…
That means a fair amount of refactoring on the routes, I know, sorry about that :/
I think you'll end up with a middleware or two (one for entities, one for collections) which should be reusable for each entity's routes. In theory, in these routes you should have access to the user information
| handleSubscribe() { | ||
| const submissionUrl = '/subscription/subscribe/collection'; | ||
| const collectionId = this.props.collection.id; | ||
| const subscriberId = this.props.userId; |
There was a problem hiding this comment.
Would this not be accessible in the route handler on the server?
How come you don't need to send the userId for the entity subscribe routes but you do send them for the collections subscribe route?
Problem
Adding a feature to subscribe entities and get notified if and when any changes are made to those entities
Solution
https://community.metabrainz.org/t/gsoc-2021-notification-service-bookbrainz-prabal/526184
Areas of Impact