Force start date before end date with range mode flatpickr

[cyrillefr]

• modify view to get a flatpickr component in range mode
• modify spec to take into account range mode

What? Why?

• Closes [Reports] Snail (error 500) if date range end is before date range start in Enterprise Fee Summary report (Bugsnag) #11424

By using the flatpickr component(node module + stimulus), to execute in range mode, we force the user to use consecutive dates. That is: a start date, then an end date.
This mode is already in use in the orders part of the admin.
The change is also effective to every reports since the datepickr is shared between all report views.

What should we test?

• Visit any report page, and see the datepicker is now only one input and pop-up component that compels to chose a range of date in proper order.
• also, the automated spec orders_and_fulfillment_spec.rb simulate the picking of dates in this new mode.

Release notes

Changelog Category (reviewers may add a label for the release notes):

• User facing changes
• API changes (V0, V1, DFC or Webhook)
• Technical changes only
• Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

[rioug]

Nice one ! 👍

[drummer83]

Hi @cyrillefr,
Thanks for yet another one! 🔥

I love using the date range selector. It's much more comfortable! 👍
As far as I can tell it is working great on all the reports - not all of them have been tested yet.

Two things come to my mind which need clarification, though:

1. We are losing the option to set start and end TIME. I could imagine that this can be useful if an order cycle ends at noon and the next one starts in the same moment. You might want to set the report filters accordingly.
2. I'm not sure if this is something we can change, but: If you select just one day (click on it twice in the range selector), I would expect to get results from time >= 00:00 until time < 00:00 next day. However the results are always empty. It seems like the query is looking from 00:00 until 00:00, which is (almost) always empty. Can we change this?
   UPDATE: Ah, even worse... If you select 17th and 18th January in the date range it will show results from 17th Jan 00:00:00 until 18th Jan 00:00:00. So the 18th is actually ignored.

I think we should hear the opinion of @openfoodfoundation/train-drivers-product-owners on how to continue with this one. But maybe you can already comment what's possible and what's not, Cyrille! Thanks!

Anyway, thanks a lot for your work already!

[mkllnk]

Those are clear regressions that need to be solved.

• Include time.
• Adjust boundaries.

Maybe it makes more sense to query the range inclusive the borders, not exclusive (<= instead of <) and then we could go from the start date at 00:00 midnight to the end date at 23:59 by default.

This is just an idea, I don't know which implementation is easiest.

[cyrillefr]

Hello @drummer83, @mkllnk sorry I did miss the issue on time ...
I assume the range mode would be ok since it is used in the orders part.

I will look at those time issues and tell what can be done to solve listed regressions.

[cyrillefr]

Hello @drummer83, @mkllnk ,

I can add time with a simple config add to configuration.
Time starts by default at 12:00, but I can change to 00:00, therefore, it is easier to get 00:00 to 23:59 reports or 00:00 to 00:00 ones.
Also, by adding time, I think there should be no need to set up some other default, since user is free to chose exactly time.

What do you think, should I do that ?

[drummer83]

Please, go ahead and add the time config.

I don't quite follow your proposal regarding the default time. Could you try to explain again? 🙃

[cyrillefr]

Hello @drummer83 , regarding the default time at 00:00

Since (I think), most of the reports will be for complete days and since time is displayed: by setting default time at 00:00, there will be less time juggling and clicking than with the previous default of 12:00(which forces to click till 00 or to enter 00).
So if you want a report for one day, you will only have in the second part of the range to click upwards from 00:00 to 23:59 to get a range of a complete day. With one click.

[drummer83]

Ok, so it's not possible to have 00:00 as default for the start day and 23:59 as default for the end day?
I think if that's not possible, then 00:00 for both is maybe best, just like you said. And instead of clicking to 23:59 I would simply select the next day and use 00:00 on that day - which is really convenient.
Thanks for thinking this through, @cyrillefr!

[cyrillefr]

Hello @drummer83 , Yes you can only chose one default hour and not an array like default dates. (I have tried and it is not working).

[drummer83]

Cool, @cyrillefr!
In case you have time for something interesting to read: #9989
The corresponding discussion in the PR is interesting as well. 😉

Long story short: 00:00 is the way to go! 💪🏻

[mkllnk]

The code is looking good but I have one concern over an issue we had before. If you select a date range, like for Monday to Friday then the times are set to 00:00 on both days, right? So the effective search is from beginning of Monday to beginning of Friday which is also end of Thursday. The user expects Friday to be included but it's not unless you adjust the time.

Is this what happens? Is there any callback we can hook into to adjust the time after date selection? Is the time reset every time you change a date or is the time preserved? For example, if I select Wednesday 10:00 am and then select Thursday, does it reset to 00:00 or keep 10:00?

[cyrillefr]

Hello @mkllnk ,

If you select Mon 00:00 to Fri 00:00, you will have a span search that includes all of the Thursday and the first minute of Friday.
Either you select Thu 23:59 or Fri 23:59. Its offers more flexibility but users must be aware of what they do a bit more.
Note that after selecting the first date, the time is highlighted to show you can modify it.
When selecting a second date, the time set off for the first one is kept: if you choose Mon 11:40, it will keep the 11:40 for the second date (that you can modify of course).

If you click a third time, you re init both dates & times.

Regarding hooks, there are some:
https://flatpickr.js.org/events/

I guess you would be interested on setting 23:59 for the 2nd date.
From the link @drummer83 mentioned above, what have been implemented on the 2 parts picker, was a 00:00 default time for both start & end dates.

[mkllnk]

I guess you would be interested on setting 23:59 for the 2nd date.

I think that would be ideal. I'm happy for @drummer83 to have the last say here.

[sigmundpetersen]

As @abdellani says in #9989 (comment), if you set 23:59 you will miss records created between 23:59:00 and 23:59:59.

His solution was

In start date:
    date: Today
    time: 00:00
In end date:
    date: Tomorrow
    time: 00:00

[drummer83]

Hi @cyrillefr,
Sorry for my late reply. I've been thinking about this again and again, started writing down scenarios, but in the end this PR is about removing the error 500 - not about improving or changing the usability.
Therefore I came to the following conclusion: Let's go for 00:00 for start and end date.
Why? Because this is the exact behaviour we have right now - just with two separate date pickers instead of one.
I hope we can move this forward now.
Apologies again!

[cyrillefr]

Hello @drummer83 ,

no problemo :)

[RachL]

hello @cyrillefr do you still plan to work on this one?

[cyrillefr]

Hello @RachL ,

I need to refresh a bit with this one, but I am pretty sure the solution that was picked in the end was what I have coded in the first place.
I will have a look and make some tests.

[cyrillefr]

Yes, the few remarks at the end to talk about the end time of the date picker.
In the end, everybody is ok with the chosen end time.
To me, there is nothing to add.

[rioug]

I am wondering why we don't need to close flatpickr here ? Anyway the tests are green, so it's not really important.

[rioug]

Nice one, thanks @cyrillefr !

[filipefurtad0]

Hey @cyrillefr ,

This is quite awesome 💪

I could not find a way to select an invalid time range on the reports date-time selector.

Just as a recap, we go from the scenario in which we had to separate date/time selectors:

to having just one. The only minor usability issue introduced is that the selected date is not really fitting the placeholder, which means it is not readable:

I think it can be easily checked in a visual way, by clicking the selector, so I don't think this is a blocker.

I've checked other report types, and the new selector is implemented everywhere, and functional.

From the testing side, I'd say this is good to merge. I'm wondering how does @openfoodfoundation/train-drivers-product-owners feel about the too short placeholder?

Adding the feedback needed label.

Edit - more of a note to self: looking at the code, I'm surprised only one report spec needed updating. I think it's a good idea to implement this test in other reports as well. I'll open an issue and assign myself to it.

[cyrillefr]

Hello @filipefurtad0 ,

I can definitively make the size for the placeholder bigger.
I will do it tomorrow ( I am UTC + 2:00)

[filipefurtad0]

Hey @cyrillefr ,

This looks great now:

I had a quick look through other reports, all good. Merging! Thanks again!