Dashboard static range issues

[zumoshi]

This is intended to fix #101

Work in progress atm. Mostly made this early to get feedback on how to proceed.

• Make backend accept the date format allowed by frontend
• Make frontend convert dates to RFC3339 before sending to backend
• Make frontend not complain about RFC3339 format or convert dates to its format before displaying them
• Improve the error message to include sample correct date formats
• Add tests (not sure where yet)

Would you prefer to include a new library (e.g. araddon/dateparse) for more flexible date input formats or is the current fix acceptable?

[jmattheis]

Would you prefer to include a new library (e.g. araddon/dateparse) for more flexible date input formats or is the current fix acceptable?

The backend should only support RFC3339, the frontend should take responsibility to parse the user input and format it correctly for the backend.

[zumoshi]

@jmattheis something like this?

Compared with the previous fix, this has the downside of not remembering how the user entered the date and converts it to rfc3339 next time the popup opens. But that could be considered a feature not a bug to keep them more consistent.

Also I noticed that the backend seems to have a bug where if fed a non-utc date it doesn't return any results. For now I just made it convert all dates to utc before sending to the backend. but we should probably convert it back to the browser's local timezone before displaying next if you're okay with the approach of the new fix.

[jmattheis]

Compared with the previous fix, this has the downside of not remembering how the user entered the date and converts it to rfc3339 next time the popup opens. But that could be considered a feature not a bug to keep them more consistent.

Traggo should probably parse the date with the date-time format of the user locale (then send it as RFC3339 to the server) and then display it again in format of the user locale. Additionally we could support parsing it in RFC3339, but this would be fine if we then display it in the user locale format.

I think the change could be done directly in the RelativeDateTimeSelector.

Also I noticed that the backend seems to have a bug where if fed a non-utc date it doesn't return any results. For now I just made it convert all dates to utc before sending to the backend. but we should probably convert it back to the browser's local timezone before displaying next if you're okay with the approach of the new fix.

I'm not sure what the right way is here, the timezone stuff is pretty complicated and sadly I haven't documented much there why I've implemented it like this. In the other endpoints the backend usually does model.Time#OmitTimeZone for date-times from the ui. Could you try this instead and verify if this also fixes the problem. Then the original supplied RFC3339 formatted date-time with the timezone should be stored in the database, and we don't have to convert it in the browser local timezone.

diff --git a/time/parse.go b/time/parse.go
index 154c2c8..850ceeb 100644
--- a/time/parse.go
+++ b/time/parse.go
@@ -46,7 +46,7 @@ func Validate(value string) error {
 func ParseTime(now time.Time, value string, startOf bool, weekday time.Weekday) (time.Time, error) {
        parse, err := time.Parse(time.RFC3339, value)
        if err == nil {
-               return parse, nil
+               return model.Time(parse).OmitTimeZone(), nil
        }

        return timemath.Parse(now, value, startOf, weekday)

[zumoshi]

I'm not sure I like the idea of OmitTimeZone. Since it changes the time.
To take the example of your test case for it, 2009-06-30T18:30:00+02:00 == 2009-06-30T16:30:00Z, not 18:30.

If you don't want to be timezone aware you should remove it everywhere and use naive times. but if you want to be timezone aware the correct way is to convert times to utc not just ignore that timezone exist and misinterpret them.

But yes, removing the .utc() in normalizeDate and adding OmitTimeZone in ParseTime does fix the issue of entries being omitted at the cost of technically querying the wrong range (by up to 12 hours at the most extreme case). although if you do the same thing while adding the entries and the user never changes their time zone (e.g. multi user instance where employee is remote and in a different country than the employer or travel and inspecting historic data) then it should be fine.

We can make a new issue about timezone correctness later, it's technically not related to this issue.

So, just to clarify, next step is to apply the normalizeRangeDateFormat calls to ui/src/common/RelativeDateTimeSelector.tsx:18 instead of closer to where the api gets called as a refactor.
And to also change how the date is displayed in addition to how the date is sent back to the parent component?

[jmattheis]

So, just to clarify, next step is to apply the normalizeRangeDateFormat calls to ui/src/common/RelativeDateTimeSelector.tsx:18 instead of closer to where the api gets called as a refactor.
And to also change how the date is displayed in addition to how the date is sent back to the parent component?

First of I think it would be okay if we only support dates and not date times when specifying the range on the dashboard page. I think this is enough and it removes the need from the user to specify a time. When the "end" range then uses endOfDay of the given date.

I imaging that the impl of the RelativeDateTimeSelector can look like this. If the parsing was unsuccessful, we just forward the invalid value to setValue, if successful, parseRelativeTime returns a normalized time which is either the raw relative time (e.g. now-1d, or the normalized date-time (parsed from local date and formatted to RFC3339).

As input we always get the formatted RFC3339 date, so we parse the value input of the component as RFC3339.

    const parsed = parseRelativeTime(value, type, moment(), 'YYYY-MM-DDTHH:mm:ssZ');
    return (
        <TextField
            fullWidth
            style={style}
            // .localized is similar to .normalized but the opposite.
            // if it's a absolute date, do date.format('l), otherwise return the raw relative date expression
            value={parsed.localized} 
            disabled={disabled}
            InputProps={{disableUnderline}}
            onChange={(e) => {
                const newValue = e.target.value;
                const result = parseRelativeTime(value, type, moment(), 'l');
                setValue(result.success ? result.normalized : newValue, result.success);
            }}
            error={!parsed.success}
            helperText={
                small ? undefined : !parsed.success ? (
                    <Typography color={'secondary'} variant={'caption'}>
                        {parsed.error}
                    </Typography>
                ) : (
                    <Typography variant={'caption'}>{parsed.preview.format('llll')}</Typography>
                )
            }
            label={label}
        />
    );

[zumoshi]

So, basically merge the functionality of normalize/friendly methods I made into parseRelativeTime and get rid of them? + allowing omitting time and entering only date

[jmattheis]

Yes

[zumoshi]

Sorry for the delay, Internet wasn't super reliable in the drama of the past weeks.

I've removed normalizeDate and friendlyDate methods, and moved their functionality into parseRelativeDatetime.
I've also added support for date without time which depending on divide uses either startOf or endOf day.

Additionally I've refactored your test to use parameterized tests from jest instead of the helper method.

One issue with the linter was that it complained about cyclomatic-complexity which for now I've supppressed since you asked to move the functionality in here, but we can refactor it to extract some functions from it.

The force-push is due to rebasing on master.

Please review and let me know if you want any other changes.

[jmattheis]

I think this should only support L and L LT from https://momentjs.com/docs/?%2Fdisplaying%2Fformat%2F#/displaying/ so that the date can be formatted in the user defined date locale. YYYY-MM-DD can be supported as a fallback. L or L LT should be used below when passing it as localized to the success function.

FYI: I'll be unable to properly review this until end of next week.