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: use the server timezone to parse the cron expression #696

Merged
merged 6 commits into from
Nov 6, 2024

Conversation

jonnochoo
Copy link
Contributor

@jonnochoo jonnochoo commented Nov 4, 2024

There is an issue if the server is running in a timezone (eg. UTC), but the client's browser timezone is in another timezone (eg. Australia/Sydney), the cron expression incorrectly shows the Next Run incorrectly.

Next Steps

Open on thoughts on the best way to get the server's timezone? Create an API endpoint? Open to feedback

@yohamta
Copy link
Collaborator

yohamta commented Nov 4, 2024

Hi @jonnochoo, thank you for draft PR! This looks amazing. Yes, this does need to be fixed. I would suggest adding a new configuration key, timezone, to pass the server's timezone to the frontend. Here are the files that need modification. If possible, it would be great if you could make these changes in this PR.

<script>
function getConfig() {
return {
apiURL: "{{ apiURL }}",
title: "{{ navbarTitle }}",
navbarColor: "{{ navbarColor }}",
version: "{{ version }}",
}
}
</script>

func defaultFunctions(cfg funcsConfig) template.FuncMap {
return template.FuncMap{
"defTitle": func(ip any) string {
v, ok := ip.(string)
if !ok || (ok && v == "") {
return ""
}
return v
},
"version": func() string {
return constants.Version
},
"navbarColor": func() string {
return cfg.NavbarColor
},
"navbarTitle": func() string {
return cfg.NavbarTitle
},
"apiURL": func() string {
return cfg.APIBaseURL
},
}
}

const datesToRun = schedules.map((s) =>
cronParser.parseExpression(s.Expression).next()
);
const tz = getConfig().timeZone;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For backwards compatibility tz won't be set, so it should defer to the original behaviour.

@@ -251,7 +251,9 @@ const defaultColumns = [
}),
columnHelper.accessor('Type', {
id: 'Schedule',
header: 'Schedule',
header: getConfig().timeZone
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be helpful to see what timezone the server is running in?

@jonnochoo
Copy link
Contributor Author

Hi @jonnochoo, thank you for draft PR! This looks amazing. Yes, this does need to be fixed. I would suggest adding a new configuration key, timezone, to pass the server's timezone to the frontend. Here are the files that need modification. If possible, it would be great if you could make these changes in this PR.

<script>
function getConfig() {
return {
apiURL: "{{ apiURL }}",
title: "{{ navbarTitle }}",
navbarColor: "{{ navbarColor }}",
version: "{{ version }}",
}
}
</script>

func defaultFunctions(cfg funcsConfig) template.FuncMap {
return template.FuncMap{
"defTitle": func(ip any) string {
v, ok := ip.(string)
if !ok || (ok && v == "") {
return ""
}
return v
},
"version": func() string {
return constants.Version
},
"navbarColor": func() string {
return cfg.NavbarColor
},
"navbarTitle": func() string {
return cfg.NavbarTitle
},
"apiURL": func() string {
return cfg.APIBaseURL
},
}
}

Thanks for the pointers, I have made the change and you can see in the screenshot it works.

The server time it UTC but the client/browser is in Sydney time, and the cron expression shows the correct next run

image

@jonnochoo jonnochoo marked this pull request as ready for review November 5, 2024 11:24
Copy link
Collaborator

@yohamta yohamta left a comment

Choose a reason for hiding this comment

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

LGTM ✨✨✨🚀🚀🚀✨✨✨ Thank you very much for the amazing work!

@yohamta yohamta merged commit 93c3710 into dagu-org:main Nov 6, 2024
4 checks passed
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 62.10%. Comparing base (22fe495) to head (c7986d2).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/config/config.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #696      +/-   ##
==========================================
- Coverage   64.08%   62.10%   -1.98%     
==========================================
  Files          53       53              
  Lines        4318     5215     +897     
==========================================
+ Hits         2767     3239     +472     
- Misses       1298     1723     +425     
  Partials      253      253              
Files with missing lines Coverage Δ
internal/config/config.go 0.00% <0.00%> (ø)

... and 50 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22fe495...c7986d2. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants