-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GH87] Add "receive reminder" setting to login flow #117
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@larkox I don't see where this value is being used to conditionally deliver the reminder. Is that intentional?
server/mscalendar/welcome_flow.go
Outdated
@@ -76,6 +76,14 @@ func (wf *welcomeFlow) makeSteps() { | |||
FalseButtonMessage: "No - Do not notify me of new events", | |||
TrueResponseMessage: "Great, you will receive a message any time you receive a new event.", | |||
FalseResponseMessage: "Great, you will not receive any notification on new events.", | |||
}, &flow.SimpleStep{ | |||
Title: "Receive reminder", | |||
Message: "Do you want to receive reminder of upcoming events?", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Message: "Do you want to receive reminder of upcoming events?", | |
Message: "Do you want to receive a reminder for upcoming events?", |
server/mscalendar/welcome_flow.go
Outdated
TrueButtonMessage: "Yes - I would like to receive reminders for upcoming events", | ||
FalseButtonMessage: "No - Do not notify me of upcoming events", | ||
TrueResponseMessage: "Great, you will receive a message before your meetings.", | ||
FalseResponseMessage: "Great, you will not receive any notification on upcoming events.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FalseResponseMessage: "Great, you will not receive any notification on upcoming events.", | |
FalseResponseMessage: "Great, you will not receive any notification for upcoming events.", |
server/store/flow_store.go
Outdated
default: | ||
return fmt.Errorf("property %s not found", propertyName) | ||
if user.WelcomeFlowStatus.PostIDs == nil { | ||
user.WelcomeFlowStatus.PostIDs = make(map[string]string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleanup here!
All the logic for receive reminder was already in place. It was only missing the setting on the login flow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave the wording suggestions to @aaronrothschild. LGTM!
Codecov Report
@@ Coverage Diff @@
## master #117 +/- ##
==========================================
- Coverage 24.19% 24.12% -0.08%
==========================================
Files 64 64
Lines 2529 2537 +8
==========================================
Hits 612 612
- Misses 1841 1849 +8
Partials 76 76
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #117 +/- ##
==========================================
- Coverage 25.01% 24.94% -0.08%
==========================================
Files 65 65
Lines 2550 2558 +8
==========================================
Hits 638 638
- Misses 1832 1840 +8
Partials 80 80
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -84,6 +84,14 @@ func (wf *welcomeFlow) makeSteps() { | |||
FalseButtonMessage: "No - Do not notify me of new events", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not be in the scope of this PR, but I was thinking we should use the language "when you are invited to an event", instead of "when you receive a new event", in the prompts above this line, as it is clearer what we mean by "event".
Also, above we use the language "while you are on a meeting", versus "while you are in a meeting". Just some small improvements for wordings. @larkox Thoughts on this?
Tested and passed
|
Summary
Add "receive reminder" setting to login flow
Ticket Link
Fix #87