Skip to content
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

Adds fetch all-domains api call #2867

Merged
merged 8 commits into from
Oct 12, 2023
Merged

Conversation

antonis
Copy link

@antonis antonis commented Oct 11, 2023

Part of wordpress-mobile/WordPress-Android#19165

This PR adds the ability to fetch all domains for the logged in user.

Note: For simplicity and given that the domains management feature will use the existing fetchSiteDomains call I decided to add the new call in the SiteStore though it is not directly related to the current site.

To test:

  1. Start the example app and sign in (note: setup oauth first)
  2. Tap on the DOMAINS button
  3. Tap on the FETCH ALL DOMAINS button
  4. Verify that your domains are listed
  5. Verify that the domains count is correct

@antonis antonis self-assigned this Oct 11, 2023
@antonis antonis requested review from mkevins and justtwago October 11, 2023 12:34
Copy link
Contributor

@justtwago justtwago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the changes, and it works as described. 🚀

Here is the fetched list of my domains.

The code looks good! I just left some minor comments that don't block the PR.

@@ -561,6 +584,13 @@ class SiteRestClientTest {
return initGetResponse(SitesFeaturesRestResponse::class.java, data ?: mock(), error)
}

private suspend fun initAllDomainsResponse(
data: AllDomainsResponse? = null,
error: WPComGsonNetworkError? = null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional nitpick: the property is never used. If we don't want to test the error cases for the "fetch all domains" flow, we can remove it and pass null directly, or we can have a test for a bad case too. What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for catching this @justtwago 🙇
I've added some tests for the error cases with 5bed1ee

Antonis Lilis and others added 2 commits October 12, 2023 09:55
Copy link
Contributor

@justtwago justtwago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you a lot for the changes! 🚀

@antonis antonis merged commit ae6a6e7 into trunk Oct 12, 2023
2 checks passed
@antonis antonis deleted the task/add-fetch-all-domains-call branch October 12, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants