Skip to content
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

Comment digest notification settings #657

Merged
merged 13 commits into from
Apr 18, 2019

Conversation

cdolm92
Copy link
Contributor

@cdolm92 cdolm92 commented Apr 8, 2019

πŸ“² What

This PR implements comment digest notification in settings, which allows the user(backers and creators) to opt in/out of email notifications.

This PR also fixes the issue of users currently not being able to update their notifications.

πŸ€” Why

Currently, backers have no way of knowing if their question was answered or post was responded to without coming back to the project and looking. Creators receive an individual email for every comment and comment reply on their project. Now, comment digest is for all users.

πŸ‘€ See

Trello, screenshots, external resources?

After πŸ¦‹
Simulator Screen Shot - iPhone 8 - 2019-04-08 at 14 17 34

βœ… Acceptance criteria

Test Creator Reply Digest

  • Navigate to Settings > Notifications Scroll down to Comment reply digest toggle the email notification
  • Log in to Kickstarter on web to see the notification update

Test Settings Notifications

  • Navigate to Settings > Notifications start toggling email and push notifications
  • Log in to Kickstarter on web to see the notification update

Project Activity notification toggles do not reflect the apps(iOS and Android) on web. This PR does not fix this issue

@cdolm92 cdolm92 added the WIP label Apr 8, 2019
@cdolm92 cdolm92 requested a review from dusi April 8, 2019 18:46
@cdolm92 cdolm92 added needs review and removed WIP labels Apr 8, 2019
Copy link
Contributor

@dusi dusi left a comment

Choose a reason for hiding this comment

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

Looking good..I've tested it an it seems to work properly!

Had couple small questions/comments for your consideration..please ping me if you feel like some of those don't make sense for us, I don't feel strong about those.

@@ -55,6 +55,7 @@ public struct User {
public struct Notifications {
public private(set) var backings: Bool?
public private(set) var comments: Bool?
public private(set) var commentReplies: Bool?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is our intent to order these alphabetically? If that's the case shouldn't commentReplies come before comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

This would probably apply to this whole PR 😭

@@ -214,6 +215,7 @@ extension User.Notifications: Argo.Decodable {
let tmp1 = curry(User.Notifications.init)
<^> json <|? "notify_of_backings"
<*> json <|? "notify_of_comments"
<*> json <|? "notify_of_comment_replies"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be tested in UserTests?

@@ -249,6 +250,9 @@ extension SettingsNotificationCellViewModel {
case .friendBacksProject:
return notificationType == .email
? UserAttribute.Notification.friendActivity : UserAttribute.Notification.mobileFriendActivity
case .commentReplyDigest:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also test this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's necessary because the notifications test that we have already are sufficient.

@dusi
Copy link
Contributor

dusi commented Apr 17, 2019

QAing this PR I've noticed the following issue...please correct me if I'm doing something wrong but it looks like in order to see changes (Comment reply digest toggle) done on the web or on other mobile device you have to navigate to Notifications screen twice.

Steps to reproduce:

  1. On iOS go to Settings
  2. On the web go to Settings > Notifications
  3. On the web toggle the New replies in your active threads setting
  4. On iOS go to Settings > Notifications
  5. Notice that the value does not reflect the value on the web (backend)
  6. On iOS pop back to Settings
  7. On iOS go to Settings > Notifications one more time
  8. Notice that now the value reflects the value on the web (backend)

It appears to me that we're either not reloading the table properly or not refreshing for some reason.

@@ -25,6 +25,7 @@ public enum UserAttribute {
case let .notification(notification):
switch notification {
case .comments: return \.notifications.comments
case .commentReplies: return \.notifications.commentReplies
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also move this above comments in order to sort alphabetically

@@ -52,6 +53,7 @@ public enum UserAttribute {

public enum Notification {
case comments
case commentReplies
Copy link
Contributor

Choose a reason for hiding this comment

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

And here as well

@@ -71,6 +73,7 @@ public enum UserAttribute {
public var trackingString: String {
switch self {
case .comments, .mobileComments: return "New comments"
case .commentReplies: return "Comment reply digest"
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

@@ -209,6 +209,7 @@ extension SettingsNotificationCellViewModel {
AppEnvironment.current.koala.trackChangePushNotification(type: notification.trackingString,
on: enabled)
case .comments,
.commentReplies,
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

@cdolm92 cdolm92 requested a review from dusi April 18, 2019 17:28
Copy link
Contributor

@dusi dusi left a comment

Choose a reason for hiding this comment

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

🚒 🚒 🚒
🚒 🚒 🚒
🚒 🚒 🚒

@cdolm92 cdolm92 merged commit 8134b56 into master Apr 18, 2019
@cdolm92 cdolm92 deleted the comment-digest-notification-settings branch April 18, 2019 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants