Issue 15828 - Notification comment support in the new comment editor #15884

RenanLukas · 2022-02-01T14:58:00Z

Currently only Site Comments are supported in the new comment editor. The goal of this PR is to also allow Notification Comments to be shown and editable in the new comment editor (instead of the legacy editor). It also tries to make it easier to support other Comment types in the future (e.g. Reader).

To test:
1 - Login with an user that has at least one notification of a comment;
2 - Select any site;
3 - Tap on "Notifications" bottom navigation icon to open Notifications screen;
4 - Tap on "Comments" tab inside Notifications screen;
5 - Tap on the comment you want to test;
6 - Tap on "More" button;
7 - Tap on "Edit" item. The notification comment edit should be opened in the new comment editor;
8 - Edit the comment text;
9 - Tap on "Done";
10 - The comment text should be updated.

Regression Notes

Potential unintended areas of impact
None
What I did to test those areas of impact (or what existing automated tests I relied on)

What automated tests I added (or what prevented me from doing so)

CommentEssentialsTest.kt
CommentSourceTest.kt
CommentEssentialsExtensionKtTest.kt
GetCommentUseCaseTest.kt
Included more tests in UnifiedCommentsEditViewModelTest.kt

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.

…ll CommentSource types (if feature flag is enabled)

…decide where to get the comment from

…tion-logic-to-new-editor

peril-wordpress-mobile · 2022-02-01T14:58:06Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2022-02-01T15:17:51Z

You can test the changes on this Pull Request by downloading the APKs:

Jetpack-pr-15884-build-118937.apk

WordPress-pr-15884-build-118937.apk

…edited notification comment

… Logic can be split among UseCases later.

khaykov

Thanks for the PR @RenanLukas ! Looks good and works well :) I left couple of small nitpicks in the code :)

As we discussed, let's keep this PR unmerged, until we can update the content of the comment in notification details (not list) after the edit.

khaykov · 2022-02-07T18:06:19Z

...ress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentsEditViewModel.kt

+        } ?: showUpdateCommentError()
+    }
+
+    private suspend fun updateCommentEntityLocalDatabase(


About the method name - updateEditComment pushes the comment to sever as well, but I don't think that VM needs to know about what is happening there :)

Good catch, thank you! I had this decision in the VM before extracting it to the UseCase and completely missed this name 😅
Changing it

khaykov · 2022-02-07T18:08:23Z

WordPress/src/main/java/org/wordpress/android/ui/notifications/utils/NotificationsActions.java

@@ -80,7 +80,7 @@ public void onErrorResponse(VolleyError error) {
        }
    }

-    public static void downloadNoteAndUpdateDB(final String noteID, final RestRequest.Listener respoListener,
+    public static void downloadNoteAndUpdateDB(final String noteID, final RestRequest.Listener requestListener,


Good catch 👍

khaykov · 2022-02-07T18:57:38Z

...ess/src/test/java/org/wordpress/android/ui/comments/unified/usecase/GetCommentUseCaseTest.kt

+        whenever(commentsStore.getCommentByLocalSiteAndRemoteId(siteModel.id, remoteCommentId))
+                .thenReturn(emptyList())
+        whenever(commentsStore.fetchComment(siteModel, remoteCommentId, null))
+                .thenReturn(CommentsActionPayload(CommentsActionData(emptyList(), 1)))


NP: Doesn't affect the test, but the rowsAffected in CommentsActionData will probably be 0 in this case :)

khaykov · 2022-02-07T19:04:24Z

...c/test/java/org/wordpress/android/ui/comments/viewmodels/UnifiedCommentsEditViewModelTest.kt

+    }
+
+    @Test
+    fun `Should map CommentIdentifier to default CommentEssentials if CommentIdentifier comment not found`() {


NP: Wonder if this method signature should have = test {...} instead of { test{...} }

khaykov · 2022-02-07T19:06:46Z

...c/test/java/org/wordpress/android/ui/comments/viewmodels/UnifiedCommentsEditViewModelTest.kt

+    }
+
+    @Test
+    fun `Should map CommentIdentifier to default CommentEssentials if CommentIdentifier not handled`() {


Do we want the test to use runBlocking (= test{...}) or is it ok like this?

RenanLukas · 2022-02-07T20:56:14Z

Thank you for the review @khaykov 🎉 I've addressed the comments.

As we discussed, let's keep this PR unmerged, until we can update the content of the comment in notification details (not list) after the edit.

Deal 👍 I'm on it

… details screen

…on comment in new editor

…CommentDetailFragment

…the CommentIdentifier

…put enabled settings based on comment identifier

…w editor

…ent edited

RenanLukas · 2022-02-11T06:13:29Z

This PR depends on #15918

…ication-comment-details-content-not-updated-after-editing Issue 15910 - Fix notification comment details content not updated after editing

…tion-logic-to-new-editor

khaykov

Thanks for addressing the issues from the review, and fixing the issue with notification content not updating. Now everything works as expected :)

RenanLukas added 14 commits January 21, 2022 15:57

Extract CommentSource class

289f71f

Migrate CommentSource to Kotlin

957e3ba

Implement unit tests for CommentSource

1a47d0d

Refactor CommentDetailFragment Intent creation

9bde88c

Open UnifiedCommentsEditActivity instead of EditCommentActivity for a…

e34a363

…ll CommentSource types (if feature flag is enabled)

Add CommentIdentifier class (as implemented in @khaykov branch)

6168640

Integrate CommentIdentifier with UnifiedCommentsEdit flow

49da328

Implement NotificationsTableWrapper

f3fe234

Update NotificationCommentIdenfitier ID type

605e0c8

Support notification comment in new editor: Use CommentIdentifier to …

d727861

…decide where to get the comment from

Fix UnifiedCommentsEditViewModel unit tests

585fb93

Merge branch 'trunk' into issue/15828-migrate-edited-comment-notifica…

ccb06d2

…tion-logic-to-new-editor

Extract and implement unit tests for CommentEssentials

7a3a2e6

Add new unit tests for UnifiedCommentsEditViewModel

ddb5e4f

RenanLukas added [Status] Not Ready for Review [Status] In Progress labels Feb 1, 2022

RenanLukas added 5 commits February 1, 2022 17:30

Fix detekt, ktlint and checkstyle

3454634

Implement save edited notification comment in new editor

e3c7554

Rename downloadNoteAndUpdateDB parameter

f8cc4e0

Update UnifiedCommentsEditViewModel unit tests considering saving of …

7459adf

…edited notification comment

Fix detekt warning for UnifiedCommentsEditViewModel: TooManyFuncions.…

535bfd7

… Logic can be split among UseCases later.

RenanLukas added [Status] Needs Code Review and removed [Status] Not Ready for Review [Status] In Progress labels Feb 3, 2022

RenanLukas marked this pull request as ready for review February 3, 2022 04:52

RenanLukas changed the title ~~[WIP] Issue 15828 - Notification comment support in the new editor~~ Issue 15828 - Notification comment support in the new editor Feb 3, 2022

Fix unresolved reference in UnifiedCommentsEditViewModelTest

172d0ae

RenanLukas removed the [Status] Needs Code Review label Feb 3, 2022

RenanLukas added [Status] Needs Code Review [Status] Not Ready for Merge labels Feb 7, 2022

khaykov approved these changes Feb 7, 2022

View reviewed changes

Implement code review improvements

1e1088d

RenanLukas added [Status] Needs Code Review and removed [Status] Needs Code Review labels Feb 7, 2022

RenanLukas added [Status] Needs Code Review [Status] Not Ready for Merge and removed [Status] Needs Code Review [Status] Not Ready for Merge labels Feb 7, 2022

RenanLukas added 10 commits February 8, 2022 15:57

Add callbacks to update edited notification comment edited in comment…

389a321

… details screen

Split CommentUserNoteBlock configureView method

3ae9e7a

Update notification comment content in comment details after editing

3639e10

Fix checkstyle errors

10de4d1

Implement VM logic to disable edit name, url and email for notificati…

4373962

…on comment in new editor

Add check to avoid NPE when calling refreshBlocksForEditedComment in …

973bb30

…CommentDetailFragment

Apply input enabled settings in UnifiedCommentsEditFragment based on …

22114ae

…the CommentIdentifier

Update unit tests for UnifiedCommentsEditViewModelTest considering in…

b7c39e6

…put enabled settings based on comment identifier

Update notification entity when comment notification edited in the ne…

e956c7b

…w editor

Refresh note blocks in comment details screen after notification comm…

314a16c

…ent edited

RenanLukas and others added 3 commits February 11, 2022 10:08

Remove Nullable annotations not found by CI

717acdf

Merge pull request #15918 from wordpress-mobile/issue/15910-fix-notif…

65853ab

…ication-comment-details-content-not-updated-after-editing Issue 15910 - Fix notification comment details content not updated after editing

Merge branch 'trunk' into issue/15828-migrate-edited-comment-notifica…

e6719f0

…tion-logic-to-new-editor

RenanLukas removed the [Status] Not Ready for Merge label Feb 11, 2022

khaykov approved these changes Feb 11, 2022

View reviewed changes

khaykov merged commit 2133ad3 into trunk Feb 11, 2022

khaykov deleted the issue/15828-migrate-edited-comment-notification-logic-to-new-editor branch February 11, 2022 19:48

RenanLukas mentioned this pull request Sep 19, 2023

Nullability Annotations to Java Classes - Activity & Fragment - Replace findViewById with ViewBinding #18906

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 15828 - Notification comment support in the new comment editor #15884

Issue 15828 - Notification comment support in the new comment editor #15884

RenanLukas commented Feb 1, 2022 •

edited

Loading

peril-wordpress-mobile bot commented Feb 1, 2022 •

edited

Loading

peril-wordpress-mobile bot commented Feb 1, 2022 •

edited

Loading

khaykov left a comment

khaykov Feb 7, 2022

RenanLukas Feb 7, 2022

khaykov Feb 7, 2022

khaykov Feb 7, 2022

khaykov Feb 7, 2022

khaykov Feb 7, 2022

RenanLukas commented Feb 7, 2022

RenanLukas commented Feb 11, 2022

khaykov left a comment

Issue 15828 - Notification comment support in the new comment editor #15884

Issue 15828 - Notification comment support in the new comment editor #15884

Conversation

RenanLukas commented Feb 1, 2022 • edited Loading

Regression Notes

peril-wordpress-mobile bot commented Feb 1, 2022 • edited Loading

peril-wordpress-mobile bot commented Feb 1, 2022 • edited Loading

khaykov left a comment

Choose a reason for hiding this comment

khaykov Feb 7, 2022

Choose a reason for hiding this comment

RenanLukas Feb 7, 2022

Choose a reason for hiding this comment

khaykov Feb 7, 2022

Choose a reason for hiding this comment

khaykov Feb 7, 2022

Choose a reason for hiding this comment

khaykov Feb 7, 2022

Choose a reason for hiding this comment

khaykov Feb 7, 2022

Choose a reason for hiding this comment

RenanLukas commented Feb 7, 2022

RenanLukas commented Feb 11, 2022

khaykov left a comment

Choose a reason for hiding this comment

RenanLukas commented Feb 1, 2022 •

edited

Loading

peril-wordpress-mobile bot commented Feb 1, 2022 •

edited

Loading

peril-wordpress-mobile bot commented Feb 1, 2022 •

edited

Loading