Add status change confirmation #73
Conversation
larkox
added
2: Dev Review
|
There is a big concern regarding the handling of the availability. Since there is no reliable way to distinguish events during the status sync, we are storing the last availability value we found. If the value has not changed, it does not try to update the status. In general this will work fine, but there are some special cases where it will not work. The most prominent I found is the following:
@aaronrothschild How big do you think is this problem? |
|
Tests are failing due to the changes on how we handle the status sync. When all the pieces are there (user config) and the way to avoid asking many times for the user are decided, I will update the tests. |
Codecov Report
@@ Coverage Diff @@
## master #73 +/- ##
==========================================
+ Coverage 23.04% 25.62% +2.57%
==========================================
Files 62 62
Lines 2274 2377 +103
==========================================
+ Hits 524 609 +85
- Misses 1692 1693 +1
- Partials 58 75 +17
Continue to review full report at Codecov.
|
| http.Error(w, "No state to change", http.StatusBadRequest) | ||
| return | ||
| } | ||
| stringChangeTo := changeTo.(string) |
There was a problem hiding this comment.
Nit: can you change this to match the type assertion strategy above?
There was a problem hiding this comment.
Sorry, forgot about that.
server/mscalendar/availability.go
Outdated
| return "Status not changed because there is no update since last status change." | ||
| } | ||
|
|
||
| u.LastStatusUpdateAvailability = currentAvailability |
There was a problem hiding this comment.
I think we need to store more information here to take into account for the user manually changing their status. Here's a proposal
When the user manually changes their status, we take all events that overlap with now, and store them as hashes (eventid-starttime). Those events are now "immune" to an availability change. If an event's start gets moved, it becomes a "new" event as far as the availability changer is concerned.
I think this solves the one-off issue when the user wants to cancel out their most recent status change. If they want it to remain untouched by the automation, then they can disable it in their settings.
There was a problem hiding this comment.
One of the main problems is that there is no eventID that I can use to handle this, I can only work with times and availability.
I can do the following:
- Check if the availability has changed. If changed:
- Check that mattermost status and handle it
- If the availability is not changed, check the latest start time registered, and compare it with the elements returned.
- If there is a new event (an event that starts later than the last registered) check with the current mattermost status.
- If no event starts later, we assume it is the same event and ignore it.
I will implement this and keep thinking about it. Probably in code make more sense.
There was a problem hiding this comment.
Hmm well if we don't have an event id we're kind of at a disadvantage in general. Maybe we shouldn't even be using GetSchedule since we aren't getting the performance we were getting before with it, which was looking at several users' schedules in one request, the main reason why we had the dedicated bot account.
GetSchedule's main purpose is, find some time where one or more of these people are free given some time constraints. That's not really what we're doing here. We just want to know if one specific person is free "right now". We might want to use GetCalendarView with a small time window for this instead.
@larkox Let me know what you think about the shortcomings of using GetSchedule, versus the alternative GetCalendarView. Which reminds me that we have a GetSchedule response json example in the server/testdata folder, but not one for GetCalendarView.
There was a problem hiding this comment.
I have a few non-blocking suggestions. @larkox Which PRs need to be merged before completing this one?
…kox/mattermost-plugin-mscalendar into GH60StatusChangeConfirmation
| @@ -168,7 +169,18 @@ func (m *mscalendar) GetRemoteUser(mattermostUserID string) (*remote.User, error | |||
| } | |||
|
|
|||
| func (m *mscalendar) IsAuthorizedAdmin(mattermostUserID string) (bool, error) { | |||
| return m.Dependencies.IsAuthorizedAdmin(mattermostUserID) | |||
| for _, userID := range strings.Split(m.AdminUserIDs, ",") { | |||
There was a problem hiding this comment.
This is a pretty unexpected change, especially for the scope of the PR. Is there value lost by removing the injected dependency here? What packages had access to this functionality that no longer have access?
There was a problem hiding this comment.
This was implemented on plugin.go, and passed to MSCalendar plugin, and only used from there. Therefore, implementing it here made more sense, and simplified the dependencies.
|
Tested this functionality including the following cases
@larkox this looks really good! Though there are many possible edge cases (not necessarily related to status confirmation) that I will continue testing in master. There were a couple small UX issues.
|
|
@larkox Based on your case, when a person changes his status to online, can we inform the plugin that the person manually made himself online, so that the next time it can ask me to change my status to dnd. |
@asaadmahmood So even if they have opted out of confirmations, we would still give them a chance to confirm? That is certainly feasible based on how we currently have things set up. Edit: Do you mean specifically when a new event starts? |
|
@DHaussermann Here's how I have it looking with some quick changes, including using a link for the event title MS Calendar refers to the subject as a Although the calendar also says Here's how I have it looking when this is the case: Also to address your point of "when the status is changed after an event has already stated": Nothing should happen for the rest of that event. The user should not receive any prompts. It is supposed to take into account the fact that you changed your status during the meeting and assume you don't want your status changed at all until a new event comes along. I am unable to reproduce your situation. Another thing, when the event is currently ongoing, we still have access to the event and its subject. Should we have the event subject and link in the prompt, instead of your suggestion of the ambiguous |
|
@larkox Here's my code in case it is useful func renderScheduleItem(event *remote.Event, status string) string {
if event == nil {
return fmt.Sprintf("You have an event ongoing.\n Shall I change your status to %s?", prettyStatuses[status])
}
subject := "(No subject)"
if event.Subject != "" {
subject = event.Subject
}
link, _ := url.QueryUnescape(event.Weblink)
fullSubject := fmt.Sprintf("`%s`", subject)
if link != "" {
fullSubject = fmt.Sprintf("[%s](%s)", subject, link)
}
resp := fmt.Sprintf("Your event %s will start soon.", fullSubject)
resp += fmt.Sprintf("\nShall I change your status to %s?", prettyStatuses[status])
return resp
} |
|
@mickmister We should not ask them again for events they have explicitly asked not to receive notifications for, but yes, it can be for any new events. |
|
Thanks @mickmister Yes I'm in favor of both of these ideas.
|
|
@DHaussermann Let me state the expected behaviour, some edge cases and the reasoning behind it. Case Update status automatically
Case Get confirmation for status update
Edge cases:
|
… and add event information to post action result.
|
@larkox I want to know your thoughts on using Microsoft uses
This is relevant to the daily summary when there is an event with no subject. The subject is blank and is therefore not clickable.
My proposal is to use |
|
@mickmister |
@larkox Makes sense 👍 |
|
Thanks for detailing the cases @larkox This all seems to be working as expected. I did see once case where the leave the confirmation message had been displayed for hours and then selecting an option no longer worked. I tested an edge case where the event is deleted before the recipient selects an option. and cases where the user has disconnected and reconnected their account before selecting an option. these both behave normally. |
|
The Edit: Ticket #110 |
|
@mickmister Given that exploratory testing found no additional issues and I have made sure cases posted above by @larkox have all been covered now - I would be happy to merge this PR and investigate the remaining issue separately. LGTM! |
DHaussermann
added
4: Reviews Complete
Summary
Add confirmation for status change. Whenever the status is about to change, if the user has it configured for so, he will receive a message to confirm whether he wants to change the status.
Ticket Link
#60