-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Scheduled Reports | X o'clock UTC >> XX:00 UTC #24039
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
base: 5.x-dev
Are you sure you want to change the base?
Conversation
…ead of just 'x:00' when x < 10
…tHour parameter shows up when timezone is not utc
sgiehl
left a comment
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.
Issues identified by Codex
| const reportHourFloat = parseFloat(reportHour); | ||
| const hours = Math.floor(reportHourFloat); | ||
| const paddedHour = `${hours}`.padStart(2, '0'); | ||
| const reportHourUtcLabel = translate('ScheduledReports_ReportHourWithUtcOnly', [`${paddedHour}:00`]); |
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.
Incorrect UTC time formatting for half-hour offsets. reportHourUtc floors the hour and always formats :00, so 10.5 becomes 10:00 instead of 10:30. This is wrong for time zones like Asia/Kolkata.
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 @sgiehl ,
I saw this when reviewing from codex as well, but when I checked with what was happening in 5.x-dev, it was also not showing the minutes and just always the hours.
Like:
Choose 7:30 -> 22 o'clock
We also have code in reportHours that changes our selectbox to add '.5' to our selections so that we also ensure we choose something with 'xx:30' when we are in timezone with 'UTC+30'
if ((this.timeZoneDifferenceInHours * 2) % 2 !== 0) {
hours.push({
key: `${i}.5`,
value: `${paddedHour}:30`,
});
} else {
hours.push({
key: `${i}`,
value: `${paddedHour}:00`,
});
}
Meaning, if we have a timezone with UTC+30, we are forced to select '00:30, 01:30, etc' instead of '00:00, 01:00'
I thought that because of these, this was what it was intending to do and we always 'try' to send the report at the hour and not in 30 mins increment.
Caveat:
This will not work for UTC+45 timezones though, but after checking its only Nepal and Chatham Islands that seem to use this timezone, but let me know if we want to make changes so that we cover this edge case
| await page.click('.entityTable tr:nth-child(11) button[title="Edit"]'); | ||
| await page.waitForNetworkIdle(); | ||
| expect(await page.evaluate(() => $('#reportHourHelpText').is(':visible'))).to.be.false; | ||
| }); |
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.
UI test only checks visibility, not correctness of the UTC text. It won’t catch the half‑hour formatting bug.
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.
Added test for actual time when the timezone value is not UTC. If it is UTC, we do not check the time value since we just hide the help text anyway
added some refactoring so that computed variables are more descriptive
Description
DEV-17057
We want to change the labeling in our Scheduled report for when to send the report.
fixes #21118
Checklist
Review