Adding Timezone Support #380

bigmstone · 2017-07-20T20:21:48Z

This PR should serve as a launch pad for a conversation on how we want to handle TZ as well as implementing what gets decided.

Fixes #292

enykeev · 2017-07-21T02:34:33Z

I tried to approach this issue previously. The two main problem are:

that we don't have a place in UI for per-user defined settings (and at the same time, don't really have a lot of this settings to justify space for them)
that the list of possible timezones is so massive, we would need some form of lazy loading for it. Besides, I wasn't able to find the complete list of TZs a browser supports. My guess that there is no single standard and browsers are still implementing it as they pleased.

bigmstone · 2017-07-21T14:58:43Z

@enykeev agreed on both points. This PR was the laziest way I could accomplish the ask once I got my bearings and get a covo going.

If we want the ability to modify then I have two options in my head:

When you click the user there's a dropdown there w/ a list of available timezones.
Have a more formal modal for "settings", again w/ the modal toggle available under the user dropdown

I'm not opposed to using something like react-timezone unless you have good reason to avoid it for the dropdown.

arm4b · 2017-07-21T16:33:56Z

My understanding is that there should be a central st2.conf setting for the time zone and all other clients (CLI, WebUI) should honor that.

What matters: when specific action execution happened on specific server. And that server even can have wrong time (ntpdate), but the idea is to follow that single point of truth (server st2.conf).

IMO having every CLI/WEB/ST2 their "special" way of recognizing the TZ/time can bring even more confusion.

bigmstone · 2017-07-21T19:43:04Z

I'm actually curious now why we have a configureable timezone in st2.conf and don't honor locally configured TZ on the server? Timezone is usually relevant per user. So if a user is configured to use CST, PST, etc we should display the time in that fashion, IMO. I get having a single source of truth, but wouldn't it make more sense to have the server's source of truth for logging etc and have a user configurable timezone for CLI/Web UI?

arm4b · 2017-07-21T21:03:27Z

I just read a bunch of old Issues related to Time Zones we had. Looks like indeed we need to make happy everyone :)

Based on that here is a chain:

Server
- st2.conf
  - CLI (~/.st2/config)
  - st2web (local storage, long cookie, whatever)

Meaning if st2web or CLI config is set, - it will be used. If not set, - use "central" st2.conf. If nothing in st2.conf, - honor server's Time Zone by default.

That'll give an option for everyone, including orgs. which have all the IT operations in a common TZ (via st2.conf).

bigmstone · 2017-07-25T03:28:58Z

To add more fuel to the fire it would appear that the only time local_timezone is used is in st2reactor. Not for logging, history, etc. The ~/.st2/config option is only for st2client. The central setting for TZ appears to not, by design, have anything to do with UI/UX timezone.

I even went as far as to implement a majority of the API endpoint to get st2config before I got enraged by oslo. That digging is what lead me to see that the setting for timezone is only used by reactor. I think I'm more confused now then when I started.

We could go one of two ways from here - IMO (Unless my deduction from above is totally off base, which is possible). We could go route 1. Where we make a big push to unify how we handle TZ - though what we'd actually do here is unclear to me. Route 2. is to just implement st2client style TZ handling in st2web where the user is given a TZ option and that is set in a cookie.

bigmstone · 2017-07-25T15:41:42Z

Per slack convo w/ @lakshmi-kannan and @Kami we're going route 2. Implement st2client style TZ handling and allowing user to select timezone. I will have it default to local timezone if no cookie is set and local timezone is available.

…timezone

Kami · 2017-07-26T16:39:39Z

Per slack convo w/ @lakshmi-kannan and @Kami we're going route 2. Implement st2client style TZ handling and allowing user to select timezone. I will have it default to local timezone if no cookie is set and local timezone is available.

Sounds good. Only thing I might consider doing differently is perhaps defaulting to UTC instead of a local user timezone.

Murica.

…o issue-292/timezone

…timezone

…it to determin utc status. Adding needed packages to support transpile of spread object. Adding st2-redux component to allow for common store for many components.

… timeState object. Updating react packages to get rid of all dem errors.

…default. Updating tests/util.js to prep for test fix.

…on in fire.

… bug with wrong date when on local time.

enykeev · 2017-08-08T13:51:06Z

apps/st2-actions/template.html

@@ -1,7 +1,7 @@
 <header class="st2-menu"></header>

 <main class="st2-panel">
-
+  <div class="st2-redux"></div>


Do we still need redux?

enykeev · 2017-08-08T13:51:19Z

apps/st2-history/template.html

@@ -2,6 +2,7 @@

 <main class="st2-panel" data-test="history_panel">

+  <div class="st2-redux"></div>


enykeev · 2017-08-08T13:51:46Z

apps/st2-history/template.html

-                {{ record.trigger_instance.occurrence_time | date:'EEE, dd MMM y HH:mm:ss UTC':'UTC' }}
+                <div class="st2-time"
+                  timestamp="record.trigger_instance.occurrence_time"
+                    utcdisplay="utcDisplay"


Mixed indent

enykeev · 2017-08-08T13:52:09Z

modules/st2-menu/menu.directive.js

@@ -11,7 +11,7 @@ module.exports =
      templateUrl: template,
      link: function postLink(scope) {
        scope.routes = scope.state.get().map(route => {
-          return Object.assign(route, {
+         return Object.assign(route, {


Messed up indent

enykeev · 2017-08-08T13:54:21Z

modules/st2-time/time.component.js

+
+  return (
+    <Time
+      data-test={datatest ? datatest : undefined}


I'd rather you would use {...restProps}. Data- props are falling through as is in react.

enykeev · 2017-08-08T13:56:36Z

package.json

@@ -13,19 +13,24 @@
    "angular-ui-notification": "0.1.0",
    "angular-ui-router": "1.0.0-alpha.1",
    "lodash": "2.4.2",
-    "ngreact": "0.2.0",
+    "ngreact": "^0.4.1",


Please stick to exact versions. I recommend to set it once with npm config set save-exact true.

LindsayHill · 2017-08-09T18:20:57Z

Adding some comments made on Slack:

I did some brief testing of this in my lab

lhill@st2:~$ npm -v
3.10.10
lhill@st2:~$ node -v
v6.11.2
lhill@st2:~$ st2 --version
st2 2.3.2
lhill@st2:~$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 14.04.5 LTS
Release:        14.04
Codename:       trusty
lhill@st2:~$

Generally looks OK, but I noticed that the TZ for child tasks was not updating. It always displayed local time, not UTC. Clicking on timestamp in parent task would change timezone, but no change for child tasks. See

bigmstone · 2017-08-13T02:22:09Z

@LindsayHill sadly I can't repro this. Can you provide system info/etc. And any console errors you're seeing?

…timezone

…er elements.

…feeding proper scope vars to component in actions page.

bigmstone · 2017-08-14T02:32:36Z

@enykeev Unfortunately using spread to get remaining props would't work in this instance since ngreact isn't sending anything w/ - in the key. I couldn't find any documentation on it - specifically if it's an angular problem or an ngreact problem. When I started parsing the ngreact code base I decided it wasn't worth the effort for what I'm doing here.

LindsayHill · 2017-08-14T19:06:40Z

👍
Now looks good to me, with sub tasks operating as expected

enykeev · 2017-08-16T13:42:29Z

package.json

    "ngify": "1.4.1",
    "npm-shrinkwrap": "200.5.1",
    "react-addons-test-utils": "0.14.8",
+    "react-time": "4.3.0",


Yep, missed that one. Both react-time and moment are supposed to be dependencies, not devDependencies. Will change it in my PR.

Prefer local timezone.

aab3545

bigmstone requested a review from enykeev July 20, 2017 20:21

Removing UTC from string

a8da570

bigmstone force-pushed the issue-292/timezone branch from 563a2c6 to a8da570 Compare July 20, 2017 20:43

bigmstone added 3 commits July 25, 2017 15:37

Moving shrinkwrap to package-lock. Adding vim swp to gitignore

513b867

Merge branch 'master' of github.com:StackStorm/st2web into issue-292/…

0757c55

…timezone

adding react-timezone

d66bdae

bigmstone added 11 commits July 26, 2017 22:01

Various non-working commits squashed into one non-working commit.

e67875a

Murica.

Not sure what's going on here

8abac12

Merge branch 'issue-292/timezone' of github.com:StackStorm/st2web int…

a579920

…o issue-292/timezone

Merge branch 'master' of github.com:StackStorm/st2web into issue-292/…

73f0b3b

…timezone

General clean up

83b2b24

Moving things around to prep for full time component

2bfa7dc

Adding reducer stub, and wrapper stub to put provider/store in.

9315122

Linking reducer to mouse click. Linking state to component and using …

68541e8

…it to determin utc status. Adding needed packages to support transpile of spread object. Adding st2-redux component to allow for common store for many components.

Removing unused wrapper

8121afd

Moving actions template over to st2-time. Adding proper prop-type for…

e1b842c

… timeState object. Updating react packages to get rid of all dem errors.

Merge branch 'master' into issue-292/timezone

8ffb28a

bigmstone self-assigned this Aug 3, 2017

bigmstone added 3 commits August 2, 2017 21:54

Adding packages and updaing shrinkwrap to install correct version by …

bbb63f7

…default. Updating tests/util.js to prep for test fix.

Adding data-test to make tests pass

e977396

Adding ability to format as needed inline.

6aea789

Hopefully final commit for this stupid timezone feature. May it live …

c3985b8

…on in fire.

bigmstone added the RFR label Aug 4, 2017

bigmstone added 3 commits August 4, 2017 08:08

Remove st2-redux from modules index. Siplifying groupby math to avoid…

1a63972

… bug with wrong date when on local time.

Fixing tests

662ab68

Fixing sort bug with new TZ feature

95af702

enykeev suggested changes Aug 8, 2017

View reviewed changes

enykeev approved these changes Aug 8, 2017

View reviewed changes

bigmstone added 8 commits August 12, 2017 21:25

Merge branch 'master' of github.com:StackStorm/st2web into issue-292/…

473555c

…timezone

Updating shrinkwrap

c51ccd2

Removing unneeded packages

445421a

Fixing indent

941f00f

Fixing indent and removing unused redux

f004d20

Moving to cleaner data-test inclusion in component w/ support for oth…

b529408

…er elements.

Cleaning up location of watch. Adding scope vars to actions page and …

a89249b

…feeding proper scope vars to component in actions page.

Moving back to dataTest element since angular wasn't giving any love

147a5fd

bigmstone added 2 commits August 14, 2017 11:59

Fixing typeo

d801662

Fixing child st2 time component.

f48dc22

bigmstone merged commit 6c15c4e into master Aug 14, 2017

bigmstone deleted the issue-292/timezone branch August 14, 2017 20:33

enykeev reviewed Aug 16, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Timezone Support #380

Adding Timezone Support #380

bigmstone commented Jul 20, 2017

enykeev commented Jul 21, 2017

bigmstone commented Jul 21, 2017

arm4b commented Jul 21, 2017 •

edited

bigmstone commented Jul 21, 2017

arm4b commented Jul 21, 2017 •

edited

bigmstone commented Jul 25, 2017

bigmstone commented Jul 25, 2017

Kami commented Jul 26, 2017

enykeev Aug 8, 2017

enykeev Aug 8, 2017

enykeev Aug 8, 2017

enykeev Aug 8, 2017

enykeev Aug 8, 2017

enykeev Aug 8, 2017

LindsayHill commented Aug 9, 2017

bigmstone commented Aug 13, 2017

bigmstone commented Aug 14, 2017

LindsayHill commented Aug 14, 2017

enykeev Aug 16, 2017

		@@ -2,6 +2,7 @@

		<main class="st2-panel" data-test="history_panel">

		<div class="st2-redux"></div>

Adding Timezone Support #380

Adding Timezone Support #380

Conversation

bigmstone commented Jul 20, 2017

enykeev commented Jul 21, 2017

bigmstone commented Jul 21, 2017

arm4b commented Jul 21, 2017 • edited

bigmstone commented Jul 21, 2017

arm4b commented Jul 21, 2017 • edited

bigmstone commented Jul 25, 2017

bigmstone commented Jul 25, 2017

Kami commented Jul 26, 2017

enykeev Aug 8, 2017

Choose a reason for hiding this comment

enykeev Aug 8, 2017

Choose a reason for hiding this comment

enykeev Aug 8, 2017

Choose a reason for hiding this comment

enykeev Aug 8, 2017

Choose a reason for hiding this comment

enykeev Aug 8, 2017

Choose a reason for hiding this comment

enykeev Aug 8, 2017

Choose a reason for hiding this comment

LindsayHill commented Aug 9, 2017

bigmstone commented Aug 13, 2017

bigmstone commented Aug 14, 2017

LindsayHill commented Aug 14, 2017

enykeev Aug 16, 2017

Choose a reason for hiding this comment

arm4b commented Jul 21, 2017 •

edited

arm4b commented Jul 21, 2017 •

edited