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 P2] Commen Moderation #23225

Conversation

alpavanoglu
Copy link
Contributor

Fixes #https://github.com/Automattic/wordpress-mobile/issues/34
Fixes #https://github.com/Automattic/wordpress-mobile/issues/35
Fixes #https://github.com/Automattic/wordpress-mobile/issues/36

Description

This PR connects the moderation actions to Network along with tracks. It also improves the loading state for the buttons.
I moved a bunch of code from CommentDetailViewController to CommentModerationViewModel. We should follow that up with tests to make it nicer since now it is possible. I'll try to do another PR for some tests.

Screen Recording

Simulator.Screen.Recording.-.iPhone.15.-.2024-05-17.at.22.24.30.mp4

Testing Steps

Install & Login to Jetpack App
Navigate to Notifications

Every test step will proceed from this point.

Approve Comment

  1. Select a Pending State notification
  2. Tap on "Approve Comment"
  3. βœ… Expect the state to change to "Approved" and moderation UI to update.
  4. Verify Track: πŸ”΅ Tracked: notifications_comment_approved

Like & Unlike

  1. Select an Approved Notification
  2. Tap on "Like" button.
  3. βœ… Expect the button to update to a Liked state.
  4. Verify Track: πŸ”΅ Tracked: notifications_comment_liked
  5. βœ… Go back and verify if the notification appears liked in the Notifications screen.
  6. βœ… Unlike the notification from the list and tap on it again. Verify if the state matches in the Detail screen.

Trash

  1. Tap on the β€’β€’β€’ context menu from the detail screen.
  2. Change Status
  3. Select "Trash"
  4. βœ… Verify the UI to update and button title should read "Delete Permanently".
  5. βœ… Tap on Delete. Expect the comment to be deleted and navigate back to the notifications list.
  6. Verify Track: πŸ”΅ Tracked: comment_trashed
  7. βœ… Verify that the notification is not listed there either.

Spam

  1. Tap on the β€’β€’β€’ context menu from the detail screen.
  2. Change Status
  3. Select "Spam"
  4. βœ… Verify the UI to update and button title should read "Delete Permanently".
  5. βœ… Tap on Delete. Expect the comment to be deleted and navigate back to the notifications list.
  6. Verify Track: πŸ”΅ Tracked: comment_trashed
  7. βœ… Verify that the notification is not listed there either.

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.

Testing checklist:

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 17, 2024

WordPress AlphaπŸ“² You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr23225-85386dd
Version24.8
Bundle IDorg.wordpress.alpha
Commit85386dd
App Center BuildWPiOS - One-Offs #9983
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 17, 2024

Jetpack AlphaπŸ“² You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr23225-85386dd
Version24.8
Bundle IDcom.jetpack.alpha
Commit85386dd
App Center Buildjetpack-installable-builds #9033
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.


private weak var changeStatusViewController: BottomSheetViewController?
weak var changeStatusViewController: BottomSheetViewController?
private var commentModerationViewModel: CommentModerationViewModel?
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this viewModel can be non-optional by making it lazy:

private lazy var commentModerationViewModel: CommentModerationViewModel = createCommentModerationViewModel()

@salimbraksa
Copy link
Contributor

Hey @alpavanoglu,

I've noticed a UI glitch in the moderations view where the hosting view doesn't properly resize when the state changes. While the SwiftUI view itself resizes, the hosting view fails to adjust accordingly. Here's how you can reproduce the issue:

  1. Have a comment in either the Trash or Spam state.
  2. Navigate to that notification comment.
  3. Change its status to "Approved."
  4. Observe that the view height doesn't adjust correctly.
  5. Go back to the previous screen.
  6. Navigate to the same comment again.
  7. Notice that the view height is now correct.

This resizing issue occurs when changing to any state but is most noticeable when switching between "Approved" and "Trash/Spam."

I encountered a similar issue in the past, and it was tricky to adjust the hosting view's height without breaking the animation. I think a simple fix for our case would be to set a fixed height for all states.

Actual Expected

@salimbraksa salimbraksa self-requested a review May 21, 2024 16:01
isLoading = true
coordinator.didSelectOption()
commentService.approve(comment, success: { [weak self] in
self?.handleStatusChangeSuccess(state: .approved(liked: false))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as this one

@salimbraksa salimbraksa self-requested a review May 22, 2024 11:57
@alpavanoglu
Copy link
Contributor Author

Hey, @salimbraksa
This resizing issue seem to only happen when I directly open a trashed comment and approve it from the menu. But doesn't happen when I open an approved comment, trash it, approve it, trash so on and so forth.
So it does work but something breaks when we first open a trashed state comment. That tells me that we should be able to solve this somehow without assigning fixed heights as they don't work great with dynamic fonts.

Copy link
Member

@justtwago justtwago left a comment

Choose a reason for hiding this comment

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

Tested it and it works cool!
I'm confirming that the resizing issue happens when entering to the notification with ether "Spam" or "Trash" status.
One more minor thing is a clickable comment's author bar between Comment Approved and Like comment sections. It's clickable but is not actionable. Are there any plans for that or we can disable the click animation?

@alpavanoglu
Copy link
Contributor Author

One more minor thing is a clickable comment's author bar between Comment Approved and Like comment sections. It's clickable but is not actionable. Are there any plans for that or we can disable the click animation?

If you mean the reply component, that will be replaced with the new reply component once it is built.

if initialIsLiked {
track(withEvent: .notificationsCommentLiked) { comment in
CommentAnalytics.trackCommentUnLiked(comment: comment)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Based on the previous implementation, the event to trigger here should be notificationsCommentUnliked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@justtwago justtwago left a comment

Choose a reason for hiding this comment

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

LG2M! 🚒

Copy link
Contributor

@salimbraksa salimbraksa left a comment

Choose a reason for hiding this comment

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

LGTM! πŸš€

@alpavanoglu alpavanoglu merged commit 8704144 into feature/notifications_refresh_p2 May 24, 2024
24 checks passed
@alpavanoglu alpavanoglu deleted the notifications_comment-moderation-approved branch May 24, 2024 09:52
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

5 participants