Add Dynamic Values to Date and Date Range Parameters

[gabrieldutra]

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)

[gabrieldutra]

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)

[ranbena]

Great to see it in action :)

[gabrieldutra]

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

(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]

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

   
   • 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)

   `

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]

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]

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]

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]

@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

[ranbena]

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?

[arikfr]

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]

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]

@gabrieldutra fair points about keeping it for all types 👍

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

[ranbena]

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]

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]

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]

Let's do it in a separate PR then.

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

[gabrieldutra]

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

[gabrieldutra]

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

[arikfr]

👍