Timezone feature #696

csduarte · 2018-02-01T23:41:19Z

Summary

Update timezone both manually and automatic
If chosen to update automatically, it updates after log in or when new session begins
List of SupportedTimezones are pulled from Config

mattermost/mattermost-mobile#1456
mattermost/mattermost#8185

Ticket Link

PLT-541 Add a "Time zone" field to account settings - Mattermost

Checklist

Ran make check-style to check for style errors (required for all pull requests)
Ran make test to ensure unit and component tests passed
Has server changes (please link)
Has UI changes
Includes text changes and localization file (.../i18n/en.json) updates

Screenshots

manual timezone selection

automatic timezone selection

Feature in action

manual

automatic

miguelespinoza · 2018-02-02T20:22:23Z

Depends on this server PR: mattermost/mattermost#8185

miguelespinoza · 2018-02-13T16:04:54Z

Latest changes include:

localized post timestamps
user local time in profile pop-over

jwilander

Looks good for the most part, just a few comments

jwilander · 2018-03-05T18:28:16Z

utils/timezone.jsx

+ return dateTimeFormat.resolvedOptions().timeZone;
+}
+
+export function autoUpdateTimezone(userTimezone) {


Couple suggestions/comments here:

Is any of this logic shared by the mobile app? Wonder if it would make sense to put it into mattermost-redux

Since an action is getting called here, I'd prefer if we just turned this function into an action itself

Doable, I currently have a PR for timezone support so adding selectors/actions for this won't be hard.

I agree this logic is shared with both webapp and mobile

jwilander · 2018-03-05T18:30:08Z

stores/user_store.jsx

@@ -459,6 +459,22 @@ class UserStoreClass extends EventEmitter {
 return this.getStatuses()[id] || UserStatuses.OFFLINE;
 }

+ getTimezone(id) {


Could you turn this into a selector instead? Preferably in mattermost-redux since I figure the mobile apps are going to need this as well

Then the timezone can get passed in as a prop to the components that need it instead of importing UserStore (which we're trying to reduce usage of so we can remove it)

It would definitely make sense as a selector. Something like this would be nice and simple.

const getCurrentTimezone = createSelector( getCurrentUser, (currentUser) => { ... } );

If we ever care about the timezone of another user, we could add one that's more detailed.

jwilander · 2018-03-05T18:31:22Z

components/user_settings/display/manage_timezones.jsx

+import React from 'react';
+import {FormattedHTMLMessage, FormattedMessage} from 'react-intl';
+
+import {updateUser} from 'actions/user_actions.jsx';


Not a big deal, but it would be a bit nicer to pass in the mattermost-redux update user action through an index.js container so we can eventually get rid of the wrapper actions like this one

@jwilander does the redux library support migrating away from these wrapper actions?

Sorry, missed your comment. Yeah if the actions are already in mattermost-redux then you can just pass it in directly and if you need to get results right away from the action, just await on it instead of passing callbacks like we do for the wrapper functions

jwilander · 2018-03-05T18:32:51Z

components/user_settings/display/manage_timezones.jsx

+ useAutomaticTimezone: PropTypes.bool.isRequired,
+ automaticTimezone: PropTypes.string.isRequired,
+ manualTimezone: PropTypes.string.isRequired,
+};


Mind defining the propTypes at the top of the class like so:

export default class ManageTimezones extends React.Component { static propTypes = { // props here } // other stuff }

jespino · 2018-03-05T18:46:47Z

utils/timezone.jsx

+const dateTimeFormat = new Intl.DateTimeFormat();
+
+export function getSupportedTimezones() {
+ if (global.mm_config && global.mm_config.SupportedTimezones) {


We are removing all global.mm_config usage in the application, you should use something like const config = getConfig(store.getState())

Although getConfig(store.getState()) is a correct workaround given that global.mm_config is now gone, note that it won't connect a component to future changes in the SupportedTimezones. We're aiming to eventually remove the required refresh when a configuration change occurs.

If possible, pass in the config.supportedTimezones to this method and invoke it from the connector. If that's not straightforward, the approach @jespino describes will suffice for now :)

This should be turned into a selector in the redux library

This has been moved to a selector 👍

jespino · 2018-03-05T18:51:30Z

components/post_view/post_time/post_time.jsx

@@ -45,17 +52,35 @@ export default class PostTime extends React.PureComponent {
 };
 }

+ getCurrentUserTimezone = () => {
+ const userId = UserStore.getCurrentId();


CurrentUserId is better passed through the redux connect, as a property.

In particular, this may not trigger a re-render when the time zone changes, unless the PostTime is re-rendered for another reason.

Agreed. These should be passed through from the redux store. We shouldn't be adding new code that uses the old stores as long as we can help it.

I'm noticing a pattern with classes that seem to be getting deprecated, examples:
user_store, user_actions.

Is there a document that describes which files to stay away from? that way contributors are able to understand which files are getting deprecated and can work with new best practices

It's kind of documented here but we should definitely do a better job of it. Would adding comments at the top of these files help?

jespino · 2018-03-05T18:54:54Z

components/post_view/post_time/post_time.jsx

- let minute = date.getMinutes();
- minute = minute >= 10 ? minute : ('0' + minute);
- let time = '';
+ // const militaryTime = this.props.useMilitaryTime;


This commented lines can be removed because the military time is already solved by the toLocaleString.

jespino · 2018-03-05T18:56:15Z

components/profile_popover/profile_popover.jsx

+ if (this.props.enableTimezone) {
+ const currentTimezone = getCurrentTimezone(this.props.user.timezone);
+ if (currentTimezone) {
+ const useMilitaryTime = PreferenceStore.getBool(Constants.Preferences.CATEGORY_DISPLAY_SETTINGS, 'use_military_time');


Probably is better option to receive this from the redux connect as a property.

lieut-data · 2018-03-05T19:03:06Z

components/post_view/post_time/post_time.jsx

@@ -45,17 +52,35 @@ export default class PostTime extends React.PureComponent {
 };
 }

+ getCurrentUserTimezone = () => {
+ const userId = UserStore.getCurrentId();


In particular, this may not trigger a re-render when the time zone changes, unless the PostTime is re-rendered for another reason.

lieut-data · 2018-03-05T19:04:35Z

components/post_view/post_time/post_time.jsx

+ /*
+ * Flag for enabling the timezone feature
+ */
+ enableTimezone: PropTypes.bool,


Mark .isRequired?

lieut-data · 2018-03-05T19:09:24Z

components/profile_popover/profile_popover.jsx

+ const useMilitaryTime = PreferenceStore.getBool(Constants.Preferences.CATEGORY_DISPLAY_SETTINGS, 'use_military_time');
+ const date = new Date();
+ dataContent.push(
+ <span>


Would it make sense to factor out a <DateTime> component that connects the useMilitaryTime and timezone preferences vs. having to pass them into every component that needs to render a time?

that would be a nice refactor 👍

lieut-data · 2018-03-05T19:16:11Z

i18n/en.json

@@ -2785,6 +2786,9 @@
 "user.settings.import_theme.submitError": "Invalid format, please try copying and pasting in again.",
 "user.settings.languages.change": "Change interface language",
 "user.settings.languages.promote": "Select which language Mattermost displays in the user interface.<br /><br />Would like to help with translations? Join the <a href='https://translate.mattermost.com/' target='_blank'>Mattermost Translation Server</a> to contribute.",
+ "user.settings.timezones.automatic": "Set automatically",
+ "user.settings.timezones.change": "Change timezone",
+ "user.settings.timezones.promote": "Select the time zone used for timestamps in the user interface and email notifications.",


time zone vs. timezone? I think they are both fungible, but perhaps we converge on one usage?

Nice catch, I'll fix that. We can go with timezone to be consistent

lieut-data · 2018-03-05T19:22:03Z

utils/timezone.jsx

+const dateTimeFormat = new Intl.DateTimeFormat();
+
+export function getSupportedTimezones() {
+ if (global.mm_config && global.mm_config.SupportedTimezones) {


Although getConfig(store.getState()) is a correct workaround given that global.mm_config is now gone, note that it won't connect a component to future changes in the SupportedTimezones. We're aiming to eventually remove the required refresh when a configuration change occurs.

If possible, pass in the config.supportedTimezones to this method and invoke it from the connector. If that's not straightforward, the approach @jespino describes will suffice for now :)

mkraft · 2018-03-05T20:01:28Z

utils/timezone.jsx

+}
+
+export function getBrowserTimezone() {
+ return dateTimeFormat.resolvedOptions().timeZone;


Did some quick tests with this JSFiddle and IE 11.15 displays "undefined" for this JS API so we'll need a fallback to support it.

@jasonblais Can you please confirm/disconfirm support for IE 11.15?

Thanks Martin. I had missed your note until now. I'll queue for our PM meeting to discuss this.

Discussed and we officially support IE11, hence IE 11.15 is included

hmhealey · 2018-03-06T18:33:54Z

components/user_settings/display/manage_timezones.jsx

+import SuggestionList from 'components/suggestion/suggestion_list.jsx';
+import TimezoneProvider from 'components/suggestion/timezone_provider.jsx';
+
+export default class ManageTimezones extends React.Component {


This should be a PureComponent

hmhealey · 2018-03-06T18:35:09Z

components/user_settings/display/manage_timezones.jsx

+ } = this.state;
+
+ const timezone = {
+ useAutomaticTimezone: useAutomaticTimezone.toString(),


Why store this as a string version of the boolean? That seems like something that'll be easy to get mixed up

hmhealey · 2018-03-06T18:43:33Z

stores/user_store.jsx

+ if (profile && profile.timezone) {
+ return {
+ ...profile.timezone,
+ useAutomaticTimezone: profile.timezone.useAutomaticTimezone === 'true',


I mentioned this above, but why switch types here?

@hmhealey this is a good point to address the convention MM uses for storing booleans in postgres.

From my investigation, MM has been storing booleans as strings. See: https://github.com/mattermost/mattermost-webapp/blob/master/components/user_settings/notifications/user_settings_notifications.jsx#L138

Looking at the database this is what the notify_props looks like:

{ "channel":"true", "comments":"never", "desktop":"mention", "desktop_duration":"5", "desktop_sound":"true", "email":"true", "first_name":"false", "mention_keys":"admin", "push":"mention", "push_status":"away" }

I'm following the same convention MM has been using for storing booleans. If this is not the preferred convention, let us know what's the best way to store booleans.

hmhealey · 2018-03-06T18:44:20Z

stores/user_store.jsx

@@ -459,6 +459,22 @@ class UserStoreClass extends EventEmitter {
 return this.getStatuses()[id] || UserStatuses.OFFLINE;
 }

+ getTimezone(id) {


It would definitely make sense as a selector. Something like this would be nice and simple.

const getCurrentTimezone = createSelector( getCurrentUser, (currentUser) => { ... } );

If we ever care about the timezone of another user, we could add one that's more detailed.

hmhealey · 2018-03-06T18:45:43Z

stores/user_store.jsx

+ };
+ }
+
+ return {


Do we care whether the timezone is automatic or manual in most places? It might make sense to just have a function that returns the timezone string without the rest of the object.

This is useful for the settings where we do need reference to the entire object, I agree that it would be great to only return a timezone string.

That's what the selector will be used for. Maybe it would be great to distinguish between settingsTimezone object and userTimezone string in the function name

hmhealey · 2018-03-06T18:48:32Z

utils/timezone.jsx

+const dateTimeFormat = new Intl.DateTimeFormat();
+
+export function getSupportedTimezones() {
+ if (global.mm_config && global.mm_config.SupportedTimezones) {


This should be turned into a selector in the redux library

hmhealey · 2018-03-06T18:52:10Z

utils/timezone.jsx

+ }
+}
+
+export function getCurrentTimezone(userTimezone) {


As mentioned above, could this be turned into a selector, possibly combined with the one I was talking about before? Outside of the settings modal, I can't think of any code that will care about the automatic vs manual timezone, so I don't think that needs to be passed around

yup, this will be turned into a selector 👍

hmhealey · 2018-03-13T13:43:57Z

@miguelespinoza You've got some style issues

$ eslint --ignore-pattern node_modules --ignore-pattern non_npm_dependencies --ignore-pattern dist --ext .js --ext .jsx . --quiet

/home/ubuntu/jenkins/workspace/mattermost-webapp_PR-696-OEE243NI6PR4ZUMUJ7SYBS2Y3FEIJCBHYFSQEGT7HFMZN5HQ633Q/components/local_date_time/index.js
  8:31  error  Unable to resolve path to module 'mattermost-redux/selectors/entities/timezone'  import/no-unresolved
  9:38  error  Unable to resolve path to module 'mattermost-redux/utils/timezone_utils'         import/no-unresolved

/home/ubuntu/jenkins/workspace/mattermost-webapp_PR-696-OEE243NI6PR4ZUMUJ7SYBS2Y3FEIJCBHYFSQEGT7HFMZN5HQ633Q/components/logged_in/index.js
  6:34  error  Unable to resolve path to module 'mattermost-redux/actions/timezone'  import/no-unresolved

/home/ubuntu/jenkins/workspace/mattermost-webapp_PR-696-OEE243NI6PR4ZUMUJ7SYBS2Y3FEIJCBHYFSQEGT7HFMZN5HQ633Q/components/suggestion/timezone_provider.jsx
  6:33  error  Unable to resolve path to module 'mattermost-redux/utils/timezone_utils'  import/no-unresolved

/home/ubuntu/jenkins/workspace/mattermost-webapp_PR-696-OEE243NI6PR4ZUMUJ7SYBS2Y3FEIJCBHYFSQEGT7HFMZN5HQ633Q/components/user_settings/display/index.js
  10:31  error  Unable to resolve path to module 'mattermost-redux/selectors/entities/timezone'  import/no-unresolved
  11:38  error  Unable to resolve path to module 'mattermost-redux/utils/timezone_utils'         import/no-unresolved

/home/ubuntu/jenkins/workspace/mattermost-webapp_PR-696-OEE243NI6PR4ZUMUJ7SYBS2Y3FEIJCBHYFSQEGT7HFMZN5HQ633Q/components/user_settings/display/manage_timezones.jsx
  6:33  error  Unable to resolve path to module 'mattermost-redux/utils/timezone_utils'  import/no-unresolved

/home/ubuntu/jenkins/workspace/mattermost-webapp_PR-696-OEE243NI6PR4ZUMUJ7SYBS2Y3FEIJCBHYFSQEGT7HFMZN5HQ633Q/components/user_settings/display/user_settings_display.jsx
  6:33  error  Unable to resolve path to module 'mattermost-redux/utils/timezone_utils'  import/no-unresolved

✖ 8 problems (8 errors, 0 warnings)

miguelespinoza · 2018-03-29T23:40:25Z

@esethna here's the server PR for reference: mattermost/mattermost#8560

miguelespinoza · 2018-03-30T12:11:01Z

Cool, we will need a dev to switch to ExperimentalTimezone: true to test timezones properly

GoldUniform · 2018-03-30T18:48:36Z

I've changed that ExperimentalTimezone setting for this spinmint and restarted it.

esethna · 2018-03-30T18:59:17Z

Hitting an issue with the timezone setting disappearing after setting it. @miguelespinoza looking into it.

miguelespinoza · 2018-03-30T20:04:24Z

Done, it was a mistake on my part.
It was hiding if the timezone was not set automatically, which didn't account for manual timezone 🤦‍♂️

That change was made after the error that popped up yesterday. For fresh new users, their timezone was never getting automatically set, so I call autoUpdateTimezone one last time in the settings screen to ensure it gets another chance.

@esethna tested with setting automatic and manual timezones.

mattermod · 2018-04-02T14:14:24Z

Setup Test Server label detected. Spinmint test server created if build succeeds (checks pass and no conflicts with base branch).

esethna · 2018-04-02T14:15:14Z

Thanks @miguelespinoza, testing now

mattermod · 2018-04-02T14:16:26Z

Spinmint test server created at: https://i-099c369abdaee52e7.spinmint.com

Test Admin Account: Username: sysadmin | Password: sysadmin

Test User Account: Username: user-1 | Password: user-1

Instance ID: i-099c369abdaee52e7

esethna

Thanks @miguelespinoza!

- Minor authorship updates - `serialize-error` adopted a differently formatted version of the MIT license text

jasonblais assigned esethna Feb 2, 2018

jasonblais requested a review from esethna February 2, 2018 02:14

jasonblais added 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server labels Feb 2, 2018

miguelespinoza mentioned this pull request Feb 2, 2018

Timezone feature mattermost/mattermost#8185

Merged

2 tasks

miguelespinoza force-pushed the timezone-feature branch from 24067bb to 72e0df0 Compare February 6, 2018 16:25

esethna added the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Feb 7, 2018

miguelespinoza force-pushed the timezone-feature branch 2 times, most recently from 3b816f6 to ae2ac79 Compare February 13, 2018 16:03

miguelespinoza mentioned this pull request Feb 13, 2018

Add automatic and manual timezone support uber-archive/mattermost-mobile#6

Closed

miguelespinoza force-pushed the timezone-feature branch from ae2ac79 to 121bf09 Compare February 20, 2018 14:37

stanchan mentioned this pull request Feb 21, 2018

Timezone feature mattermost/mattermost-mobile#1456

Merged

2 tasks

miguelespinoza force-pushed the timezone-feature branch 2 times, most recently from a64d683 to e390a56 Compare March 5, 2018 16:52

jwilander added the 2: Dev Review Requires review by a core commiter label Mar 5, 2018

jwilander requested review from jespino and jwilander March 5, 2018 18:12

jwilander reviewed Mar 5, 2018

View reviewed changes

jespino reviewed Mar 5, 2018

View reviewed changes

lieut-data reviewed Mar 5, 2018

View reviewed changes

mkraft reviewed Mar 5, 2018

View reviewed changes

hmhealey previously requested changes Mar 6, 2018

View reviewed changes

hmhealey self-assigned this Mar 6, 2018

SmartHoneybee mentioned this pull request Mar 8, 2018

Firefox: Posts show wrong time if resistFingerprinting is enabled mattermost/mattermost#8419

Closed

esethna removed the Setup Old Test Server Triggers the creation of a test server label Mar 30, 2018

mattermost deleted a comment from mattermod Mar 30, 2018

esethna added the Setup Old Test Server Triggers the creation of a test server label Mar 30, 2018

Ensure automaticTImezone is properly not se

5ed9b12

jasonblais added this to the v4.9.0 milestone Apr 2, 2018

mattermost deleted a comment from mattermod Apr 2, 2018

esethna added Setup Old Test Server Triggers the creation of a test server and removed Setup Old Test Server Triggers the creation of a test server labels Apr 2, 2018

esethna approved these changes Apr 2, 2018

View reviewed changes

esethna removed 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server labels Apr 2, 2018

hmhealey approved these changes Apr 2, 2018

View reviewed changes

hmhealey removed the 2: Dev Review Requires review by a core commiter label Apr 2, 2018

hmhealey merged commit 431051f into mattermost:master Apr 2, 2018

amyblais added Changelog/Done Required changelog entry has been written Docs/Done Required documentation has been written labels Apr 4, 2018

lindalumitchell added the Tests/Done Release tests have been written label Apr 5, 2018

hmhealey pushed a commit that referenced this pull request Aug 28, 2020

Update NOTICE.txt (#696)

38fd7be

- Minor authorship updates - `serialize-error` adopted a differently formatted version of the MIT license text

hmhealey pushed a commit that referenced this pull request Mar 17, 2021

Update NOTICE.txt (#696)

8bc1e6c

- Minor authorship updates - `serialize-error` adopted a differently formatted version of the MIT license text

Timezone feature #696

Timezone feature #696

Conversation

csduarte commented Feb 1, 2018 • edited by hmhealey Loading

Summary

Ticket Link

Checklist

Screenshots

manual timezone selection

automatic timezone selection

Feature in action

manual

automatic

miguelespinoza commented Feb 2, 2018

miguelespinoza commented Feb 13, 2018

jwilander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmhealey commented Mar 13, 2018

miguelespinoza commented Mar 29, 2018

miguelespinoza commented Mar 30, 2018

GoldUniform commented Mar 30, 2018

esethna commented Mar 30, 2018

miguelespinoza commented Mar 30, 2018

mattermod commented Apr 2, 2018

esethna commented Apr 2, 2018

mattermod commented Apr 2, 2018

esethna left a comment

Choose a reason for hiding this comment

csduarte commented Feb 1, 2018 •

edited by hmhealey

Loading