🤖 Bring the user straight to the comments reader view #20186

antonis · 2024-02-14T18:11:23Z

Fixes #20096

Description

This PR navigates non-admin users that cannot moderate a comment directly to the comments reader view

To Test:

A. Non-admin user

Create a post with an authorised user and add some comments
Log in with another user in the Jetpack app that does not administer the blog from (1)
Search and follow the blog from (1)
Find the post from (1) and follow the conversation in the reader enabling notifications
Write another comment as the authorised user on the post from (1) (e.g. on the web)
Verify that you receive a notification for that comment
Tap on the comment notification from 3 in the notifications list
Expect the Reader Comments screen to appear scrolled (if needed) to the comment from 3.

B. Admin user ( Regression )

Log in with an admin user
Open the notifications in the app
Navigate to Notifications.
Tap on the comment notification from A5 in the notifications list
Expect the Comment Detail screen to appear were the user can moderate the comment

C. Push notifications

Repeat (A) and (B) above by tapping on the system push notification

Regression Notes

Potential unintended areas of impact
- Notifications detail view
What I did to test those areas of impact (or what existing automated tests I relied on)
- Manual testing
What automated tests I added (or what prevented me from doing so)
- Added NotificationsListViewModelTest with the logic for opening the comment in the reader

PR Submission Checklist:

I have completed the Regression Notes.
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.

Testing Checklist:

wpmobilebot · 2024-02-14T18:31:47Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20186-ea7ee3b
	Commit	`ea7ee3b`
	Direct Download	`wordpress-prototype-build-pr20186-ea7ee3b.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2024-02-14T18:32:46Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20186-ea7ee3b
	Commit	`ea7ee3b`
	Direct Download	`jetpack-prototype-build-pr20186-ea7ee3b.apk`

Note: Google Login is not supported on these builds.

codecov · 2024-02-14T18:46:11Z

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (fdd373b) 40.11% compared to head (ea7ee3b) 40.14%.

Files	Patch %	Lines
.../ui/notifications/NotificationsListFragmentPage.kt	0.00%	13 Missing ⚠️
...oid/ui/notifications/NotificationsListViewModel.kt	93.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                         Coverage Diff                          @@
##           feature/notifications_refresh_p1   #20186      +/-   ##
====================================================================
+ Coverage                             40.11%   40.14%   +0.02%     
====================================================================
  Files                                  1469     1469              
  Lines                                 67755    67782      +27     
  Branches                              11226    11230       +4     
====================================================================
+ Hits                                  27179    27209      +30     
+ Misses                                38082    38078       -4     
- Partials                               2494     2495       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

...Press/src/test/java/org/wordpress/android/ui/notifications/NotificationsListViewModelTest.kt

…CommentInTheReader # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt # WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListViewModel.kt

jarvislin

Thanks for making the change! Code looks good.

A. Non-admin user
I found the behavior is different when I clicked on the system notification and the item in the notification tab.

From system notification	From notification tab

B. Admin user
The behaviors are the same as expected.

From system notification	From notification tab

WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListViewModel.kt

mkevins · 2024-02-16T05:59:24Z

I've tested this on a Pixel 3a (physical device) and it is working as described. I left one minor question in the code, but feel free to consider it non-blocking.

One minor issue I noticed, though I don't think it is related with this PR is that when tapping the notification in steps for test (A), from the system UI, the app got stuck in a loading state:

The reason I think this is possibly unrelated is that I also happened later by simply opening the app from the home screen. I'm mentioning it, in case there is something about the steps followed here that causes this.

antonis · 2024-02-16T06:33:25Z

Thank you both for testing 🙇

I found the behavior is different when I clicked on the system notification and the item in the notification tab.

Good catch @jarvislin 👍
I didn't cover this flow with my PR. I'll check this now.

The reason I think this is possibly unrelated is that I also happened later by simply opening the app from the home screen. I'm mentioning it, in case there is something about the steps followed here that causes this.

Thank you for reporting @mkevins 🙇
I'll have a look in case this can be covered in this PR.

…the reader

antonis · 2024-02-16T08:12:07Z

WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java

-                    NotificationsListFragment
-                            .openNoteForReply(this, noteId, shouldShowKeyboard, null,
-                                    NotesAdapter.FILTERS.FILTER_ALL, true);
+                    if (mNotificationsViewModel == null) {


I've added the initialisation here since the initViewModel() has not been called in this flow

antonis · 2024-02-16T08:14:17Z

Thanks again for catching this @jarvislin 🙇

I found the behavior is different when I clicked on the system notification and the item in the notification tab.

I've cover this case with d089b05
I don't like the way this is structured much and I'll examine a possible refactoring in a follow up PR.

antonis · 2024-02-16T08:24:15Z

The reason I think this is possibly unrelated is that I also happened later by simply opening the app from the home screen. I'm mentioning it, in case there is something about the steps followed here that causes this.

@mkevins I've run into the loading state (with flow B) but wasn't able to reproduce the stuck state. I also tried disabling the network after receiving the notification but in this case the notifications disappear on my device and I'm not able to continue the flow.

Screen_recording_20240216_101936.mp4

I've tested this on a Pixel 3a (physical device)

What Android version are you running? Any special setup in your network?

…CommentInTheReader # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListViewModel.kt

jarvislin

I've cover this case with d089b05
I don't like the way this is structured much and I'll examine a possible refactoring in a follow up PR.

The test result is as expected, LGTM!

Open comments that cannot be moderated in the reader

8c83377

antonis added Reader Notifications [Type] Task labels Feb 14, 2024

antonis changed the base branch from trunk to feature/notifications_refresh_p1 February 14, 2024 21:52

Antonis Lilis added 4 commits February 15, 2024 09:42

Moves logic in the viewModel

2532ec2

Optimise function parameter list

4b0d3a6

Replaces static function calls with wrapped objects for testability

b75ec32

Adds test cases

d29cf50

antonis marked this pull request as ready for review February 15, 2024 10:35

antonis requested review from mkevins and jarvislin February 15, 2024 10:36

antonis mentioned this pull request Feb 15, 2024

The inline action of Like Comment #20187

Merged

13 tasks

antonis commented Feb 15, 2024

View reviewed changes

...Press/src/test/java/org/wordpress/android/ui/notifications/NotificationsListViewModelTest.kt Outdated Show resolved Hide resolved

Antonis Lilis added 3 commits February 15, 2024 14:15

Changed mocked action handler to interface to avoid unneeded code

47720af

Fixes broken tests after merge from trunk and feature branch

0c3601e

jarvislin reviewed Feb 16, 2024

View reviewed changes

mkevins reviewed Feb 16, 2024

View reviewed changes

WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListViewModel.kt Show resolved Hide resolved

Antonis Lilis added 2 commits February 16, 2024 08:39

Adds log in the case of post retrieval failure

b2d02dd

Opens push notification comment that cannot be moderated directly in …

d089b05

…the reader

antonis commented Feb 16, 2024

View reviewed changes

antonis requested review from jarvislin and mkevins February 16, 2024 08:14

Antonis Lilis added 2 commits February 16, 2024 11:03

Merge branch 'feature/notifications_refresh_p1' into issue/20096-open…

339f887

…CommentInTheReader # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListViewModel.kt

Updates broken tests after merge

ea7ee3b

jarvislin approved these changes Feb 16, 2024

View reviewed changes

antonis merged commit e302eb7 into feature/notifications_refresh_p1 Feb 16, 2024
19 checks passed

antonis deleted the issue/20096-openCommentInTheReader branch February 16, 2024 11:30

antonis mentioned this pull request Feb 16, 2024

🤖 Bring the user straight to the comments reader view #20096

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🤖 Bring the user straight to the comments reader view #20186

🤖 Bring the user straight to the comments reader view #20186

antonis commented Feb 14, 2024 •

edited

Loading

wpmobilebot commented Feb 14, 2024 •

edited

Loading

wpmobilebot commented Feb 14, 2024 •

edited

Loading

codecov bot commented Feb 14, 2024 •

edited

Loading

jarvislin left a comment •

edited

Loading

mkevins commented Feb 16, 2024

antonis commented Feb 16, 2024

antonis Feb 16, 2024

antonis commented Feb 16, 2024

antonis commented Feb 16, 2024 •

edited

Loading

jarvislin left a comment

🤖 Bring the user straight to the comments reader view #20186

🤖 Bring the user straight to the comments reader view #20186

Conversation

antonis commented Feb 14, 2024 • edited Loading

Description

To Test:

A. Non-admin user

B. Admin user ( Regression )

C. Push notifications

Regression Notes

PR Submission Checklist:

Testing Checklist:

wpmobilebot commented Feb 14, 2024 • edited Loading

wpmobilebot commented Feb 14, 2024 • edited Loading

codecov bot commented Feb 14, 2024 • edited Loading

Codecov Report

jarvislin left a comment • edited Loading

Choose a reason for hiding this comment

mkevins commented Feb 16, 2024

antonis commented Feb 16, 2024

antonis Feb 16, 2024

Choose a reason for hiding this comment

antonis commented Feb 16, 2024

antonis commented Feb 16, 2024 • edited Loading

jarvislin left a comment

Choose a reason for hiding this comment

antonis commented Feb 14, 2024 •

edited

Loading

wpmobilebot commented Feb 14, 2024 •

edited

Loading

wpmobilebot commented Feb 14, 2024 •

edited

Loading

codecov bot commented Feb 14, 2024 •

edited

Loading

jarvislin left a comment •

edited

Loading

antonis commented Feb 16, 2024 •

edited

Loading