-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue/12039 reader like action #12454
Conversation
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
WordPress/src/main/java/org/wordpress/android/ui/reader/actions/ReaderPostActions.java
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/actions/ReaderPostActions.java
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/actions/ReaderPostActions.java
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt
Outdated
Show resolved
Hide resolved
You can test the changes on this Pull Request by downloading the APK here. |
...s/src/main/java/org/wordpress/android/ui/reader/repository/usecases/PostLikeActionUseCase.kt
Outdated
Show resolved
Hide resolved
Great job @zwarm! I've reviewed the code and it all LGTM. The only thing I'm not sure about is if we will need to propagate to the UI that the like state has changed in the db right after it happens -> when the user clicks on the "Like" button I think we'll want to change the state on the Ui to "Liked". It seems the current implementation waits for the request to finish. Wdyt? Update: Another approach would be to show some kind of loading/inProgress state. Wdyt? |
@malinajirka This is definitely one of my open questions.
Is this an area where we'd be better off with a direct response from the method or a reactive stream? So a direct return of LikeState or LiveData<Event> with postId, blogId, etc? I'm leaning towards the latter because we can continue to update as things occur - we could possibly include the updated ReaderPost on success. Thoughts? |
My first thought is that it'd just complicate things. I can think of two relatively easy options
Either case we can discuss all this on Monday. I'm sure there are more options I haven't considered. |
Ha - that's the same conclusion I came to yesterday when thinking about the other parts of the app updating the tables. I agree, we have plenty to discuss next week. :) |
Logic for ReaderPostTable modifications will be in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @zwarm! I've found a few more issues and left some questions.
WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderDiscoverRepository.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderRepositoryEvents.kt
Show resolved
Hide resolved
...s/src/main/java/org/wordpress/android/ui/reader/repository/usecases/PostLikeActionUseCase.kt
Outdated
Show resolved
Hide resolved
...s/src/main/java/org/wordpress/android/ui/reader/repository/usecases/PostLikeActionUseCase.kt
Outdated
Show resolved
Hide resolved
...s/src/main/java/org/wordpress/android/ui/reader/repository/usecases/PostLikeActionUseCase.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the changes!!
Part of #12039
This PR is
Background:
ReaderPostActions.performLikeAction
is a static method that runs an async operation to submit the "like" request to the server. The method does not wait for the request to return, but instead returns a true/false to the user immediately. The method constructs two listeners (error/success) for callbacks on the rest request. There is no action if the rest call is successful, but in the case of a failure the local DB changes are reverted.With this implementation I worked within the confines of the existing
ReaderPostActions.performLikeAction
method and created a new EventBus eventOnReaderRepositoryEvent
which will get posted once the rest callbacks (success or error) have been completed or there are no changes to be made. The addition of these events will not effect the existing functionality and will only be listened for inPostLikeActionUseCase
.The
PostListActionUseCase
transforms theOnReaderRepositoryEvent
to a coroutine so the rest callbacks can be handled outside of the static method. This gives us flexibility in the way we handle and/or communicate back to the caller.I am hopeful that we can use this pattern for the other Reader Actions implementations.
In terms of the Like action itself, we should consider what the caller needs to be aware of. Do they need to receive Unchanged, Success, and Failure states? Should the new methods return a
LiveData<Event<Boolean>>
or possibly aLiveData<Event<ReaderPost>>
?It was decided that no response is returned to the caller of the Like function and optimistic success will granted. Failures will be returned on the communicationChannel. In either case, the repository will determine if posts need to be reloaded and the observers to LiveData will be notified.
I would love to hear your thoughts. Also, I am not tied to any of the chosen event or use case names, so feel free to offer suggestions. There are plenty of todos that will also give some clues to what I am thinking for a future iteration.
FYI: Sorry about the number of file changed, this PR had to be based off of PR #12270 which hasn't been merged into develop yet.