IA Reader self-hosted improvements #11270

develric · 2020-02-10T01:05:57Z

Replaces and supersedes #10986 (point 1 was fixed in #11206)

This PR introduces enhancements to the new IA reader for users that have self-hosted sites only or they do have also WP.com account but currently logged off from it. (cc @osullivanchris )

Note for the reviewer: the PR dimension/scope should be still good enough I hope, but if you feel it's too large/wide let me know and will try to split in some way 🙇‍♂️.

What we introduced:

When a user is logged out from WP.com and has at least a self-hosted site in the app, the reader is as follows:

Id	Description	Image
Fig1	On fresh install the Discover page is presented first (both wp.com and self-hosted)
Fig2	3 tabs and `Filter` subfiltering + Empty view with dedicated message + no access to settings cog (nor search icon but this was not introduced in this PR)
Fig3	Empty SITES in bottomsheet with CTA to login; if tapped on CTA, user is landed to Me page to login

Notes

On user id switches or when the user passes from having access token (wp.com user) to not having it (pure self-hosted) and viceversa, we trigger a Blogs/Tags auto refresh when the user comes to the reader
As a follow up to Refactor getValidTagForSharedPrefs and don't return null value #11206 we implemented an in memory default tag used as a sort of null object in cases where the tag table could be empty (see getDbOrInMemoryDefaultTag function)
We added a TagUpdateClientUtilsProvider class to create a single point where to get the client for tags update

To test

Test case 1

Make a fresh install and login with WP.com (no self-hosted)
Go to Reader and check you are landed in the Discover page
Go to FOLLOWING tab
Logout from WP.com and add a self-hosted site
Go to Reader and check you are landed in the Discover page as well (Fig1)
Go to Following and check you get something similar to Fig2

Test case 2

Verify you are logged out from WP.com account and you have added a pure self-hosted site
In FOLLOWING open bottom sheet and check you get something similar to Fig3
Press the Login CTA and check you are landed into Me page
Login into WP.com and come back to Reader
Check you have now your followed SITES and TAGS similar to below image

Test case 3

Smoke test the reader both with wp.com and self-hosted sites (below some of the possible steps for smoke testing but feel free to add 😊):

Move between tabs
Use the subfilter to select different sources (Sites/Tags) to navigate
Open posts from the reader
Save posts for later and open them from the SAVED tab (both WP.com and self-hosted)
Follow/Unfollow tags/sites from the Reader (only for WP.com)
Follow/Unfollow tags/sites from the Setting (only for WP.com)
Whatever else you can think of 😊...

Test case 4

See comment here

NOTE: for the modifications 2 and 3 reported in the NOTES above, a part the smoke test I retested the steps reported in #11206 (good if you could also retry those steps, thanks).

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.

…view model to auto update posts when user changes.

peril-wordpress-mobile · 2020-02-10T01:11:02Z

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

peril-wordpress-mobile · 2020-02-12T00:25:01Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

osullivanchris · 2020-02-12T18:12:20Z

LGTM @develric think we have discussed all over slack. All works as intended

the link to login is working much better for me now than the old link we had, thank you
only other note that maybe we should consider the new copy for copy review, as we've added some ourselves

otherwise looking good, thanks!

develric · 2020-02-13T00:51:10Z

FAO editorial team. As per Chris request above, we need a copy review for the following (let me know if more context is needed, thanks 🙇‍♂️):

Users with self-hosted sites only, will need to use the Filter button to select from available Tags. We currently used Use the filter button to see posts from selected subjects as in below image

Users with self-hosted sites only (not logged in into WP.com), will have no followed sites and we call to action them to login into WP.com to be able to follow sites and get their posts (see below image):

About this last one, we originally had this:

but we changed it also because the call to action text seemed too long for a button action.

michelleweber · 2020-02-13T11:20:52Z

Some small tweaks:

Use the filter button to find posts on specific subjects

Log in to WordPress.com to see the latest posts from sites you follow

Button: Log in to WordPress.com

develric · 2020-02-13T13:13:25Z

Hi @michelleweber , thanks for checking in 🙇‍♂️!

I changed the copy as per your advice; for tracking purpose here below the result:

khaykov

I love the new behavior for self-hosted sites! Glad to see CTA in the bottom sheet, and some default tags to help explore the reader.

@osullivanchris I was a bit bummed when I logged in to wpcom account, and all those default tags were gone, including one I selected.

@develric It's a big PR, full of changes and new logic, so I will need another pass to get a better idea of it. Most of my initial questions are about what is going on since I probably lack some context. And naming :)

khaykov · 2020-02-17T04:36:05Z

WordPress/src/main/java/org/wordpress/android/ui/prefs/AppPrefs.java

+        boolean wasFollowing = false;
+
+        if (BuildConfig.INFORMATION_ARCHITECTURE_AVAILABLE) {
+            String wasFallowingString = getString(DeletablePrefKey.READER_TAG_WAS_FOLLOWING);


Maybe I'm missing something, but I don't see us ever calling setString with DeletablePrefKey.READER_TAG_WAS_FOLLOWING. Will wasFallowingString always be null in this case?

The intention here was to check if the DeletablePrefKey.READER_TAG_WAS_FOLLOWING key was present at all in the Shared Prefs. We could have it not set for example in cases where user is upgrading from a previous version of the app. In those cases I think we do not have enough information as of the saved tag was a Following tag or not, so I was managing it the same as the case where we have empty DeletablePrefKey.READER_TAG_NAME (that is we cannot rely on that to get the information we want so let's do not use it). I added a comment here to report this logic in code, let me know if it makes any sense and wdyt, thanks 😊.

Thanks for the explanation! I think my confusion came from the fact that boolean is stored as a string :)

khaykov · 2020-02-17T04:48:52Z

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

@@ -2673,16 +2765,28 @@ public void onScrollToTop() {
    public static void resetLastUpdateDate() {
        mLastAutoUpdateDt = null;
    }
+
+    private boolean isCurrentTagTheFollowingTag() {


What is the difference between the current and the following tags?

Good question 😊. Trying to give some high level context.

Before IA reader changes

All Tags were at the same level selectable by a spinner.

After IA reader changes

Now there are two level of filtering:

tabs (1st level)

subfilter component (2nd level). This only available in the FOLLOWING tab

Also we introduced the possibility to filter and navigate posts from a single site/blog directly in the FOLLOWING tab.

Given this, we can have different kind of tags categories:

special tags that are hosted in the tabs 1st level filter (namely DISCOVER, LIKES, SAVED)

tags related to custom lists of tags (tbh I think these are no more supported (that is new users cannot create them) but it’s possible some users still have them configured). These or other special TAGS like AUTOMATTIC tag are hosted to the right of SAVED tab as additional tabs themselves.

Then we have the FOLLOWING tab that hosts
a. Unfiltered posts from all followed sources (when no subfilter is selected)
b. Filtered streams of posts as filtered from the bottomsheet subfilter SITES/TAGS lists

So in this context, the currentTag behaves as usual when you are in a tab other than FOLLOWING or you are in FOLLOWING tab without a 2nd level subfilter selected.

When in FOLLOWING tab, the currentTag can additionally be set to a tag from the list of followed tags or not be valid at all since the reader is set to show posts from a site (in this last case the mPostListType is also set to ReaderPostListType.BLOG_PREVIEW
instead of ReaderPostListType.TAG_FOLLOWED).

[Note: not directly related to this PR but maybe could be helpful to see how the post list type is set in ReaderPostListViewModel.onSubfilterChanged]

Hope the above helps more than confuse; this said, the answer this method should answer is if currentTag is one of those for which we should be in the FOLLOWING tab, so we can maybe change the name from isCurrentTagTheFollowingTag to isCurrentTagInFollowingTab? (if it makes a better sense at all 😊). Changed here

Note for myself: make a flowchart/scheme of the above at least to document the end of the project 😊.

khaykov · 2020-02-17T04:52:05Z

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

-            mViewModel.getStartSubsActivity().observe(this, event -> {
-                event.applyIfNotHandled(tabIndex -> {
-                    ReaderActivityLauncher.showReaderSubs(requireActivity(), tabIndex);
+            mViewModel.getExecEmptyViewAction().observe(this, event -> {


Just curious, what Exec stands for here?

It stays for execute but looking at it with fresh eyes maybe better to just rename from _execEmptyViewAction to _bottomSheetEmptyViewAction 😊. Changed here.

khaykov · 2020-02-17T04:59:00Z

WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/BottomSheetEmptyUiState.kt

@@ -8,6 +8,14 @@ sealed class BottomSheetEmptyUiState {
    data class VisibleEmptyUiState(
        val title: UiStringRes,
        val buttonText: UiStringRes,
-        val actionTabIndex: Int
+        val action: ActionType
    ) : BottomSheetEmptyUiState()


Not really related to this PR, but what do you think about renaming this class to something that points to which bottom sheet it applies too (eg. SubfilterBottomSheetEmptyUiState)? I know it's located in a subfilter package, but we often time don't see or overlook this information :)

Good point 👍! Changed here

khaykov · 2020-02-17T05:19:58Z

WordPress/src/main/java/org/wordpress/android/ui/reader/utils/ReaderUtils.java

@@ -246,6 +247,32 @@ public static ReaderTag getDefaultTag() {
        return defaultTag;
    }

+    public static @NonNull ReaderTag getDbOrInMemoryDefaultTag(


Logic in switching from the concept of default tag to InMemoryDefault is not super clear to me, so I was wondering maybe you can explain what is going on here :) Word memory brings all kinds of questions :) Maybe we can simplify the naming in some way. Looking at the logic of this method, what I understand happens is - we are getting default tag, and if it was not existing and was just created, we turn it into Following tag. If I my understanding is correct, would it be fair to call this method something like getDefaultOrFollowingTag?

This one is not straightforward sorry 😊.

Default Tag is actually the Following (at least currently). It is possible that that tag is not present in the db (for example when user has just switched app language or for network issues reasons). In those occasions there were logic in the ReaderPostListFragment that was trying to ReaderTagTable.getFirstTag() from the db, but also it is possible that getFirstTag returns null if tags table is empty (see #11206 for some more details and for a way to force the condition).

In cases like this, we do not want to return a null tag but instead a defaultTag created in memory (not updating the db table) also populating the endpoint with the correct address (that is not the case when calling the standard getDefaultTag function.

So the intention of the name getDbOrInMemoryDefaultTag was: get default tag from db or create it in memory.

Tried to rename it like getDefaultTagFromDbOrCreateInMemory here ; let me know if it seems a better fit or not, thanks 🙇‍♂️!

khaykov · 2020-02-17T05:24:12Z

WordPress/src/main/java/org/wordpress/android/ui/reader/utils/ReaderUtils.java

@@ -331,6 +358,21 @@ public static ReaderTagList getOrderedTagsList(ReaderTagList tagList, Map<String
        return orderedTagList;
    }

+    public static boolean containsFollowing(ReaderTagList tagList) {


Just for clarity, what do you think about renaming this on to containsFollowingTag? Also, we can even potentially move this method into ReaderTagList so it would be able to tell us by itself - does it contain the following tag or not :)

Yep! Like it, changed here 👍

…erTag function.

…side ReaderTagList.

…ultTagFromDbOrCreateInMemory.

develric · 2020-02-17T17:47:53Z

I love the new behavior for self-hosted sites! Glad to see CTA in the bottom sheet, and some default tags to help explore the reader.

Glad to hear this 🙇‍♂️!

@osullivanchris I was a bit bummed when I logged in to wpcom account, and all those default tags were gone, including one I selected.

TBH this was also a comment from @osullivanchris at first. We agree we should try to not lose the tags a WP.com signed out user seems to be interested in. IIRC currently a signed out user cannot follow/unfollow sites/tags but he can bookmark locally to the device posts for later. A similar local solution (save local lists of follow tags/sites) or a cloud solution for self-hosted users (if this can make sense) could be of value and maybe something that could be tackle in the upcoming projects that are reader related (wdyt @osullivanchris ?).

Most of my initial questions are about what is going on since I probably lack some context. And naming :)

Yep I had the suspect it could be a bit wide (as said in the description) sorry 😅! I tried to follow up to your initial comments trying to give a bit more context. Hope it helps. Let me know 😊!

…separating responsabilities: loadSubFilters only needs to load subfilters and onSubfilterClicked only needs to update the current subfilter both without deciding on isSelected.

… even if not fully having all same fields.

develric · 2020-02-18T21:45:30Z

Hi @khaykov , thanks for reaching out to report an issue found while you were testing (I know you are fully in support rotation so additional kudos for looking into this one 😊).

Issue

Reproduction of this issue is time dependent but:

Go to Settings > Site you may like
Click on one of the suggested blog and follow it
Come (the quicker the best) back to reader and try to select the just followed blog from the bottom sheet
The posts of the blogs are loaded but opening again the bottom sheet it's possible you get the relevant row not selected (this happens only while following SITES YOU MAY LIKE blogs

Analysis

This is what happens:

Following a site from the list of recommended site, the site is added to the wpreader.db > tbl_blog_info table like below
AFAIU this is a strange state since feed_id should be <> 0 only for feeds and in those case blog_id and feed_id should be the same; also you have a feed_url but this is not a feed. Below a standard case of two feeds from same base url
if a trigger to update tag/blogs si performed (like for example when you open the bottom sheet), the record is fetched again from the API (this time being a followed site) and it gets correctly purged like below
The issue happens if you are fast enough to select the subfilter so assigning the _currentSubfilter value with the wrong not purged data taken from the recommended blogs status. From that time on, the call to ReaderBlog.isSameAs function to check if the blog should be the selected one will fail until you select it again updating the content of _currentSubfilter

Changes

First suspect was a sort of race condition in loadSubfilters, so moved all the update to _subFilters and _currentSubfilter to the main ui thread; this eventually is not the issue but seems a better fit so I kept the modifications
Separating responsibilities:
a. loadSubFilters only needs to load subfilters
b. onSubfilterClicked only needs to update the current subfilter
c. none of them needs to decide on if a filter item isSelected. Moving that logic to SubfilterPageFragment where observing updates to _subFilters
To decide if a blog/feed is the same as another for bottomsheet/subfilter we do not need to check all the fields but only a subset -> introduced a ReaderBlog.isSameBlogsAs function that checks blogId and blogUrl; feeds Url and FeedId are checked only if

 boolean areBothValidFeeds = this.blogId == this.feedId
                                    && blogInfo.blogId == blogInfo.feedId
                                    && this.hasFeedUrl()
                                    && blogInfo.hasFeedUrl();

This should save us from false negative when adding followed sites from recommended blogs (the issue above) and also it seems logically more correct (we are not checking all fields are the same but that we are dealing with the same blog/feed).

Side note: looking into this one I also found this where ReaderBlogList.hasSameBlogs is used to decided if to notify of FollowedBlogsChanged only in case of different blogId. Not sure it is correct given the above (since we miss to be notified for example in this case when recommended blogs record is purged) but it seems beyond the scope of this PR also given we managed it anyway not relaying on this.

khaykov

Thank you for the changes and explanation, @develric !

Tons of code here, so I can't guarantee that I covered/noticed everything, but I did my best to test the functionality and it works without any issues 👍

Ideally, we would split PR's like this into smaller components (even if they might not have any visible changes OR only have changes in presentation layer). But I'm not the one to say this, hehe :)

osullivanchris · 2020-02-20T13:34:30Z

@osullivanchris I was a bit bummed when I logged in to wpcom account, and all those default tags were gone, including one I selected.

Aw I know! We added this as an empty state but I had the same feeling too...it seems generally nice to have! I think maybe it could be a signal to improve the discovery/adding of tags. There's no way to know what tags exist. I know @mbshakti has started some work on this so I'll ping here here

develric added 3 commits February 10, 2020 01:58

Field renaming and method access changing.

7280544

Storing in shared prefs if the tag is Following tag.

74d8d33

Changes to manage self-hosted sites scenario and adding logic in the …

bca7d13

…view model to auto update posts when user changes.

develric added [Type] Enhancement Reader Self-hosted IA labels Feb 10, 2020

develric added this to the 14.3 milestone Feb 10, 2020

develric added 3 commits February 12, 2020 01:21

Adding methods to save user account fields in shared prefs.

345d20d

Adding strings resources for self-hosted.

61413c2

Refactoring the CTA(s) in bottom sheet for self-hosted sites.

1ed243c

develric added 4 commits February 12, 2020 11:51

Fixing unit tests and adding a few more.

076f9c7

getReaderTag return null if wasFollowing field not available yet.

c397602

Adding Unit Test for TagUpdateClientUtilsProviderTest.

f168337

Enforcing Feature Flag.

ededbca

develric requested review from malinajirka and khaykov February 12, 2020 15:47

develric marked this pull request as ready for review February 12, 2020 15:51

develric added the [Status] Needs Code Review label Feb 12, 2020

develric mentioned this pull request Feb 12, 2020

IA reader self hosted enhancements #10986

Closed

2 tasks

develric added the [Status] Needs Copy Review label Feb 13, 2020

Changed strings as per copy review.

f5632c1

develric removed the [Status] Needs Copy Review label Feb 13, 2020

khaykov self-assigned this Feb 13, 2020

khaykov reviewed Feb 17, 2020

View reviewed changes

develric added 7 commits February 17, 2020 12:07

Adding comment to better explain a piece of logic in AppPrefs.getRead…

4dc164e

…erTag function.

Renaming containsFollowing into containsFollowingTag and moving it in…

bb5c692

…side ReaderTagList.

Refactoring isCurrentTagTheFollowingTag and isFollowing.

bb7b190

Renaming _execEmptyViewAction to _bottomSheetEmptyViewAction

bc5a92f

Renaming BottomSheetEmptyUiState to SubfilterBottomSheetEmptyUiState.

8502a91

Renaming ReaderUtils.getDbOrInMemoryDefaultTag to ReaderUtils.getDefa…

6562ce8

…ultTagFromDbOrCreateInMemory.

Fixing Unit Testing.

b1d594c

develric requested a review from khaykov February 17, 2020 17:47

develric mentioned this pull request Feb 18, 2020

IA reader accessibility audit #11333

Merged

3 tasks

develric added 3 commits February 18, 2020 21:45

Moving _subFilters and _currentSubfilter to the Main UI Thread. Also …

d5c29f4

…separating responsabilities: loadSubFilters only needs to load subfilters and onSubfilterClicked only needs to update the current subfilter both without deciding on isSelected.

Creating method isSameBlogAs to check if two blogs/feeds are the same…

cbcc52a

… even if not fully having all same fields.

Fixing Unit Test.

a29b98a

Renaming isSameBlogAs to isSameBlogOrFeedAs.

82b884e

khaykov approved these changes Feb 19, 2020

View reviewed changes

khaykov merged commit d65d8c4 into develop Feb 19, 2020

develric mentioned this pull request Feb 20, 2020

Removing Feature Flag for IA reader feature. #11343

Merged

3 tasks

jkmassel deleted the issue/ia-reader-selfhosted-improvements branch October 17, 2024 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IA Reader self-hosted improvements #11270

IA Reader self-hosted improvements #11270

develric commented Feb 10, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Feb 10, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Feb 12, 2020

osullivanchris commented Feb 12, 2020

develric commented Feb 13, 2020

michelleweber commented Feb 13, 2020

develric commented Feb 13, 2020

khaykov left a comment

khaykov Feb 17, 2020

develric Feb 17, 2020

khaykov Feb 18, 2020

khaykov Feb 17, 2020

develric Feb 17, 2020

khaykov Feb 17, 2020

develric Feb 17, 2020

khaykov Feb 17, 2020

develric Feb 17, 2020

khaykov Feb 17, 2020

develric Feb 17, 2020

khaykov Feb 17, 2020

develric Feb 17, 2020

develric commented Feb 17, 2020

develric commented Feb 18, 2020 •

edited

Loading

khaykov left a comment

osullivanchris commented Feb 20, 2020

IA Reader self-hosted improvements #11270

IA Reader self-hosted improvements #11270

Conversation

develric commented Feb 10, 2020 • edited Loading

Notes

To test

Test case 1

Test case 2

Test case 3

Test case 4

peril-wordpress-mobile bot commented Feb 10, 2020 • edited Loading

peril-wordpress-mobile bot commented Feb 12, 2020

osullivanchris commented Feb 12, 2020

develric commented Feb 13, 2020

michelleweber commented Feb 13, 2020

develric commented Feb 13, 2020

khaykov left a 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

Before IA reader changes

After IA reader changes

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

develric commented Feb 17, 2020

develric commented Feb 18, 2020 • edited Loading

Issue

Analysis

Changes

khaykov left a comment

Choose a reason for hiding this comment

osullivanchris commented Feb 20, 2020

develric commented Feb 10, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Feb 10, 2020 •

edited

Loading

develric commented Feb 18, 2020 •

edited

Loading