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

Add rs.wordpress.api:android dependency #21108

Merged
merged 12 commits into from
Sep 4, 2024
Merged

Conversation

oguzkocer
Copy link
Contributor

@oguzkocer oguzkocer commented Aug 1, 2024

This PR adds the WordPress.rs library and uses it to attempt URL auto discovery for new XMLRPC sites that are added to the apps. This should be a very small number of users, and it's performed in the background so it shouldn't affect UX.

To Test:

  1. Run this PR on a device or emulator
  2. Add a new XMLRPC site (vanilla.wpmt.co is a good one)
  3. Note in logcat that the auto discovery is successful
  4. Try a site that doesn't support auto discovery (google.com is a good one)
  5. Note in logcat that the auto discovery is not successful

@oguzkocer oguzkocer added this to the 25.3 milestone Aug 1, 2024
@oguzkocer oguzkocer requested a review from jkmassel August 1, 2024 19:39
@oguzkocer oguzkocer enabled auto-merge August 1, 2024 19:39
@oguzkocer oguzkocer mentioned this pull request Aug 1, 2024
@oguzkocer oguzkocer disabled auto-merge August 1, 2024 19:55
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 1, 2024

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr21108-9c61018
Commit9c61018
Direct Downloadwordpress-prototype-build-pr21108-9c61018.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 1, 2024

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr21108-9c61018
Commit9c61018
Direct Downloadjetpack-prototype-build-pr21108-9c61018.apk
Note: Google Login is not supported on these builds.

@oguzkocer oguzkocer force-pushed the add/wordpress-rs-dependency branch from bbed50f to fe4572f Compare August 7, 2024 18:44
@oguzkocer oguzkocer marked this pull request as draft August 7, 2024 18:46
Copy link

codecov bot commented Aug 7, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 40.48%. Comparing base (a77d3cb) to head (9c61018).
Report is 1 commits behind head on trunk.

Files with missing lines Patch % Lines
...rg/wordpress/android/ui/accounts/LoginViewModel.kt 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #21108      +/-   ##
==========================================
- Coverage   40.49%   40.48%   -0.01%     
==========================================
  Files        1530     1530              
  Lines       69818    69821       +3     
  Branches    11443    11443              
==========================================
  Hits        28270    28270              
- Misses      39094    39097       +3     
  Partials     2454     2454              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oguzkocer oguzkocer removed the request for review from jkmassel August 7, 2024 19:20
@oguzkocer oguzkocer force-pushed the add/wordpress-rs-dependency branch from 1435cdc to c45fd95 Compare August 16, 2024 01:25
@jkmassel jkmassel force-pushed the add/wordpress-rs-dependency branch from c45fd95 to d2a6d63 Compare August 29, 2024 20:34
@wpmobilebot
Copy link
Contributor

Found 1 violations:

The PR caused some dependency changes (expand to see details)

-+--- org.jetbrains.kotlin:kotlin-stdlib:1.9.22 -> 2.0.0 (*)
++--- org.jetbrains.kotlin:kotlin-stdlib:1.9.24 -> 2.0.0 (*)
 +--- project :libs:image-editor
-|    +--- org.jetbrains.kotlin:kotlin-stdlib:1.9.22 -> 2.0.0 (*)
+|    +--- org.jetbrains.kotlin:kotlin-stdlib:1.9.24 -> 2.0.0 (*)
-|    \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.22 -> 1.9.25
+|    \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 -> 1.9.25
 +--- project :libs:editor
-|    +--- org.jetbrains.kotlin:kotlin-stdlib:1.9.22 -> 2.0.0 (*)
+|    +--- org.jetbrains.kotlin:kotlin-stdlib:1.9.24 -> 2.0.0 (*)
-|    \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.22 -> 1.9.25 (*)
+|    \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 -> 1.9.25 (*)
 +--- org.wordpress:fluxc:{strictly trunk-94d25d35fb4bf58a2e90741a6a5d56b8c6c3ce42} -> trunk-94d25d35fb4bf58a2e90741a6a5d56b8c6c3ce42
-|    \--- net.java.dev.jna:jna:5.5.0
+|    \--- net.java.dev.jna:jna:5.5.0 -> 5.14.0
-\--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.22 -> 1.9.25 (*)
++--- rs.wordpress.api:android:trunk-50f703a7f677084157d02f05d4d477d7eaf960b1
+|    +--- com.squareup.okhttp3:okhttp:4.12.0 (*)
+|    +--- net.java.dev.jna:jna:5.14.0
+|    +--- rs.wordpress.api:kotlin:trunk-50f703a7f677084157d02f05d4d477d7eaf960b1
+|    |    +--- com.squareup.okhttp3:okhttp:4.12.0 (*)
+|    |    +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1 (*)
+|    |    \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.24 -> 2.0.0 (*)
+|    \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.24 -> 2.0.0 (*)
+\--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 -> 1.9.25 (*)

Please review and act accordingly

This will help us understand if it’s working or not
@jkmassel jkmassel modified the milestones: 25.3 ❄️, 25.6 Aug 29, 2024
@jkmassel jkmassel marked this pull request as ready for review August 29, 2024 20:58
@@ -1,6 +1,6 @@
pluginManagement {
gradle.ext.kotlinVersion = '1.9.22'
gradle.ext.kspVersion = '1.9.22-1.0.17'
gradle.ext.kotlinVersion = '1.9.24'
Copy link
Contributor

@nbradbury nbradbury Aug 30, 2024

Choose a reason for hiding this comment

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

Do we really want to update the kotlinVersion and kspVersion in this PR? That seems like something that should be handled separately.

Likewise with androidxComposeCompilerVersion.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was bumped to align with the https://github.com/automattic/wordpress-rs project,t which used very slightly newer versions

Copy link
Contributor

@nbradbury nbradbury left a comment

Choose a reason for hiding this comment

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

This works as expected when I use vanilla.wpmt.co but when I use google.com, instead of seeing "auto discovery is not successful" in logcat, I see "onDiscoveryResponse has error: XMLRPC_BLOCKED - XMLRPC_BLOCKED."

In fact, in this situation LoginActivity.gotXmlRpcEndpoint isn't called. Is this expected?

@jkmassel
Copy link
Contributor

jkmassel commented Sep 3, 2024

Ah, yep - this is expected, sorry - LoginActivity.gotXmlRpcEndpoint is only called if the endpoint is valid – I should've noted that any other output is indicative of a failure.

@jkmassel jkmassel requested a review from nbradbury September 3, 2024 20:25
@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is assigned to the milestone 25.6. This milestone is due in less than 4 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

Copy link

sonarqubecloud bot commented Sep 4, 2024

Copy link
Contributor

@nbradbury nbradbury left a comment

Choose a reason for hiding this comment

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

@jkmassel This is looking good to go! Feel free to merge when ready. :shipit:

@jkmassel jkmassel merged commit 5712e94 into trunk Sep 4, 2024
20 checks passed
@jkmassel jkmassel deleted the add/wordpress-rs-dependency branch September 4, 2024 19:26
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.

5 participants