Skip to content

Display post / comment authors at the top of the suggestions list#18979

Merged
twstokes merged 18 commits intowordpress-mobile:trunkfrom
salimbraksa:task/prominent-suggestions
Jul 8, 2022
Merged

Display post / comment authors at the top of the suggestions list#18979
twstokes merged 18 commits intowordpress-mobile:trunkfrom
salimbraksa:task/prominent-suggestions

Conversation

@salimbraksa
Copy link
Contributor

@salimbraksa salimbraksa commented Jul 3, 2022

Issue

Closes #18818

Description

This PR adds prominentSuggestionsIds attribute to SuggestionsTableView, then the view controllers that own this view populate that attribute with the post author id or comment author id if they exist.

Solution

See "implementation" section (ref: pbMoDN-3DH-p2) for more details on how this PR is implemented.

I searched for all the view controllers that use the SuggestionsTableView view, and they are:

  • ReaderCommentsViewController: This VC has access to both the post and the selected comment. So it can pass their ids to SuggestionsTableView.
  • FullScreenCommentReplyViewController: This VC receives the prominentSuggestionsIds from the VC that presented it.
  • CommentDetailViewController: This VC has only access to the comment author id but not the post author id. It does have access to the postId, which means we can fetch the whole post to access the author id. But I don't think it's a good solution.
  • NotificationDetailsViewController: Same issue as CommentDetailViewController. Has access to comment author id but not the post author id.
  • GutenbergSuggestionsViewController: I believe this VC is used when adding a post. So the post author id or comment author id is not relevant here.

Test Instructions:

N.B: This feature is not enabled in self-hosted sites ( wordpress.org ).

There are different scenarios to take into consideration:

Scenario 1 - Reader Tab

  1. Tap the "Reader" tab.
  2. Click on a post.
  3. Tap "Comments" icon.
  4. Insert @-mention in the reply post. You should see the post author at the top of the suggestions list.
  5. Insert @-mention in a comment reply. You should see both the post author and the comment author at the top of the suggestions list.
  6. Expand the reply input, then insert @-mention. You should see both the post author and the comment author at the top of the suggestions list.

Scenario 2 - My Site Tab

  1. Tap the "My Site" tab.
  2. Click on "Comments" row.
  3. Tap any comment.
  4. Tap "Reply" button.
  5. Insert @-mention. You should see the comment author at the top of the suggestions list.
  6. Expand the reply input, then insert @-mention. You should see the comment author at the top of the suggestions list.

Scenario 3 - Notifications Tab - Comment Details

  1. Tap the "Notifications" tab.
  2. Tap a comment notification.
  3. Tap "Reply" button.
  4. Insert @-mention. You should see the comment author at the top of the suggestions list.
  5. Expand the reply input, then insert @-mention. You should see the comment author at the top of the suggestions list.

Scenario 4 - Notifications Tab - Notification Details
First, we need to disable a feature flag to access the Notification Details screen. But this scenario can only be tested by developers.

  1. "My Site" Tab > "Me" profile picture > App Settings > Debug.
  2. Disable "Notification Comment Details" feature flag.
  3. Close everything then navigate to "Notifications" screen.
  4. Tap a comment notification.
  5. Insert @-mention. You should see the comment author at the top of the suggestions list.
  6. Expand the reply input, then insert @-mention. You should see the comment author at the top of the suggestions list.

Regression Notes

  1. Potential unintended areas of impact
    Maybe there is a case where I forgot to setup prominentSuggestionsIds. But I searched everywhere SuggestionsTableView is mentioned and I think the VCs mentioned above are all of them.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    I've manually tested all the scenarios where we access SuggestionsTableView.

  3. What automated tests I added (or what prevented me from doing so)
    Unfortunately the SuggestionsTableView is not unit-testable. I'll open another PR to make it unit-testable.

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
Copy link
Contributor Author

@twstokes Few questions:

  1. Should I update the RELEASE-NOTES.txt?
  2. Regarding CommentDetailViewController and NotificationDetailsViewController, right now they only display the comment author id at the top of the suggestions list because those VCs don't have access to post author id. If we want to fix this efficiently, we may need the backend to send us back the postAuthorID under the comment object. Let me know if I'm missing something.

@salimbraksa salimbraksa changed the title Task/prominent suggestions Display prominent suggestions at the top of the suggestions list Jul 3, 2022
@salimbraksa salimbraksa changed the title Display prominent suggestions at the top of the suggestions list Display post author / comment author at the top of the suggestions list Jul 3, 2022
@salimbraksa salimbraksa changed the title Display post author / comment author at the top of the suggestions list Display post / comment authors at the top of the suggestions list Jul 3, 2022
@salimbraksa salimbraksa force-pushed the task/prominent-suggestions branch from 6b94466 to 1b30b9b Compare July 4, 2022 17:58
@twstokes twstokes self-requested a review July 5, 2022 02:34
@twstokes twstokes added this to the 20.3 milestone Jul 5, 2022
@twstokes
Copy link
Contributor

twstokes commented Jul 5, 2022

  1. Should I update the RELEASE-NOTES.txt?

Please do! We'll target milestone 20.3, so let's add a line under it describing the feature. For each bullet item we add a * that serves as a priority indicator. This particular issue fits the single * level IMO. ref: PCYsg-guH-p2

  1. Regarding CommentDetailViewController and NotificationDetailsViewController, right now they only display the comment author id at the top of the suggestions list because those VCs don't have access to post author id. If we want to fix this efficiently, we may need the backend to send us back the postAuthorID under the comment object. Let me know if I'm missing something.

Thanks for the heads up on this! I'll check it out during my review to understand it better.

Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

👋 @salimbraksa! Thanks for your work on this. 🙇 I've dropped some answers, questions, and comments in the review.

  1. Can we expand on the Test Instructions area to cover all the scenarios for loading comments? Even if they're quite redundant, detailed and numbered steps for our testers are really valuable to ensure we have coverage.
  2. Is this feature only applicable to WordPress.com sites and WordPress.org + Jetpack? In other words, will users connected to a self-hosted site (which communicates over XML-RPC) be able to use the feature when replying to comments on their blog? ref: PCYsg-g6b-p2

@salimbraksa
Copy link
Contributor Author

salimbraksa commented Jul 6, 2022

@twstokes Thanks for taking the time to review my PR 🙏

Can we expand on the Test Instructions area to cover all the scenarios for loading comments? Even if they're quite redundant, detailed and numbered steps for our testers are really valuable to ensure we have coverage.

Sure thing 👍

Is this feature only applicable to WordPress.com sites and WordPress.org + Jetpack? In other words, will users connected to a self-hosted site (which communicates over XML-RPC) be able to use the feature when replying to comments on their blog?

Good catch, I haven't thought about this. I'll test with a fake site then get back to you 🙏

@salimbraksa salimbraksa force-pushed the task/prominent-suggestions branch 2 times, most recently from f4c2410 to 7f6e9e5 Compare July 7, 2022 17:53
@salimbraksa salimbraksa force-pushed the task/prominent-suggestions branch from 7f6e9e5 to 8b2dca6 Compare July 7, 2022 17:54
@twstokes twstokes self-requested a review July 7, 2022 18:12
@twstokes
Copy link
Contributor

twstokes commented Jul 7, 2022

This is looking good @salimbraksa!

I tested each scenario with a WordPress.com account:

  • ✅ Scenario 1 - Reader Tab
  • ✅ Scenario 2 - My Site Tab
  • ✅ Scenario 3 - Notifications Tab - Comment Details
  • ✅ Scenario 4 - Notifications Tab - Notification Details

I have not tested with a self-hosted non-Jetpack account.

A few outstanding items that you may be aware of:

  • RELEASE-NOTES.txt has a conflict
  • MIGRATIONS.md should be updated with the Core Data model changes

@twstokes
Copy link
Contributor

twstokes commented Jul 7, 2022

I've created a PR for running CI tests: #19009

@twstokes
Copy link
Contributor

twstokes commented Jul 7, 2022

👋 @salimbraksa - this looks good!

CI PR passed

After updating the release notes we should be good to merge!

Co-authored-by: Tanner Stokes <tanner@tannr.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mention UI should make parent comment's author prominent in list of suggestions

2 participants