Remove selectedEndTime.max

[ryanbrainard]

As submitted in #194 and #167, users are running into issues with the max end time. I ran into this too and it makes the app unusable. I looked into this, and there are three issues:

• iso is adjusted from utcNow, but then calling toISOString(), so essentially double offset
• the elem.max is only set on $(document).ready, which means if a user presses the "Now" button, it ends up after the max
• elem.value is set twice

I started to fix these, but stepping back, I don't understand why we need a max at all. It really should be just "now" when the server accepts the request, not when the page first (or even subsequently) loaded on the client side. As such, this drops the setting of elem.max. It also cleaned up the unused vars and the double setting of elem.value.

Fixes #167
Fixes #194
cc: @mengyaoyang11

[mengyaoyang11]

@ryanbrainard thanks for the contribution! could you please attach screenshot or describe the effect after your change(removing maxTime)? It would be great if we have the final fix before we approve/merge anything, only removing maxTime wouldn't fix the issue described, right?

[ryanbrainard]

@mengyaoyang11 Thanks for the quick response.

could you please attach screenshot or describe the effect after your change(removing maxTime)?

The effect is that a max end time is no longer enforced. This means that the error reported in #194 no longer happens. There's not really anything to screenshot in the UI since it's removing the error, but here it is in the HTML at runtime:

Before:

After:

Note, I believe the reason there was difficulty consistently reproducing #194 and #167 is because of timezones. For users in the western hemisphere (UTC-), the max would have been in the future so they would not see the problem, but for users (like me and @xcrezd) in the eastern hemisphere (UTC+) the max would be in the past causing the problem reported.

As I mentioned above, while we could fix the timezone offset problem, having a max time isn't really needed, and even if we did, it doesn't make sense to do on the client side. If a user chooses a time in the future, then the query should still work. In fact this could be helpful to do on purpose to avoid having to click "Now" to get the latest data.

It would be great if we have the final fix before we approve/merge anything, only removing maxTime wouldn't fix the issue described, right?

This is the final fix and should address the issue. The issue was happening client side with browser field validation.

[ryanbrainard]

I noticed the Updating webfiles folder section of the readme that says to run go-bindata. I tried doing this with both github.com/jteeuwen/go-bindata (from go.mod; however is marked as v3.0.7+incompatible) and github.com/go-bindata/go-bindata, but both of them produce pretty different diffs from what is currently in this repo. Which fork and version of go-bindata should I use?

github.com/go-bindata/go-bindata seems to be closer to what's on master, so going to push up that, but it would be good to have this documented in the readme and have it match what it is used in go.mod.

cc: @annelausf (last edit to pkg/sloop/webserver/bindata.go)
cc: @kritikapradhan, @tyrken (edits to readme instructions)

[mengyaoyang11]

@ryanbrainard got it, thanks for the explanation, ran locally and the fix removed maxTime totally, so I am okay to approve/merge your changes, Thanks!

For updating bindata.go file, I think all you need to do is to run go-bindata -pkg webserver -o bindata.go webfiles/ within webserver directory, are you running the command under the correct directory? this commit seems doing the right work, any other concerns?

[ryanbrainard]

@ryanbrainard got it, thanks for the explanation, ran locally and the fix removed maxTime totally, so I am okay to approve/merge your changes, Thanks!

Thanks for trying it out!

For updating bindata.go file, I think all you need to do is to run go-bindata -pkg webserver -o bindata.go webfiles/ within webserver directory, are you running the command under the correct directory? this commit seems doing the right work, any other concerns?

Yes, that's what I did, and it does seem ok for this PR. My concern was that there are multiple forks and versions of go-bindata all with different formats, so wanted to make I was using the right one.

[kritikapradhan]

@ryanbrainard thanks for looking into this.

Context for the feature: To reiterate that we do not store data longer than 2 weeks, as a result of which that avoids allowing customers to even select a range outside of that. Having said that catering to different time zones is something we will need to look into.

If it is possible to account for the above context in your changes, that would be great.

Wrt to the go-bindata version, github.com/go-bindata/go-bindata is correct. Thanks for pointing out that go.mod should reflect that.

[ryanbrainard]

Context for the feature: To reiterate that we do not store data longer than 2 weeks, as a result of which that avoids allowing customers to even select a range outside of that. Having said that catering to different time zones is something we will need to look into.

Thanks, that help; however, in that case, wouldn't we want a min end time, not a max end time?

If it is possible to account for the above context in your changes, that would be great.

While we could move it to a min 2 weeks in the past offset by the time range, doing this on the client side is still going to have the problem of the date getting out of sync with clock time since this is set on page load, not to mention problems with the changes to the offset. In my opinion, it would be better to remove the validation for now (this PR), and then we can always add a more robust solution if validation is still needed. This would at least make the app usable for anyone in UTC+ timezones.

[ryanbrainard]

Wrt to the go-bindata version, github.com/go-bindata/go-bindata is correct. Thanks for pointing out that go.mod should reflect that.

Logged #203 for this.

[sana-jawad]

Context for the feature: To reiterate that we do not store data longer than 2 weeks, as a result of which that avoids allowing customers to even select a range outside of that. Having said that catering to different time zones is something we will need to look into.

Thanks, that help; however, in that case, wouldn't we want a min end time, not a max end time?

If it is possible to account for the above context in your changes, that would be great.

While we could move it to a min 2 weeks in the past offset by the time range, doing this on the client side is still going to have the problem of the date getting out of sync with clock time since this is set on page load, not to mention problems with the changes to the offset. In my opinion, it would be better to remove the validation for now (this PR), and then we can always add a more robust solution if validation is still needed. This would at least make the app usable for anyone in UTC+ timezones.

I agree @ryanbrainard