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.

Sign up for GitHub

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

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

Closed
wants to merge 6 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
78ef79f
Resolves #3278 - put child name in notification, also added count of …
homeautomaton Jan 15, 2024
4e9afcf
eslint-recommended fixes
homeautomaton Jan 15, 2024
59d5148
Update server/model/monitor.js
homeautomaton Jan 15, 2024
a1fbc6f
Update server/model/monitor.js
homeautomaton Jan 15, 2024
63a41cf
Update server/model/monitor.js
homeautomaton Jan 15, 2024
d379157
variable name changes
homeautomaton Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 33 additions & 3 deletions server/model/monitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,8 @@ class Monitor extends BeanModel {
bean.msg = "Monitor under maintenance";
bean.status = MAINTENANCE;
} else if (this.type === "group") {
let downChildName = "";
let failureCount = 0;
Comment on lines +386 to +387
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 = [];

const children = await Monitor.getChildren(this.id);

if (children.length > 0) {
Expand All @@ -395,19 +397,28 @@ class Monitor extends BeanModel {
}
const lastBeat = await Monitor.getPreviousHeartbeat(child.id);

// Only change state if the monitor is in worse conditions then the ones before
// Only change state if the monitor is in worse conditions than the ones before
// 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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
downChildName = child.name;
downChildNames.push(child.name);

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

Choose a reason for hiding this comment

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

Suggested change
downChildName = child.name;
downChildNames.push(child.name);

}
if (lastBeat.status === DOWN) {
failureCount++;
Comment on lines +410 to +412
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}
}

if (bean.status !== UP) {
bean.msg = "Child inaccessible";
if (failureCount <= 1) {
bean.msg = `Child '${downChildName}' inaccessible`;
} else {
bean.msg = `Child '${downChildName}' and ${failureCount - 1} others inaccessible`;
}
Comment on lines +417 to +421
Copy link
Collaborator

@CommanderStorm CommanderStorm Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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`;
}

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@louislam Do you have an opinion?

Copy link
Collaborator

@CommanderStorm CommanderStorm Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 source1

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

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

}
} else {
// Set status pending if group is empty
Expand Down Expand Up @@ -971,7 +982,13 @@ class Monitor extends BeanModel {
} else if (bean.status === MAINTENANCE) {
log.warn("monitor", `Monitor #${this.id} '${this.name}': Under Maintenance | Type: ${this.type}`);
} else {
log.warn("monitor", `Monitor #${this.id} '${this.name}': Failing: ${bean.msg} | Interval: ${beatInterval} seconds | Type: ${this.type} | Down Count: ${bean.downCount} | Resend Interval: ${this.resendInterval}`);
let groupText = "";
if (this.type !== "group" && this.parent !== "") {
let parentName = await this.getParentName();
groupText = ` under group '${parentName}'`;
Comment on lines +987 to +988
Copy link
Collaborator

Choose a reason for hiding this comment

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

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}'`;

}

log.warn("monitor", `Monitor #${this.id} '${this.name}'${groupText}: Failing: ${bean.msg} | Interval: ${beatInterval} seconds | Type: ${this.type} | Down Count: ${bean.downCount} | Resend Interval: ${this.resendInterval}`);
}

// Calculate uptime
Expand Down Expand Up @@ -1526,6 +1543,19 @@ class Monitor extends BeanModel {
]);
}

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

Choose a reason for hiding this comment

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

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

*/
async getParentName() {
if (this.parent === null) {
return "";
}

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

Comment on lines +1546 to +1558
Copy link
Collaborator

Choose a reason for hiding this comment

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

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;
}

/**
* Gets Full Path-Name (Groups and Name)
* @returns {Promise<string>} Full path name of this monitor
Expand Down
Loading