Refactor calendar engine into its own package #267

mickmister · 2023-07-24T22:33:57Z

Summary

This pull requests refactors the main calendar engine used by the mscalendar plugin and leverages the remotes implementation to allow other plugins to easily import this plugin providing its own remote to easily create other calendar plugins.

This work was done to create the mattermost-plugin-google-calendar that could be used as a template, with the caveheat that the google calendar plugin contains it's own frontend code to create events and that is currently not shared and it's a task to tackle in the future.

…tion gcal methods

mickmister · 2023-07-24T22:39:18Z

@fmartingr I've reopened this on a Mattermost-owned branch. Also notice that the base branch is gcal-master, which is currently equal to the master branch. 1/5 Let's use gcal-master as the protected branch for this project.

mickmister · 2023-07-24T22:40:32Z

server/mscalendar/notification_test.go

@@ -91,6 +92,8 @@ func newTestNotification(clientState string, recommendRenew bool) *remote.Notifi
 }

 func TestProcessNotification(t *testing.T) {
+	t.Skip("TODO gcal implement TestProcessNotification")


Test needs to be updated

codecov-commenter · 2023-07-25T05:11:36Z

Codecov Report

Attention: Patch coverage is 23.51454% with 605 lines in your changes are missing coverage. Please review.

Project coverage is 20.94%. Comparing base (b1bcb7b) to head (1de44d0).

Files	Patch %	Lines
calendar/engine/notification.go	5.78%	114 Missing ⚠️
calendar/engine/welcome_flow.go	0.00%	82 Missing ⚠️
calendar/command/daily_summary.go	0.00%	65 Missing ⚠️
calendar/utils/kvstore/crypt.go	35.00%	45 Missing and 7 partials ⚠️
calendar/engine/availability.go	53.77%	38 Missing and 11 partials ⚠️
msgraph/event.go	0.00%	44 Missing ⚠️
calendar/command/command.go	30.30%	22 Missing and 1 partial ⚠️
calendar/engine/daily_summary.go	62.06%	16 Missing and 6 partials ⚠️
calendar/engine/user.go	19.23%	21 Missing ⚠️
msgraph/msgraph.go	0.00%	18 Missing ⚠️
... and 27 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #267      +/-   ##
==========================================
- Coverage   23.71%   20.94%   -2.77%     
==========================================
  Files          62       67       +5     
  Lines        3087     3442     +355     
==========================================
- Hits          732      721      -11     
- Misses       2274     2628     +354     
- Partials       81       93      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mickmister

@fmartingr I gave the plugin a review, and noted some things within this commit d78381a. Note that the commit itself is the review. Please take a look and share any thoughts you might have.

I added some PR review comments on top of the commit, to gather info that is provider-specific. The goal is to put them all in one struct. I plan to search for these strings in the code to find more:

microsoft
msgraph
mscalender
mscal
gcal
google

I think the notification subscription lifecycle is the one thing that seems murky with regards to remaining work to handle both providers. I need to better understand gcal's subscription lifecycle to connect the dots on the code related to subscriptions.

mickmister · 2023-07-25T05:11:10Z

server/command/info.go

+// REVIEW: hardcoded "microsoft"
 func (c *Command) info(parameters ...string) (string, bool, error) {
 	resp := fmt.Sprintf("Mattermost Microsoft Calendar plugin version: %s, "+
 		"[%s](https://github.com/mattermost/%s/commit/%s), built %s\n",


config: plugin name

mickmister · 2023-07-25T05:12:08Z

server/config/const.go

+// REVIEW: need an interface for returning bot info
+// probably good to have a struct to capture the data clump
 const (
 	BotUserName    = "gcal"
 	BotDisplayName = "Google Calendar"


all fields being used here, that should be put into a struct

BotUserName = "gcal" BotDisplayName = "Google Calendar" BotDescription = "Created by the Google Calendar Plugin." ApplicationName = "Google Calendar" Repository = "mattermost-plugin-gcal" CommandTrigger = "gcal" TelemetryShortName = "gcal"

mickmister · 2023-07-25T05:14:17Z

server/mscalendar/settings_notifications.go

+		// REVIEW: notification subscription logic in notification settings. this seems a bit weird. what does this function/block have to do specifically with notifications?
 		_, err := cal.LoadMyEventSubscription()
 		if err != nil {


Not really sure what's going on here. It seems like:

"If a setting is a boolean, create an event subscription if not exists"

This is notificationSetting.Set, so I suppose this is "I want to change notification settings". what do we do when the value hasn't changed? I suppose this function takes care of no-ops

mickmister · 2023-07-25T05:16:05Z

server/mscalendar/subscription.go

@@ -21,6 +21,7 @@ type Subscriptions interface {
 	LoadMyEventSubscription() (*store.Subscription, error)
 }

+// REVIEW: depends on the overlap of subscription logic between providers, but lots of logic about supscription lifecycle in this file


Need to figure out if google's subscription lifecycle lines up with this

mickmister · 2023-07-25T05:17:10Z

server/mscalendar/welcomer.go

+	// REVIEW: "microsoft" referenced here
 	WelcomeMessage = `Welcome to the Microsoft Calendar plugin.
 	[Click here to link your account.](%s/oauth2/connect)`


"Microsoft Calendar plugin" mentioned here

mickmister · 2023-07-25T05:17:21Z

server/mscalendar/welcomer.go

+	// REVIEW: "microsoft" referenced here
 	text := ":tada: Congratulations! Your microsoft account (*" + userLogin + "*) has been connected to Mattermost."
 	return &model.SlackAttachment{


"Microsoft" mentioned here

mickmister · 2023-07-25T05:17:42Z

server/plugin/plugin.go

+		// REVIEW: need to make this provider agnostic terminology
 		mscalendarBot := mscalendar.NewMSCalendarBot(e.bot, e.Env, pluginURL)


mscalendar terminology leaked into plugin package. not much else than what's visible here though

mickmister · 2023-07-25T05:18:31Z

server/utils/oauth2connect/oauth2_complete.go

@@ -28,6 +28,7 @@ func (oa *oa) oauth2Complete(w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	// REVIEW "microsoft" is hardcoded


"Microsoft Calendar" is hardcoded

mickmister · 2023-07-25T05:29:54Z

Follow ups:

make tickets for all known remaining tasks
review original migration PR to make note of any significant changes for gcal case
remove features/endpoints:
- accept/decline/tentative
- create/delete calendar

fmartingr · 2023-07-25T06:22:04Z

assets/profile.png

This we should take care as well.

fmartingr · 2023-07-25T06:38:04Z

I need to better understand gcal's subscription lifecycle to connect the dots on the code related to subscriptions

Using the Events.watch endpoint to create a channel, with a default TTL of 7 days, channel can be stopped using Channels.stop.

* remote logic * allow datetimes without timezone * add calendar settings readonly scope to get tz * event conversion * scopes * revert datetime.time

…268) * plugin profile images * receive provider flag * manifests * retrieve manifest id from file * Build time configuration * revert manifest * using go static configuration for providers * removed unused build flags * revert manifest * Added basic Google Calendar instructions * Update providers/GoogleCalendar.md Co-authored-by: Michael Kochell <[email protected]> * Update providers/GoogleCalendar.md Co-authored-by: Michael Kochell <[email protected]> * Update server/plugin/plugin.go Co-authored-by: Michael Kochell <[email protected]> * remoove log --------- Co-authored-by: Michael Kochell <[email protected]>

* user_store: more information fields on the index * feat: endpoint to autocomplete calendar connected users * revert test event store hashed * Update server/api/autocomplete.go Co-authored-by: Michael Kochell <[email protected]> * Update server/api/autocomplete.go Co-authored-by: Michael Kochell <[email protected]> * dont expose private data in endpoint * lint * Update server/store/user_store.go Co-authored-by: Michael Kochell <[email protected]> --------- Co-authored-by: Michael Kochell <[email protected]>

…276) * chore: replace provider display name * chore: replace command trigger references * revert previous changes

…ot supported (#272) * nil control * use subscriptions default ttl * return specific error when no superuser token can be used * notification formatting logic to other file * Client.GetEventsBetweenDates * typo: import * shorter command name :) * availability without super user token * delete store subs only if we successfuly do on remote * comment * ignore missing subs * update sync method * allow switching acting user on the fly * daily summary without super user token * formatted imports * calendar -> calendarEvents * processAllDailySummaryByUser -> processAllDailySummaryWithIndividualCredentials * revert silenced error under notifications * msgraph GetEventsBetweenDates * Update server/mscalendar/daily_summary.go Co-authored-by: Michael Kochell <[email protected]> * [GCAL] Create event logic (#269) * remote logic * allow datetimes without timezone * add calendar settings readonly scope to get tz * event conversion * scopes * revert datetime.time * Client.GetEventsBetweenDates * fix: store last post time * refactored availability logic * availability test * refactored daily summary logic * test: availability * daily summary tests (wip) * daily summary test * fix: merge duplications * slack attachment for notifications * lint and test * goimports * Update server/mscalendar/availability.go Co-authored-by: Michael Kochell <[email protected]> * remove logges * nullify client when changing acting user * move withActiveUser filter to fenced code outside of general method * tests: handling more scenarios * Fixed test after bugfix * fixed test assertions * allow engine copy to perform safe state mutations --------- Co-authored-by: Michael Kochell <[email protected]>

* Added encrypted kvstore * encrypt only oauth2 kv store * retrieve encryption setting from provider config * explicitly disable encrypted store for mscalendar * remove old comment

AayushChaudhary0001 · 2023-11-30T07:42:37Z

Tested this PR.

2 commands, namely createevent and findmeetings do not have any functionality mentioned anywhere and is not clear after executing those commands as well.
Also, on adding any calendar through createcal, I am not able to find from where to add it in the Outlook window. Can you please have a look at those two and this issue?

Rest all are tested and approved from my side.

mickmister · 2023-11-30T07:50:17Z

Thanks so much @AayushChaudhary0001. All 3 of those commands are generally unimplemented and should be removed from the codebase

fmartingr · 2023-12-01T08:45:24Z

Thanks so much @AayushChaudhary0001. All 3 of those commands are generally unimplemented and should be removed from the codebase

Should I do that in a separate PR?

mickmister · 2023-12-01T18:48:08Z

@fmartingr Yeah that sounds good to me

mickmister · 2024-02-01T16:07:03Z

Rest all are tested and approved from my side.

@fmartingr It seems this PR has been approved by QA. Should we merge this and move forward with a release of this plugin, and also update the gcal plugin's commit to the next tag once created?

fmartingr · 2024-02-02T08:41:25Z

Rest all are tested and approved from my side.

@fmartingr It seems this PR has been approved by QA. Should we merge this and move forward with a release of this plugin, and also update the gcal plugin's commit to the next tag once created?

Absolutely! You take care of mscalendar and I take care of gcal?

fmartingr · 2024-02-15T11:13:30Z

Solved conflicts from latest changes.

mickmister · 2024-02-20T08:21:45Z

@fmartingr Do you think this is good to merge based on QA's approval?

fmartingr · 2024-02-20T09:07:24Z

@fmartingr Do you think this is good to merge based on QA's approval?

Yeah, apart from QA we did some testing ourselves back in the day, so I think this is good to go.

mickmister · 2024-02-20T17:26:16Z

@fmartingr I need at least one dev's explicit approval on this to merge it. Can you please take a look and approve if it looks good? (which it does based on your comment)

fmartingr

Considering this passed through QA and the Google Calendar plugin works from this, I'm giving my approval, but if we need a proper 2nd dev review I would add someone else, since we are the authors of this PR so my approval seems like cheating.

mickmister · 2024-02-28T02:09:10Z

@larkox Do you mind giving this a review?

larkox

Apart of the many "REVIEW" comments, everything seems fine. And I find this a great idea!

Are we planning on dealing with the review comments now or later?

mickmister · 2024-03-01T04:01:59Z

@larkox I created two followup issues from those "REVIEW" comments. 3/5 none of them should be blocking for this

#357
#358

mickmister and others added 12 commits December 11, 2020 11:37

oauth works

00e1eb0

autocomplete

742a49c

viewcal works

7f238a2

add unimplemented if blocks

19ff374

Merge branch 'master' into migrate-to-gcal

38ebd1f

fix tests and lint

a88921f

implement CreateMySubscription, RenewSubscription, and DeleteSubscrip…

8259bde

…tion gcal methods

WIP gcal subs

83db731

setup-attach make target

b057dcf

Merge branch 'master' into gcal-merge

863cb37

fix lint and tests

38f3fdf

chore: more compatible tar command

9ed5ee6

mickmister mentioned this pull request Jul 24, 2023

[Gcal] Update master and dependencies #266

Closed

mickmister mentioned this pull request Jul 24, 2023

Initial gcal implementation (from fork) #265

Closed

mickmister commented Jul 24, 2023

View reviewed changes

add review comments

d78381a

mickmister commented Jul 25, 2023

View reviewed changes

fmartingr reviewed Jul 25, 2023

View reviewed changes

assets/profile.png Outdated

Copy link

Contributor

fmartingr Jul 25, 2023

Choose a reason for hiding this comment

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

This we should take care as well.

fmartingr added 2 commits July 25, 2023 16:54

[GCAL] Create event logic (#269)

728630e

* remote logic * allow datetimes without timezone * add calendar settings readonly scope to get tz * event conversion * scopes * revert datetime.time

bug: get timezone from datetime instead of event data (#271)

585e821

fmartingr changed the title ~~Initial gcal implementation~~ [GCAL] Initial Google Calendar implementation Jul 27, 2023

fmartingr and others added 5 commits July 31, 2023 13:15

[GCAL] Refactor Microsoft Calendar references to provider references (#…

e3d6a94

…276) * chore: replace provider display name * chore: replace command trigger references * revert previous changes

[GCAL] Added encrypted key value storage (#270)

34ca3ed

* Added encrypted kvstore * encrypt only oauth2 kv store * retrieve encryption setting from provider config * explicitly disable encrypted store for mscalendar * remove old comment

mickmister mentioned this pull request Nov 29, 2023

Disable unused-parameter golangci check #346

Merged

This was referenced Dec 14, 2023

Trying to apply a user settings fails with a 400 bad request error mattermost/mattermost-plugin-google-calendar#66

Open

refactor: remove unused commands #349

Merged

mickmister mentioned this pull request Feb 15, 2024

[MM-102] Update plugin with respect to phase 1 upgrades #351

Merged

fmartingr added 3 commits February 15, 2024 11:50

Merge remote-tracking branch 'origin/master' into migrate-to-gcal

bf4cefe

goimports

783c2f0

removed apply command from merge

ae5f3d8

mickmister requested a review from raghavaggarwal2308 February 20, 2024 07:54

fmartingr approved these changes Feb 22, 2024

View reviewed changes

mickmister requested a review from larkox February 28, 2024 02:09

larkox approved these changes Feb 29, 2024

View reviewed changes

remove "REVIEW:" comments

1de44d0

mickmister mentioned this pull request Mar 1, 2024

Followups for shared calendar engine PR #267 #357

Open

mickmister merged commit 7f1bbcd into master Mar 1, 2024
6 checks passed

mickmister deleted the migrate-to-gcal branch March 1, 2024 04:02

raghavaggarwal2308 mentioned this pull request Mar 4, 2024

[MM-188] Add feature to update custom status and status of the user during meeting #355

Closed

mickmister mentioned this pull request Mar 14, 2024

Release 1.3.0 #361

Merged

raghavaggarwal2308 added this to the v1.3.0 milestone Aug 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor calendar engine into its own package #267

Refactor calendar engine into its own package #267

mickmister commented Jul 24, 2023 •

edited by fmartingr

Loading

mickmister commented Jul 24, 2023

mickmister Jul 24, 2023 •

edited

Loading

codecov-commenter commented Jul 25, 2023 •

edited

Loading

mickmister left a comment

mickmister Jul 25, 2023

mickmister Jul 25, 2023

mickmister Jul 25, 2023

mickmister Jul 25, 2023

mickmister Jul 25, 2023

mickmister Jul 25, 2023

mickmister Jul 25, 2023

mickmister Jul 25, 2023

mickmister commented Jul 25, 2023

fmartingr Jul 25, 2023

fmartingr commented Jul 25, 2023

AayushChaudhary0001 commented Nov 30, 2023

mickmister commented Nov 30, 2023

fmartingr commented Dec 1, 2023

mickmister commented Dec 1, 2023

mickmister commented Feb 1, 2024

fmartingr commented Feb 2, 2024

fmartingr commented Feb 15, 2024

mickmister commented Feb 20, 2024

fmartingr commented Feb 20, 2024

mickmister commented Feb 20, 2024

fmartingr left a comment

mickmister commented Feb 28, 2024

larkox left a comment

mickmister commented Mar 1, 2024 •

edited

Loading

		// REVIEW: need to make this provider agnostic terminology
		mscalendarBot := mscalendar.NewMSCalendarBot(e.bot, e.Env, pluginURL)

Refactor calendar engine into its own package #267

Refactor calendar engine into its own package #267

Conversation

mickmister commented Jul 24, 2023 • edited by fmartingr Loading

Summary

mickmister commented Jul 24, 2023

mickmister Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jul 25, 2023 • edited Loading

Codecov Report

mickmister 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

mickmister commented Jul 25, 2023

Choose a reason for hiding this comment

fmartingr commented Jul 25, 2023

AayushChaudhary0001 commented Nov 30, 2023

mickmister commented Nov 30, 2023

fmartingr commented Dec 1, 2023

mickmister commented Dec 1, 2023

mickmister commented Feb 1, 2024

fmartingr commented Feb 2, 2024

fmartingr commented Feb 15, 2024

mickmister commented Feb 20, 2024

fmartingr commented Feb 20, 2024

mickmister commented Feb 20, 2024

fmartingr left a comment

Choose a reason for hiding this comment

mickmister commented Feb 28, 2024

larkox left a comment

Choose a reason for hiding this comment

mickmister commented Mar 1, 2024 • edited Loading

mickmister commented Jul 24, 2023 •

edited by fmartingr

Loading

mickmister Jul 24, 2023 •

edited

Loading

codecov-commenter commented Jul 25, 2023 •

edited

Loading

mickmister commented Mar 1, 2024 •

edited

Loading