-
-
Notifications
You must be signed in to change notification settings - Fork 504
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
Add more date range selection options #4518 #4533
Add more date range selection options #4518 #4533
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.
Checks out functionally. @dorner - do you want to take a look from the technical pov?
end | ||
expect(page).to have_text("25", count: 4) | ||
expect(page).to have_text("$262.50", count: 4) | ||
end |
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.
Aren't these tests identical? Ideally to test this we'd want to have a distribution that is included and one that's not included, specifically for these 2 blocks of time.
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.
Yeah.... I was curious about those expects
. I will dig in a bit deeper to see what I can figure out.
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.
Sorry! I haven't forgotten about this.... Work has picked up a bit so been eating up a lot of my time at the moment!
Sorry, it looks like work may continue to take up most of my time for the next few weeks. If someone wants to pick this up and take it across the finish line, that is totally fine. Otherwise, I can come back to it once work settles back down again. The only remaining task left to do is fix these two tests: |
@pshong79 I can help wrap this up. Fixed those two leftover tests so it should be ready for review again! |
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.
Looks good!
@pshong79: Your PR |
Resolves #4518
Description
This change is to add more date range selections through the application - specifically "Prior Year" and "Last 12 Months".
"Prior Year" is from January 1 through December 31 of the previous year.
"Last 12 Months" is from the day after today of the previous year through today. For example, if today is July 14, 2024, the range should be July 15, 2023 to July 14, 2024.
Type of change
How Has This Been Tested?
Tested manually
Add a couple of specs in
distribution_by_county_system_spec
for "Prior Year" and "Last 12 Months" date range selections.Screenshots
Before:
After:
(Last 12 Months)
Note: Today's date is July 14, 2024.
(Prior Year)