Enable Search for Self-Hosted Sites #12565

ParaskP7 · 2020-07-30T11:27:38Z

Fixes #12058
Prerequisite WordPress-FluxC-Android #1646

Description

This feature enables search for self-hosted sites, something that was previously only available for WordPress.com/Jetpack sites. See issue above for more info.

In general the solution is split into two parts:

Make the search icon available for when the user navigates to a self-hosted site, just as it is available for non self-hosted sites.
Add the user's search query to the XML RPC descriptor that is responsible to later make an API request for the self-hosted site. Note that with the develop version of this repo the search query argument is not available since a new version of Android FluxC is required (see To test section below).

So, the above work is closely related to the [Feature] Add Search Filter for XML RPC Requests #1646 PR, which needs to be merge first.

Blog Posts for Self-Hosted Site (Before)

Blog Posts for Self-Hosted Site (After)

Test:

Build the WordPress app with a fluxcVersion of e32d20633e68ca1fba1726b75463aa7219ca88f0 (update that within the project's parent build.gradle file)
Sign in to the app and add a self-hosted site
Navigate to Blog Posts
Verify that the Search Icon is available on the top right
Click on the Search Icon and verify the search functionality works as expected and in general works as it used to work for non self-hosted site
The above means that search is enabled for self-hosted sites

While testing note that search in general, for both for self-hosted and non self-hosted sites has a configuration change related bug, see #12561. However, it is out of the scope of this task to fix this bug, but it will be considered as part of another task, later on in the future.

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.

This tests were written on purpose before implementing the 'isAccessedViewXmlRpc(...)' method, TDD style, to prove that the next commit adds value. As such the last test will fail because it expects 'true' but the template utility method currently always returns 'false'.

The corresponding failing test now successfully passes.

By enabling search for self-hosted site, this test should pass with the current change. However, as it is now it doesn't since the main code is not yet implemented. Thus, updating first the test to fail and in the next commit the change will be implemented so that the test passes.

The corresponding failing test now successfully passes. This is the first (UI) part of the overall solution, which makes the search icon appear for self-hosted sites (previously hidden). This, along with the second (Backend) part of the overall solution (coming soon) will close the cycle.

This is done for readability purposes.

Also, this part is done to make the diffing easier on the next commit.

This is the second (Backend) part of the overall solution, which along with the first (UI) part, effectively enables search for self-hosted sites.

ParaskP7 · 2020-07-30T11:35:07Z

I am not sure about the two items in the PR Submission Checklist so please guide me to be able to ✅ them as well:

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.

Also, another ✅ that I would have liked to check are adding UI tests where possible, much I am not sure what is the process for adding those.

ParaskP7 · 2020-07-30T12:56:55Z

lint and test are failing on for CircleCI due to compilation errors. This is expected, since this PR is based on a future version of FluxC. As such, only when a commit with the newest version of FluxC will be added to this PR (or the develop gets it), will CircleCI pass these checks.

PS: Locally, both lint and test are running fine.

malinajirka

Thanks for the changes @ParaskP7! The apps works as expected, great job!

I have a couple of questions/suggestions.

The step 3 is mentioning "Site Pages". I'm wondering if we need to test site pages, since we modified just Post List's VMs. Wdyt?
I agree that a SiteModel can theoretically have "UNKNOWN" origin - at least the "enum" supports it. However, afaik we don't support that type anywhere else in the app. The app needs to communicate with either WPComRest or XML-RPC. If it doesn't know the type it can't communicate with the site at all. That's the main reason why we never found a need to introduce "isAccessedViaXmlRpc" -> !isAccessedViaWPComRest == isAccessedViaXmlRpc (btw there is a typo in the code "View" vs "Via"). I'm wondering if we really want to introduce it now. Wdyt?

ParaskP7 · 2020-07-30T14:11:39Z

Thanks for the changes @ParaskP7! The apps works as expected, great job!

I have a couple of questions/suggestions.

The step 3 is mentioning "Site Pages". I'm wondering if we need to test site pages, since we modified just Post List's VMs. Wdyt?

I agree that a SiteModel can theoretically have "UNKNOWN" origin - at least the "enum" supports it. However, afaik we don't support that type anywhere else in the app. The app needs to communicate with either WPComRest or XML-RPC. If it doesn't know the type it can't communicate with the site at all. That's the main reason why we never found a need to introduce "isAccessedViaXmlRpc" -> !isAccessedViaWPComRest == isAccessedViaXmlRpc (btw there is a typo in the code "View" vs "Via"). I'm wondering if we really want to introduce it now. Wdyt?

Many thanks for reviewing my PR @malinajirka and verifying its functionality! 🌟

You are right, this change is only about Blog Posts, I wonder why I added a step to verify Site Pages as well. I am going to update the description and remove it. It is totally misleading. I am guessing since I was constantly testing both to verify they work the same, I posted the same testing instructions. 😶
I wan't aware about the UNKNOWN indeed, thanks for clarifying that for me. Also, thanks for noting the View typo as well, will update it with a commit. So based on what you say, and if I understand correctly, then overall the isSearchAvailable LiveData value is not needed anymore and can be removed from the PostListMainViewModel. Then, every site with have search available by default. Is that right?

malinajirka · 2020-07-30T14:15:43Z

You are right, this change is only about Blog Posts, I wonder why I added a step to verify Site Pages as well. I am going to update the description and remove it. It is totally misleading. I am guessing since I was constantly testing both to verify they work the same, I posted the same testing instructions. 😶

No worries ;)!

I wan't aware about the UNKNOWN indeed, thanks for clarifying that for me. Also, thanks for noting the View typo as well, will update it with a commit. So based on what you say, and if I understand correctly, then overall the isSearchAvailable LiveData value is not needed anymore and can be removed from the PostListMainViewModel. Then, every site with have search available by default. Is that right?

Yep, sounds good to me ;).

ParaskP7 · 2020-07-30T14:17:43Z

Great, thanks again and I am on it, please expect an update to this PR very soon!

After a PR discussion it turns out that 'UNKNOWN' sites cannot exist and as such all sites, both WPCom and XML-RPC, can have search enabled by default.

This version contains the search filter for XML PRC requests functionality that the enable search for self-hosted sites solution depends on.

ParaskP7 · 2020-07-30T14:51:05Z

@malinajirka all your suggestions have been now added in this PR. Please let me know if everything looks as expected!

malinajirka

Thanks for the changes Petros! It all looks good to me. We can merge this PR when the conflicts are resolved and all the checks pass. GJ 🎉

…-sites

ParaskP7 added 9 commits July 30, 2020 13:16

Add a couple of missing is accessed via wp com rest tests

e5c726b

Implement is accessed view xml rpc utility method

818baf7

The corresponding failing test now successfully passes.

Add search is not available for unknown sites test case

777b630

Extract is search available private method

cb23ed3

This is done for readability purposes.

Reformat post list descriptor for xml rpc site to match rest site above

61e8149

Also, this part is done to make the diffing easier on the next commit.

Add search query to post list descriptor for xml rpc site

bc2f96b

This is the second (Backend) part of the overall solution, which along with the first (UI) part, effectively enables search for self-hosted sites.

malinajirka self-requested a review July 30, 2020 11:42

malinajirka self-assigned this Jul 30, 2020

malinajirka added Post List Self-hosted labels Jul 30, 2020

malinajirka added this to the 15.5 milestone Jul 30, 2020

malinajirka reviewed Jul 30, 2020

View reviewed changes

ParaskP7 added 5 commits July 30, 2020 17:22

Fix typo for is accessed via xml rpc site utils method

31cc65a

Remove is available check and enable search by default on post list

180d057

After a PR discussion it turns out that 'UNKNOWN' sites cannot exist and as such all sites, both WPCom and XML-RPC, can have search enabled by default.

Remove unused is accessed via xml rpc site utils method

6ca46b1

Update fluxc version to beta 1.6.18-beta-1

a802312

This version contains the search filter for XML PRC requests functionality that the enable search for self-hosted sites solution depends on.

Update release notes for 15.5

0ef50b2

malinajirka approved these changes Jul 31, 2020

View reviewed changes

Merge branch 'develop' into issue/12058-enable-search-for-self-hosted…

4c28975

…-sites

malinajirka merged commit a0fd478 into wordpress-mobile:develop Jul 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Search for Self-Hosted Sites #12565

Enable Search for Self-Hosted Sites #12565

ParaskP7 commented Jul 30, 2020 •

edited by malinajirka

ParaskP7 commented Jul 30, 2020

ParaskP7 commented Jul 30, 2020

malinajirka left a comment

ParaskP7 commented Jul 30, 2020 •

edited

malinajirka commented Jul 30, 2020

ParaskP7 commented Jul 30, 2020

ParaskP7 commented Jul 30, 2020

malinajirka left a comment

Enable Search for Self-Hosted Sites #12565

Enable Search for Self-Hosted Sites #12565

Conversation

ParaskP7 commented Jul 30, 2020 • edited by malinajirka

Description

Blog Posts for Self-Hosted Site (Before)

Blog Posts for Self-Hosted Site (After)

Test:

PR submission checklist:

ParaskP7 commented Jul 30, 2020

ParaskP7 commented Jul 30, 2020

malinajirka left a comment

Choose a reason for hiding this comment

ParaskP7 commented Jul 30, 2020 • edited

malinajirka commented Jul 30, 2020

ParaskP7 commented Jul 30, 2020

ParaskP7 commented Jul 30, 2020

malinajirka left a comment

Choose a reason for hiding this comment

ParaskP7 commented Jul 30, 2020 •

edited by malinajirka

ParaskP7 commented Jul 30, 2020 •

edited