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

ayusht2810 · 2024-02-06T12:28:05Z

Summary

Added feature to update custom status of the user during meeting
Added feature to set status of the user from the options provided in the dropdown during the meeting

Ticket Link

#228

Screenshots

What to test?

Custom status gets updated during a meeting.
User status gets updated to the selected value during a meeting.
No change in user status during overlapping meetings or when there are no attendees.
No change of user custom status during overlapping meetings, when there are no attendees, or when there is already a custom status set.

How to test?

Create a meeting in the MS Calendar.
Run slash command /mscalendar settings, select an option from the Update Status setting, and check the updated status on the Mattermost.
Run slash command /mscalendar settings, set the Set Custom Status setting to yes, and check the updated custom status on the Mattermost.

…uring meeting

codecov-commenter · 2024-02-06T12:42:29Z

Codecov Report

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

Project coverage is 24.72%. Comparing base (b1bcb7b) to head (0ceffcd).

Files	Patch %	Lines
server/mscalendar/availability.go	53.68%	33 Missing and 11 partials ⚠️
server/mscalendar/welcome_flow.go	0.00%	13 Missing ⚠️
server/mscalendar/settings.go	0.00%	9 Missing ⚠️
server/mscalendar/welcomer.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #355      +/-   ##
==========================================
+ Coverage   23.71%   24.72%   +1.01%     
==========================================
  Files          62       62              
  Lines        3087     3163      +76     
==========================================
+ Hits          732      782      +50     
- Misses       2274     2292      +18     
- Partials       81       89       +8

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

fmartingr

Hey @ayusht2810, thanks for taking care of this! I just added some minor comments, will defer to @mickmister & @matthewbirtch on flow logic.

I've pointed out some minor things.

Also some screenshots with the changes to easily see the changes without a cloud/local env would be awesome. (I'm talking here about the settings, specifically).

server/mscalendar/notification.go

server/mscalendar/availability.go

…move extra condition

mickmister

Nice work on this @ayusht2810 👍 This feature is pretty complicated in general

I have some comments for discussion before approving. I'd also like to get QA eyes on this before approval as well

mickmister · 2024-02-20T20:33:52Z

server/mscalendar/availability.go

@@ -157,7 +159,7 @@ func (m *mscalendar) setUserStatuses(users []*store.User, calendarViews []*remot
 	numberOfLogs, numberOfUserStatusChange, numberOfUserErrorInStatusChange := 0, 0, 0
 	toUpdate := []*store.User{}
 	for _, u := range users {
-		if u.Settings.UpdateStatus {
+		if u.Settings.UpdateStatusFromOptions != NotSetStatusOption || u.Settings.SetCustomStatus {


What about the existing UpdateStatus setting? Can we still respect that setting? Also, it's probably good to lump this check into a method on the u.Settings struct. We can factor in the legacy UpdateStatus setting there if needed as well

@mickmister I think there is some confusion here.

There were two settings before:

If a user needs to change their status or not.

If yes, it should be away or DND.

In the new flow we have s single setting for this. The user gets three options in a single setting.

Away

DND

Do not set

Because of the new flow the UpdateStatus field does not exist anymore

server/mscalendar/welcomer.go

mickmister · 2024-02-20T20:39:41Z

server/store/flow_store.go

+		user.Settings.UpdateStatusFromOptions = value.(string)
+		s.Tracker.TrackAutomaticStatusUpdate(userID, value.(string), "flow")


Same here to avoid panic. We pretty much never want to do the syntax .(type) without the double return value syntax

Suggested change

user.Settings.UpdateStatusFromOptions = value.(string)

s.Tracker.TrackAutomaticStatusUpdate(userID, value.(string), "flow")

stringValue, _ := value.(string)

user.Settings.UpdateStatusFromOptions = stringValue

s.Tracker.TrackAutomaticStatusUpdate(userID, stringValue, "flow")

Wondering why this didn't panic when value is checked as a bool above. Maybe a change in the behavior of Go in some release, though that's highly doubtful. @hanzei Curious what you think about this

Type assertions with a check variable do not panic if the type doesn't match.

The value of ok is true if the assertion holds. Otherwise it is false and the value of v is the zero value for type T. No run-time panic occurs in this case.

https://go.dev/ref/spec#Type_assertions

See https://goplay.tools/snippet/ETzO-4sgImZ as an example. Only L8 panics, not L6.

@hanzei Yes but there was no check variable on this before

server/store/flow_store.go

server/mscalendar/welcome_flow.go

mickmister · 2024-02-20T20:51:15Z

server/mscalendar/availability.go

+	if checkOverlappingEvents(events) {
+		return "Overlapping events, not setting a custom status", isStatusChanged, nil


What's the reasoning for never setting status if the events overlap? Does this check ever occur before the first event?

What's the reasoning for never setting status if the events overlap?

It was mentioned in the issue here: #228 (comment)

Does this check ever occur before the first event?

@mickmister Not sure what you are asking here. Can you please provide some details?

mickmister · 2024-02-20T20:51:40Z

server/mscalendar/availability.go

+	if events[0].IsCancelled {
+		return "Event cancelled, not setting custom status", isStatusChanged, nil
+	}


I assume the events are sorted by start time? Edit: This is implicitly being done in the call to checkOverlappingEvents above. Note that if the order of operations changes, this will result in a bug here

Updating the logic to filter out the canceled event in the filterBusyEvents function.

mickmister · 2024-02-20T20:54:49Z

server/mscalendar/availability.go

+	sort.Slice(events, func(i, j int) bool {
+		return events[i].Start.Time().UnixMicro() < events[j].Start.Time().UnixMicro()
+	})


This pattern is a little dangerous - implicitly sorting these events in this function. This will have side effects as other piece of code relying on the original order will have the incorrect assumption. Maybe that's not an issue, but something to keep in mind

mickmister · 2024-02-20T20:55:27Z

server/mscalendar/availability.go

+	})
+
+	for i := 1; i < len(events); i++ {
+		if events[i-1].End.Time().UnixMicro() > events[i].Start.Time().UnixMicro() {


Should this be >=?

@mickmister I think what we have is fine as an event can end at 6:30 and another can start at 6:30. I don't think we should consider this as overlapping. Please let me know your opinions on this.

server/mscalendar/availability_test.go

mickmister · 2024-02-20T21:05:07Z

@cwarnermm Can you please take a look at the user-facing strings in this PR? Thanks 🙂

These are primarily in server/mscalendar/welcome_flow.go

cwarnermm

Provided editorial feedback inline for in-product messages.

server/mscalendar/welcome_flow.go

raghavaggarwal2308 · 2024-03-04T13:46:03Z

@mickmister @cwarnermm @hanzei @fmartingr Because of a lot of conflicts on this PR after merging #267 PR. We are creating a separate PR by creating a new branch from the master and cherry-picking the commits of this PR there so that the changes on the master branch are not affected by mistake.

All the review fixes mentioned in this PR are fixed there

New PR link: #359

[MM-188] Add feature to update custom status and status of the user d…

e52c6a3

…uring meeting

ayusht2810 requested a review from mickmister as a code owner February 6, 2024 12:28

ayusht2810 self-assigned this Feb 6, 2024

ayusht2810 requested a review from raghavaggarwal2308 February 6, 2024 12:28

ayusht2810 linked an issue Feb 6, 2024 that may be closed by this pull request

Use custom status for "In a meeting" user state #228

Open

[MM-188] Add test case for custom status change

c36282f

mickmister requested a review from fmartingr February 8, 2024 18:12

fmartingr reviewed Feb 15, 2024

View reviewed changes

server/mscalendar/notification.go Outdated Show resolved Hide resolved

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

server/mscalendar/availability.go Show resolved Hide resolved

server/mscalendar/availability.go Show resolved Hide resolved

[MM-188] Review fixes: Update constant name, add some comments and re…

e625740

…move extra condition

ayusht2810 requested a review from fmartingr February 16, 2024 04:46

Merge branch 'master' into MM-188

43c362f

fmartingr approved these changes Feb 16, 2024

View reviewed changes

mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Feb 20, 2024

mickmister requested a review from AayushChaudhary0001 February 20, 2024 21:01

mickmister reviewed Feb 20, 2024

View reviewed changes

mickmister requested a review from cwarnermm February 20, 2024 21:04

cwarnermm reviewed Feb 21, 2024

View reviewed changes

raghavaggarwal2308 added 2 commits February 27, 2024 21:13

[MM-252] Review fixes

0ceffcd

[MM-250] Review fixes

014bfb6

raghavaggarwal2308 closed this Mar 4, 2024

raghavaggarwal2308 mentioned this pull request Mar 4, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ayusht2810 commented Feb 6, 2024 •

edited

Loading

codecov-commenter commented Feb 6, 2024 •

edited

Loading

fmartingr left a comment

mickmister left a comment

mickmister Feb 20, 2024

raghavaggarwal2308 Feb 26, 2024

mickmister Feb 20, 2024

mickmister Feb 20, 2024

hanzei Feb 28, 2024

mickmister May 21, 2024

mickmister Feb 20, 2024

raghavaggarwal2308 Feb 28, 2024

mickmister Feb 20, 2024

raghavaggarwal2308 Feb 28, 2024

mickmister Feb 20, 2024

mickmister Feb 20, 2024

raghavaggarwal2308 Feb 28, 2024

mickmister commented Feb 20, 2024 •

edited

Loading

cwarnermm left a comment

raghavaggarwal2308 commented Mar 4, 2024 •

edited

Loading

		user.Settings.UpdateStatusFromOptions = value.(string)
		s.Tracker.TrackAutomaticStatusUpdate(userID, value.(string), "flow")

-		user.Settings.UpdateStatusFromOptions = value.(string)
-		s.Tracker.TrackAutomaticStatusUpdate(userID, value.(string), "flow")
+		stringValue, _ := value.(string)
+		user.Settings.UpdateStatusFromOptions = stringValue
+		s.Tracker.TrackAutomaticStatusUpdate(userID, stringValue, "flow")

		if checkOverlappingEvents(events) {
		return "Overlapping events, not setting a custom status", isStatusChanged, nil

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

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

Conversation

ayusht2810 commented Feb 6, 2024 • edited Loading

Summary

Ticket Link

Screenshots

What to test?

How to test?

codecov-commenter commented Feb 6, 2024 • edited Loading

Codecov Report

fmartingr left a comment

Choose a reason for hiding this comment

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

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 Feb 20, 2024 • edited Loading

cwarnermm left a comment

Choose a reason for hiding this comment

raghavaggarwal2308 commented Mar 4, 2024 • edited Loading

ayusht2810 commented Feb 6, 2024 •

edited

Loading

codecov-commenter commented Feb 6, 2024 •

edited

Loading

mickmister commented Feb 20, 2024 •

edited

Loading

raghavaggarwal2308 commented Mar 4, 2024 •

edited

Loading