Skip to content

Schedule weekly reports to run Monday for the previous week#10805

Merged
h-m-m merged 2 commits intomainfrom
hmm/weekly-report-timing-fix
Jun 18, 2024
Merged

Schedule weekly reports to run Monday for the previous week#10805
h-m-m merged 2 commits intomainfrom
hmm/weekly-report-timing-fix

Conversation

@h-m-m
Copy link
Contributor

@h-m-m h-m-m commented Jun 12, 2024

🎫 Ticket

Link to the relevant ticket:
LG-13559

🛠 Summary of changes

We're missing data for Sunday in the weekly report because the report runs Sunday morning. This delays the report until Monday and then runs it for the previous week.

📜 Testing Plan

We can confirm in staging that this change schedules the expected report

@h-m-m h-m-m requested review from Jeremy1026, ajfarkas and nprimak June 12, 2024 13:27
@h-m-m h-m-m marked this pull request as ready for review June 12, 2024 14:12
@h-m-m h-m-m force-pushed the hmm/weekly-report-timing-fix branch 4 times, most recently from 2a83977 to d1008e4 Compare June 12, 2024 14:26
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add a freeze_time around these specs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zachmargolis Do we do this automatically because of:

config.around(:each, freeze_time: true) do |example|
freeze_time { example.run }
end

Copy link
Contributor

Choose a reason for hiding this comment

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

That only applies to specs/groups with a freeze_time: true tag, I didn't see that here?

@h-m-m h-m-m force-pushed the hmm/weekly-report-timing-fix branch 3 times, most recently from 20c19ec to 42560ec Compare June 14, 2024 14:25
@h-m-m h-m-m requested a review from zachmargolis June 14, 2024 14:44
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM but I would definitely prefer an explicit freeze_time before merging

@h-m-m h-m-m force-pushed the hmm/weekly-report-timing-fix branch from 42560ec to 3aafb6e Compare June 14, 2024 16:53
@h-m-m
Copy link
Contributor Author

h-m-m commented Jun 14, 2024

LGTM but I would definitely prefer an explicit freeze_time before merging

Sure, that's reasonable and understandable. Updated

@zachmargolis
Copy link
Contributor

Looks like this is failing in CI due to our commit message changelog requirement (logs link)

A valid changelog line was not found.
A commit message should contain a line in the form of:
changelog: CATEGORY, SUBCATEGORY, CHANGE_DESCRIPTION
example:
changelog: User-Facing Improvements, WebAuthn, Improve error flow for WebAuthn (LG-5515)
categories:
- User-Facing Improvements
- Bug Fixes
- Internal
- Upcoming Features
Include "[skip changelog]" in a commit message to bypass this check.
Note: the changelog message must be separated from any other commit message by a blank line.

@h-m-m h-m-m force-pushed the hmm/weekly-report-timing-fix branch from 3aafb6e to dccbf45 Compare June 17, 2024 21:40
@h-m-m h-m-m force-pushed the hmm/weekly-report-timing-fix branch from dccbf45 to 0bc6f37 Compare June 17, 2024 21:47
@h-m-m
Copy link
Contributor Author

h-m-m commented Jun 17, 2024

Looks like this is failing in CI due to our commit message changelog requirement (logs link)

Thanks for the handy link. Fixed ✔️

h-m-m added 2 commits June 18, 2024 10:21
Our jobs use the default Rails definition of a week starting on a Monday. I think this is the ISO standard. Running a job on Monday and giving it the argument `Time.zone.yesterday` means that weekly jobs will process data for the previous week

changelog: Internal, Reporting, Move weekly reports to run at 00:00 Monday UTC
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

changelog: Internal, Reporting, Move weekly reports to run at 00:00 Monday UTC
@h-m-m h-m-m force-pushed the hmm/weekly-report-timing-fix branch from 0bc6f37 to 452fe40 Compare June 18, 2024 14:21
@h-m-m h-m-m merged commit 030a4da into main Jun 18, 2024
@h-m-m h-m-m deleted the hmm/weekly-report-timing-fix branch June 18, 2024 20:11
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.

3 participants