Skip to content
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

[Integration] Discord integration #244

Merged
merged 17 commits into from
Jun 30, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,17 @@ namespace Notifo.Domain.Integrations;

public sealed class MessagingMessage : BaseMessage
{
public IReadOnlyDictionary<string, string> Targets { get; set; }

public string Text { get; set; }
public string? DetailedBodyText { get; init; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an extra line here. Why are you not just call it body btw: And Why do you need it? The text gives you the opportunity to actually control the formatting. Is it not superior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thank you!
Text is like subject, body is like content.
The Discord Embed messages allow you to set the title and then the body. If user doesn't provide the body, only the title is set.


public IReadOnlyDictionary<string, string> Targets { get; set; }

public string? ImageLarge { get; init; }

public string? ImageSmall { get; init; }

public string? LinkText { get; init; }

public string? LinkUrl { get; init; }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// =====================================================
// Notifo.io
// ==========================================================================
// Copyright (c) Sebastian Stehle
// Author of the file: Artur Nowak
// All rights reserved. Licensed under the MIT license.
// ==========================================================================

using Discord;
using Microsoft.Extensions.Caching.Memory;

namespace Notifo.Domain.Integrations.Discord;
public class DiscordBotClientPool : CachePool<IDiscordClient>
SebastianStehle marked this conversation as resolved.
Show resolved Hide resolved
{
public DiscordBotClientPool(IMemoryCache memoryCache)
: base(memoryCache)
{
}

public IDiscordClient GetDiscordClient(string botToken, CancellationToken ct)
{
var cacheKey = $"{nameof(IDiscordClient)}_{botToken}";

var found = GetOrCreate(cacheKey, TimeSpan.FromMinutes(5), () =>
{
var client = new DiscordClient();

client.LoginAsync(TokenType.Bot, botToken).Wait(ct);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never, ever use Wait(). We can make an async version of the GetOrCreate method.

All methods of the client accept a RequestOption with cancellationToken. We should always forward that if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thank you so much!

Sadly the LoginAsync method from DiscordRestClient (or rather BaseDiscordClient) from Discord.NET is a wrapper over the internal ApiClient.LoginAsync method (this one accepts the RequestOptions parameter, which has a CancellationToken property). But the base function calls it with the default RequestOptions parameter.

Therefore, the problem is that the LoginAsync function doesn't expose the RequestOptions parameter (while other operations like e.g. SendMessage do expose it).

Technically I could write some kind of a workaround (e.g. WaitAsync), but even if the cancellation token fires and WaitAsync completes, the LoginAsync Task will still get executed on that another thread. On the other hand, interrupting the Thread with it seems like overengineering and can lead to some integrity issues.

What do you think? Maybe I should just forget about the cancellation token in that specific method?


return client;
});

return found;
}

}
19 changes: 19 additions & 0 deletions backend/src/Notifo.Domain.Integrations/Discord/DiscordClient.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// =====================================================
// Notifo.io
// ==========================================================================
// Copyright (c) Sebastian Stehle
// Author of the file: Artur Nowak
// All rights reserved. Licensed under the MIT license.
// ==========================================================================

using Discord.Rest;

namespace Notifo.Domain.Integrations.Discord;
public class DiscordClient : DiscordRestClient, IAsyncDisposable
SebastianStehle marked this conversation as resolved.
Show resolved Hide resolved
{
public async new ValueTask DisposeAsync()
{
await LogoutAsync();
await base.DisposeAsync();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
// =====================================================
// Notifo.io
// ==========================================================================
// Copyright (c) Sebastian Stehle
// Author of the file: Artur Nowak
// All rights reserved. Licensed under the MIT license.
// ==========================================================================

using Discord;
using Discord.Net;

namespace Notifo.Domain.Integrations.Discord;
public sealed partial class DiscordIntegration : IMessagingSender
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a new line after namespace declaration

{
private const int Attempts = 5;
public const string DiscordChatId = nameof(DiscordChatId);

public void AddTargets(IDictionary<string, string> targets, UserInfo user)
{
var userId = GetUserId(user);

if (!string.IsNullOrWhiteSpace(userId))
{
targets[DiscordChatId] = userId;
}
}

public async Task<DeliveryResult> SendAsync(IntegrationContext context, MessagingMessage message,
CancellationToken ct)
Copy link
Collaborator

@SebastianStehle SebastianStehle Jun 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please intend CancellationToken. See next method.

{
if (!message.Targets.TryGetValue(DiscordChatId, out var chatId))
{
return DeliveryResult.Skipped();
}

return await SendMessageAsync(context, message, chatId, ct);
}

private async Task<DeliveryResult> SendMessageAsync(IntegrationContext context, MessagingMessage message, string chatId,
CancellationToken ct)
{
var botToken = BotToken.GetString(context.Properties);

for (var i = 1; i <= Attempts; i++)
{
try
{
var client = discordBotClientPool.GetDiscordClient(botToken, ct);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either the client should be disposed in the case of an error or we should put it outside of the loop, which makes probably more sense.


var user = await client.GetUserAsync(ulong.Parse(chatId));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forward cancellation token.

if (user is null)
{
throw new InvalidOperationException("User not found.");
}

EmbedBuilder builder = new EmbedBuilder();

builder.WithTitle(message.Text);
builder.WithDescription(message.DetailedBodyText);

if (!string.IsNullOrWhiteSpace(message.ImageSmall))
SebastianStehle marked this conversation as resolved.
Show resolved Hide resolved
{
builder.WithThumbnailUrl(message.ImageSmall);
}

if (!string.IsNullOrWhiteSpace(message.ImageLarge))
{
builder.WithImageUrl(message.ImageLarge);
}

if (!string.IsNullOrWhiteSpace(message.LinkUrl))
{
builder.WithFields(new EmbedFieldBuilder().WithName(message.LinkText ?? message.LinkUrl).WithValue(message.LinkUrl));
}

builder.WithFooter("Sent with Notifo");

await user.SendMessageAsync(string.Empty, false, builder.Build()); // Throws HttpException if the user has some privacy settings that make it impossible to text them.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually put the comment above the line to keep the column width short.

break;
}
catch (HttpException ex) when (ex.DiscordCode == DiscordErrorCode.CannotSendMessageToUser)
SebastianStehle marked this conversation as resolved.
Show resolved Hide resolved
{
return DeliveryResult.Failed("User has privacy settings that prevent sending them DMs on Discord.");
}
catch
{
if (i == Attempts)
{
return DeliveryResult.Failed("Unknown error when sending Discord DM to user.");
}
}
}

return DeliveryResult.Handled;
}

private static string? GetUserId(UserInfo user)
{
return UserId.GetString(user.Properties);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// =====================================================
// Notifo.io
// ==========================================================================
// Copyright (c) Sebastian Stehle
// Author of the file: Artur Nowak
// All rights reserved. Licensed under the MIT license.
// ==========================================================================

using Discord;
using Notifo.Domain.Integrations.Resources;

namespace Notifo.Domain.Integrations.Discord;
public sealed partial class DiscordIntegration : IIntegration
SebastianStehle marked this conversation as resolved.
Show resolved Hide resolved
{
private readonly DiscordBotClientPool discordBotClientPool;

public static readonly IntegrationProperty UserId = new IntegrationProperty("discordUserId", PropertyType.Text)
{
EditorLabel = Texts.Discord_UserIdLabel,
EditorDescription = Texts.Discord_UserIdDescription
};

public static readonly IntegrationProperty BotToken = new IntegrationProperty("discordBotToken", PropertyType.Text)
{
EditorLabel = Texts.Discord_BotTokenLabel,
EditorDescription = Texts.Discord_BotTokenDescription,
IsRequired = true
};

public IntegrationDefinition Definition { get; } =
new IntegrationDefinition(
"Discord",
Texts.Discord_Name,
"<svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 127.14 96.36\"><path fill=\"#5865f2\" d=\"M107.7,8.07A105.15,105.15,0,0,0,81.47,0a72.06,72.06,0,0,0-3.36,6.83A97.68,97.68,0,0,0,49,6.83,72.37,72.37,0,0,0,45.64,0,105.89,105.89,0,0,0,19.39,8.09C2.79,32.65-1.71,56.6.54,80.21h0A105.73,105.73,0,0,0,32.71,96.36,77.7,77.7,0,0,0,39.6,85.25a68.42,68.42,0,0,1-10.85-5.18c.91-.66,1.8-1.34,2.66-2a75.57,75.57,0,0,0,64.32,0c.87.71,1.76,1.39,2.66,2a68.68,68.68,0,0,1-10.87,5.19,77,77,0,0,0,6.89,11.1A105.25,105.25,0,0,0,126.6,80.22h0C129.24,52.84,122.09,29.11,107.7,8.07ZM42.45,65.69C36.18,65.69,31,60,31,53s5-12.74,11.43-12.74S54,46,53.89,53,48.84,65.69,42.45,65.69Zm42.24,0C78.41,65.69,73.25,60,73.25,53s5-12.74,11.44-12.74S96.23,46,96.12,53,91.08,65.69,84.69,65.69Z\"/></svg>",
new List<IntegrationProperty>
{
BotToken
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting is broken here. Should be 4 spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I'm so sorry for all of those formatting issues, my Visual Studio is messed up so much recently and I didn't even notice the indentation changes that happened in some of the files.

},
new List<IntegrationProperty>
{
UserId
},
new HashSet<string>
{
Providers.Messaging,
})
{
Description = Texts.Discord_Description
};

public DiscordIntegration(DiscordBotClientPool discordBotClientPool)
{
this.discordBotClientPool = discordBotClientPool;
}

public async Task<IntegrationStatus> OnConfiguredAsync(IntegrationContext context, IntegrationConfiguration? previous,
CancellationToken ct)
{
// Validate if the token is valid.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the comment for? It is weird!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should explain they why and not the what and should also not explain other methods from the same code base.

var botToken = BotToken.GetString(context.Properties);
SebastianStehle marked this conversation as resolved.
Show resolved Hide resolved
if (botToken == null)
{
return IntegrationStatus.VerificationFailed;
}

try
{
TokenUtils.ValidateToken(TokenType.Bot, botToken);
}
catch
{
return IntegrationStatus.VerificationFailed;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to throw a ValidationException here (ensure that we use our own exception, not from System.DataAnnotations)

}

return IntegrationStatus.Verified;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// =====================================================
// Notifo.io
// ==========================================================================
// Copyright (c) Sebastian Stehle
// Author of the file: Artur Nowak
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the header is fixed and it should give a compiler warning if it is not the correct one. You can add yourself as contributor to the readme and we can also modify the header, but we cannot change it on a per-file basis.

// All rights reserved. Licensed under the MIT license.
// ==========================================================================

using Microsoft.Extensions.DependencyInjection;

namespace Notifo.Domain.Integrations.Discord;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the namespace of other ServiceExtensions classes, I usually put them into "Microsoft.Extensions.DependencyInjection". it is a best practice to make it more discoverable and to void dozens and hundreds of usings in the Startup file.

public static class DiscordServiceExtensions
SebastianStehle marked this conversation as resolved.
Show resolved Hide resolved
{
public static IServiceCollection AddIntegrationDiscord(this IServiceCollection services)
{
services.AddSingletonAs<DiscordIntegration>()
.As<IIntegration>();

services.AddSingletonAs<DiscordBotClientPool>()
.AsSelf();

return services;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
<ItemGroup>
<PackageReference Include="AWSSDK.SimpleEmail" Version="3.7.300.60" />
<PackageReference Include="Confluent.Kafka" Version="2.3.0" />
<PackageReference Include="Discord.Net" Version="3.15.2" />
<PackageReference Include="FirebaseAdmin" Version="2.4.0" />
<PackageReference Include="FluentValidation" Version="11.9.0" />
<PackageReference Include="Fluid.Core" Version="2.7.0" />
Expand Down
54 changes: 54 additions & 0 deletions backend/src/Notifo.Domain.Integrations/Resources/Texts.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 18 additions & 0 deletions backend/src/Notifo.Domain.Integrations/Resources/Texts.resx
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,24 @@
<data name="AmazonSES_ReservedEmailAddress" xml:space="preserve">
<value>Email address {0} is already used by another app.</value>
</data>
<data name="Discord_BotTokenDescription" xml:space="preserve">
<value>You will find it on your Discord Developer Platform. Go to your app and then to the "Bot" tab.</value>
</data>
<data name="Discord_BotTokenLabel" xml:space="preserve">
<value>Discord Bot token</value>
</data>
<data name="Discord_Description" xml:space="preserve">
<value>Send Notifo's notifications via direct messages to your Discord account.</value>
</data>
<data name="Discord_Name" xml:space="preserve">
<value>Discord</value>
</data>
<data name="Discord_UserIdDescription" xml:space="preserve">
<value>The user_id of the user the bot will send the DMs to. User is required to install the Discord bot on their account.</value>
</data>
<data name="Discord_UserIdLabel" xml:space="preserve">
<value>Discord user id</value>
</data>
<data name="Email_AdditionalFromEmailsDescription" xml:space="preserve">
<value>Comma or line-separated list of additional email-addresses.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,11 @@ private async Task<DeliveryResult> SendCoreAsync(string appId, MessagingMessage
Targets = lastJob.Configuration,
// We might also format the text without the template if no primary template is defined.
Text = text,
DetailedBodyText = lastJob.Notification.Formatting.Body,
ImageLarge = lastJob.Notification.Formatting.ImageLarge,
ImageSmall = lastJob.Notification.Formatting.ImageSmall,
LinkText = lastJob.Notification.Formatting.LinkText,
LinkUrl = lastJob.Notification.Formatting.LinkUrl,
};

return (default, message.Enrich(lastJob, Name));
Expand Down
Loading
Loading