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

fix: showing the notification activation switch properly #5196

Conversation

julian-piehl
Copy link
Contributor

@julian-piehl julian-piehl commented Oct 13, 2024

⚠️⚠️⚠️ 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

Resolves #5206
Last week there where some performance optimizations (#5025). I noticed some things being a bit buggy afterwards, so I looked into it.

  • Notifications always showing as disabled when editing.
  • Tags not being visible.

Type of change

Please delete any options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

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 (if any)

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

@julian-piehl
Copy link
Contributor Author

Hey @louislam, sorry for pinging.
I noticed that the database library used here was also developed by you. Therefore, I think you might be the best person to answer my question.

I've observed a somewhat strange behavior. You can use "?" as a placeholder for values, but it seems to work sometimes and not at other times.

Here, it works without any problems:
https://github.com/julian-piehl/uptime-kuma/blob/aa056bf3085950ebfa249f34a265bb09931d35f7/server/model/monitor.js#L1508-L1514

Here, the question mark doesn't seem to be replaced correctly:
https://github.com/julian-piehl/uptime-kuma/blob/aa056bf3085950ebfa249f34a265bb09931d35f7/server/model/monitor.js#L1523-L1530

And here, it seems that only the first question mark was replaced correctly (which is why I temporarily removed the second one):

let query = " user_id = ? ";
let queryParams = [ userID ];
if (monitorID) {
query += "AND id = ? ";
queryParams.push(monitorID);
}
let monitorList = await R.find("monitor", query + "ORDER BY weight DESC, name", queryParams);

https://github.com/julian-piehl/uptime-kuma/blob/aa056bf3085950ebfa249f34a265bb09931d35f7/server/uptime-kuma-server.js#L243-L249

Is this currently a bug in RedBean, or am I misunderstanding how to use these placeholders?

@CommanderStorm
Copy link
Collaborator

Child Monitors not updating if parent is paused/unpaused

Not an error in #5025, but rather how we have always handled this => #4696

=> please move any changes to this bugfix #5193 instead

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Hi,
I think in #5196 (comment), your testing might have been a bit flawed. I have noted a solution for the underlying problem with the library below

server/model/monitor.js Outdated Show resolved Hide resolved
server/uptime-kuma-server.js Outdated Show resolved Hide resolved
@CommanderStorm CommanderStorm added pr:please address review comments this PR needs a bit more work to be mergable area:monitor Everything related to monitors labels Oct 13, 2024
@CommanderStorm CommanderStorm added this to the 2.0.0 milestone Oct 13, 2024
@louislam louislam modified the milestones: 2.0.0, 2.1.0 Oct 14, 2024
@julian-piehl
Copy link
Contributor Author

Child Monitors not updating if parent is paused/unpaused

Not an error in #5025, but rather how we have always handled this => #4696

=> please move any changes to this bugfix #5193 instead

I’m sorry, but I don’t quite understand what you mean by ‘how we have always handled this.’ When I added the groups feature last year, pausing a group instantly showed the children as paused as well. I've reviewed commits #5025 and the one prior, and I can confirm that the bug indeed originated from #5025.

@julian-piehl
Copy link
Contributor Author

@louislam I don't recommend shifting the milestone from 2.0.0 to 2.1.0, as the current interface is quite buggy in its current state.

  • When pausing a group, child monitors aren't updated until page is reloaded.
  • Tags on Monitors aren't visible in the Tree or Tags Settings Page.
  • Filtering for Tags isn't possible.
  • Notifications are always unchecked when editing a monitor.

@julian-piehl julian-piehl marked this pull request as ready for review October 14, 2024 18:42
@CommanderStorm
Copy link
Collaborator

pausing a group instantly showed the children as paused as well

Yes, but they are not paused. They were just displayed as such.

Please shift changing the display to the PR where they are getting paused.

@louislam louislam modified the milestones: 2.1.0, 2.0.0 Oct 14, 2024
server/model/monitor.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

The notification part works => merging that part

The tags don't seem to sync quite correctly, but that being a different PR is fine my be.
Will have to look into this
image

@CommanderStorm CommanderStorm merged commit 85dfe1f into louislam:master Oct 16, 2024
18 checks passed
@CommanderStorm CommanderStorm changed the title fix: some errors from the performance optimization fix: showing the notification activation switch properly Oct 16, 2024
@julian-piehl
Copy link
Contributor Author

julian-piehl commented Oct 16, 2024

The tags don't seem to sync quite correctly, but that being a different PR is fine my be.
Will have to look into this

Before, the tags weren't visible at all, but it seems I overlooked this tag sync issue 😅.
It looks like the call to sendUpdateMonitorIntoList is missing for both events addMonitorTag and deleteMonitorTag.

@ro7it
Copy link

ro7it commented Oct 16, 2024

it looks like the notification broken for Slack

2024-10-16T17:53:24+05:30 [MONITOR] ERROR: Cannot send notification to Superman
2024-10-16T17:53:24+05:30 [MONITOR] ERROR: Error: Error: TypeError: this.extractAddress is not a function
at Slack.throwGeneralAxiosError (/Users/rohitti/Downloads/demo-app/server/notification-providers/notification-provider.js:69:15)
at Slack.send (/Users/rohitti/Downloads/demo-app/server/notification-providers/slack.js:167:18)
at async Monitor.sendNotification (/Users/rohitti/Downloads/demo-app/server/model/monitor.js:1340:21)
at async beat (/Users/rohitti/Downloads/demo-app/server/model/monitor.js:922:21)
at async safeBeat (/Users/rohitti/Downloads/demo-app/server/model/monitor.js:1007:17)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:monitor Everything related to monitors pr:please address review comments this PR needs a bit more work to be mergable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notification auto disabling on clicking save.button
4 participants