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] Add Like Post and Comment Inline Actions #22688

Merged

Conversation

salimbraksa
Copy link
Contributor

@salimbraksa salimbraksa commented Feb 23, 2024

Fixes #22466

Description

This PR adds the Post Like inline action.

Challenges

Row Fade Animation

When toggling the post liked status, the notification row reloads with a fade animation. The reason behind this is because the notifications screen is using the WPTableViewHandler object, which observes Core Data changes. So when the notification object is updated locally, the WPTableViewHandler catches this update and reloads the associated row. I couldn't figure out an easy way to disable this behavior.

I have tried setting tableViewHandler.updateRowAnimation = .none but it didn't work.

Change Propagation

When a post's liked status is toggled anywhere on the app, this change isn't broadcasted to the Notifications screen. Here is how to reproduce this issue:

  1. Navigate to Notifications screen
  2. Tap on any new post notification
  3. Expect the Reader Detail screen to appear
  4. Like the post
  5. Go back to Notifications screen
  6. The post isn't liked

Ideally, when a post is liked on one screen, the same post should appear liked on all other loaded screens. I tested on Android and I was able to reproduce this issue.

Test Instructions

Posts

  1. Run the Jetpack app and switch to the notifications tab.
  2. Navigate to Notifications tab.
  3. Expect to see the "Star" icon on new post notifications.
  4. Tap the "Star" to like / un-like a new post notification.
  5. Expect the like status to immediately change
  6. Tap on the notification row.
  7. Expect the Reader Detail screen to appear
  8. Verify the post is liked

Comments

  1. Run the Jetpack app and switch to the notifications tab.
  2. Navigate to Notifications tab.
  3. Expect to see the "Star" icon on comment notifications.
  4. Tap the "Star" to like / un-like a comment.
  5. Expect the like status to immediately change
  6. Tap on the notification row.
  7. Depending on whether the user can moderate comments:
    • User is admin: Expect the Comment Detail screen to appear.
    • User is not admin: Expect the Reader Comments screen to appear.
  8. Verify the comment is liked.

Regression Notes

  1. Potential unintended areas of impact
    Smoke test post like feature in other parts of the app ( e.g Reader )

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

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

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.

@salimbraksa salimbraksa changed the base branch from trunk to feature/notifications_refresh_p1 February 23, 2024 01:53
@salimbraksa salimbraksa changed the base branch from feature/notifications_refresh_p1 to task/22473-share-own-published-post February 23, 2024 01:55
@salimbraksa salimbraksa changed the base branch from task/22473-share-own-published-post to task/22473-share-own-published-post-hassaan-suggestion February 23, 2024 01:55
@salimbraksa salimbraksa changed the base branch from task/22473-share-own-published-post-hassaan-suggestion to task/22473-share-own-published-post February 23, 2024 01:56
@salimbraksa salimbraksa changed the title Task/22466 like post inline action [Notifications] Add Like Post Inline Action Feb 23, 2024
@salimbraksa salimbraksa marked this pull request as ready for review February 23, 2024 02:08
Base automatically changed from task/22473-share-own-published-post to feature/notifications_refresh_p1 February 23, 2024 02:14
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 23, 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 Numberpr22688-55200cd
Version24.3
Bundle IDorg.wordpress.alpha
Commit55200cd
App Center BuildWPiOS - One-Offs #8981
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 Feb 23, 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 Numberpr22688-55200cd
Version24.3
Bundle IDcom.jetpack.alpha
Commit55200cd
App Center Buildjetpack-installable-builds #8011
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@hassaanelgarem hassaanelgarem marked this pull request as ready for review February 23, 2024 15:13
// MARK: - Init

init?(note: Notification) {
guard let postID = note.metaPostID?.uintValue, let siteID = note.metaSiteID?.uintValue else {
Copy link
Contributor

Choose a reason for hiding this comment

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

postID and siteID seem to be required arguments for this struct. Maybe we can add the otherwise synthesized init too so this one acts more like a convenience nullable init?

That way the call-site can unwrap them before if needed. Right now it may be slightly implicit when init is called with a Notification but returned value is nil.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

So when this init return nil URL(string:), it is obvious to why that would be the case. But for this one, Notification being a very large and complicated entity, it would be very implicit why the init fails.

My point is, the caller would have to read the implementation of the init to find out why init returns nil.

I think the proper way of addressing this would be passing exactly what the struct needs rather than passing the whole Notification. So instead of doing note.body(ofType, the struct can accept a closure that returns [String: Any]? and an updateClosure.

Comment on lines 323 to 326
func toggleLikeForPostNotification(like: Bool,
postID: UInt,
siteID: UInt,
completion: @escaping (Result<Bool, Swift.Error>) -> Void) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think if we select these lines & CTRL + M, it would style the syntax differently.

@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

@alpavanoglu
Copy link
Contributor

🐛 When the like status is changed from the details screen bottom bar, it doesn't reflect on the row after going back. This is still the case after a pull-to-refresh action.

@hassaanelgarem hassaanelgarem changed the title [Notifications] Add Like Post Inline Action [Notifications] Add Like Post and Comment Inline Actions Feb 23, 2024
…post-inline-action

# Conflicts:
#	WordPress/Classes/ViewRelated/Notifications/Controllers/NotificationsViewController/NotificationsViewController.swift
#	WordPress/Classes/ViewRelated/Views/List/NotificationsList/NotificationsTableViewCellContent.swift
#	WordPress/WordPress.xcodeproj/project.pbxproj
@hassaanelgarem hassaanelgarem added this to the 24.4 milestone Feb 26, 2024
@alpavanoglu alpavanoglu merged commit 8edc127 into feature/notifications_refresh_p1 Feb 26, 2024
@alpavanoglu alpavanoglu deleted the task/22466-like-post-inline-action branch February 26, 2024 19:15
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.

5 participants