fix(explore): time picker can not be switched between now and specific #12793

zhaoyongjie · 2021-01-27T14:39:51Z

SUMMARY

fix time picker can not be switched between "now" and "specific".
closes: #12780

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before

Screen.Recording.2021-01-26.at.2.07.48.PM.mov

fixed

Jan-27-2021.22-37-25.mp4

TEST PLAN

open new timepicker
switch to custom frame
select "now"(or "midnight") for start(or "end")
able to switch to "specific date/time"

ADDITIONAL INFORMATION

Has associated issue: [explore]time picker switch from NOW to Specific time range #12780
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

zhaoyongjie · 2021-01-27T14:41:33Z

superset-frontend/src/explore/components/controls/DateFilterControl/frame/CustomFrame.tsx

 import {
  CustomRangeKey,
  SelectOptionType,
  FrameComponentProps,
 } from '../types';

-const dttmToMoment = (dttm: string): Moment => {


move these to utils.js

zhaoyongjie · 2021-01-27T14:48:09Z

superset-frontend/src/explore/components/controls/DateFilterControl/utils.ts

@@ -183,31 +197,27 @@ export const customTimeRangeEncode = (customRange: CustomRangeType): string => {
  } = { ...customRange };
  // specific : specific
  if (SPECIFIC_MODE.includes(sinceMode) && SPECIFIC_MODE.includes(untilMode)) {
-    const since = sinceMode === 'specific' ? sinceDatetime : sinceMode;
-    const until = untilMode === 'specific' ? untilDatetime : untilMode;
+    const since = sinceMode === 'specific' ? dttmToString(sinceDatetime) : sinceMode; // eslint-disable-line


from "now/midnight" switch to "specific", "since" requires materialization from "now" to "YYYY-mm-ddThh:mm:ss".

adam-stasiak · 2021-01-27T15:58:09Z

Tested manually works fine 🟢
Tested on chrome,safari,firefox - ok

zhaoyongjie · 2021-01-27T16:22:00Z

Thanks for the quick testing

…

On Wed, Jan 27, 2021 at 11:58 PM adam-stasiak-polidea < ***@***.***> wrote: Tested manually works fine 🟢 Tested on chrome,safari,firefox - ok — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#12793 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPMKUTF2DIZNQNTVRU6KJTS4AZ2HANCNFSM4WVL4BOQ> .

codecov-io · 2021-01-27T16:44:27Z

Codecov Report

Merging #12793 (a67906d) into master (34549cb) will increase coverage by 0.13%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master   #12793      +/-   ##
==========================================
+ Coverage   66.86%   67.00%   +0.13%     
==========================================
  Files        1022     1021       -1     
  Lines       50107    50095      -12     
  Branches     5193     5190       -3     
==========================================
+ Hits        33506    33568      +62     
+ Misses      16470    16393      -77     
- Partials      131      134       +3

Flag	Coverage Δ
cypress	`50.86% <16.66%> (-0.05%)`	⬇️
javascript	`62.00% <86.66%> (+0.26%)`	⬆️
python	`63.97% <ø> (+0.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ore/components/controls/DateFilterControl/types.ts	`100.00% <ø> (ø)`
...ore/components/controls/DateFilterControl/utils.ts	`93.58% <85.71%> (+45.76%)`	⬆️
...s/controls/DateFilterControl/frame/CustomFrame.tsx	`40.81% <100.00%> (+1.53%)`	⬆️
superset/db_engine_specs/presto.py	`82.25% <0.00%> (-6.28%)`	⬇️
...frontend/src/explore/components/OptionControls.tsx	`74.07% <0.00%> (-2.12%)`	⬇️
...set-frontend/src/dashboard/util/getDropPosition.js	`92.06% <0.00%> (-1.59%)`	⬇️
...tend/src/dashboard/components/DashboardBuilder.jsx	`85.89% <0.00%> (-0.53%)`	⬇️
superset/models/core.py	`88.58% <0.00%> (ø)`
superset-frontend/src/explore/App.jsx	`100.00% <0.00%> (ø)`
superset-frontend/src/dashboard/App.jsx	`100.00% <0.00%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34549cb...a67906d. Read the comment docs.

graceguo-supercat

LGTM

etr2460 · 2021-01-27T17:12:49Z

superset-frontend/src/explore/components/controls/DateFilterControl/utils.ts

@@ -183,31 +197,27 @@ export const customTimeRangeEncode = (customRange: CustomRangeType): string => {
  } = { ...customRange };
  // specific : specific
  if (SPECIFIC_MODE.includes(sinceMode) && SPECIFIC_MODE.includes(untilMode)) {
-    const since = sinceMode === 'specific' ? sinceDatetime : sinceMode;
-    const until = untilMode === 'specific' ? untilDatetime : untilMode;
+    const since = sinceMode === 'specific' ? dttmToString(sinceDatetime) : sinceMode; // eslint-disable-line


Why do all of these lines have eslint-disable-line on them? Can we either not introduce linting errors, or only disable the specific rule that's failing here?

I guess this comes down to personal preference (these lines are easier to read as one-liners, but the linter wants to break them up). I've personally come to appreciate uniformity over legibility. I'd vote for removing the eslint-disable-lines here and fixing the linting errors.

oh yeah, if it's only formatting, then 100% let's follow the linter. @zhaoyongjie mind updating?

glad to follow to linting rules and make the change, however, I still believe breaking the lines is unnecessary. It makes it harder to read this code, especially in the case where we want to compare since and until side by side.

but I am still willing to follow everyone's conventions.

⇓ disable linting

... // specific : specific if (SPECIFIC_MODE.includes(sinceMode) && SPECIFIC_MODE.includes(untilMode)) { const since = sinceMode === 'specific' ? dttmToString(sinceDatetime) : sinceMode; // eslint-disable-line const until = untilMode === 'specific' ? dttmToString(untilDatetime) : untilMode; // eslint-disable-line return `${since} : ${until}`; } // specific : relative if (SPECIFIC_MODE.includes(sinceMode) && untilMode === 'relative') { const since = sinceMode === 'specific' ? dttmToString(sinceDatetime) : sinceMode; // eslint-disable-line const until = `DATEADD(DATETIME("${since}"), ${untilGrainValue}, ${untilGrain})`; // eslint-disable-line return `${since} : ${until}`; } // relative : specific if (sinceMode === 'relative' && SPECIFIC_MODE.includes(untilMode)) { const until = untilMode === 'specific' ? dttmToString(untilDatetime) : untilMode; // eslint-disable-line const since = `DATEADD(DATETIME("${until}"), ${-Math.abs(sinceGrainValue)}, ${sinceGrain})`; // eslint-disable-line return `${since} : ${until}`; } // relative : relative const since = `DATEADD(DATETIME("${anchorValue}"), ${-Math.abs(sinceGrainValue)}, ${sinceGrain})`; // eslint-disable-line const until = `DATEADD(DATETIME("${anchorValue}"), ${untilGrainValue}, ${untilGrain})`; return `${since} : ${until}`; ...

⇓ enable linting

... // specific : specific if (SPECIFIC_MODE.includes(sinceMode) && SPECIFIC_MODE.includes(untilMode)) { const since = sinceMode === 'specific' ? dttmToString(sinceDatetime) : sinceMode; const until = untilMode === 'specific' ? dttmToString(untilDatetime) : untilMode; return `${since} : ${until}`; } // specific : relative if (SPECIFIC_MODE.includes(sinceMode) && untilMode === 'relative') { const since = sinceMode === 'specific' ? dttmToString(sinceDatetime) : sinceMode; const until = `DATEADD(DATETIME("${since}"), ${untilGrainValue}, ${untilGrain})`; return `${since} : ${until}`; } // relative : specific if (sinceMode === 'relative' && SPECIFIC_MODE.includes(untilMode)) { const until = untilMode === 'specific' ? dttmToString(untilDatetime) : untilMode; const since = `DATEADD(DATETIME("${until}"), ${-Math.abs( sinceGrainValue, )}, ${sinceGrain})`; return `${since} : ${until}`; } // relative : relative const since = `DATEADD(DATETIME("${anchorValue}"), ${-Math.abs( sinceGrainValue, )}, ${sinceGrain})`; const until = `DATEADD(DATETIME("${anchorValue}"), ${untilGrainValue}, ${untilGrain})`; return `${since} : ${until}`; ...

export const customTimeRangeEncode = (customRange: CustomRangeType): string => { const { sinceDatetime, sinceMode, sinceGrain, sinceGrainValue, untilDatetime, untilMode, untilGrain, untilGrainValue, anchorValue, } = { ...customRange }; let since: string = sinceMode; let until: string = untilMode; if (since === 'specific') { since = dttmToString(sinceDatetime); } if (until === 'specific') { until = dttmToString(untilDatetime); } if (since === 'relative' && until === 'relative') { since = `DATEADD(DATETIME("${anchorValue}"), ${-Math.abs( sinceGrainValue, )}, ${sinceGrain})`; until = `DATEADD(DATETIME("${anchorValue}"), ${untilGrainValue}, ${untilGrain})`; } if (since === 'relative') { since = `DATEADD(DATETIME("${until}"), ${-Math.abs( sinceGrainValue, )}, ${sinceGrain})`; } if (until === 'relative') { until = `DATEADD(DATETIME("${since}"), ${untilGrainValue}, ${untilGrain})`; } return `${since} : ${until}`; };

@zhaoyongjie would this be cleaner? It passes all your test cases.

villebro

LGTM, but I'd prefer to reformat according to the linter's preference.

junlincc

approve as product/design/QA sign off.

tried switching between all different settings ✅
apologize that we didn't catch it earlier ..

zhaoyongjie · 2021-01-28T02:46:18Z

LGTM, but I'd prefer to reformat according to the linter's preference.

I removed eslint-disable-line and fix this linting issue.

ktmud · 2021-01-28T04:25:01Z

superset-frontend/src/explore/components/controls/DateFilterControl/utils.ts

+
+export const dttmToString = (dttm: string): string =>
+  dttmToMoment(dttm).format(MOMENT_FORMAT);
+
 export const customTimeRangeDecode = (


Style nit: can we break utils.ts into separate files under a utils folder? These functions are complex enough to warrant a separate file.

ktmud · 2021-01-28T04:31:20Z

superset-frontend/src/explore/components/controls/DateFilterControl/utils.ts

@@ -183,31 +197,27 @@ export const customTimeRangeEncode = (customRange: CustomRangeType): string => {
  } = { ...customRange };
  // specific : specific
  if (SPECIFIC_MODE.includes(sinceMode) && SPECIFIC_MODE.includes(untilMode)) {
-    const since = sinceMode === 'specific' ? sinceDatetime : sinceMode;
-    const until = untilMode === 'specific' ? untilDatetime : untilMode;
+    const since = sinceMode === 'specific' ? dttmToString(sinceDatetime) : sinceMode; // eslint-disable-line


export const customTimeRangeEncode = (customRange: CustomRangeType): string => { const { sinceDatetime, sinceMode, sinceGrain, sinceGrainValue, untilDatetime, untilMode, untilGrain, untilGrainValue, anchorValue, } = { ...customRange }; let since: string = sinceMode; let until: string = untilMode; if (since === 'specific') { since = dttmToString(sinceDatetime); } if (until === 'specific') { until = dttmToString(untilDatetime); } if (since === 'relative' && until === 'relative') { since = `DATEADD(DATETIME("${anchorValue}"), ${-Math.abs( sinceGrainValue, )}, ${sinceGrain})`; until = `DATEADD(DATETIME("${anchorValue}"), ${untilGrainValue}, ${untilGrain})`; } if (since === 'relative') { since = `DATEADD(DATETIME("${until}"), ${-Math.abs( sinceGrainValue, )}, ${sinceGrain})`; } if (until === 'relative') { until = `DATEADD(DATETIME("${since}"), ${untilGrainValue}, ${untilGrain})`; } return `${since} : ${until}`; };

@zhaoyongjie would this be cleaner? It passes all your test cases.

ktmud · 2021-01-28T04:35:06Z

BTW, I found this line:

superset/superset-frontend/src/explore/components/controls/DateFilterControl/types.ts

Line 60 in 6731cd0

untilMode: 'specific' | 'relative' | 'now' | 'today';

could probably be updated to

  untilMode: DateTimeModeType;

ktmud · 2021-01-28T05:06:42Z

superset-frontend/spec/javascripts/explore/components/DateFilterControl/utils_spec.ts

+        ),
+      ).toEqual({
+        customRange: {
+          sinceDatetime: '2021-01-20T00:00:00',


I'm not sure I fully understand the purpose of the customTimeRangeDecode function. It is supposed to apply the dateadd and datetime functions? However, according to the code, this sinceDatetime seems to always return 7_DAYS_AGO when the input format is relative : specific.

Then shouldn't sinceDatetime be set to 2021-01-27T00:00:00 here, just to be more consistent with what is set in the input values?

Do you also do decode/encode when switching between non-advanced modes (i.e. modes that doesn't allow free text)? Not sure how feasible it is, but gut feeling tells me that it might be easier and cleaner to use he same config object internally for all frames and only do decode/encode when the time picker needs to communicate with the outside world.

customTimeRangeEncode is used only in the CustomFrame.

customTimeRangeDecode is used in 2 places: CustomFrame and guessFrame function in DateFilterControl.

customTimeRangeDecode can determine whether an expression can be used in CustomFrame.

sinceDatetime set to 2021-01-27T00:00:00 just to be more consistent in CustomFrame.

Thanks for the explanation.

zhaoyongjie · 2021-01-28T07:28:59Z

BTW, I found this line:

superset/superset-frontend/src/explore/components/controls/DateFilterControl/types.ts

Line 60 in 6731cd0

untilMode: 'specific' | 'relative' | 'now' | 'today';

could probably be updated to
  untilMode: DateTimeModeType;

thanks for pointing this out. I will fix it.

etr2460

thanks for the updates, lgtm!

#12793) * fix(explore): time picker can not be switched between now and specifc * fix linting * fix type * fix UT

apache#12793) * fix(explore): time picker can not be switched between now and specifc * fix linting * fix type * fix UT (cherry picked from commit 642a76f)

pull-request-size bot added the size/L label Jan 27, 2021

zhaoyongjie commented Jan 27, 2021

View reviewed changes

zhaoyongjie requested review from ktmud, junlincc and villebro January 27, 2021 14:49

junlincc added the explore:time Related to the time filters in Explore label Jan 27, 2021

graceguo-supercat approved these changes Jan 27, 2021

View reviewed changes

etr2460 reviewed Jan 27, 2021

View reviewed changes

villebro approved these changes Jan 27, 2021

View reviewed changes

junlincc approved these changes Jan 28, 2021

View reviewed changes

ktmud reviewed Jan 28, 2021

View reviewed changes

junlincc added the v1.0.1 label Jan 28, 2021

zhaoyongjie added 3 commits January 29, 2021 09:26

fix(explore): time picker can not be switched between now and specifc

97e2b18

fix linting

79627d7

fix type

8142a86

zhaoyongjie force-pushed the fix_switch_now_specific branch from 152f7c2 to 8142a86 Compare January 29, 2021 01:27

fix UT

a67906d

etr2460 approved these changes Jan 29, 2021

View reviewed changes

zhaoyongjie merged commit 642a76f into apache:master Jan 29, 2021

villebro pushed a commit that referenced this pull request Jan 29, 2021

fix(explore): time picker can not be switched between now and specific (

926fdce

#12793) * fix(explore): time picker can not be switched between now and specifc * fix linting * fix type * fix UT

mistercrunch added the 🍒 1.0.1 label Mar 12, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(explore): time picker can not be switched between now and specific #12793

fix(explore): time picker can not be switched between now and specific #12793

zhaoyongjie commented Jan 27, 2021 •

edited

Loading

zhaoyongjie Jan 27, 2021

zhaoyongjie Jan 27, 2021

adam-stasiak commented Jan 27, 2021

zhaoyongjie commented Jan 27, 2021 via email

codecov-io commented Jan 27, 2021 •

edited

Loading

graceguo-supercat left a comment

etr2460 Jan 27, 2021

villebro Jan 27, 2021

etr2460 Jan 27, 2021

zhaoyongjie Jan 28, 2021 •

edited by ktmud

Loading

ktmud Jan 28, 2021

villebro left a comment

junlincc left a comment

zhaoyongjie commented Jan 28, 2021

ktmud Jan 28, 2021

ktmud Jan 28, 2021

ktmud commented Jan 28, 2021

ktmud Jan 28, 2021

zhaoyongjie Jan 28, 2021 •

edited

Loading

ktmud Jan 28, 2021

zhaoyongjie Jan 28, 2021

zhaoyongjie Jan 28, 2021

ktmud Jan 28, 2021

zhaoyongjie commented Jan 28, 2021

etr2460 left a comment

fix(explore): time picker can not be switched between now and specific #12793

fix(explore): time picker can not be switched between now and specific #12793

Conversation

zhaoyongjie commented Jan 27, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before

fixed

TEST PLAN

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-stasiak commented Jan 27, 2021

zhaoyongjie commented Jan 27, 2021 via email

codecov-io commented Jan 27, 2021 • edited Loading

Codecov Report

graceguo-supercat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaoyongjie Jan 28, 2021 • edited by ktmud Loading

Choose a reason for hiding this comment

⇓ disable linting

⇓ enable linting

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

junlincc left a comment

Choose a reason for hiding this comment

zhaoyongjie commented Jan 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud commented Jan 28, 2021

Choose a reason for hiding this comment

zhaoyongjie Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaoyongjie commented Jan 28, 2021

etr2460 left a comment

Choose a reason for hiding this comment

zhaoyongjie commented Jan 27, 2021 •

edited

Loading

codecov-io commented Jan 27, 2021 •

edited

Loading

zhaoyongjie Jan 28, 2021 •

edited by ktmud

Loading

zhaoyongjie Jan 28, 2021 •

edited

Loading