-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
feat: add Current time-range options for time filter #28637
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.
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.
Thanks for adding this! I think it generally should be an uncontroversial addition and provide something that lots of people have asked for. My initial thoughts:
- Looks like it is failing CI, please investigate the failed checks and address. Ping me if you need me to re-approve the checks to run. I see at least
prettier
needs to be run to format the code. - To clarify, I don't think this addresses the SIP-133 discussion you linked which is fine. It does address a common feature request: Feature request: Time filter range type of "Current" #21804. Please read through that discussion for context. Could you also add
Current Day
andCurrent Quarter
per my request there? This would give it parity with the other time types. - Suggestion: the "current" time ranges ends at
DATETRUNC(DATETIME('today'), DAY)
, should they instead continue through a full day/week/month/quarter/year? E.g., for current week would endDATETRUNC(DATEADD(DATETIME('today'), 1, WEEK), WEEK)
, while current day would end atDATETRUNC(DATEADD(DATETIME('today'), 1, DAY), DAY)
etc.
also related to #28626 (I hadn't found the other feature request when I created mine). |
Looking at the AFTER screenshot, it looks like current calendar week should be from |
I agree with @michael-s-molina, I have edited my comment above to align with this suggestion. |
…"Current Week" range, fix CI formatting, address apache#21804
Hey @sfirke @michael-s-molina , I have made all the changes you mentioned. Could you please review it now? Thanks! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #28637 +/- ##
==========================================
+ Coverage 60.48% 70.28% +9.79%
==========================================
Files 1931 1946 +15
Lines 76236 77531 +1295
Branches 8568 8736 +168
==========================================
+ Hits 46114 54489 +8375
+ Misses 28017 20911 -7106
- Partials 2105 2131 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
All checks passed. Please review and merge if possible. Thanks! |
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.
Looking good to me! I had one comment about a line whose formatting didn't match the surrounding. Otherwise I'm good with this, though let's see if we can get a review from someone who knows this code.
I see Codecov is warning about lines not being covered by tests, should that be addressed?
superset-frontend/src/explore/components/controls/DateFilterControl/utils/constants.ts
Outdated
Show resolved
Hide resolved
@betodealmeida or @villebro is one of you able to review this? I approved, the functionality looks good to me and it's a nice addition that's passing all tests. But I'd like someone who can evaluate the code quality + style and determine if the Codecov warnings are relevant. |
/testenv up |
@rusackas Ephemeral environment spinning up at http://18.236.146.162:8080. Credentials are |
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.
I played around with this on the ephemeral and it works as it should. I will vouch for the logic being correct and the feature needed; can another contributor approve + merge after taking a look at the code quality/style/coverage? This would be a nice win to ship in 4.1.0 and should not linger, given that it works great and is in high demand.
Agreed this is worth shipping as-is, and I'm tempted to merge it, BUT I need to ask the obvious question: @pranav1699 would you be able/willing to add any tests? I know these time ranges are not particularly well tested, but maybe this is a good excuse to add a few tests that make sure it's calculating the time ranges as expected. This would help affirm expectations around time ranges as discussed/fixed in the thread above. |
@rusackas I've added Python unit tests for the current time range. Thanks for suggesting it. |
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.
Thanks, this looks great!
...ontend/src/explore/components/controls/DateFilterControl/tests/CurrentCalendarFrame.test.tsx
Show resolved
Hide resolved
…ntrol/tests/CurrentCalendarFrame.test.tsx Adding ASF license boilerplate to new test file
Running CI 🤞 |
Ephemeral environment shutdown and build artifacts deleted. |
Thanks again for this @pranav1699! I hope you will consider future contributions to Superset, I appreciated how thorough your PR code was and your responsiveness 🙏 |
You're welcome! I'm glad you found my contributions helpful. Thank you for reviewing the changes and all. I'll definitely keep Superset on my radar for future contributions. |
Co-authored-by: Evan Rusackas <[email protected]>
Co-authored-by: Evan Rusackas <[email protected]>
SUMMARY
This pull request introduces a new "Current" option to the time filter component. The "Current" option allows users to select and view data for the current week, current month, and current year.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After :
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION