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 bug with emailing custom enterprise report date range #35204

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

mjriley
Copy link
Contributor

@mjriley mjriley commented Oct 10, 2024

Technical Summary

Bug was included in #35166, where any customized data range was ignored for actually generating the CSV download. This fix passes the custom date parameters on to the download generation task.

Feature Flag

Safety Assurance

Safety story

Verified the fix locally and on staging.

Automated test coverage

No tests.

QA Plan

No QA

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@mjriley mjriley requested review from biyeun and orangejenny October 10, 2024 16:15
Copy link
Contributor

@millerdev millerdev left a comment

Choose a reason for hiding this comment

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

Did you think about writing a test to reproduce the bug? A follow-up PR would be fine if it's urgent to get this deployed.

@mjriley
Copy link
Contributor Author

mjriley commented Oct 10, 2024

What test would you want to see here? The test that comes to mind is just verifying that the parameters get passed to the celery task? Or are you thinking of an end-to-end test where I verify the output? I do want to get this merged, but I'm not opposed to writing a test to verify this behavior if there's a good one to write

@millerdev
Copy link
Contributor

I wasn't thinking of an end-to-end test. Verifying that the correct kwargs are passed to celery seems adequate.

@mjriley
Copy link
Contributor Author

mjriley commented Oct 10, 2024

Sure, I will handle that in a follow up PR, thanks!

@mjriley mjriley merged commit d38aa29 into master Oct 10, 2024
13 checks passed
@mjriley mjriley deleted the mjr/enterprise-date-range-fix branch October 10, 2024 16:35
@mjriley
Copy link
Contributor Author

mjriley commented Oct 10, 2024

Sure, I will handle that in a follow up PR, thanks!

Follow-up PR: #35205

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