Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[MM-21689] Handle timezone complications between Go and Microsoft Graph #25

Merged
merged 15 commits into from
Jan 30, 2020
Merged

[MM-21689] Handle timezone complications between Go and Microsoft Graph #25

merged 15 commits into from
Jan 30, 2020

Conversation

mickmister
Copy link
Contributor

@mickmister mickmister commented Jan 24, 2020 •
edited
Loading

Summary

This PR handles two things regarding timezones:

  • The conversion of Microsoft-compatible datetime values to/from Go-compatible datetime values.
  • When processing user-provided data, or displaying data to the user, the user's calendar timezone is taken into account. This is applied to the viewcal and findmeetings, and createevent commands at the moment.

Ticket Link

https://mattermost.atlassian.net/browse/MM-21689

@mickmister mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jan 24, 2020
mickmister
mickmister commented Jan 24, 2020
View reviewed changes
server/utils/timezones.go Outdated Show resolved Hide resolved
mickmister
mickmister commented Jan 24, 2020
View reviewed changes
server/remote/common.go Outdated Show resolved Hide resolved
levb
levb suggested changes Jan 25, 2020
View reviewed changes
server/remote/msgraph/get_user_mailbox_settings.go Outdated Show resolved Hide resolved
server/remote/common.go Outdated Show resolved Hide resolved
server/utils/timezones.go Outdated Show resolved Hide resolved
server/utils/timezones.go Outdated Show resolved Hide resolved
server/utils/timezones.go Outdated Show resolved Hide resolved
server/utils/timezones.go Outdated Show resolved Hide resolved
server/utils/timezones.go Outdated Show resolved Hide resolved
@levb
Copy link
Contributor

levb commented Jan 25, 2020

overall comment on naming the util functions, tz.Go(), and tz.Microsoft() seem to be the most intention-revealing, concise names?

@mickmister
address PR feedback:
97cb3c3 
* make utils/tz package
* convert mapping array to a map available at compile-time to avoid map creation at run-time
* unit test for testing go-compatibility of all timezones in conversion map
* some renaming
@mickmister
Merge branch 'master' into timezones
f8aa82f
@mickmister
lint
60e5249
crspeller
crspeller requested changes Jan 27, 2020
View reviewed changes
go.sum Outdated Show resolved Hide resolved
server/api/calendar.go Outdated
return "", err
}

var remoteUserID string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this is handled in solar lottery as a separate function do the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crspeller Can you confirm I've addressed this in the most recent push?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not sure what the ask is here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I wasn't clear. I like the split out you did. I really meant the withX stuff the solar lottery plugin does.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming in #29

jfrerich
jfrerich previously approved these changes Jan 27, 2020
View reviewed changes
Copy link
Contributor

@jfrerich jfrerich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Lots of work put into this, @mickmister

crspeller
crspeller previously approved these changes Jan 27, 2020
View reviewed changes
@mickmister mickmister dismissed stale reviews from crspeller and jfrerich via d4851dc January 28, 2020 00:48
@mickmister mickmister requested a review from levb January 28, 2020 00:48
levb
levb suggested changes Jan 28, 2020
View reviewed changes
Copy link
Contributor

@levb levb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some nits and musings

server/utils/tz/conversion.go Outdated Show resolved Hide resolved
server/remote/common.go Outdated Show resolved Hide resolved
server/utils/tz/conversion.go Outdated Show resolved Hide resolved
server/remote/msgraph/get_user_mailbox_settings.go Outdated Show resolved Hide resolved
server/remote/common.go Outdated Show resolved Hide resolved
server/remote/common.go Outdated Show resolved Hide resolved
server/remote/common.go Outdated Show resolved Hide resolved
server/remote/common.go Show resolved Hide resolved
server/plugin/command/view_calendar.go Outdated Show resolved Hide resolved
server/api/event.go Show resolved Hide resolved
@levb
Copy link
Contributor

levb commented Jan 28, 2020

Totally OT, 0/5: I really like looking at the godoc for each package I make, should do it more. Without much actual contents of comments, it should be readable, intuitive, and typographically sound. Combine that with never indenting the go code over 4 deep, and one is 80% perfect.

@mickmister
address PR feedback:
7a76d8b 
* rename ConvertToTimezone -> In
* inline tz.Go()
* rename remote.NewMicrosoftDateTime -> remote.NewDateTime
* rename common.go -> date_time.go
* remove tz package alias
* use URL builder
@mickmister
move timezone data to tz/data.go
5b519c9
@mickmister mickmister requested a review from levb January 28, 2020 16:50
@levb levb merged commit 556cf68 into master Jan 30, 2020
@levb levb deleted the timezones branch January 30, 2020 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crspeller crspeller crspeller approved these changes

@levb levb levb approved these changes

@jfrerich jfrerich jfrerich left review comments

@DHaussermann DHaussermann Awaiting requested review from DHaussermann

Assignees
No one assigned
Labels
2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mickmister @levb @crspeller @jfrerich