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

Follow conversation by push notification. #15459

Merged
merged 36 commits into from Oct 15, 2021

Conversation

develric
Copy link
Contributor

@develric develric commented Oct 14, 2021

Fixes #15460

References: pctCYC-9x-p2

This PR adds to the reader the possibility to activate push notifications related to comments after following a post comments conversation by email already.

Screen.Recording.2021-10-14.at.03.51.30.mov

Note 1: the feature is currently behind a feature flag and you should enable it to test. [UPDATE: the feature flag has been activated in #15469
Note 2: the image in the linked issue reports the threaded comments with the new design but this issue is being tackled before the new threaded comments is in place.

Dedicated unit testing will be added in a separate PR. [UPDATE: added here]

To test

New feature testing

  • Go to the reader and select one of the tabs (Following/Discover etc...)
  • Open the comments on a post from the reader stream (do the same later from the comment details)
  • Check the option menu shows a FOLLOW shimmer item until the following status is recovered.
  • If you are not following the post comments already, select the FOLLOW menu and check a snackbar notifies you of the action result
  • If it succeeded the snackbar present an action to enable push notifications; try to not activate push notifications and check you get only emails for new comments on that post
  • try to activate the push notifications and check you get both emails and push notifications for new comments on that post
  • Check you can enable and undo push notifications activation from the snackbar actions
  • Check you receive notifications and email for new comments when push notifications are on, only emails when push notificaiton are off, no email nor push notifications when you unsubscribe from a post comments
  • Check you get an error when in airplane mode (note that on errors on post following information rescue, the menu is placed in an unknown state as a dimmed FOLLOW button)
  • Check a PTR on the comments list updates the state of the following menu
  • Check you can change the push notification status from the bottom sheet switch and you can fully unfollow the conversation
  • Check that if you are not logged in in .COM and you go to the reader, accessing the comments on a post does not present the follow menu

Smoke testing

  • With the feature flag enabled, smoke test the threaded comments; try to like and reply to a comment, post a new comment etc...
  • Disable the feature flag and smoke test the previous follow by email functionality

NOTE: push notifications for replies to your comments are still managed as before, comments that are coming from push notifications specifically are identified with a remark wording

Regression Notes

  1. Potential unintended areas of impact
    N/A: the feature is behind a feature flag.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    N/A: the feature is behind a feature flag.

  3. What automated tests I added (or what prevented me from doing so)
    N/A: the feature is behind a feature flag.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 14, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 14, 2021

You can test the changes on this Pull Request by downloading the APKs:

@develric develric requested a review from ashiagr October 14, 2021 03:09
@develric develric marked this pull request as ready for review October 14, 2021 03:11
@ashiagr ashiagr self-assigned this Oct 14, 2021
Copy link
Contributor

@ashiagr ashiagr left a comment

Choose a reason for hiding this comment

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

Thanks for working on this task, I'm so very happy that we have this option to be notified in-app for the conversations we follow! 🎉

I was able to receive notifications as per instructions, only noticed a couple of issues that I already discussed with you. As discussed, they might be linked to this known issue: p1633941032482100-slack-C01CW1VMLAF

  1. I enabled in-app push notifications for conversation as mentioned in the instructions, but did not receive them in an instance when I had notifications disabled for the comments on my own (atomic) site under the Notifications tab. Once I enabled the setting, I started receiving them.

  2. I disabled notifications for conversations, yet was able to receive them when someone commented on the thread. Comments needed moderation, once approved, new comments from the same user were displayed as remarked comments under the Notifications tab.

    notification_when.option.is.disabled.mp4

Besides I left minor code-level comments, some are very easy to fix - detekt warnings etc or related to Fragment/ Activity to ViewModel logic refactoring.

Once tests are included for the feature, we can further test it exhaustively.

Hope this helps!

@develric
Copy link
Contributor Author

develric commented Oct 14, 2021

Thanks so much for deep reviewing and testing this PR @ashiagr 🙇 . I'm going to work on the feedbacks and come back to you 👍 !

Hi @mattmiklic 👋 , I didn't ping you yesterday since I knew you were AFK and this is still behind feature flag, but happy if you could have a look design-wise to the current implementation while I cover the code feedbacks (the feature is behind a feature flag as said and you should be able to activate it installing the jalapeno APK from this CI. Just ping me if any issue with that 🙇 ). Thanks in advance sir 😄 !

@develric develric added Tooling and removed Tooling labels Oct 14, 2021
@develric develric added Tooling and removed Tooling labels Oct 14, 2021
@mattmiklic
Copy link
Member

Looks like the design was implemented perfectly! 👨‍🍳👌 Nice work with this one.

@develric
Copy link
Contributor Author

Hey @ashiagr 👋 , I followed up to your feedbacks, thanks again 🙇 . This should be ready for another pass now.

Re the 2 issues you reported, I was not able to reproduce on my side actually. If possible we can maybe sync check when we are both online in case I'm missing any step to reproduce 🙇

Note: I'm keeping this PR branch like a feature branch creating a couple PRs against it. Let's align on the 2 mentioned issue before merge to develop, thanks.

@develric develric requested a review from ashiagr October 14, 2021 23:28
Copy link
Contributor

@ashiagr ashiagr left a comment

Choose a reason for hiding this comment

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

Thanks for all the updates, @develric! All looks good to me. 👍

As discussed in our call, let's create separate issues to test those edge cases later on as they're not consistently reproducable.

I'm approving the PR, please feel free to merge once unit tests are integrated.

…ification-enable

Follow by notification enable feature flag
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.

Follow conversation by push notification.
3 participants