RI: Fetch "Interests" tags from ReaderTagRepository #12490

ashiagr · 2020-07-21T13:17:46Z

Parent Issue: #12027

This PR

Updates ReaderTagRepository to return Interest API results to the Interests VM utilising ReaderRepositoryCommunication and FetchInterestTagsUseCase
Handles error

~~Looking for an early feedback before moving ahead, so marking it as draft.~~

To Test

Prerequisite - RI FF flag s enabled

Launch and go to Reader tab
Interests from server are shown

Enable Airplane mode
Launch and go to Reader tab
Error layout is displayed with Retry option
Disable Airplane mode
Click Retry
Error layout is hidden and interests are shown

To test remote request failure error, comment out SuccessWithData response and return RemoteRequestFailure here.

While the interests are shown, go to My Site -> Me -> App Settings and change interface language (e.g. Spanish)
Return to Reader tab
Notice that interests are shown in the selected language (returned from the server).

ToDo

Clicking "Comics" interest is not highlighting it 🤷‍♀️, although it enables Done button. Looking into it.
UPDATE: There are duplicate tags with "Comics" title in the feed. There shouldn't be such a case in the final feed. I'll put a "unique" check to fix it. - 2843b0f
I'm looking for a better solution for language change support. I've removed isStarted check so that we can re-request interests from the server for the changed language but it now happens every time we toggle bottom tabs or rotate screen. I'm thinking to compare last and new language in the interests VM for this something like conditional re-request of tags here. - 7ac80c2
Do we want to show "Contact Support" on remote request failure? We haven't yet decided on the error layout design.
Tests for FetchInterestTagsUseCase not yet added. Will include along with other use cases to save interests and get user tags.

PR submission checklist:

I have considered adding unit tests where possible.
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.

…nterests-api-results-from-repo

peril-wordpress-mobile · 2020-07-21T13:17:51Z

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

peril-wordpress-mobile · 2020-07-21T13:25:00Z

You can test the changes on this Pull Request by downloading the APK here.

ashiagr · 2020-07-21T13:22:55Z

...rc/main/java/org/wordpress/android/ui/reader/repository/usecases/FetchInterestTagsUseCase.kt

+            eventBusWrapper.register(this)
+
+            ReaderUpdateServiceStarter.startService(
+                contextProvider.getContext(),
+                EnumSet.of(INTEREST_TAGS)
+            )


I'm registering to event bus inside the use case, I've not seen this practice elsewhere in the code. Let me know if you see any downside.

@ashiagr - There is similar logic in #12454, but I used init & stop to set up the bus to support multiple requests at the same time. Not sure if you'll need to do that here, but wanted to mention it. Also, in the same PR, I have a ReaderRepositoryDispatchingUseCase that I use for all the repo usecases to set up coroutine context.

@malinajirka what do you think? Should we use init & stop pattern in this case? I'll make changes accordingly.

Since this useCase doesn't support parallel request by its nature, I think the current implementation looks good. I don't think we need to change anything.

ashiagr · 2020-07-21T13:29:30Z

...ss/src/main/java/org/wordpress/android/ui/reader/repository/ReaderRepositoryCommunication.kt

@@ -2,6 +2,7 @@ package org.wordpress.android.ui.reader.repository

 sealed class ReaderRepositoryCommunication {
    object Success : ReaderRepositoryCommunication()
+    data class SuccessWithData<out T>(val data: T) : ReaderRepositoryCommunication()


Passing interests directly to the VM via SuccessWithData. We might want to keep a single Success state with an optional data param. Lmkwyt.

@ashiagr - I like this idea for those non-reactive repository situations and I can go with either option. In one situation we check for the class and the other a quick optional check.

Looks like we'll have to keep just one Success state to avoid else condition in when clause as seen here.

ashiagr · 2020-07-21T13:37:59Z

WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderEvents.java

        private final ReaderTagList mInterestTags;
+        private final boolean mDidSucceed;


Added boolean to detect if request succeeded similar to SearchPostsEnded

zwarm

Looks good so far. Nice work.

zwarm · 2020-07-21T21:04:33Z

...rc/main/java/org/wordpress/android/ui/reader/repository/usecases/FetchInterestTagsUseCase.kt

+            eventBusWrapper.register(this)
+
+            ReaderUpdateServiceStarter.startService(
+                contextProvider.getContext(),
+                EnumSet.of(INTEREST_TAGS)
+            )


@ashiagr - There is similar logic in #12454, but I used init & stop to set up the bus to support multiple requests at the same time. Not sure if you'll need to do that here, but wanted to mention it. Also, in the same PR, I have a ReaderRepositoryDispatchingUseCase that I use for all the repo usecases to set up coroutine context.

zwarm · 2020-07-21T21:10:48Z

...ss/src/main/java/org/wordpress/android/ui/reader/repository/ReaderRepositoryCommunication.kt

@@ -2,6 +2,7 @@ package org.wordpress.android.ui.reader.repository

 sealed class ReaderRepositoryCommunication {
    object Success : ReaderRepositoryCommunication()
+    data class SuccessWithData<out T>(val data: T) : ReaderRepositoryCommunication()


@ashiagr - I like this idea for those non-reactive repository situations and I can go with either option. In one situation we check for the class and the other a quick optional check.

…ts screen is showing

ashiagr · 2020-07-22T13:53:03Z

@zwarm , @malinajirka - ready for another look 🙇‍♀️.

# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderRepositoryCommunication.kt

zwarm

All tests pass as expected, nice work. I left one comment, but it won't stop approval. 👍

zwarm · 2020-07-23T14:58:10Z

...rc/main/java/org/wordpress/android/ui/reader/repository/usecases/FetchInterestTagsUseCase.kt

+        }
+    }
+
+    @Subscribe(threadMode = ThreadMode.MAIN)


@ashiagr Just curious if this could be a Background thread. It's been a while since I used GreenRobot, so maybe things have changed, but iirc the post request is not made on the Main thread, so it will thread switch to emit on Main. Or I may be completely off the mark. wdyt?

Switched to BACKGROUND thread in ad7c6b0.

ashiagr added 8 commits July 21, 2020 11:23

Merge branch 'issue/interests-endpoint-integration' into issue/pass-i…

47e6296

…nterests-api-results-from-repo

Fix margins

28d1951

Update InterestsTagsFetched to include request succeeded state

810dbcc

Allow data to be passed in ReaderRepositoryCommunication.SuccessWithData

3e49fc3

Fetch interests in a UseCase

f3fa354

Pass interests result from repo to VM

0101957

Update existing tests

b3f082a

Merge branch 'issue/interests-endpoint-integration' into issue/pass-i…

4319c1c

…nterests-api-results-from-repo

ashiagr added Reader New Reader Android labels Jul 21, 2020

ashiagr self-assigned this Jul 21, 2020

Remove blank line

d5dd3a9

ashiagr commented Jul 21, 2020

View reviewed changes

ashiagr requested review from malinajirka and zwarm July 21, 2020 13:30

ashiagr changed the title ~~RI: Fetch "Interests" tags from real API from ReaderTagRepository~~ RI: Fetch "Interests" tags from ReaderTagRepository Jul 21, 2020

ashiagr commented Jul 21, 2020

View reviewed changes

zwarm reviewed Jul 21, 2020

View reviewed changes

ashiagr added 6 commits July 22, 2020 14:25

Move n/w check inside use case

c7c1b44

Remove isStarted check to allow reload on language change

9c44435

Add check to not load tabs on lang change via app settings if interes…

1f3b1b2

…ts screen is showing

Merge branch 'develop' into issue/pass-interests-api-results-from-repo

ae0373e

Replace if with when

baab29d

Add Interest VM tests for errors

2f7d46c

Base automatically changed from issue/interests-endpoint-integration to develop July 22, 2020 12:54

Merge branch 'develop' into issue/pass-interests-api-results-from-repo

f16419d

ashiagr marked this pull request as ready for review July 22, 2020 13:51

ashiagr added 5 commits July 23, 2020 11:08

Remove tags with duplicate slugs

2843b0f

Do not reload interests if language is same

7ac80c2

Merge branch 'develop' into issue/pass-interests-api-results-from-repo

618ab85

# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderRepositoryCommunication.kt

Fix merge conflict

b10e0ba

Minor fix

52469eb

zwarm approved these changes Jul 23, 2020

View reviewed changes

ashiagr added this to the 15.5 milestone Jul 24, 2020

Switch to background thread

ad7c6b0

zwarm merged commit 7e20200 into develop Jul 24, 2020

ashiagr modified the milestones: 15.5, 15.4 Jul 24, 2020

ashiagr deleted the issue/pass-interests-api-results-from-repo branch July 24, 2020 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RI: Fetch "Interests" tags from ReaderTagRepository #12490

RI: Fetch "Interests" tags from ReaderTagRepository #12490

ashiagr commented Jul 21, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jul 21, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jul 21, 2020 •

edited

Loading

ashiagr Jul 21, 2020 •

edited

Loading

zwarm Jul 21, 2020

ashiagr Jul 22, 2020

malinajirka Jul 23, 2020

ashiagr Jul 21, 2020

zwarm Jul 21, 2020

ashiagr Jul 22, 2020

ashiagr Jul 21, 2020

zwarm left a comment

zwarm Jul 21, 2020

zwarm Jul 21, 2020

ashiagr commented Jul 22, 2020

zwarm left a comment

zwarm Jul 23, 2020

ashiagr Jul 24, 2020

		private final ReaderTagList mInterestTags;
		private final boolean mDidSucceed;

RI: Fetch "Interests" tags from ReaderTagRepository #12490

RI: Fetch "Interests" tags from ReaderTagRepository #12490

Conversation

ashiagr commented Jul 21, 2020 • edited Loading

To Test

ToDo

peril-wordpress-mobile bot commented Jul 21, 2020 • edited Loading

peril-wordpress-mobile bot commented Jul 21, 2020 • edited Loading

ashiagr Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwarm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashiagr commented Jul 22, 2020

zwarm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashiagr commented Jul 21, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jul 21, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jul 21, 2020 •

edited

Loading

ashiagr Jul 21, 2020 •

edited

Loading