-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Updated croner to 8.1.0 and fixed "recurring-interval" type maintenance #4939
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good to me.
Thanks for the work ❤️
I have left a question below
server/model/maintenance.js
Outdated
@@ -253,6 +253,10 @@ class Maintenance extends BeanModel { | |||
duration = this.duration * 1000; | |||
} | |||
|
|||
if (duration === undefined && this.strategy === "recurring-interval") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need a bit of help understanding the if
-clause you added 😅:
Is this not already covered by the if
or else
branch above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @CommanderStorm, so above is an if
- else if
- else
, and the else if
is when this.end_date
is set, and it calculates if this.end_date
will be before the end of the interval. If it is not, then this.duration
is left undefined
.
I guess my solution is not the best one, I was not sure if it was left undefined
for some other purpose, but if it is not, then I guess the else if
should look something like this:
else if (this.end_date) {
let d = dayjs(this.end_date).diff(dayjs(), "second");
if (d < this.duration) {
duration = d * 1000;
}
else {
duration = this.duration * 1000;
}
}
Let me know if you want me to make this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is better.
duration
is never explicitly checked for undefined => I think it being undefined is a bug ^^
While you are at it, I think that while if
/... clause would be way better if extracted into a function this.duration = this.inferDuration()
That way the controll flow is not as confusing in the future..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inferDuration(customDuration) {
// Check if duration is still in the window. If not, use the duration from the current time to the end of the window
if (customDuration > 0) {
return customDuration;
} else if (this.end_date) {
let d = dayjs(this.end_date).diff(dayjs(), "second");
if (d < this.duration) {
return d * 1000;
}
}
return this.duration * 1000;
}
Code here
Lmk if this is good.
Co-authored-by: Frank Elsinga <frank@elsinga.de>
Co-authored-by: Louis Lam <louislam@users.noreply.github.com>
Thanks for the fix! 🎉 Note This PR is part of the All changes in this PR are small and uncontroversial ⇒ merging with junior maintainer approval |
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]:
Description
Fixes #4634
Possibly #4930
Type of change
Please delete any options that are not relevant.
Checklist
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.