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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Dynamic Values to Date and Date Range Parameters #3904

Merged
merged 47 commits into from Jul 26, 2019

Conversation

@gabrieldutra
Copy link
Member

commented Jun 16, 2019

What type of PR is this? (check all applicable)

  • Feature

Description

Allow Dynamic Values to Date and Date Range Parameters 馃帀.

See preview below for details :)

Related Tickets & Documents

Closes #3009

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

dynamic-values-apply-all

@gabrieldutra

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

Idea of implementation updated considering Arik's comment.

  • Dynamic Values are just like regular values, but with a standard syntax (current strings with "d_" prefix);
  • There will be Date Range Dynamic options and Date Dynamic options. Date Ranges can be composed by a static or dynamic date values or have a Dynamic Date Range. This will make some more specific cases possible;
  • Dynamic Date Values can hold a mapped string (e.g: "d_yesterday") or a "delta" datetime number (e.g: "d_-[7days in ms]"). This will cover cases where you want custom values and cases where the date cannot be expressed as deltas (e.g: last month);

Currently applied only for DateRange for comments.

A possible refactor that I was thinking is that at some point parameters can be rewritten taking advantage of inheritance (should reduce complexity for the methods like getValue, setValue, toUrlParams as each parameter type will have it's own logic)

@getredash getredash deleted a comment from cypress bot Jun 17, 2019

@getredash getredash deleted a comment from cypress bot Jun 18, 2019

@getredash getredash deleted a comment from cypress bot Jun 23, 2019

@ranbena

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Great to see it in action :)

@gabrieldutra gabrieldutra force-pushed the parameter-dynamic-date-values branch from 471c595 to 7efcd39 Jun 27, 2019

gabrieldutra added some commits Jun 27, 2019

@gabrieldutra

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

@ranbena @arikfr finally I could sit down and work on this 馃檪, the fist part of the feature already has its shape:

dynamic-dates-1 (preview)

I'm pretty happy with the UX/UI result :)

Currently the Dynamic information is saved in the Parameter value as a string (e.g: "d_last_week"), can there be an issue backend-wise with queries saved with it? (it seemed right to put it there, but I can move it elsewhere if so).

The next is to implement a "Custom option" dialog (should make this reach all of its potential) and then it's a matter of final touches.

One note is that I reverted the DateTime Parameter apply only onOk or close thing so this one would be easier (I think it won't be a problem considering #3907).

@ranbena

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

I'm pretty happy with the UX/UI result :)

Me too 馃憤

  1. Some UI refinements for size and pos:

    .dynamic-button {
      height: 100%;
    
      &:after {
        height: 19px;
      }
    
     .ant-dropdown-trigger {
        height: 100%;
      }
    }
  2. I think, if it's possible, to get the "clear" button in there for both static and dynamic selected values (requiring an x offset).

    Screen Shot 2019-06-29 at 17 08 49
    • If so, the "Static value" option can be omitted.
    • If not, I think moving it to last is better. And some other suggested changes (selected state, divider, back button, lighter text color)

    ` Screen Shot 2019-06-29 at 16 30 24

Currently the Dynamic information is saved in the Parameter value as a string.

Probably need to make scheduled queries convert dynamic to static values upon execution (backend).

The next is to implement a "Custom option" dialog (should make this reach all of its potential) and then it's a matter of final touches.

You sure it's a must? I would abstain from it at this point.

@gabrieldutra
Copy link
Member Author

left a comment

Regarding the Custom Option: It's indeed not a must, but I feel like half of this feature is incomplete without it as all options like "Last X days" could be done there, thus avoiding overload on the dropdown.

This can be delivered separately, if the custom option happens to be worth it, perhaps open a Part 2 to make review a bit easier and explore it without compromising this one?

@ranbena

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

The clear button was actually fine in this implementation, but there was a different issue: when there's no value the clear button is not rendered and since we need the placeholder there... There was probably a way to workaround that with css, but I preferred to go with option 2.

But that does mean that there's no way to clear a static value, no?
Perhaps a compromise with allowClear={!this.state.dynamicValue}?

@gabrieldutra

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

But that does mean that there's no way to clear a static value, no?

I wasn't considering this as required 馃槄, anyway I updated implementation and put the clear button back in there :)

@gabrieldutra

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

@gabrieldutra one thing I didn't test is "Refresh Schedule". If you're confident about it - go ahead and merge

Well reminded. I guess so far the pending discussion points in this PR are the "Refresh Schedule" and the size changing for Dynamic Date Ranges.

Refresh Schedule
@arikfr mentioned that in the issue, it's a point where we will decide whether it's valuable to replicate the frontend logic in the backend (which is a bit different as in frontend we can use the user's time). From what I understood, Refresh Schedule for Parameterized queries can't be used on Alerts, so it's purpose is to keep results updated when you open the page.

Also, when trying to work on that part in #3952, I couldn't get the Refresh Schedule to work (used Python debugger and logs to track).

Changing size for Date Ranges

Not a blocker, but I noticed that date range changes size when switching between dynamic and static. How about we keep it same size and show the dates along with the range name when it's dynamic?

It's possible, different styling can be done by using the two inputs placeholders. One con is that it will differ from Date Parameters (also Date Ranges are very wide 馃槄). @ranbena any quick ideas for this? So far I'm ok with the changing size thing for styling purposes

image

@ranbena

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

any quick ideas for this? So far I'm ok with the changing size thing for styling purposes

I don't think we can justify this huge component with dynamic values.
How about a transition to soften it up?

@gabrieldutra gabrieldutra force-pushed the parameter-dynamic-date-values branch from 5bd403d to 34ee589 Jul 22, 2019

@arikfr

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

I don't think we can justify this huge component with dynamic values.

In your example it's huge because it's a date range with time. But I wonder if date range with time needs dynamic values? All the dynamic values are date based, so maybe it should be only for date range and date picker? Then the size delta isn't that big.

Refresh Schedule

The timezone issue is indeed tricky in this case :-| With dates only it's less an issue, but still can manifest on day changes. Not sure what to do about this, we can save the current timezone when saving the dynamic values but that's not great.

We can decide that there is no scheduling for such queries. Although this opens its own Pandora's box -- what if the query is already scheduled, etc.

@gabrieldutra

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

so maybe it should be only for date range and date picker

I don't think we should limit the scope of the feature, from a "fixed" value standpoint, yes, it makes no sense for Time pickers (except for the "Now" one). However I think it's convenient to have the Dynamic Values available: you can have a dashboard with a Date Range Time specified and if you want to check out the result for "Last week" you are just a few clicks away.

@arikfr

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

@gabrieldutra fair points about keeping it for all types 馃憤

So what do you think about the option of disabling scheduled queries?

@ranbena

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

so maybe it should be only for date range and date picker

I don't think we should limit the scope of the feature, from a "fixed" value standpoint, yes, it makes no sense for Time pickers (except for the "Now" one). However I think it's convenient to have the Dynamic Values available: you can have a dashboard with a Date Range Time specified and if you want to check out the result for "Last week" you are just a few clicks away.

That's my thinking as well.
Moreover, I'm not certain users would necessarily pick up on the logic behind the differentiation, which might lead to frustration.

@gabrieldutra

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

So what do you think about the option of disabling scheduled queries?

I got a feeling that scheduled queries are not working for queries with parameters, which makes "what if the query is already scheduled" not an issue.

But I agree with the "disabling scheduled" for such queries, in backend we can actually use the existing validation to skip execution (d_values are not valid dates, thus the query is not valid) and in the UI we can disable the option with a tooltip.

My points are:

  • Currently we don't have much usage for Refresh Schedule for queries with parameters (alerts don't work with them);
  • it's just a matter of changing the default value for the parameter;
  • It's the easiest option and we can see how this goes over time 馃檪
@arikfr

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

I got a feeling that scheduled queries are not working for queries with parameters, which makes "what if the query is already scheduled" not an issue.

馃 I just checked and it does seem like it has no effect / not working. I will check this out.

But I agree with the "disabling scheduled" for such queries

Let's do it in a separate PR then.

@gabrieldutra

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

Let's do it in a separate PR then.

I'll wrap this with the transition and fix Cypress tests then

@gabrieldutra

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

@ranbena, I've updated this with the transition + a fix to a bug (Date Ranges with no previous value + click on "Back to static value" was resulting in moment(null.start)). If you wanna check those :)

CR
@gabrieldutra gabrieldutra referenced this pull request Jul 24, 2019
12 of 12 tasks complete
@gabrieldutra

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

@arikfr I guess this one is on you now, whenever you think it's convenient :)

@arikfr arikfr merged commit cd4daf8 into master Jul 26, 2019

14 of 18 checks passed

percy/redash 8 visual changes need review
Details
codeclimate 4 issues to fix
Details
Header rules No header rules processed
Details
Pages changed 7 new files uploaded
Details
Datree Smart Policy Best Practices Verification
Details
Datree insights datreeio insights events
Details
Mixed content No mixed content detected
Details
Redirect rules 7 redirect rules processed
Details
WIP Ready for review
Details
build Workflow: build
Details
ci/circleci: backend-unit-tests Your tests passed on CircleCI!
Details
ci/circleci: frontend-e2e-tests Your tests passed on CircleCI!
Details
ci/circleci: frontend-lint Your tests passed on CircleCI!
Details
ci/circleci: frontend-unit-tests Your tests passed on CircleCI!
Details
ci/circleci: legacy-python-flake8-tests Your tests passed on CircleCI!
Details
ci/circleci: python-flake8-tests Your tests passed on CircleCI!
Details
cypress: default-group 88 tests passed in 09:52
Details
deploy/netlify Deploy preview ready!
Details

@arikfr arikfr deleted the parameter-dynamic-date-values branch Jul 26, 2019

@arikfr

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

馃憤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.