[GH-89] Allow status change to support DND and Away #118
Conversation
…during a meeting (away) or not (DND)
larkox
added
2: Dev Review
server/mscalendar/availability.go
Outdated
| if err != nil { | ||
| return "", err | ||
| } | ||
| err = m.Store.StoreUserActiveEvents(user.MattermostUserID, remoteHashes) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return "User was free, but is now busy. Set status to DND.", nil | ||
| return "User was free, but is now busy. Set status to busy.", nil |
There was a problem hiding this comment.
Can you inject the busyStatus variable here? As well as the rest of the strings mentioning set status to busy. Also keep in mind these are debug strings, and only show up when using /availability as opposed to the scheduled status sync job.
server/store/flow_store.go
Outdated
| SubscribePropertyName = "subscribe" | ||
| UpdateStatusPropertyName = "update_status" | ||
| GetConfirmationPropertyName = "get_confirmation" | ||
| ReceiveNotificationsWhileOnMeetingName = "receive_notifications" |
There was a problem hiding this comment.
Not sure if we can make the value shorter while still being clear. Any changes should apply to other similarly named vars of course
| ReceiveNotificationsWhileOnMeetingName = "receive_notifications" | |
| ReceiveNotificationsDuringMeetingName = "receive_notifications_during_meeting" |
There was a problem hiding this comment.
I added your suggestion. If you have any other idea to make it shorter but giving the same information, please let me know and I will change it.
There was a problem hiding this comment.
You've changed the name of the var, but the main thing I was suggesting was to change the value to receive_notifications_during_meeting. The value receive_notifications is too vague imo and I would assume by default that it is referring to receiving event notifications in general.
There was a problem hiding this comment.
The code changes look good. I'm leaving the UX decisions to @mickmister and @asaadmahmood
server/store/flow_store.go
Outdated
| UpdateStatusPropertyName = "update_status" | ||
| GetConfirmationPropertyName = "get_confirmation" | ||
| ReceiveNotificationsWhileOnMeetingName = "receive_notifications" |
There was a problem hiding this comment.
Nit: By typeing these it would be more clear that these form a group.
There was a problem hiding this comment.
Not sure what you mean with this. Do you mind to elaborate?
There was a problem hiding this comment.
I'm think he means something like
type SettingProperty string
const (
UpdateStatusPropertyName SettingProperty = "update_status"There was a problem hiding this comment.
Then anywhere else in the code, you can reference these values as SettingProperty instead of string
There was a problem hiding this comment.
Exactly that @mickmister. Thanks for making this clear.
There was a problem hiding this comment.
Making this change now would imply touching several files (6 + mocks), and feel a bit out of this PR scope. I will take note of this improvement, and apply it to the extracted flow and settings.
There was a problem hiding this comment.
LGTM other than changing receive_notifications to receive_notifications_during_meetings and @hanzei's request for typing the enums together.
server/mscalendar/availability.go
Outdated
| @@ -199,7 +199,7 @@ func (m *mscalendar) setStatusFromCalendarView(user *store.User, currentStatus s | |||
| } | |||
| message = "User is no longer busy in calendar. Set status to online." | |||
| } else { | |||
| message = "User is no longer busy in calendar, but is not set to busy. No status change." | |||
| message = fmt.Sprintf("User is no longer busy in calendar, but is not set to busy(%s). No status change.", busyStatus) | |||
There was a problem hiding this comment.
Non-blocking nit, I'm thinking there should be a space between "busy" and the busyStatus
| message = fmt.Sprintf("User is no longer busy in calendar, but is not set to busy(%s). No status change.", busyStatus) | |
| message = fmt.Sprintf("User is no longer busy in calendar, but is not set to busy (%s). No status change.", busyStatus) |
…me to the receive notification step name
Codecov Report
@@ Coverage Diff @@
## master #118 +/- ##
==========================================
- Coverage 24.81% 24.67% -0.14%
==========================================
Files 65 65
Lines 2535 2553 +18
==========================================
+ Hits 629 630 +1
- Misses 1830 1846 +16
- Partials 76 77 +1
Continue to review full report at Codecov.
|
|
/update-branch |
There was a problem hiding this comment.
Tested and passed. This is working as expected
- Login workflow now allows user to select from DND or Away
- When set to receive notification user is moved to away as expected
- Once user is free again MM will prompt to switch status back to Online
- User Can use new Yes / No in settings panel to flip between Online and Away
- Updated between DND and Away in panel will update to selected status for next event
LGTM!
DHaussermann
added
4: Reviews Complete
Codecov Report
@@ Coverage Diff @@
## master #118 +/- ##
==========================================
- Coverage 24.63% 24.48% -0.16%
==========================================
Files 65 65
Lines 2549 2569 +20
==========================================
+ Hits 628 629 +1
- Misses 1844 1861 +17
- Partials 77 79 +2
Continue to review full report at Codecov.
|
Summary
Allow status change to support DND and Away. Settings added to both settings and login flow.
Ticket Link
Fix #89