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

Notifications > Comments: remove Notification when Comment is removed #17992

Merged
merged 6 commits into from
Feb 21, 2022

Conversation

ScoutHarris
Copy link
Contributor

@ScoutHarris ScoutHarris commented Feb 18, 2022

Ref: #17790

When comments are moderated from the Notification Comment details view, the affected notifications are updated when the notifications list is re-shown. The effect is if a Comment is trashed, spammed, or deleted, its Notification is removed from the Notifications list.

In addition, comment moderation confirmation notices are no longer displayed for Notification comments.

Implementation notes:

If the Notification in NotificationsViewController is updated as soon as its Comment is trashed or spammed, the Notification will be removed from fetched results. This causes a couple of issues:

  • In regular view, the nav bar previous/next buttons wouldn't find the previous/next notification.
  • In split view, it would attempt to remove the notification as the user was possibly viewing it.

So the affected Notifications are tracked by NotificationCommentDetailCoordinator. NotificationsViewController syncs the Notifications when appropriate to update the Notifications list:

  • In any view - when the Notifications list is shown (viewWillAppear).
  • In split view - when another Notification is selected.

To test:

Regular View:

  • Select a Comment notification.
    • In the details, select trash or spam.
    • Using previous/next nav bar buttons, trash/spam more Comment notifications.
    • Return to the Notifications list.
    • Verify all the Notifications for the trashed/spammed Comments are removed.
  • Select a Comment notification.
    • In the details, select trash or spam, then Delete Permanently.
    • Verify the view is dismissed, and the notification is removed from the Notifications list.

Split View:

  • Select a Comment notification.
    • In the details, select trash or spam.
    • Select another Notification.
    • Verify the Notification for the trashed/spammed Comment is removed from the list.
    • Note: Delete Permanently does not immediately nix the notification as in regular view. If you select another Notification, it will then be removed. I'll address this separately as it is related to the issue I commented on below.

Regression Notes

  1. Potential unintended areas of impact

  2. What I did to test those areas of impact (or what existing automated tests I relied on)

  3. What automated tests I added (or what prevented me from doing so)

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • 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.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 18, 2022

You can test the Jetpack changes on this Pull Request by downloading it from AppCenter here with build number: pr17992-fb021f3. IPA is available here. If you need access to this, you can ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 18, 2022

You can test the WordPress changes on this Pull Request by downloading it from AppCenter here with build number: pr17992-fb021f3. IPA is available here. If you need access to this, you can ask a maintainer to add you.

@ScoutHarris
Copy link
Contributor Author

ScoutHarris commented Feb 18, 2022

In split view, there is a legacy issue where, if you trash/spam a Comment while viewing it, the Notification is removed from the Notifications list but the details are still shown. Even though this PR improves that, it can still be seen by:

  • In split view, trash/spam a comment.
  • While still viewing the comment, navigate away from Notifications (ex: tap My Site or Reader tab).
  • Come back to Notifications.
  • The Notification is removed from the list, but the details is still displayed.

I'll look at this separately since it's not caused by this change.

@ScoutHarris ScoutHarris requested a review from dvdchr February 18, 2022 23:52
@ScoutHarris ScoutHarris marked this pull request as ready for review February 18, 2022 23:53
@@ -9,6 +9,7 @@ import CoreData
protocol CommentDetailsNotificationNavigationDelegate: AnyObject {
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 just realized this protocol name isn't really accurate with the new method I added. I'll make a note to rename it in a future PR.

Copy link
Contributor

@dvdchr dvdchr left a comment

Choose a reason for hiding this comment

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

Tested and it works as described. Changes are LGTM! :shipit:

In split view, there is a legacy issue where, if you trash/spam a Comment while viewing it, the Notification is removed from the Notifications list but the details are still shown.
... I'll look at this separately since it's not caused by this change.

👍 .

I'd like to add that, for documentation purposes, this PR slightly changes the flow of comment moderation (especially related to Spam and Trash) in Notifications. Previously:

  • When tapping Spam or Trash, the details view is immediately popped.
  • In the Notifications list view, an undo overlay is displayed on top of the spammed/trashed Notification cell.
  • The spam/trash action is only invoked from the list view after a certain timeout (Syncing.undoTimeout).
  • Finally, the list view is refreshed to remove the notification cell.

I think the flow needed some adjustments here because the moderation logic is now invoked by the (new) detail view instead of the list view. This led to the core issue mentioned in the implementation notes and addressed by this PR,

If the Notification in NotificationsViewController is updated as soon as its Comment is trashed or spammed, the Notification will be removed from fetched results.

@ScoutHarris ScoutHarris merged commit 472a967 into trunk Feb 21, 2022
@ScoutHarris ScoutHarris deleted the feature/17790-remove_from_list branch February 21, 2022 20:46
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.

3 participants