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

Fix off-by-one error in post likes #23912

Merged
merged 5 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
* [*] Reader: The post cover now uses the standard aspect ratio for covers, so there is no jumping. There are also a few minor improvements to the layout and animations of the cover and the header [#23897, #23909]
* [*] Reader: Move the "Reading Preferences" button to the "More" menu [#23897]
* [*] Reader: Hide post toolbar when reading an article and fix like button animations [#23909]
* [*] Reader: Fix off-by-one error in post details like counter when post is liked by you [#23912]

25.5
-----
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Foundation
import WordPressShared
import Combine

class ReaderDetailCoordinator {

Expand Down Expand Up @@ -43,6 +44,8 @@ class ReaderDetailCoordinator {
/// Called if the view controller's post fails to load
var postLoadFailureBlock: (() -> Void)? = nil

private var likesAvatarURLs: [String]?

/// An authenticator to ensure any request made to WP sites is properly authenticated
lazy var authenticator: RequestAuthenticator? = {
guard let account = try? WPAccount.lookupDefaultWordPressComAccount(in: coreDataStack.mainContext) else {
Expand Down Expand Up @@ -102,10 +105,7 @@ class ReaderDetailCoordinator {
}

private var followCommentsService: FollowCommentsService?

/// The total number of Likes for the post.
/// Passed to ReaderDetailLikesListController to display in the view title.
private var totalLikes = 0
private var likesObserver: AnyCancellable?

/// Initialize the Reader Detail Coordinator
///
Expand Down Expand Up @@ -156,41 +156,50 @@ class ReaderDetailCoordinator {
}
}

/// Fetch Likes for the current post.
/// Returns `ReaderDetailLikesView.maxAvatarsDisplayed` number of Likes.
///
func fetchLikes(for post: ReaderPost) {
guard let postID = post.postID else {
return
}
guard let postID = post.postID else { return }

// Fetch a full page of Likes but only return the `maxAvatarsDisplayed` number.
// That way the first page will already be cached if the user displays the full Likes list.
postService.getLikesFor(postID: postID,
siteID: post.siteID,
success: { [weak self] users, totalLikes, _ in
var filteredUsers = users
var currentLikeUser: LikeUser? = nil
let totalLikesExcludingSelf = totalLikes - (post.isLiked ? 1 : 0)

// Split off current user's like from the list.
// Likes from self will always be placed in the last position, regardless of the when the post was liked.
if let userID = try? WPAccount.lookupDefaultWordPressComAccount(in: ContextManager.shared.mainContext)?.userID.int64Value,
let userIndex = filteredUsers.firstIndex(where: { $0.userID == userID }) {
currentLikeUser = filteredUsers.remove(at: userIndex)
}

self?.totalLikes = totalLikes
self?.view?.updateLikes(with: filteredUsers.prefix(ReaderDetailLikesView.maxAvatarsDisplayed).map { $0.avatarUrl },
totalLikes: totalLikesExcludingSelf)
// Only pass current user's avatar when we know *for sure* that the post is liked.
// This is to work around a possible race condition that causes an unliked post to have current user's LikeUser, which
// would cause a display bug in ReaderDetailLikesView. The race condition issue will be investigated separately.
self?.view?.updateSelfLike(with: post.isLiked ? currentLikeUser?.avatarUrl : nil)
}, failure: { [weak self] error in
self?.view?.updateLikes(with: [String](), totalLikes: 0)
DDLogError("Error fetching Likes for post detail: \(String(describing: error?.localizedDescription))")
})
postService.getLikesFor(postID: postID, siteID: post.siteID, success: { [weak self] users, totalLikes, _ in
guard let self else { return }

var filteredUsers = users
if let userID = try? WPAccount.lookupDefaultWordPressComAccount(in: ContextManager.shared.mainContext)?.userID.int64Value,
let userIndex = filteredUsers.firstIndex(where: { $0.userID == userID }) {
filteredUsers.remove(at: userIndex)
}

self.likesAvatarURLs = filteredUsers.prefix(ReaderDetailLikesView.maxAvatarsDisplayed).map(\.avatarUrl)
self.updateLikesView()
self.startObservingLikes()
}, failure: { error in
DDLogError("Error fetching Likes for post detail: \(String(describing: error?.localizedDescription))")
})
}

private func startObservingLikes() {
guard let post else {
return wpAssertionFailure("post missing")
}

likesObserver = Publishers.CombineLatest(
post.publisher(for: \.likeCount, options: [.new]).removeDuplicates(),
post.publisher(for: \.isLiked, options: [.new]).removeDuplicates()
).sink { [weak self] _, _ in
self?.updateLikesView()
}
}

private func updateLikesView() {
guard let post, let likesAvatarURLs else { return }

let viewModel = ReaderDetailLikesViewModel(
likeCount: post.likeCount.intValue,
avatarURLs: likesAvatarURLs,
selfLikeAvatarURL: post.isLiked ? try? WPAccount.lookupDefaultWordPressComAccount(in: ContextManager.shared.mainContext)?.avatarURL : nil
)
view?.updateLikesView(with: viewModel)
}

/// Fetch Comments for the current post.
Expand Down Expand Up @@ -590,8 +599,7 @@ class ReaderDetailCoordinator {
guard let post else {
return
}

let controller = ReaderDetailLikesListController(post: post, totalLikes: totalLikes)
let controller = ReaderDetailLikesListController(post: post, totalLikes: post.likeCount.intValue)
viewController?.navigationController?.pushViewController(controller, animated: true)
}

Expand Down Expand Up @@ -712,30 +720,19 @@ extension ReaderDetailCoordinator: ReaderDetailHeaderViewDelegate {
}
}

// MARK: - ReaderDetailFeaturedImageViewDelegate
extension ReaderDetailCoordinator: ReaderDetailFeaturedImageViewDelegate {
func didTapFeaturedImage(_ sender: AsyncImageView) {
showFeaturedImage(sender)
}
}

// MARK: - ReaderDetailLikesViewDelegate
extension ReaderDetailCoordinator: ReaderDetailLikesViewDelegate {
func didTapLikesView() {
showLikesList()
}
}

// MARK: - ReaderDetailToolbarDelegate
extension ReaderDetailCoordinator: ReaderDetailToolbarDelegate {
func didTapLikeButton(isLiked: Bool) {
guard let userAvatarURL = try? WPAccount.lookupDefaultWordPressComAccount(in: ContextManager.shared.mainContext)?.avatarURL else {
return
}

self.view?.updateSelfLike(with: isLiked ? userAvatarURL : nil)
}
}
extension ReaderDetailCoordinator: ReaderDetailToolbarDelegate {}

// MARK: - Private Definitions

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,7 @@ protocol ReaderDetailView: AnyObject {
func showErrorWithWebAction()
func scroll(to: String)
func updateHeader()

/// Shows likes view containing avatars of users that liked the post.
/// The number of avatars displayed is limited to `ReaderDetailView.maxAvatarDisplayed` plus the current user's avatar.
/// Note that the current user's avatar is displayed through a different method.
///
/// - Seealso: `updateSelfLike(with avatarURLString: String?)`
/// - Parameters:
/// - avatarURLStrings: A list of URL strings for the liking users' avatars.
/// - totalLikes: The total number of likes for this post.
func updateLikes(with avatarURLStrings: [String], totalLikes: Int)

/// Updates the likes view to append an additional avatar for the current user, indicating that the post is liked by current user.
/// - Parameter avatarURLString: The URL string for the current user's avatar. Optional.
func updateSelfLike(with avatarURLString: String?)
func updateLikesView(with viewModel: ReaderDetailLikesViewModel)

/// Updates comments table to display the post's comments.
/// - Parameters:
Expand Down Expand Up @@ -374,38 +361,17 @@ class ReaderDetailViewController: UIViewController, ReaderDetailView {
header.refreshFollowButton()
}

func updateLikes(with avatarURLStrings: [String], totalLikes: Int) {
// always configure likes summary view first regardless of totalLikes, since it can affected by self likes.
likesSummary.configure(with: avatarURLStrings, totalLikes: totalLikes)

guard totalLikes > 0 else {
func updateLikesView(with viewModel: ReaderDetailLikesViewModel) {
guard viewModel.likeCount > 0 else {
hideLikesView()
return
}

if likesSummary.superview == nil {
configureLikesSummary()
}

scrollView.layoutIfNeeded()
}

func updateSelfLike(with avatarURLString: String?) {
// only animate changes when the view is visible.
let shouldAnimate = isVisibleInScrollView(likesSummary)
guard let someURLString = avatarURLString else {
likesSummary.removeSelfAvatar(animated: shouldAnimate)
if likesSummary.totalLikesForDisplay == 0 {
hideLikesView()
}
return
}

if likesSummary.superview == nil {
configureLikesSummary()
}

likesSummary.addSelfAvatar(with: someURLString, animated: shouldAnimate)
likesSummary.configure(with: viewModel)
}

func updateComments(_ comments: [Comment], totalComments: Int) {
Expand Down
Loading