fix: remove service name/type from discord notifier #5181

DeJayDev · 2024-10-09T22:16:03Z

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

I have read and understand the pull request rules.

Description

Fixes #3327, closes #4495.

To do this, I have fully removed the Service Name/Service Type field from the Discord notifier embed. As discussed in the #4495 PR, no other monitor has a distinction between Service Type and Service URL. In alignment with the same comment, I've also moved the fields to their own variable so we are not maintaining two versions of the fields array.

I'd also like to use this PR to discuss reformatting the Discord embed entirely? In my opinion, repeating the service name so close to each other makes the embed look bad. Additionally, we repeat the time of the outage in an embed field and the embed footer.

Type of change

Other

Checklist

My code follows the style guidelines of this project
I ran ESLint and other linters for modified files
I have performed a self-review of my own code and tested it
I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
My changes generates no new warnings
My code needed automated testing. I have added them (this is optional task)

Screenshots

Current (and the issue I aim to fix):

Proposed:

CommanderStorm

I should have been more clear in my previous comment.
The extraction is what I wanted, but the URL should be added if the value for them exists. This is still a valuable context.

I have left two other comments below.

Not quite sure about ping given that this will only show on up pings..
What do you think about this?

CommanderStorm · 2024-10-09T23:29:13Z

server/notification-providers/discord.js

+                },
+                {
+                    name: "Error",
+                    value: heartbeatJSON["msg"] == null ? "N/A" : heartbeatJSON["msg"],


Let's use this instead

Suggested change

value: heartbeatJSON["msg"] == null ? "N/A" : heartbeatJSON["msg"],

value: msg,

Scratch that. Looking at #5181 (comment) this looks bad (👀).

The original author had a point..

Let's do the following instead:

Suggested change

value: heartbeatJSON["msg"] == null ? "N/A" : heartbeatJSON["msg"],

value: heartbeatJSON["msg"] ?? msg,

server/notification-providers/discord.js

Co-authored-by: Frank Elsinga <[email protected]>

DeJayDev · 2024-10-10T01:33:33Z

I made a mess trying to clean up that conflict. Sort of expected it to apply easier... (and then I force pushed because it was the last thing I ran so I had to do it again :()

I think it makes sense to exclude it if extractAddress returns "" or monitorJSON["hostname"] (ping and steam can both do this). Additionally, I'm not sure if using the msg is a great idea bc it repeats a bunch of information. I think it works better if we change the heading from "Error" to "Message", but still feels sort of :/

myredis is redis
Discord is HTTP
localhost is a ping

DeJayDev · 2024-10-21T00:30:58Z

:)

I also added a couple translation keys for this:

It's a little cursed, the wayToGetDiscordURL key needs a fullstop on the end for it to flow better but I wasn't sure if I should change it because it's translated already.

CommanderStorm

Looks pretty good. Could basically merged, except for a few nits.

Because we have had problems with this in the past and the codepath is different for that:
Could you post a screenshot of a UP notifcation to ensure that you have tested this? ^^

PSA: force pushing looses the state the reviewer was in => reviewiers cannot review "from the last review" => unless being asked to, merging is almost always better. ^^
The PR will likely be squashed at the end anyway.

CommanderStorm · 2024-10-21T01:37:27Z

src/components/notifications/Discord.vue

+                <a
+                    href="https://discord.com/developers/docs/resources/webhook#create-webhook"
+                    target="_blank"
+                >{{ $t("withADiscordBot") }}</a>


I think this might be simpler for our translators

Suggested change

<a

href="https://discord.com/developers/docs/resources/webhook#create-webhook"

target="_blank"

>{{ $t("withADiscordBot") }}</a>

<template #webhookWithADiscordBotAPI>

<a

href="https://discord.com/developers/docs/resources/webhook#create-webhook"

target="_blank"

>{{ $t("webhookWithADiscordBotAPI") }}</a>

</template>

CommanderStorm · 2024-10-21T01:42:27Z

server/notification-providers/discord.js

                await axios.post(webhookUrl.toString(), discordupdata);
                return okMsg;
            }
        } catch (error) {
+            console.log(error);


if it is not already done so, it should be logged for all and using our log.error facities.
If this is the case, that would be for another PR.

Suggested change

console.log(error);

CommanderStorm · 2024-10-21T01:48:43Z

server/notification-providers/discord.js

+
+            const baseURL = await Settings.get("primaryBaseURL");
+            const address = this.extractAddress(monitorJSON);
+            const hasAddress = address !== "" && address !== monitorJSON.hostname;


why != monitorJSON.hostname?
As far as I can remeber, none of the other monitors do that => lets also not do this, unless there is a reason for this.

If there is no particular reason, please inline hasAddress.
If there is, please add a comment why.

CommanderStorm · 2024-10-21T01:54:22Z

src/components/notifications/Discord.vue

@@ -4,6 +4,12 @@
        <input id="discord-webhook-url" v-model="$parent.notification.discordWebhookUrl" type="text" class="form-control" required autocomplete="false">
        <div class="form-text">
            {{ $t("wayToGetDiscordURL") }}


Lets add a line break here, to prevent those lines being mushed together (think this adresses It's a little cursed from #5181 (comment))

Suggested change

{{ $t("wayToGetDiscordURL") }}

{{ $t("wayToGetDiscordURL") }}<br>

CommanderStorm · 2024-10-21T01:57:55Z

src/lang/en.json

+    "wayToGetDiscordURLEnhanced": "Using the Discord API, you may also create the webhook {0} for button support.",
+    "withADiscordBot": "with a bot",


This word order reads a bit nicer in my mind. What do you think?

Suggested change

"wayToGetDiscordURLEnhanced": "Using the Discord API, you may also create the webhook {0} for button support.",

"withADiscordBot": "with a bot",

"wayToGetDiscordURLEnhanced": "For button support, you need to create create {webhookWithADiscordBotAPI}.",

"webhookWithADiscordBotAPI": "the webhook with a bot using the Discord API",

CommanderStorm · 2024-10-21T02:01:16Z

server/notification-providers/discord.js

+                },
+                {
+                    name: "Error",
+                    value: msg,


#5181 (comment)

Suggested change

value: msg,

value: heartbeatJSON["msg"] ?? msg,

github-actions bot added the needs:resolve-merge-conflict label Oct 9, 2024

CommanderStorm requested changes Oct 9, 2024

View reviewed changes

DeJayDev and others added 3 commits October 9, 2024 20:01

fix: misspelling of 'address' (louislam#5180)

0d328e7

fix: remove service name/type from discord notifier

c28710c

Use msg rather than parse it out of the heartbeat

af230ba

Co-authored-by: Frank Elsinga <[email protected]>

DeJayDev force-pushed the dj/discord-service-url branch from 7ca5150 to af230ba Compare October 10, 2024 01:02

github-actions bot removed the needs:resolve-merge-conflict label Oct 10, 2024

Conditionally add service url

8b82520

DeJayDev force-pushed the dj/discord-service-url branch from b620a02 to 8b82520 Compare October 10, 2024 01:35

github-actions bot added the needs:resolve-merge-conflict label Oct 10, 2024

Merge branch 'master' into dj/discord-service-url

ecac291

github-actions bot removed the needs:resolve-merge-conflict label Oct 10, 2024

CommanderStorm added area:notifications Everything related to notifications type:enhance-existing feature wants to enhance existing monitor pr:please address review comments this PR needs a bit more work to be mergable labels Oct 10, 2024

DeJayDev added 2 commits October 20, 2024 19:28

add discord buttons to payload

cd69146

Merge branch 'master' into dj/discord-service-url

64f5284

CommanderStorm reviewed Oct 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove service name/type from discord notifier #5181

fix: remove service name/type from discord notifier #5181

DeJayDev commented Oct 9, 2024

CommanderStorm left a comment

CommanderStorm Oct 9, 2024

CommanderStorm Oct 10, 2024

DeJayDev commented Oct 10, 2024 •

edited

Loading

DeJayDev commented Oct 21, 2024

CommanderStorm left a comment

CommanderStorm Oct 21, 2024

CommanderStorm Oct 21, 2024

CommanderStorm Oct 21, 2024

CommanderStorm Oct 21, 2024

CommanderStorm Oct 21, 2024

CommanderStorm Oct 21, 2024

	value: heartbeatJSON["msg"] == null ? "N/A" : heartbeatJSON["msg"],
	value: msg,

	value: heartbeatJSON["msg"] == null ? "N/A" : heartbeatJSON["msg"],
	value: heartbeatJSON["msg"] ?? msg,

	{{ $t("wayToGetDiscordURL") }}
	{{ $t("wayToGetDiscordURL") }}<br>

		"wayToGetDiscordURLEnhanced": "Using the Discord API, you may also create the webhook {0} for button support.",
		"withADiscordBot": "with a bot",

fix: remove service name/type from discord notifier #5181

Are you sure you want to change the base?

fix: remove service name/type from discord notifier #5181

Conversation

DeJayDev commented Oct 9, 2024

Description

Type of change

Checklist

Screenshots

CommanderStorm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeJayDev commented Oct 10, 2024 • edited Loading

DeJayDev commented Oct 21, 2024

CommanderStorm 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

DeJayDev commented Oct 10, 2024 •

edited

Loading