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.

Sign up for GitHub

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

Jump to bottom

[MBL-1026] Filter comments from blocked users #1883

Merged
merged 8 commits into from
Nov 14, 2023
Merged

[MBL-1026] Filter comments from blocked users #1883

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ec8ec5d
Add blocked field to User object
ifosli Nov 8, 2023
17be120
Replace info in comments that shouldn't show
ifosli Nov 13, 2023
0f99421
Delete separate RootCommentCellViewModel
ifosli Nov 13, 2023
5a1d4bb
Cleanup and adding test
ifosli Nov 13, 2023
8d87688
Fix UserTests
ifosli Nov 13, 2023
ba910f8
Only show blocking options if user isn't blocked
ifosli Nov 13, 2023
e5e7e75
Merge branch 'main' into filterComments
ifosli Nov 13, 2023
1d6ca14
Update comment
ifosli Nov 14, 2023
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 Kickstarter-iOS/Features/Comments/Controllers/CommentRepliesViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ final class CommentRepliesViewController: UITableViewController, MessageBannerVi

private func handleCommentCellHeaderTapped(in cell: UITableViewCell, _ author: Comment.Author) {
guard AppEnvironment.current.currentUser != nil, featureBlockUsersEnabled() else { return }
guard author.isBlocked == false else { return }

let actionSheet = UIAlertController
.blockUserActionSheet(
Expand Down
1 change: 1 addition & 0 deletions Kickstarter-iOS/Features/Comments/Controllers/CommentsViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ extension CommentsViewController {
extension CommentsViewController: CommentCellDelegate {
func commentCellDidTapHeader(_ cell: CommentCell, _ author: Comment.Author) {
guard AppEnvironment.current.currentUser != nil, featureBlockUsersEnabled() else { return }
guard author.isBlocked == false else { return }

let actionSheet = UIAlertController
.blockUserActionSheet(
Expand Down
8 changes: 4 additions & 4 deletions Kickstarter-iOS/Features/Comments/Views/Cells/RootCommentCell.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
|> \.translatesAutoresizingMaskIntoConstraints .~ false
}()

private let viewModel = RootCommentCellViewModel()
private let viewModel = CommentCellViewModel()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice cleanup here 👍


// MARK: - Lifecycle

Expand Down Expand Up @@ -76,7 +76,7 @@
internal func configureWith(value: Comment) {
self.commentCellHeaderStackView
.configureWith(comment: value)
self.viewModel.inputs.configureWith(comment: value)
self.viewModel.inputs.configureWith(comment: value, project: nil)
}

private func configureViews() {
Expand Down Expand Up @@ -124,7 +124,7 @@
internal override func bindViewModel() {
self.bodyTextView.rac.text = self.viewModel.outputs.body

self.viewModel.outputs.commentAuthor
self.viewModel.outputs.cellAuthor
.observeForUI()
.observeValues { [weak self] author in
guard let self = self else { return }
Expand All @@ -135,6 +135,6 @@
// MARK: - Actions

@objc private func commentCellHeaderTapped() {
self.viewModel.inputs.commentCellHeaderTapped()
self.viewModel.inputs.cellHeaderTapped()

Check warning on line 138 in Kickstarter-iOS/Features/Comments/Views/Cells/RootCommentCell.swift

View check run for this annotation

Codecov / codecov/patch

Kickstarter-iOS/Features/Comments/Views/Cells/RootCommentCell.swift#L138 

Added line #L138 was not covered by tests
}
}
6 changes: 4 additions & 2 deletions Kickstarter-iOS/Features/Comments/Views/CommentCellHeaderStackView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,10 @@ internal final class CommentCellHeaderStackView: UIStackView {
self?.circleAvatarImageView.image = nil
})
.observeValues { [weak self] url in
self?.circleAvatarImageView
.ksr_setImageWithURL(url)
if let url {
self?.circleAvatarImageView
.ksr_setImageWithURL(url)
}
}

self.viewModel.outputs.authorBadge
Expand Down
8 changes: 0 additions & 8 deletions Kickstarter.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,9 +181,7 @@
06DAAE5A26AA3CD500194E58 /* CategoryFragment.graphql in Resources */ = {isa = PBXBuildFile; fileRef = 8A1556EE269398CA00017845 /* CategoryFragment.graphql */; };
06DAAE5B26AA3CD800194E58 /* BackingFragment.graphql in Resources */ = {isa = PBXBuildFile; fileRef = 8A1556D7269394A400017845 /* BackingFragment.graphql */; };
06DAAE5D26AA3CF500194E58 /* UserStoredCardsFragment.graphql in Resources */ = {isa = PBXBuildFile; fileRef = 06DAAE5C26AA3CF500194E58 /* UserStoredCardsFragment.graphql */; };
06DC4138266FEE89005205F7 /* RootCommentCellViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 06DC4137266FEE89005205F7 /* RootCommentCellViewModel.swift */; };
06DC4144266FF184005205F7 /* RootCommentCell.swift in Sources */ = {isa = PBXBuildFile; fileRef = 06DC413C266FF179005205F7 /* RootCommentCell.swift */; };
06DC4152266FFE81005205F7 /* RootCommentCellViewModelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 06DC4151266FFE81005205F7 /* RootCommentCellViewModelTests.swift */; };
06E3296B270E39B300216306 /* ProjectPageNavigationBarView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 06E3296A270E39B300216306 /* ProjectPageNavigationBarView.swift */; };
06EA2D4C280F76B700F4DE2E /* Prelude in Frameworks */ = {isa = PBXBuildFile; productRef = 06EA2D4B280F76B700F4DE2E /* Prelude */; };
06EB4E3127B5D32000D8BFCC /* PinchToZoom.swift in Sources */ = {isa = PBXBuildFile; fileRef = 06EB4E2F27B5D26800D8BFCC /* PinchToZoom.swift */; };
Expand Down Expand Up @@ -1725,9 +1723,7 @@
06D23BBE26F2484500F76122 /* Project+FetchProjectFriendsQueryDataTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Project+FetchProjectFriendsQueryDataTests.swift"; sourceTree = "<group>"; };
06D23BC026F2498100F76122 /* FetchProjectFriendsQueryTemplate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FetchProjectFriendsQueryTemplate.swift; sourceTree = "<group>"; };
06DAAE5C26AA3CF500194E58 /* UserStoredCardsFragment.graphql */ = {isa = PBXFileReference; lastKnownFileType = text; path = UserStoredCardsFragment.graphql; sourceTree = "<group>"; };
06DC4137266FEE89005205F7 /* RootCommentCellViewModel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RootCommentCellViewModel.swift; sourceTree = "<group>"; };
06DC413C266FF179005205F7 /* RootCommentCell.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RootCommentCell.swift; sourceTree = "<group>"; };
06DC4151266FFE81005205F7 /* RootCommentCellViewModelTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RootCommentCellViewModelTests.swift; sourceTree = "<group>"; };
06E3296A270E39B300216306 /* ProjectPageNavigationBarView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ProjectPageNavigationBarView.swift; sourceTree = "<group>"; };
06EA2D36280F1F1900F4DE2E /* XCTest.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = XCTest.framework; path = Platforms/iPhoneOS.platform/Developer/Library/Frameworks/XCTest.framework; sourceTree = DEVELOPER_DIR; };
06EB4E2D27B5D06A00D8BFCC /* OverlayView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OverlayView.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -6409,8 +6405,6 @@
77C7B653226E0E54001101AC /* RewardsCollectionViewModelTests.swift */,
470B771926FCDC8900EBD5CA /* RiskMessagingViewModel.swift */,
470B771B26FD022900EBD5CA /* RiskMessagingViewModelTests.swift */,
06DC4137266FEE89005205F7 /* RootCommentCellViewModel.swift */,
06DC4151266FFE81005205F7 /* RootCommentCellViewModelTests.swift */,
A775B5451CA871D700BBB587 /* RootViewModel.swift */,
A7ED205F1E83256700BFFA01 /* RootViewModelTests.swift */,
D7A37CCE1E2FF93D00EA066D /* SearchEmptyStateCellViewModel.swift */,
Expand Down Expand Up @@ -7580,7 +7574,6 @@
D798A6AE21656D970053D097 /* Currency.swift in Sources */,
77F6E73521222E7C005A5C55 /* EmailFrequency.swift in Sources */,
D0237C2622BD7B540092C792 /* PledgeSummaryViewModel.swift in Sources */,
06DC4138266FEE89005205F7 /* RootCommentCellViewModel.swift in Sources */,
01A7A4C01C9690220036E553 /* UITextField+LocalizedPlaceholderKey.swift in Sources */,
3777F2F72343C7900030BEF5 /* ManageViewPledgeRewardReceivedViewModel.swift in Sources */,
0176E13B1C9742FD009CA092 /* UIBarButtonItem.swift in Sources */,
Expand Down Expand Up @@ -7668,7 +7661,6 @@
A7ED1F491E831BA200BFFA01 /* DispatchTimeInterval-Extensions.swift in Sources */,
77AA2B3D233D10D3008BBCB8 /* CreateBackingConstructorTests.swift in Sources */,
8AD9CF1424C8D46800F77223 /* PledgeShippingSummaryViewModelTests.swift in Sources */,
06DC4152266FFE81005205F7 /* RootCommentCellViewModelTests.swift in Sources */,
77D19FF22406D5A40058FC8E /* CategorySelectionViewModelTests.swift in Sources */,
3706408822A8A6F200889CBD /* PledgeShippingLocationViewModelTests.swift in Sources */,
8AFB8C99233E9A1F006779B5 /* CreatePaymentSourceInput+ConstructorTests.swift in Sources */,
Expand Down
3 changes: 3 additions & 0 deletions KsApi/models/Comment.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ public struct Comment {
public struct Author: Decodable, Equatable {
public var id: String
public var imageUrl: String
/// TODO(MBL-1025): Get isBlocked status from the backend instead.
public var isBlocked: Bool = false
public var isCreator: Bool
public var name: String
}
Expand Down Expand Up @@ -60,6 +62,7 @@ extension Comment {
let author = Author(
id: "\(user.id)",
imageUrl: user.avatar.medium,
isBlocked: user.isBlocked ?? false,
isCreator: project.creator == user,
name: user.name
)
Expand Down
4 changes: 4 additions & 0 deletions KsApi/models/User.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ public struct User {
public var isAdmin: Bool?
public var isEmailVerified: Bool?
public var isFriend: Bool?
public var isBlocked: Bool?
public var location: Location?
public var name: String
public var needsFreshFacebookToken: Bool?
Expand Down Expand Up @@ -119,6 +120,7 @@ extension User: Decodable {
self.isAdmin = try values.decodeIfPresent(Bool.self, forKey: .isAdmin)
self.isEmailVerified = try values.decodeIfPresent(Bool.self, forKey: .isEmailVerified)
self.isFriend = try values.decodeIfPresent(Bool.self, forKey: .isFriend)
self.isBlocked = try values.decodeIfPresent(Bool.self, forKey: .isBlocked)
self.location = try? values.decodeIfPresent(Location.self, forKey: .location)
self.name = try values.decode(String.self, forKey: .name)
self.needsFreshFacebookToken = try values.decodeIfPresent(Bool.self, forKey: .needsFreshFacebookToken)
Expand All @@ -140,6 +142,7 @@ extension User: Decodable {
case isAdmin = "is_admin"
case isEmailVerified = "is_email_verified"
case isFriend = "is_friend"
case isBlocked = "is_blocked"
case location
case name
case needsFreshFacebookToken = "needs_fresh_facebook_token"
Expand All @@ -160,6 +163,7 @@ extension User: EncodableType {
result["is_admin"] = self.isAdmin ?? false
result["is_email_verified"] = self.isEmailVerified ?? false
result["is_friend"] = self.isFriend ?? false
result["is_blocked"] = self.isBlocked ?? false
result["location"] = self.location?.encode()
result["name"] = self.name
result["needs_password"] = self.needsPassword ?? false
Expand Down
2 changes: 2 additions & 0 deletions KsApi/models/UserTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ final class UserTests: XCTestCase {
"is_admin": false,
"is_email_verified": false,
"is_friend": false,
"is_blocked": false,
"opted_out_of_recommendations": true,
"show_public_profile": false,
"social": true
Expand Down Expand Up @@ -98,6 +99,7 @@ final class UserTests: XCTestCase {
"is_admin": false,
"is_email_verified": false,
"is_friend": false,
"is_blocked": false,
"opted_out_of_recommendations": true,
"show_public_profile": false,
"social": true
Expand Down
17 changes: 17 additions & 0 deletions KsApi/models/templates/CommentTemplates.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,23 @@ extension Comment {
status: .success
)

public static let blockedTemplate = Comment(
author: Author(
id: "ADG8hYbp7gsDAZ==",
imageUrl: "https://ks/img/cordero.jpg",
isBlocked: true,
isCreator: false,
name: "Cordero"
),
authorBadges: [.superbacker],
body: "Since the author is blocked, this comment text should never be seen.",
createdAt: Date(timeIntervalSince1970: 1_475_361_115).timeIntervalSince1970,
id: "BOD5af89jdDA4fG==",
isDeleted: false,
replyCount: 0,
status: .success
)

public static let failedTemplate = Comment(
author: Author(
id: "AKLEhYbp7CDO6E==",
Expand Down
35 changes: 28 additions & 7 deletions Library/ViewModels/CommentCellViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public protocol CommentCellViewModelOutputs {
var authorBadge: Signal<Comment.AuthorBadge, Never> { get }

/// Emits a url to the comment author's image.
var authorImageURL: Signal<URL, Never> { get }
var authorImageURL: Signal<URL?, Never> { get }

/// Emits text containing author's fullname or username.
var authorName: Signal<String, Never> { get }
Expand Down Expand Up @@ -85,13 +85,32 @@ public final class CommentCellViewModel:
.map { comment, _ in comment }

self.authorImageURL = comment
.map(\.author.imageUrl)
.map {
if $0.author.isBlocked {
return "" // Use default avatar.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this default avatar URL. be hardcoded somewhere? Or is some other piece of code checking for this empty string?

You could also throw this in the model code, instead. It's a little sneaky but means anyone working with/implementing comment code in the future doesn't need to remember this edge case.

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 think we may want to replace the current gray circle with an icon instead, but for now empty string -> nil url -> gray placeholder circle. I added the comment since I didn't feel "" was clear about that by itself. Glad to see I was right about that! Would it be more clear if I said "Keep placeholder avatar" instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, maybe something like // Use placeholder avatar instead of URL?

} else {
return $0.author.imageUrl
}
}
.map(URL.init)
.skipNil()

self.body = comment.map(\.body)
self.body = comment.map {
if $0.author.isBlocked {
// TODO(MBL-1026): Use translated string once available.
return "This user has been blocked"
} else {
return $0.body
}
}

self.authorName = comment.map(\.author.name)
self.authorName = comment.map {
if $0.author.isBlocked {
// TODO(MBL-1026): Use translated string once available.
return "Blocked User"
} else {
return $0.author.name
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments here, re: putting this directly in the model. Would it make it simpler at all?

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 don't think this belongs in the model. I think it would complicate things (the model would no longer be encodable which would mess with tests & decoding would be more of a pain). Also, whether a user is blocked or not isn't purely a display thing; it also affects functionality, so it's not like we could do no special handling at all. We also can't consistently always hide blocked user info, since we do want to display usernames of blocked users in messages. Plus, at some point, we might want to add an "unblock" option here, in which case the view model will need to know the real user info.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair!

}
}

let status = comment.map(\.status)

Expand All @@ -109,7 +128,9 @@ public final class CommentCellViewModel:
let badge = self.commentAndProject.signal
.skipNil()
.map { comment, _ in
comment.author.id == AppEnvironment.current.currentUser?.id.description ? .you : comment.authorBadge
if comment.author.isBlocked { return Comment.AuthorBadge.backer }
return comment.author.id == AppEnvironment.current.currentUser?.id.description ? .you : comment
.authorBadge
}

self.authorBadge = Signal.merge(
Expand Down Expand Up @@ -195,7 +216,7 @@ public final class CommentCellViewModel:
}

public let authorBadge: Signal<Comment.AuthorBadge, Never>
public var authorImageURL: Signal<URL, Never>
public var authorImageURL: Signal<URL?, Never>
public let authorName: Signal<String, Never>
public let body: Signal<String, Never>
public let bottomRowStackViewIsHidden: Signal<Bool, Never>
Expand Down
12 changes: 11 additions & 1 deletion Library/ViewModels/CommentCellViewModelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ internal final class CommentCellViewModelTests: TestCase {
let vm: CommentCellViewModelType = CommentCellViewModel()

private let authorBadge = TestObserver<Comment.AuthorBadge, Never>()
private let authorImageURL = TestObserver<URL, Never>()
private let authorImageURL = TestObserver<URL?, Never>()
private let authorName = TestObserver<String, Never>()
private let body = TestObserver<String, Never>()
private let bottomRowStackViewIsHidden = TestObserver<Bool, Never>()
Expand Down Expand Up @@ -323,6 +323,16 @@ internal final class CommentCellViewModelTests: TestCase {
self.authorBadge.assertValues([.backer], "The author's badge is emitted.")
}

func testBlockedAuthor() {
let comment = Comment.blockedTemplate

self.vm.inputs.configureWith(comment: comment, project: .template)
self.authorBadge.assertValues([.backer], "The default badge is emitted.")
self.authorName.assertValue("Blocked User", "The author's name is hidden.")
self.authorImageURL.assertValue(nil, "The author's avatar is hidden.")
self.body.assertValue("This user has been blocked", "The comment text is hidden.")
}

func testBindStylesEmitsAuthorBadge() {
self.authorBadge.assertDidNotEmitValue()

Expand Down
100 changes: 0 additions & 100 deletions Library/ViewModels/RootCommentCellViewModel.swift
View file
Open in desktop

This file was deleted.

Loading