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

Filter Jetpack sites for Jetpack app #1936

Merged
merged 18 commits into from
Apr 6, 2021
Merged

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Mar 26, 2021

Description

  • This PR adds filtering for Jetpack sites if the request to fetch sites comes from the Jetpack app.

How to test

Merge Instructions

This PR targets feature/jetpack-app-basics-fluxc. We'll decide next week if we want to merge these changes to develop.

@ashiagr
Copy link
Contributor Author

ashiagr commented Apr 1, 2021

👋 @ParaskP7

As per our discussion on isJetpackApp handling, instead of getting app info from context.getPackageName(), I've added filters param to the fetchSites() method. I now pass jetpack filter from the client app if Jetpack product flavor is used.

P.S.

  1. There were three site filters in the Calypso code, I've supported all of them and added connected tests in f940f79.
  2. Since SiteStore and SiteRestClient classes don't seem to be tests friendly (also no tests exists for them), I didn't include unit tests.
  3. You might need to update .mobile-secrets locally as I updated it.

Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @ashiagr !

I have reviewed and tested this PR as per the instructions, everything works as expected, good job! 🌟

I have left a couple of warning (⚠️) comments, some minor (🔍) comments and a few suggestions (💡) for you to consider.

I am still skeptical whether this change needs to target the jetpack feature branch or directly the develop. While reviewing this PR I got to thinking that Woo is going to be using this new enhanced api as well, through newFetchSitesAction(...). As such, when that eventually enters develop, the Woo team would need to update their usages of that api. This makes me think that it is better if that is merged to develop sooner that later so that we don't forget about this going forward and so that we don't create too many requirements before merging later on. I also suggest that after this is merged to develop, we ourselves go and created two PRs, one on the WordPress and one on the Woo side to merge this change with the apps' develop as soon as possible. This way, all apps will be able to reuse this functionality from there after (WordPress, Jetpack and Woo) and we will stop thinking about that anymore, progressing with the next task in line. Also, since the tests.properties where updates to account for this change, new tests where written to support this, this makes me feel that merging to develop now is better.

Having said that, if you and @zwarm feel strongly about keeping the feature branch on FluxC and do the merge later on, I am okay with that path too. Even thought this path will make us start maintain and update yet another feature branch, on a separate repo, it hopeful shouldn't be that difficult. 🤞

@ashiagr ashiagr changed the base branch from feature/jetpack-app-basics-fluxc to develop April 5, 2021 07:12
Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @ashiagr !

I have reviewed and tested this PR as per the instructions, everything works as expected, good job! 🌟

PS: Thanks for the updates! ❤️

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.

None yet

2 participants