[MBL-1026] Filter comments from blocked users #1883
Conversation
ifosli
force-pushed
the
filterComments
branch
from
4b46ec2
to
8d87688
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1883 +/- ##
==========================================
- Coverage 87.78% 83.75% -4.03%
==========================================
Files 860 1222 +362
Lines 76998 111393 +34395
Branches 20411 29623 +9212
==========================================
+ Hits 67591 93299 +25708
- Misses 8667 17077 +8410
- Partials 740 1017 +277 ☔ View full report in Codecov by Sentry. |
ifosli
force-pushed
the
filterComments
branch
from
008e823
to
742f4a3
Compare
ifosli
force-pushed
the
filterComments
branch
from
742f4a3
to
e5e7e75
Compare
| @@ -34,7 +34,7 @@ final class RootCommentCell: UITableViewCell, ValueCell { | |||
| |> \.translatesAutoresizingMaskIntoConstraints .~ false | |||
| }() | |||
|
|
|||
| private let viewModel = RootCommentCellViewModel() | |||
| private let viewModel = CommentCellViewModel() | |||
There was a problem hiding this comment.
Nice cleanup here 👍
| .map(\.author.imageUrl) | ||
| .map { | ||
| if $0.author.isBlocked { | ||
| return "" // Use default avatar. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah, maybe something like // Use placeholder avatar instead of URL?
| // TODO(MBL-1026): Use translated string once available. | ||
| return "Blocked User" | ||
| } else { | ||
| return $0.author.name |
There was a problem hiding this comment.
Same comments here, re: putting this directly in the model. Would it make it simpler at all?
There was a problem hiding this comment.
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.
📲 What
Filter comments from blocked users by replacing the author name, avatar, and comment text with "Blocked user" placeholders.
Note: This also deletes the
RootCommentCellViewModeland switches theRootCommentCellto using theCommentCellViewModelinstead. TheRootCommentCellViewModelcode was just a subset of the entireCommentCellViewModeland keeping it as a separate copy just meant that we needed to duplicate work to keep them in sync. With this change, all comment cells (RootCommentCell,CommentCell,CommentRemovedCell,CommentPostFailedCell) all use the same view model, so the author filtering will apply to all of them.👀 See
Jira
♿️ Accessibility / 🏎 Performance
Not tested; this change doesn't add any new UI.
✅ Acceptance criteria
isBlockedvalue to true or have it read from some other field instead. But when theisBlockedvalue is true, the author's name and avatar as well as the comment text should be hidden (comment functionality is unchanged).⏰ TODO
isBlockedfield based on backend info.