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

[ntfy] priority level is not clearly communicated to the user #4174

Closed
2 tasks done
tdavis6 opened this issue Dec 5, 2023 · 11 comments · Fixed by #4175
Closed
2 tasks done

[ntfy] priority level is not clearly communicated to the user #4174

tdavis6 opened this issue Dec 5, 2023 · 11 comments · Fixed by #4175
Labels
area:notifications Everything related to notifications bug Something isn't working

Comments

@tdavis6
Copy link

tdavis6 commented Dec 5, 2023

⚠️ Please verify that this bug has NOT been raised before.

  • I checked and didn't find similar issue

🛡️ Security Policy

Description

When a notification is triggered, it sends to ntfy with a priority of 5. This is the incorrect behavior because it is set to send with a priority of 4. The double chevron indicates a priority of 4, while the triple chevron indicates a priority of 5.

image
image

Thank you so much for such an awesome app!

👟 Reproduction steps

This is triggered by setting a notification to use ntfy, with a priority of 4. Then, if a monitor goes down, the notification is sent with a priority of 5.

👀 Expected behavior

It would be expected that the notification is sent with a priority of 4.

😓 Actual Behavior

The notification was sent with a priority of 5.

🐻 Uptime-Kuma Version

1.23.8

💻 Operating System and Arch

Synology DSM 7.2.1-69057 Update 3

🌐 Browser

Firefox 120.0.1

🐋 Docker Version

20.10.23

🟩 NodeJS Version

No response

📝 Relevant log output

No response

@tdavis6 tdavis6 added the bug Something isn't working label Dec 5, 2023
@CommanderStorm CommanderStorm added the area:notifications Everything related to notifications label Dec 5, 2023
@tdavis6
Copy link
Author

tdavis6 commented Dec 5, 2023

So sorry, just read the ntfy.js file and this appears to be the intended behavior. This is based on lines 44-45.

@tdavis6 tdavis6 closed this as completed Dec 5, 2023
@CommanderStorm
Copy link
Collaborator

The behaviour is this way since #2863

// if priority is not 5, increase priority for down alerts
priority = priority === 5 ? priority : priority + 1;

=> down monitors have n+1 priority

@CommanderStorm CommanderStorm reopened this Dec 5, 2023
@tdavis6
Copy link
Author

tdavis6 commented Dec 5, 2023

Thank you so much for the quick reply!

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Dec 5, 2023

appears to be the intended behavior

Missing documentation / confusing UX is a bug too (!).
I think this should have a help text in the frontend

@CommanderStorm
Copy link
Collaborator

@tdavis6
What do you think about such a helptext ?
I am struggling to find a formulation which does not include max(5, priority + 1). Do you have any good ideas?

image

@CommanderStorm
Copy link
Collaborator

Okay.
I think I found a way which does not include a formula. Oppinions?

image
image

@tdavis6
Copy link
Author

tdavis6 commented Dec 5, 2023

I think that looks good! However, if the set priority is 3 and it bumps it to 4 that might not be clear. I'm trying to think of something that would make that behavior clear, without being too long to put there.

@tdavis6
Copy link
Author

tdavis6 commented Dec 5, 2023

Perhaps "All events are sent with this priority, except DOWN-events, which have a priority one above this, with a maximum of 5". Although this might be too long.

@tdavis6
Copy link
Author

tdavis6 commented Dec 5, 2023

Thank you so much for helping me!

@sevmonster
Copy link

I wasn't aware of this functionality. Would it be too much to have a separate priority for down alerts?

@CommanderStorm CommanderStorm changed the title ntfy notification sends as the incorrect priority level [ntfy] priority level is not clearly communicated to the user Dec 14, 2023
@CommanderStorm
Copy link
Collaborator

Resolved by #4175

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:notifications Everything related to notifications bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants