feat: Add notification support #1381
Conversation
…r into notification-support
|
/release-pr |
|
|
|
/release-pr |
|
Released to |
|
The "About to be Handled" notification type is still a work in progress. I’m still getting the cron job fully set up. Once it’s ready, I’ll update this PR. |
…r into notification-support
|
/release-pr |
|
Released to |
|
Todo:
|
|
I'll try and get to reviewing this sometime this week. |
|
TODO: There might be a logic issue in how notifications are being sent. In certain parts of the code, I call a method to send a notification to all agents active for a specific notification type. However, it might be possible that the notification is also being sent out for rules that don't have that notification agent configured. This is not tested though, it's just an afterthought I just had. |
|
/release-pr |
|
Released to |
There was a problem hiding this comment.
I just had a quick pass through of the code, nice work! I'll try and make some time to test a few of the agents soon as I use a few of them myself.
Most of the comments are around missing awaits. I believe this might have been on purpose, so that you can fire off notifications without slowing down the workers? You also don't really want to explode a worker if the notification code fails for whatever reason as it's not that important. If that's the case I'd suggest using a queue: https://docs.nestjs.com/techniques/queues. Or attaching . catch() to them so unhandledrejections aren't thrown.
handleNotification could simply become await this.notificationQueue.add(), and the existing handling code moved to a different function to be called by a consumer? Though the agent is being passed in a few places so might need a little refactor, or you don't queue those? You could also have another queue that takes a constructed message and the agent to send to, that would mean you can retry individual notifications if the service is having an issue.
- Add item to queue with the required data (type, mediaItems, collectionName, dayAmount)
- Consumer takes from that queue, runs transformMessageContent and adds to another queue, one queue message per agent.
- Consumer takes from that queue and calls agent.send().
EDIT:
I've just realized this will require redis which might be a pain in the ass for some users (seedbox etc). We could do something similar with an in-process task runner though, or possibly events: https://docs.nestjs.com/techniques/events. I need to experiment with nestjs error handling, from what I've read they've got a global handler for exceptions but I've seen it crash from unhandledrejections.
|
|
||
| // wait 5 seconds to make sure we're not executing together with the rule handler | ||
| await new Promise((resolve) => setTimeout(resolve, 5000)); | ||
| this.notificationService.registerConfiguredAgents(true); // re-register notification agents, to avoid flukes |
There was a problem hiding this comment.
| this.notificationService.registerConfiguredAgents(true); // re-register notification agents, to avoid flukes | |
| await this.notificationService.registerConfiguredAgents(true); // re-register notification agents, to avoid flukes |
| for (const media of collectionMedia) { | ||
| // handle media addate <= due date | ||
| if (new Date(media.addDate) <= dangerDate) { | ||
| await this.handleMedia(collection, media); |
There was a problem hiding this comment.
I brought up in Discord here about a possible handleMedia improvement. We may want to notify on individual media handling failures at some point once we've figured out what to do there.
| }, 7000); | ||
| // handle notification | ||
| if (handledMediaForNotification.length > 0) { | ||
| this.notificationService.handleNotification( |
There was a problem hiding this comment.
| this.notificationService.handleNotification( | |
| await this.notificationService.handleNotification( |
| ); | ||
|
|
||
| // notify | ||
| this.notificationService.handleNotification( |
There was a problem hiding this comment.
| this.notificationService.handleNotification( | |
| await this.notificationService.handleNotification( |
| this.logger.debug(e); | ||
|
|
||
| // notify | ||
| this.notificationService.handleNotification( |
There was a problem hiding this comment.
| this.notificationService.handleNotification( | |
| await this.notificationService.handleNotification( |
| @@ -118,6 +122,10 @@ export class RuleExecutorService extends TaskBase { | |||
| this.logger.log( | |||
| 'Not all applications are reachable.. Skipped rule execution.', | |||
| ); | |||
| this.notificationService.handleNotification( | |||
There was a problem hiding this comment.
| this.notificationService.handleNotification( | |
| await this.notificationService.handleNotification( |
| @@ -264,6 +272,12 @@ export class RuleExecutorService extends TaskBase { | |||
| : collection.title | |||
| }'.`, | |||
| ); | |||
|
|
|||
| this.notificationService.handleNotification( | |||
There was a problem hiding this comment.
| this.notificationService.handleNotification( | |
| await this.notificationService.handleNotification( |
| @@ -274,6 +288,13 @@ export class RuleExecutorService extends TaskBase { | |||
| : collection.title | |||
| }'.`, | |||
| ); | |||
|
|
|||
| this.notificationService.handleNotification( | |||
There was a problem hiding this comment.
| this.notificationService.handleNotification( | |
| await this.notificationService.handleNotification( |
| @@ -295,9 +316,17 @@ export class RuleExecutorService extends TaskBase { | |||
| return collection; | |||
| } else { | |||
| this.logInfo(`collection not found with id ${rulegroup.collectionId}`); | |||
| this.notificationService.handleNotification( | |||
There was a problem hiding this comment.
| this.notificationService.handleNotification( | |
| await this.notificationService.handleNotification( |
| } | ||
| } catch (err) { | ||
| this.logger.warn(`Execption occurred whild handling rule: `, err); | ||
| this.logger.error(`Execption occurred while handling rule: `, err); | ||
| this.notificationService.handleNotification( |
There was a problem hiding this comment.
| this.notificationService.handleNotification( | |
| await this.notificationService.handleNotification( |
|
|
||
| <div className="form-row"> | ||
| <label htmlFor="about-scale" className="text-label"> | ||
| Days for About To Be Handled |
There was a problem hiding this comment.
I was a bit confused at first when I saw this as I hadn't read the types. Perhaps move this after Types and only make it appear when selecting Media About To Be Handled?
There was a problem hiding this comment.
I spent too much time considering how to name this and eventually settled on this option, though I wasn't entirely satisfied with it.
You're right that showing this only when the type is selected is a better option. I'll try to disable the option when that type is unchecked.
| }} | ||
| className="rounded-l-only" | ||
| > | ||
| {availableAgents?.map((agent, index) => ( |
There was a problem hiding this comment.
Does the 'none' option need filtering out here, or just be made into an empty string? Having a mapping from the enum value to a friendly type would be nice too. discord -> Discord etc.
There was a problem hiding this comment.
I was planning to remove the 'none' option entirely since it's not being used anywhere.
Adding a friendly name mapping sounds like a great idea.
ui/src/components/Settings/Notifications/CreateNotificationModal/index.tsx
Show resolved
Hide resolved
|
|
||
| class DiscordAgent implements NotificationAgent { | ||
| constructor( | ||
| private readonly settings: NotificationAgentConfig, |
There was a problem hiding this comment.
| private readonly settings: NotificationAgentConfig, | |
| private readonly settings: NotificationAgentDiscord, |
You can use the specific config types in all of the agents to get type checking on the options.
| </div> | ||
| <div className="m-auto mb-5 flex"> | ||
| <div className="ml-2 mr-auto sm:mr-0"> | ||
| <ExecuteButton |
There was a problem hiding this comment.
The first time around I found the test notification thing confusing. I added a notification, smashed Test Notifications a bunch of times and went on a entrypoint spree because nothing was happening. Only when I went back to the modal did I see the 'Test Notification' type and then it clicked in my head. Also the flow of edit notification -> save -> close -> test -> repeat seems awkward.
I think a friendlier version would be having the Test button within the notification modal that just uses the Test Notification type in the background. Looking at the code it might just be a case of passing the non-saved settings up, create a new agent (maybe can extract the factory logic from registerConfiguredAgents) and calling sendNotificationToAgent?
There was a problem hiding this comment.
There's also no feedback after testing currently, and for Gotify (what I've tested so far) the error & debug don't really help much (I'll add a separate comment for that).
Exceptions in the agents send() could bubble up (or throw a new one) with the actual exception message, that could be returned to the user when they hit the /test endpoint so they don't have to dig through logs.
There was a problem hiding this comment.
Good point, I hadn't really considered that. Looking at it now, it does seem odd. I took the easiest route at the time.
Moving the test button to the creation modal seems like the best approach, and we could also remove the 'test' type in that case.
As for the error, I'll try to return the exception, or at least a part of it to the user.
| const settings = this.getSettings(); | ||
|
|
||
| if ( | ||
| !payload.notifySystem || |
There was a problem hiding this comment.
What's notifySystem used for? From what I can see it's always set to false so this and a few other agents will always return early.
There was a problem hiding this comment.
That's a leftover i still need to remove. I copied the notification agents from Overseerr and adapted them to something Nestjs understands. I believe they use it for notifications regarding the Overseerr application itself (like errors).
| return true; | ||
| } catch (e) { | ||
| this.logger.error('Error sending Gotify notification'); | ||
| this.logger.debug(e); |
There was a problem hiding this comment.
The URL could be included here, e.g.:
const url = axios.getUri({ url: endpoint });
this.logger.debug(`GET request to ${url} failed: ${e}`);
|
|
||
| <p className="text-gray-300 space-x-2 mb-4 truncate"> | ||
| <span className="font-semibold">Agent</span> | ||
| <a href={config.agent} className="hover:underline"> |
There was a problem hiding this comment.
I think having a link here was a leftover from where it was copied.
There was a problem hiding this comment.
Ah, yes, I mindlessly borrowed it from your multi-arr page. I should have caught that.
| @@ -383,6 +386,7 @@ export class RulesService { | |||
| params.isActive !== undefined ? params.isActive : true, | |||
| params.dataType !== undefined ? params.dataType : undefined, | |||
| group.id, | |||
| params.notifications, | |||
There was a problem hiding this comment.
This also needs adding in setRules when calling createOrUpdateGroup, I can't save new collections at the moment due to the error below, but it also means it won't save any notifications I've selected as well.
The notifications param on createOrUpdateGroup is optional but .map() is called on it later. This is throwing causing groupId to be null in setRules.
|
|
||
| <div> | ||
| {/* Load fields */} | ||
| {targetAgent?.options.map((option) => { |
There was a problem hiding this comment.
I think we should have a different input for the Json Payload option. The monaco-editor would be perfect here.
| notifySystem: boolean; | ||
| image?: string; | ||
| message?: string; | ||
| extra?: { name: string; value: string }[]; |
There was a problem hiding this comment.
I think this is a leftover from overseerr as I can't see it being used.
There was a problem hiding this comment.
Indeed, also something that still needs cleaning up
| export interface NotificationPayload { | ||
| event?: string; | ||
| subject: string; | ||
| notifySystem: boolean; |
There was a problem hiding this comment.
Same with this and the leftover conditions
| NotificationType, | ||
| } from '../notifications-interfaces'; | ||
|
|
||
| export interface NotificationPayload { |
There was a problem hiding this comment.
It would be nice to include some metadata in this payload where relevant, such as the collection name, and/or the media item name. The webhook agent only has access to the subject and message which isn't super useful.
There was a problem hiding this comment.
That's worth an exploration, i'll look into it.
| const settings = this.getSettings(); | ||
|
|
||
| if ( | ||
| !payload.notifySystem || |
There was a problem hiding this comment.
| !payload.notifySystem || |
| return true; | ||
| } catch (e) { | ||
| this.logger.error('Error sending webhook notification'); | ||
| this.logger.debug(e); |
There was a problem hiding this comment.
Same as comment before around including the URL in the message.
Co-authored-by: Ben Scobie <[email protected]>
This PR introduces notification support to Maintainerr. You can now add notification providers via the settings. When creating a rule, you’ll be able to configure which notification provider to use for that rule.
Please note: I’ve only tested this with the Discord and Pushover notification providers. The others have not yet been tested.