[Integration] Discord integration #244

Arciiix · 2024-06-25T18:20:44Z

Hey! I finally finished building the Discord integration.
Can you please take a look at the code and review it?

Later on, I'd also like to add some additional features (e.g. configuration hints) for this (and other) integrations. Also, I'd work on writing some tests (as you mentioned). Furthermore, I noticed some additional bugs which I'd like to write the fixes to.

About this Discord integration itself, I modified the MessagingMessage since Discord allows us to send more sophisticated messages. For example, this is how the messages from the tests I wrote look like:

they can also contain images, links, etc.

To obtain the bot token, you have to go onto the Discord Developer Platform, create an application, go to the Bot tab from the sidebar and get the token.

Also, to allow the bot to send you DMs, you have to install it on your Discord account (as we mentioned in #38 ). To do this, you first have to enable those installations on the bot.

Go to the Installation tab on the sidebar on the Developer Platform
Check User install option in Authorization methods
Select "Discord provided link" in Install link section
Copy that link
Open that link and select "Try it Now" (the first option)
Authorize

Then, in Notifo, you can set the Discord user_id for the given user.
To learn how to find the user id on Discord, see this tutorial.

Have a great evening!

SebastianStehle

Generally it looks good.

Please ensure that there are not compiler warnings. I also added a lot of small comments.

backend/src/Notifo.Domain.Integrations/Discord/DiscordClient.cs

backend/src/Notifo.Domain.Integrations/Discord/DiscordBotClientPool.cs

SebastianStehle · 2024-06-25T20:35:33Z

backend/src/Notifo.Domain.Integrations/Discord/DiscordBotClientPool.cs

+        {
+            var client = new DiscordClient();
+
+            client.LoginAsync(TokenType.Bot, botToken).Wait(ct);


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.

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?

SebastianStehle · 2024-06-25T20:36:13Z

backend/src/Notifo.Domain.Integrations.Abstractions/MessagingMessage.cs

    public string Text { get; set; }
+    public string? DetailedBodyText { get; init; }


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?

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.

SebastianStehle · 2024-06-25T20:37:11Z

backend/src/Notifo.Domain.Integrations/Discord/DiscordIntegration.Messaging.cs

+            {
+                var client = discordBotClientPool.GetDiscordClient(botToken, ct);
+
+                var user = await client.GetUserAsync(ulong.Parse(chatId));


Forward cancellation token.

backend/tests/Notifo.Domain.Tests/Integrations/Discord/DiscordTests.cs

SebastianStehle · 2024-06-25T20:52:26Z

backend/tests/Notifo.Domain.Tests/Integrations/Discord/DiscordTests.cs

+using Microsoft.Extensions.DependencyInjection;
+
+namespace Notifo.Domain.Integrations.Discord;
+public sealed class DiscordTests


Does it make sense to have a custom base class as we have for the other providers?

I don't think so since this is more likely an integration test that tests specific functionality related to my implementation. E.g. some other implementations from Messaging don't support images, links etc.

backend/tests/Notifo.Domain.Tests/Integrations/Discord/DiscordTests.cs

SebastianStehle · 2024-06-25T20:53:04Z

backend/tests/Notifo.Domain.Tests/appsettings.json

@@ -1,47 +1,51 @@
 {
-


What are all these changes? I think VS fucks up the formatting sometimes.

Btw: now those formatting issues are fixed. You can't see it here in this commit, but if you compare it to the current version from the main branch, the indentation is similar.

Arciiix · 2024-06-25T20:59:43Z

Okay, thank you so much Sebastian! Will apply the changes soon and do all the other stuff I mentioned. I will be busy tomorrow, but I'll work on it later and let you know. Take care!

Arciiix · 2024-06-27T12:48:45Z

Hey! Hope you're having a great day.
I added some comments to your review. Once again thank you so much for your code review, it helps me write better code and be more helpful 🤗

After I get replies to my questions I wrote there, I'll push another commit with my changes.

Arciiix · 2024-06-28T16:45:19Z

Just pushed the commit fixing those changes that you suggested.
If you have some time, you can look at them and also at the comments under your comments on the code review.
Take care!

SebastianStehle · 2024-06-30T13:21:48Z

backend/src/Notifo.Domain.Integrations/Discord/DiscordBotClientPool.cs

+        {
+            var client = new DiscordClient();
+
+            // Sadly it can't receive the cancellation token.


No need to write that much, nobody reads it. Just

"Method provides no option to pass CancellationToken" is enough

SebastianStehle · 2024-06-30T13:22:16Z

backend/src/Notifo.Domain.Integrations/Discord/DiscordIntegration.Messaging.cs

+using Discord.Net;
+
+namespace Notifo.Domain.Integrations.Discord;
+public sealed partial class DiscordIntegration : IMessagingSender


Please add a new line after namespace declaration

SebastianStehle · 2024-06-30T13:22:33Z

backend/src/Notifo.Domain.Integrations/Discord/DiscordIntegration.Messaging.cs

+    }
+
+    public async Task<DeliveryResult> SendAsync(IntegrationContext context, MessagingMessage message,
+    CancellationToken ct)


Please intend CancellationToken. See next method.

SebastianStehle · 2024-06-30T13:23:49Z

backend/src/Notifo.Domain.Integrations/Discord/DiscordIntegration.Messaging.cs

+                var client = await discordBotClientPool.GetDiscordClient(botToken, ct);
+                var requestOptions = new RequestOptions { CancelToken = ct };
+
+                var user = await client.GetUserAsync(ulong.Parse(chatId), CacheMode.AllowDownload, requestOptions);


I think the Parse should be TryParse with a proper exception.

SebastianStehle · 2024-06-30T13:24:37Z

backend/src/Notifo.Domain.Integrations/Discord/DiscordIntegration.cs

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


What is the comment for? It is weird!

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

SebastianStehle · 2024-06-30T13:26:42Z

backend/tests/Notifo.Domain.Tests/Integrations/Discord/DiscordTests.cs

+    {
+        var userId = GetUserId();
+
+        var message = new MessagingMessage


They way you add extra lines is a little bit inconsistent here. I like this method, but other methods look different. Especially before "var message" and before "var result"

SebastianStehle · 2024-06-30T13:27:59Z

backend/src/Notifo.Domain.Integrations/Discord/DiscordServiceExtensions.cs

+
+using Microsoft.Extensions.DependencyInjection;
+
+namespace Notifo.Domain.Integrations.Discord;


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.

SebastianStehle

I have a few small change requests. Nothing serious, should take not more than 10 minutes to solve that.

Overall good work.

Arciiix · 2024-06-30T14:15:27Z

Thank you so much! I will apply those changes as soon as I find some time, have a nice day!

Arciiix · 2024-06-30T15:02:49Z

Just pushed those changes

Arciiix and others added 12 commits June 24, 2024 16:48

Fix updating the user preferences

25f906e

Fix adding user properties to UserInfo context

2f40264

Fix adding user properties to UserInfo context

118cfc0

Make the dictionaries merge code more imperative

f20fc36

Remove unnecessary import

b97b900

Create Discord integration

4b72ef2

Fix invalid status on Discord integration

7a77581

Add tests for Discord integration

47dc766

Change appSettings indentation to match the previous one

3bf2f50

Update appSettings for tests

604baa4

Add empty line at the end of the file to match the convention

4d735e5

Merge branch 'main' into main

1060862

SebastianStehle requested changes Jun 25, 2024

View reviewed changes

Arciiix added 2 commits June 28, 2024 18:43

Apply code review advice

858ebe0

Merge branch 'main' of https://github.com/Arciiix/notifo

62db2f4

Add integration image

ffae14f

SebastianStehle reviewed Jun 30, 2024

View reviewed changes

SebastianStehle requested changes Jun 30, 2024

View reviewed changes

Apply code refactoring

4c603dd

Add a new line at the end of file

ce06ed4

SebastianStehle merged commit d0db2df into notifo-io:main Jun 30, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Integration] Discord integration #244

[Integration] Discord integration #244

Arciiix commented Jun 25, 2024

SebastianStehle left a comment

SebastianStehle Jun 25, 2024

Arciiix Jun 27, 2024

SebastianStehle Jun 25, 2024

Arciiix Jun 27, 2024

SebastianStehle Jun 25, 2024

SebastianStehle Jun 25, 2024

Arciiix Jun 30, 2024

SebastianStehle Jun 25, 2024

Arciiix Jun 30, 2024

Arciiix commented Jun 25, 2024

Arciiix commented Jun 27, 2024

Arciiix commented Jun 28, 2024

SebastianStehle Jun 30, 2024

SebastianStehle Jun 30, 2024

SebastianStehle Jun 30, 2024 •

edited

Loading

SebastianStehle Jun 30, 2024

SebastianStehle Jun 30, 2024

SebastianStehle Jun 30, 2024

SebastianStehle Jun 30, 2024

SebastianStehle Jun 30, 2024

SebastianStehle left a comment

Arciiix commented Jun 30, 2024

Arciiix commented Jun 30, 2024

		public string Text { get; set; }
		public string? DetailedBodyText { get; init; }


		using Microsoft.Extensions.DependencyInjection;

		namespace Notifo.Domain.Integrations.Discord;

[Integration] Discord integration #244

[Integration] Discord integration #244

Conversation

Arciiix commented Jun 25, 2024

SebastianStehle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arciiix commented Jun 25, 2024

Arciiix commented Jun 27, 2024

Arciiix commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianStehle Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianStehle left a comment

Choose a reason for hiding this comment

Arciiix commented Jun 30, 2024

Arciiix commented Jun 30, 2024

SebastianStehle Jun 30, 2024 •

edited

Loading