FluxC: CommentStore integration

maxme commented 7 years ago

Features reworked / tested:

General tests

[x] Comments persistence, My Site -> Comments list -> Comment Detail, start to reply to a comment, close the app, come back the comment you've started should still be there.
[x] Change filter (all/unapproved/approved/spam/trash) + pull to refresh on My Site -> Comments list.

Self hosted (without Jetpack)

Comment list:

[x] Edit a comment on My Site -> Comments list -> Comment Detail.
[x] Like should be disabled a comment on My Site -> Comments list -> Comment Detail.
[x] Approve/Unapprove a comment on My Site -> Comments list -> Comment Detail.
[x] Trash a comment on My Site -> Comments list -> Comment Detail.
[x] Delete a comment on My Site -> Comments list (trash filter) -> Comment Detail.
[x] Restore a comment on My Site -> Comments list (trash filter) -> Comment Detail.
[x] Reply to a comment from My Site -> Comments list -> Comment Detail.
[x] Multi select comments on My Site -> Comments list, moderate them.

.com / jetpack

Comment list:

[x] Edit a comment on My Site -> Comments list -> Comment Detail.
[x] Approve/Unapprove a comment on My Site -> Comments list -> Comment Detail.
[x] Trash a comment on My Site -> Comments list -> Comment Detail.
[x] Delete a comment on My Site -> Comments list (trash filter) -> Comment Detail.
[x] Restore a comment on My Site -> Comments list (trash filter) -> Comment Detail.
[x] Reply to a comment from My Site -> Comments list -> Comment Detail.
[x] Multi select comments on My Site -> Comments list, moderate them.

Notification list:

[x] Reply to a comment from the Notification list -> Comment Detail.
[x] Comments persistence, Notification list -> Comment Detail, start to reply to a comment, close the app, come back the comment you've started should still be there.
[x] Swipe between Notifications

Push notification:

[x] Like a comment from a Push Notification (Android system Notification).
[x] Reply to a comment from a Push Notification (Android system Notification).

Note: if we wanted to refactor this, we should remove the mNote references in CommentDetailFragment and EditCommentActivity, that complicates a lot of things and we could probably get the comment from the Notification[List/Activity] before.

aforcier commented 7 years ago

The switch to CommentStatusCriteria in CommentsListFragment means that these two equals() calls are now comparing different objects:

https://github.com/wordpress-mobile/WordPress-Android/blob/6e2a2d37daf8c2f8f1b659599742c90844435285/WordPress/src/main/java/org/wordpress/android/ui/comments/CommentsListFragment.java#L402

and

https://github.com/wordpress-mobile/WordPress-Android/blob/6e2a2d37daf8c2f8f1b659599742c90844435285/WordPress/src/main/java/org/wordpress/android/ui/comments/CommentsListFragment.java#L597

aforcier commented 7 years ago

Note.java now contains several unused methods, including canEdit(), which relies on localBlogId. Should we clear them out?

maxme commented 7 years ago

Note.java now contains several unused methods, including canEdit(), which relies on localBlogId. Should we clear them out?

done in e256bb7 (some other methods were not used, I kept them and they should work)

maxme commented 7 years ago

If I'm not mistaken, here are the remaining tasks before your second pass:

[x] Use a SiteModel in SuggestionServices (instead of a remote site id).
[x] Drop the commented setCommentIsModerating (or implement a better way to display them).
[x] Clean up CommentStatus equality and CommentStatusCriteria checks ( https://github.com/wordpress-mobile/WordPress-Android/pull/4864#issuecomment-265122025)
[x] Add asc/desc option in the getCommentsForSite CommentStore getter, (and use the same order in getCommentsForSite and Collection.sort calls)

maxme commented 7 years ago

CommentStatus(Criteria) comparisons fixed in 21fdd34fafe0dfe210900d7f0a22b88a41f2c2ff and 703b52c5c9b3fae682a36343bd16b2c654d70242 (not a big fan of fromComment() since it's just a one liner)

maxme commented 7 years ago

Drop the commented setCommentIsModerating (or implement a better way to display them).

I enabled this, (via https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/250 and 592b167) I'm not a big fan of this - I liked the optimistic approach (no loading indicator at all when moderating).

maxme commented 7 years ago

Add asc/desc option in the getCommentsForSite CommentStore getter, (and use the same order in getCommentsForSite and Collection.sort calls)

Fixed in https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/249 and bbbf93b

maxme commented 7 years ago

[x] Notification list stranger things

maxme commented 7 years ago

There is still one thing that bother me in this PR, the notification list is not refreshed after an action is applied in a comment from the notification list. I'll have to modify the note, save it and refresh the which is a bit sad since it's also done in the comment store (would be great to have a NotificationStore to fix this cleanly).

[x] Note update, save and Notification list refresh on comment modified.

aforcier commented 7 years ago

Drop the commented setCommentIsModerating (or implement a better way to display them).

I enabled this, (via wordpress-mobile/WordPress-FluxC-Android#250 and 592b167) I'm not a big fan of this - I liked the optimistic approach (no loading indicator at all when moderating).

I'm getting an issue with this change. when moderating items from the comment list the moderation action will complete server-side, and the status of the comments in the list will update, but the loading animation on the author icon continues indefinitely.

I'm not a big fan of this - I liked the optimistic approach (no loading indicator at all when moderating)

FWIW I prefer the optimistic approach as well, my original comment was only intended to point out the stale commented out code. If you want to drop the loading indicators I'm all for it.

aforcier commented 7 years ago

The filter list labels in the comment list are now capitalized (I don't think this was intentional):

comment-filter-list-current

Original view:

comment-filter-list-original

Also, the list now says UNAPPROVED, but we use Pending elsewhere (e.g. the empty view message shown in the first screenshot).

oguzkocer commented 7 years ago

If you mark a comment as spam or trash and go to spam/trash filter, that comment sometimes show up there and sometimes doesn't. If you refresh the list, the comment shows up OK. However, as @aforcier mentioned, there is an indefinite progress bar on top of gravatar AND you can't go into the detail view for that comment. You need to go back to the My Site page for it to properly work.

Edit: Interestingly enough marking approved/unapproved works perfectly. Both the list and the detail page works as expected.

screenshot_1481635425

oguzkocer commented 7 years ago

Suggestions don't seem to be working on Comment page anymore, it still works on Reader though. I've verified that it was working before on 6.1 on my phone.

maxme commented 7 years ago

Remaining issues are:

[x] Back to the optimistic comment moderation approach, drop the loading indicators. (c669d76)
[x] Filter labels shouldn't be capitalized (+ s/UNAPPROVED/Pending/) (fixed in 6268501)
[x] Fix the "@" suggestions on Comment reply (it still works on Reader though). (69e3761)
[x] Note update, save and Notification list refresh on comment modified.

maxme commented 7 years ago

Note: I fixed the loading indicator issue in a5cc0dd in case we want to add it back

aforcier commented 7 years ago

If I'm in a filtered view and change a comment's status, it still remains in the filtered view - shouldn't it disappear (it does on current develop)?

For example, select the 'Pending' filter and approve a pending comment (by long-pressing and using the action bar, not by going into detail view). The comment loses its PENDING label, but stays in the Pending list until you refresh or switch filters.

aforcier commented 7 years ago

I'm unable to edit the comment author name, email, and URL fields on either WP.com or self-hosted sites. It looks like we're not sending those fields in the edit comment requests at all.

For WP.com the edit appears to succeed but the comment is actually unchanged.

For self-hosted we get a 500 error from the server (presumably because the wp.editComment request wasn't modifying the comment) and display a An error occurred while editing the comment message to the user.

maxme commented 7 years ago

If I'm in a filtered view and change a comment's status, it still remains in the filtered view - shouldn't it disappear (it does on current develop)?

Fixed a29debf

I'm unable to edit the comment author name, email, and URL fields on either WP.com or self-hosted sites. It looks like we're not sending those fields in the edit comment requests at all.

I'll remove these fields

maxme commented 7 years ago

Note update, save and Notification list refresh on comment modified.

Fixed in 85f4eb9 - but only when a comment is moderated

@aforcier should be ready for another pass.

There is at least one bug: when you edit the comment text from the notication list, the text is not modified when you get back to the list (This happens on develop as well). And there is also a backend issue: the note is not updated on our server when a comment has been modified.

If this was just me, I'd simply remove that feature from the app, not because it's bugged, but because moderating someone else comment is fine, but editing it, it's a bit weird.

aforcier commented 7 years ago

I noticed an issue while testing the filter changes. Since we're not clearing existing comments on fetch (like we do with posts), we can have 'ghost' comments that no longer exist on the server but still exist in the local db and show up on the list. Example:

Make a comment on the site
Refresh the comment list in the app so that the new comment shows up
On the web, trash and delete (permanently) that comment
Refresh the app's comment list
Notice that the comment is still there

The above is the worst case scenario, though there are other problematic cases. If, e.g., the comment is only trashed on the web, it will still show up in the All filter, but refreshing the Trashed filter will update the comment in the db with its new status, and it won't show up under the All filter anymore.

aforcier commented 7 years ago

...we can have 'ghost' comments that no longer exist on the server but still exist in the local db and show up on the list.

Confirming that this is fixed with the changes in https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/256.

aforcier commented 7 years ago

Comment actions from push notifications aren't working for me.

Liking or replying to a comment from a PN (not from the notifications list in the app) starts a loading animation that never ends, and sniffing shows that no network request is made at all.

maxme commented 7 years ago

Comment actions from push notifications aren't working for me.

Fixed in b98706a

aforcier commented 7 years ago

:shipit:

wordpress-mobile / WordPress-Android