[MM-19893] Outlook user status sync intial implementation #18

mickmister · 2019-12-19T05:44:46Z

Summary

This PR implements the user status sync feature, between the Outlook Calendar and Mattermost.

Ticket Link

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

server/api/availability.go

server/job/recurring_job.go

server/plugin/plugin.go

server/remote/common.go

mickmister · 2019-12-19T05:59:22Z

server/remote/msgraph/get_schedule_batched.go

+	AvailabilityViewInterval int `json:"availabilityViewInterval"`
+}
+
+func (c *client) GetSchedule(remoteUserID string, schedules []string, startTime, endTime *remote.DateTime, availabilityViewInterval int) ([]*remote.ScheduleInformation, error) {


Even though we are using app-level creds, we still need to provide a user id to the request. The body of the request contains several users' information, but the url needs to have one in it as well.

Our options are

POST /me/calendar/getSchedule POST /users/{id|userPrincipalName}/calendar/getSchedule

me doesn't work for the app-level user, and I don't think it has a user id. This endpoint specifically supports app-level users, so I'm not sure why we need to provide a user id in the url, or what the point of the of id is if the users we are searching for are in the body.

https://docs.microsoft.com/en-us/graph/api/calendar-getschedule

We are currently using the first user's id in the kvstore.

See my comment about using (and mapping) the BotAccount. We can still use special-case the credentials to invoke graph, but require that a user is specified.

server/remote/msgraph/get_schedule_batched.go

server/remote/msgraph/remote.go

levb

@mickmister this is awesome! great job figuring things out. Unfortunately, this is a lot of code and I can't do a thorough-enough review of everything. I wanted to comment on a couple style/code structure things that jumped out at me right away, will add more later.

plugin.json

server/api/api.go

server/api/availability.go

levb · 2019-12-22T16:16:43Z

server/api/availability.go

+)
+
+type availabilityJob struct {
+	api API


I tried to avoid this pattern in notification.go - the thinking was that api is for an "acting user", has (at least) the current user's MattermostUserID etc. associated with it.

As I was working on solar lottery, i realized that this totally makes sense, because the api should just be operating in the context of the bot user. In fact, I am thinking that maybe the Bot User is the right concept to use for the "application-level" client. Can talk offline about it, no immediate change request here.

levb · 2019-12-22T16:18:27Z

server/api/availability.go

+}
+
+func (j *availabilityJob) Run() {
+	c := cron.New()


Need (a separate ticket) for HA-proofing this by using a distributed lock to run the cron job on one server only. (failover??? does the lock expire?)

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

Removed cron dependency in favor of using a ticker select loop, so we have more control.

server/api/availability.go

levb · 2019-12-22T16:40:42Z

server/remote/msgraph/get_schedule_batched.go

+	AvailabilityViewInterval int `json:"availabilityViewInterval"`
+}
+
+func (c *client) GetSchedule(remoteUserID string, schedules []string, startTime, endTime *remote.DateTime, availabilityViewInterval int) ([]*remote.ScheduleInformation, error) {


See my comment about using (and mapping) the BotAccount. We can still use special-case the credentials to invoke graph, but require that a user is specified.

server/remote/msgraph/get_schedule_batched.go

server/remote/schedule.go

server/store/user_store.go

crspeller

Couple comments. Will do a more through review when this has been cleaned up.

server/remote/schedule.go

crspeller · 2020-01-07T18:21:29Z

server/remote/schedule.go

+
+	// 0= free, 1= tentative, 2= busy, 3= out of office, 4= working elsewhere.
+	// example "0010", which means free for first and second block, tentative for third, and free for fourth
+	AvailabilityView string `json:"availabilityView,omitempty"`


0/5 Maybe define a type for this? type Availability string

types aliases are in fashion these days

server/remote/msgraph/get_schedule_batched.go

crspeller · 2020-01-07T18:31:15Z

server/remote/msgraph/get_schedule_batched.go

+	numRequestsInBatch := 1
+	// numRequestsInBatch := 20
+
+	for i := 0; i < numRequestsInBatch; i++ {


Is this for testing? I am not sure what this function is trying to accomplish.

Yes, this was a quick and dirty way to test the sizes of batches the MSGraph API accepts, because it is not documented. This will be cleaned up in this PR.

…thods

mickmister · 2020-01-11T00:28:02Z

This is ready for review. I've created tickets to track further work in the project's epic https://mattermost.atlassian.net/browse/MM-19881

levb

Some nits and style suggestions, all optional.

server/api/api.go

server/api/availability.go

server/config/config.go

server/store/user_store.go

server/store/store.go

server/remote/schedule.go

server/remote/msgraph/call.go

server/remote/msgraph/get_notification_data.go

server/remote/client.go

server/plugin/plugin.go

* rename NewClient to MakeClient * rename EnableStatusSyncJob to EnableStatusSync * rename CallURLEncodedForm to CallFormPost * rename allUsers to userIndex * Move availability view constants to remote package * Implement UserIndex methods to access as a map * Remove call to status sync job in OnActivate * Remove redundant Call method on remote client interface

mickmister · 2020-01-13T01:09:17Z

@levb I've addressed your feedback in the latest commits.

server/store/user_store.go

crspeller · 2020-01-13T16:55:31Z

server/api/api.go

@@ -65,6 +74,7 @@ type Dependencies struct {
 	Poster            bot.Poster
 	Remote            remote.Remote
 	IsAuthorizedAdmin func(userId string) (bool, error)
+	API               plugin.API


Probably solve this now. Also maybe change to PluginAPI because it doesn't read well with the whole package being called api.

crspeller · 2020-01-13T16:58:22Z

server/api/availability.go

+	return api.setUserStatusFromAvailability(api.mattermostUserID, status.Status, av), nil
+}
+
+func (api *api) SyncStatusForAllUsers() (string, error) {


Could we split this into SyncStatusForUsers(users) and SyncStatusForAllUsers? Then we could make the implementation of SyncStatusForSingleUser just calling SyncStatusForUsers with one user.

I've made the SyncStatusForUsers method and have the other two methods calling that one, to reduce code duplication.

crspeller · 2020-01-13T17:01:54Z

server/api/status_sync_job.go

+const JOB_INTERVAL = 5 * time.Minute
+
+type StatusSyncJob struct {
+	api        API


This is confusing because we usually call the plugin API this.

I think that once the other is renamed to PluginAPI and p is not replicated in the code, api becomes self-explanatory; but I'd love a better name; maybe app (for everything, package and all) despite all of its negative properties?

Separate ticket for renaming package if we plan on doing so?

crspeller · 2020-01-13T17:03:58Z

server/plugin/plugin.go

@@ -207,3 +213,26 @@ func (p *Plugin) loadTemplates(bundlePath string) error {
 	p.Templates = templates
 	return nil
 }
+
+func (p *Plugin) initUserStatusSyncJob() {


This is going to get run once for every node. Can we put a comment here to make sure that is known?

I started naming methods that are not ready for production more explicitly, Debug... for instance (or maybe POC in this case?). 0/5 Intention-revealing names are better than comments.

* extract PluginAPI interface * refactor sync status code * write test for user status sync * comment on POC_initStatusSyncJob * type alias for remote AvailabilityView string

mickmister · 2020-01-14T08:24:54Z

@levb @crspeller Please re-review when you have the chance

crspeller

@mickmister Should there be a QA review label on this or do we have an agreement to test on master?

mickmister · 2020-01-17T15:56:36Z

@crspeller So far, we have not gotten @DHaussermann involved with testing. I'll meet with him and get him set up to test master.

* [MM-843]: Added server testcase for kvstore/plugin_store.go file * [MM-843]: removed ununsed vars

* [MM-843]: Added server testcase for kvstore/plugin_store.go file (#18) * [MM-843]: Added server testcase for kvstore/plugin_store.go file * [MM-843]: removed ununsed vars * updated go.mod and go.sum entries * refactored mock plugin setup --------- Co-authored-by: Doug Lauder <[email protected]> Co-authored-by: Raghav Aggarwal <[email protected]>

mickmister added 5 commits December 13, 2019 17:41

availability slash command updates user status based on getSchedule

8661144

app-only working. batching needs to be done now

c4696c7

batch request works

21ea2c6

get rid of AppLevelClient struct/interface

71f51de

remove dead code

562957f

mickmister added the 2: Dev Review Requires review by a core committer label Dec 19, 2019

mickmister requested review from levb and jfrerich December 19, 2019 05:44

lint

62eb118

mickmister commented Dec 19, 2019

View reviewed changes

mickmister added 2 commits December 19, 2019 01:00

move app-level user auth token logic into its own file

ba38c04

lint

d9572cd

levb reviewed Dec 19, 2019

View reviewed changes

plugin.json Outdated Show resolved Hide resolved

server/api/api.go Outdated Show resolved Hide resolved

server/api/api.go Outdated Show resolved Hide resolved

server/api/availability.go Outdated Show resolved Hide resolved

levb suggested changes Dec 22, 2019

View reviewed changes

levb requested review from crspeller and removed request for jfrerich January 6, 2020 16:33

crspeller requested changes Jan 7, 2020

View reviewed changes

mickmister added 3 commits January 8, 2020 14:10

make job cancelable through system console. rename status sync api me…

cb18446

…thods

rename AppLevelClient to SuperuserClient

517ce03

batch requests properly to handle > 400 users

c7ef852

mickmister requested review from levb and crspeller January 9, 2020 16:44

mickmister added 3 commits January 10, 2020 12:08

change AllUsers name to UserIndex. handle getSchedule error.

b029e30

Merge branch 'master' into MM-19893-availability

9a2f647

clean up batch response unmarshaling

95e9d6f

levb previously approved these changes Jan 11, 2020

View reviewed changes

mickmister dismissed levb’s stale review via d37666a January 13, 2020 01:08

mickmister requested a review from levb January 13, 2020 01:09

mickmister added 2 commits January 12, 2020 20:13

rename var

c3f428b

reorder methods

7fd1a69

mickmister commented Jan 13, 2020

View reviewed changes

server/store/user_store.go Show resolved Hide resolved

update mocks

c2f1e71

crspeller requested changes Jan 13, 2020

View reviewed changes

update for PR feedback:

dcc16ef

* extract PluginAPI interface * refactor sync status code * write test for user status sync * comment on POC_initStatusSyncJob * type alias for remote AvailabilityView string

mickmister requested a review from crspeller January 14, 2020 08:24

crspeller approved these changes Jan 16, 2020

View reviewed changes

levb approved these changes Jan 20, 2020

View reviewed changes

levb added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jan 20, 2020

levb merged commit 6040591 into master Jan 20, 2020

levb deleted the MM-19893-availability branch January 20, 2020 21:39

Kshitij-Katiyar added a commit that referenced this pull request Oct 14, 2024

[MM-843]: Added server testcase for kvstore/plugin_store.go file (#18)

3a2b112

* [MM-843]: Added server testcase for kvstore/plugin_store.go file * [MM-843]: removed ununsed vars

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-19893] Outlook user status sync intial implementation #18

[MM-19893] Outlook user status sync intial implementation #18

mickmister commented Dec 19, 2019 •

edited

Loading

mickmister Dec 19, 2019 •

edited

Loading

levb Dec 22, 2019

levb left a comment

levb Dec 22, 2019

levb Dec 22, 2019

mickmister Jan 9, 2020

levb Dec 22, 2019

crspeller left a comment

crspeller Jan 7, 2020

levb Jan 7, 2020

crspeller Jan 7, 2020

mickmister Jan 7, 2020

mickmister commented Jan 11, 2020

levb left a comment

mickmister commented Jan 13, 2020 •

edited

Loading

crspeller Jan 13, 2020

crspeller Jan 13, 2020

mickmister Jan 14, 2020

crspeller Jan 13, 2020

levb Jan 13, 2020

mickmister Jan 14, 2020

crspeller Jan 13, 2020

levb Jan 13, 2020

mickmister commented Jan 14, 2020

crspeller left a comment

mickmister commented Jan 17, 2020

[MM-19893] Outlook user status sync intial implementation #18

[MM-19893] Outlook user status sync intial implementation #18

Conversation

mickmister commented Dec 19, 2019 • edited Loading

Summary

Ticket Link

mickmister Dec 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levb 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

crspeller 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

mickmister commented Jan 11, 2020

levb left a comment

Choose a reason for hiding this comment

mickmister commented Jan 13, 2020 • edited Loading

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

mickmister commented Jan 14, 2020

crspeller left a comment

Choose a reason for hiding this comment

mickmister commented Jan 17, 2020

mickmister commented Dec 19, 2019 •

edited

Loading

mickmister Dec 19, 2019 •

edited

Loading

mickmister commented Jan 13, 2020 •

edited

Loading