[groups] Put child's name in the notification, also added count of additional children #4371

homeautomaton · 2024-01-15T14:40:29Z

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

Type of change

Please delete any options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
User interface (UI)
New feature (non-breaking change which adds functionality)

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.

…ount of add'l children

CommanderStorm

Thank you for your PR!
I have left some inline comments

server/model/monitor.js

CommanderStorm · 2024-01-15T15:14:06Z

server/model/monitor.js

@@ -383,6 +383,8 @@ class Monitor extends BeanModel {
                    bean.msg = "Monitor under maintenance";
                    bean.status = MAINTENANCE;
                } else if (this.type === "group") {
+                    let failChild = "";


nit:
please don't hamming code variable names here.
I think downChildrenName and downChildrenCount are better names

I'll go with downChildName and failureCount

names are different despite close in meaning => don't think this is good for long-term maintence

Your petulant complaints have made contributing to this project far more trouble than it is worth. Good day, sir.

I think we are not on the same page what the job as a maintainer is. I see it as making sure that the project can and will move in the right direction.
This does mean that the primary job is to make sure that the code stays maintainable and readable.

The job is not to blindly accept any contribution.

What might be a source of miscommunication is my usage of nit: comments. These comments can be addressed, but does not have to.

CommanderStorm · 2024-01-15T15:15:45Z

server/model/monitor.js

@@ -383,6 +383,8 @@ class Monitor extends BeanModel {
                    bean.msg = "Monitor under maintenance";
                    bean.status = MAINTENANCE;
                } else if (this.type === "group") {
+                    let failChild = "";


I think it would be better to collect the DOWN-children in a list instead of doing this double accounting (last name + count).
This would allow adding this to the logging output, and I think the code might look better.

Even including the count was beyond the scope of the original request. It's simple, doesn't use any extra memory, and makes the message more informative. I'll stick with what I have.

I am not convinced that the extra memory of a few child-names will matter for overall performance/memory consumption.
Yes, my approach does consume a bit more memory, but assuming reasonably long names and a reasonable ammount of monitors per group this likely won't be a problem.
I do think adding a separate mechanism for accounting for the failed number is sort of a footgun => I would prefer a list for tracking this ^^

server/model/monitor.js

CommanderStorm · 2024-01-15T15:24:18Z

server/model/monitor.js

@@ -1526,6 +1543,23 @@ class Monitor extends BeanModel {
        ]);
    }

+    /**
+     * Gets Parent name
+     * @returns {Promise<string>} Name of this monitor's parent


Suggested change

* @returns {Promise<string>} Name of this monitor's parent

* @returns {Promise<string>} Name of this monitor's parent or '' if no parent exists

server/model/monitor.js

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

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

CommanderStorm · 2024-01-16T00:34:39Z

server/model/monitor.js

+                    let parentName = await this.getParentName();
+                    groupText = ` under group '${parentName}'`;


Re https://github.com/louislam/uptime-kuma/pull/4371/files#r1452660704

(I can't add the second line to the suggestion)

I did forget to add the second line to the code suggestion.
I don't think the comparison with getPathName is fair as said function does do a lot more.

Suggested change

let parentName = await this.getParentName();

groupText = ` under group '${parentName}'`;

let parent = await Monitor.getParent(this.id);

groupText = ` under group '${parent.name}'`;

CommanderStorm · 2024-01-16T00:36:14Z

server/model/monitor.js

+    /**
+     * Gets Parent name
+     * @returns {Promise<string>} Name of this monitor's parent
+     */
+    async getParentName() {
+        if (this.parent === null) {
+            return "";
+        }
+
+        let parent = await Monitor.getParent(this.id);
+        return parent.name;
+    }
+


Continiuation of the suggested changes in https://github.com/louislam/uptime-kuma/pull/4371/files#r1452821597

Suggested change

/**

* Gets Parent name

* @returns {Promise<string>} Name of this monitor's parent

*/

async getParentName() {

if (this.parent === null) {

return "";

}

let parent = await Monitor.getParent(this.id);

return parent.name;

}

CommanderStorm · 2024-01-16T00:37:31Z

server/model/monitor.js

+                    let downChildName = "";
+                    let failureCount = 0;


Continuation of #4371 (comment)

I can only suggest refactorings for lines you changed => this is why this suggestion is split into multiple parts

My consearn is mostly around future maintenence effort and if this introduces bugs like the faulty (not taking into account PENDING) accounting using failureCount

Suggested change

let downChildName = "";

let failureCount = 0;

let downChildNames = [];

CommanderStorm · 2024-01-16T00:39:53Z

server/model/monitor.js

                            // lastBeat.status could be null
                            if (!lastBeat) {
                                bean.status = PENDING;
                            } else if (bean.status === UP && (lastBeat.status === PENDING || lastBeat.status === DOWN)) {
                                bean.status = lastBeat.status;
+                                downChildName = child.name;


Suggested change

downChildName = child.name;

downChildNames.push(child.name);

CommanderStorm · 2024-01-16T00:40:10Z

server/model/monitor.js

                            } else if (bean.status === PENDING && lastBeat.status === DOWN) {
                                bean.status = lastBeat.status;
+                                downChildName = child.name;


Suggested change

downChildName = child.name;

downChildNames.push(child.name);

CommanderStorm · 2024-01-16T00:40:21Z

server/model/monitor.js

+                            }
+                            if (lastBeat.status === DOWN) {
+                                failureCount++;


Suggested change

}

if (lastBeat.status === DOWN) {

failureCount++;

CommanderStorm · 2024-01-16T00:42:50Z

server/model/monitor.js

+                            if (failureCount <= 1) {
+                                bean.msg = `Child '${downChildName}' inaccessible`;
+                            } else {
+                                bean.msg = `Child '${downChildName}' and ${failureCount - 1} others inaccessible`;
+                            }


Suggested change

if (failureCount <= 1) {

bean.msg = `Child '${downChildName}' inaccessible`;

} else {

bean.msg = `Child '${downChildName}' and ${failureCount - 1} others inaccessible`;

}

if (downChildNames.lenght === 1) {

bean.msg = `Child '${downChildName[0]}' inaccessible`;

} else {

bean.msg = `Child '${downChildNames[0]}' and ${downChildNames.lengh - 1} others inaccessible`;

}

This is nothing short of ridiculous. Collect a list of names, just to get the first name from the list.

And you've misspelled length. Your code wouldn't even work. I'll just keep my own private fork of this project, and shape it to my own needs. I have many enhancements in mind, and working on this trivial already-existing issue was just my way of testing the waters to see if it is worth contributing to the project. People like you make open source worse than working for a corporation with bad management.

Good riddance.

@louislam Do you have an opinion?

Collect a list of names, just to get the first name from the list.

first name and the lenght of the list.

you've misspelled length

Good catch. fixed it

I'll just keep my own private fork of this project, and shape it to my own needs

That is fine by us.
Forking a project doesn’t have to be a bad thing. Being able to copy and modify projects is one of the best things about open source¹

People like you make open source worse than working for a corporation with bad management.

Sorry that you feel this way. If there is something concrete that I can change in the way I review work, that would be appreciated (I am not even a junior dev in terms of job experience right now)

Footnotes

https://opensource.guide/best-practices/#let-others-build-the-solutions-they-need ↩

tylerjwatson · 2024-02-11T22:56:30Z

hi all, looks like some trouble with code review here.

would you all like me to pick up this PR? I would really like this feature and will donate some time to it.

CommanderStorm · 2024-02-11T23:27:09Z

would you all like me to pick up this PR?

@tylerjwatson
Yes, that would be appreciated.
please rebase any change on #4395 instead (basically a code-move) => lower merge effort.

I am going to close this PR as I don't see @homeautomaton contributing to this PR further as they stated:

I'll just keep my own private fork of this project, and shape it to my own needs [...]
Good riddance

homeautomaton added 2 commits January 15, 2024 08:24

Resolves louislam#3278 - put child name in notification, also added c…

78ef79f

…ount of add'l children

eslint-recommended fixes

4e9afcf

CommanderStorm requested changes Jan 15, 2024

View reviewed changes

CommanderStorm changed the title ~~Resolves #3278 - put child name in notification, also added count of add'l children~~ [groups] Put child's name in the notification, also added count of additional children Jan 15, 2024

CommanderStorm added the area:notifications Everything related to notifications label Jan 15, 2024

homeautomaton and others added 3 commits January 15, 2024 12:17

Update server/model/monitor.js

59d5148

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

Update server/model/monitor.js

a1fbc6f

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

Update server/model/monitor.js

63a41cf

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

homeautomaton requested a review from CommanderStorm January 15, 2024 18:49

variable name changes

d379157

CommanderStorm reviewed Jan 16, 2024

View reviewed changes

CommanderStorm closed this Feb 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[groups] Put child's name in the notification, also added count of additional children #4371

[groups] Put child's name in the notification, also added count of additional children #4371

homeautomaton commented Jan 15, 2024

CommanderStorm left a comment

CommanderStorm Jan 15, 2024

homeautomaton Jan 15, 2024

CommanderStorm Jan 16, 2024

homeautomaton Jan 16, 2024

CommanderStorm Jan 16, 2024

CommanderStorm Jan 15, 2024

homeautomaton Jan 15, 2024

CommanderStorm Jan 16, 2024 •

edited

Loading

CommanderStorm Jan 15, 2024

CommanderStorm Jan 16, 2024

CommanderStorm Jan 16, 2024

CommanderStorm Jan 16, 2024

CommanderStorm Jan 16, 2024

CommanderStorm Jan 16, 2024

CommanderStorm Jan 16, 2024

CommanderStorm Jan 16, 2024 •

edited

Loading

homeautomaton Jan 16, 2024

homeautomaton Jan 16, 2024

CommanderStorm Jan 16, 2024 •

edited

Loading

tylerjwatson commented Feb 11, 2024

CommanderStorm commented Feb 11, 2024

	* @returns {Promise<string>} Name of this monitor's parent
	* @returns {Promise<string>} Name of this monitor's parent or '' if no parent exists

		let parentName = await this.getParentName();
		groupText = ` under group '${parentName}'`;

	let downChildName = "";
	let failureCount = 0;
	let downChildNames = [];

[groups] Put child's name in the notification, also added count of additional children #4371

[groups] Put child's name in the notification, also added count of additional children #4371

Conversation

homeautomaton commented Jan 15, 2024

Description

Type of change

Checklist

Screenshots (if any)

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

Choose a reason for hiding this comment

CommanderStorm Jan 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CommanderStorm Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CommanderStorm Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Footnotes

tylerjwatson commented Feb 11, 2024

CommanderStorm commented Feb 11, 2024

CommanderStorm Jan 16, 2024 •

edited

Loading

CommanderStorm Jan 16, 2024 •

edited

Loading

CommanderStorm Jan 16, 2024 •

edited

Loading