-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Jetpack REST connection: install plugin #22119
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
Conversation
…ir is an HTTP status code
…ir is an HTTP status code
…-Android into feature/jetpack-connect-install
Generated by 🚫 Danger |
|
App Name | ![]() |
|
Flavor | Jalapeno | |
Build Type | Debug | |
Version | pr22119-77eaae8 | |
Commit | 77eaae8 | |
Direct Download | jetpack-prototype-build-pr22119-77eaae8.apk |
|
App Name | ![]() |
|
Flavor | Jalapeno | |
Build Type | Debug | |
Version | pr22119-77eaae8 | |
Commit | 77eaae8 | |
Direct Download | wordpress-prototype-build-pr22119-77eaae8.apk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements Jetpack plugin installation functionality for the Jetpack REST connection flow. When users connect to a self-hosted site without Jetpack installed, the app now handles plugin installation automatically as part of the connection process.
- Adds a new
JetpackInstaller
class that uses the WordPress REST API to install and activate the Jetpack plugin - Updates the connection view model to integrate plugin installation as a proper step in the flow
- Adds error handling for installation failures and inactive plugin states
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
strings.xml | Adds error messages for Jetpack installation failures |
JetpackRestConnectionViewModel.kt | Integrates plugin installation step and improves flow control |
JetpackRestConnectionScreen.kt | Updates UI to handle new installation error types |
JetpackInstaller.kt | New class implementing Jetpack plugin installation via REST API |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackInstaller.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackInstaller.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackInstaller.kt
Outdated
Show resolved
Hide resolved
...c/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackRestConnectionViewModel.kt
Outdated
Show resolved
Hide resolved
} | ||
|
||
private fun initApiClient(site: SiteModel): WpApiClient { | ||
return WpApiClient( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏️ What about moving this WpApiClient
creation to. WpApiClientProvider
, so we are sure every instance creation is done there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I can look into this for this subsequent PR which moves the WpApiClient
initialization to a helper class.
|
@@ -60,6 +63,9 @@ class JetpackRestConnectionViewModel @Inject constructor( | |||
} | |||
} | |||
|
|||
/** | |||
* Called when all steps have completed successfully | |||
*/ | |||
private fun onJobCompleted() { | |||
appLogWrapper.d(AppLog.T.API, "$TAG: Jetpack connection job completed") | |||
job?.cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Not related to this PR, but out of curiosity. Why do we need to cancel the job when it's completed? A job is automatically finished when all its code is run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, that's not really necessary. I'll remove that in the next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest auditing the job lifecycle because if the internal called methods can create their own scope, and the current job will finish while internally other threads are running. This happened to me when I tried to cancel uploads using jobs. It happened that OkHttp
was handling the network calls on its own, so the external job was useless. So, since you are using worpdress-rs
, which uses OkHttp, it would be great to double-check it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using a job so the flow could be cancellable, but you make a very good point. There really isn't a way to cancel any of the steps in this flow once they're underway - it only cancels moving to the next step. Perhaps it would make more sense to simply have an isCancelled
flag so we can stop after the current step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, makes sense. Maybe just having a flag or a state is simpler and effective at the same time. If the job is not adding extra cancellable value, I would go for the simplest approach then, because of readability and maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the Linear project to include removing the job
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart form the minor comments I did, it looks good to me!
@nbradbury I looked at the |
This PR updates the Jetpack REST connection flow to install the Jetpack plugin.
@oguzkocer I don't expect a full review from you, but it would be good to have your eyes on JetpackInstaller to verify I'm using
wordpress-rs
correctly.To test
Note: The connection flow will be stuck on the "Connect your site" step after the plugin is installed. That will be tackled in the next PR.
Screen_recording_20250813_154231.mp4