Skip to content

Conversation

gabrieldutra
Copy link
Member

@gabrieldutra gabrieldutra 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
Copy link
Member Author

gabrieldutra 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
Copy link
Contributor

ranbena commented Jun 24, 2019

Great to see it in action :)

@gabrieldutra gabrieldutra force-pushed the parameter-dynamic-date-values branch from 471c595 to 7efcd39 Compare June 27, 2019 16:01
@gabrieldutra
Copy link
Member Author

gabrieldutra 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
Copy link
Contributor

ranbena 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.

Copy link
Member Author

@gabrieldutra gabrieldutra left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

ranbena 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
Copy link
Member Author

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
Copy link
Member Author

gabrieldutra 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
Copy link
Contributor

ranbena 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 Compare July 22, 2019 17:31
@arikfr
Copy link
Member

arikfr 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
Copy link
Member Author

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
Copy link
Member

arikfr 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
Copy link
Contributor

ranbena 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
Copy link
Member Author

gabrieldutra 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
Copy link
Member

arikfr 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
Copy link
Member Author

Let's do it in a separate PR then.

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

@gabrieldutra
Copy link
Member Author

gabrieldutra 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 :)