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

Support media deep links #19768

Merged
merged 14 commits into from
Dec 13, 2023
Merged

Support media deep links #19768

merged 14 commits into from
Dec 13, 2023

Conversation

zwarm
Copy link
Contributor

@zwarm zwarm commented Dec 11, 2023

This PR adds support for media deeplinks as a part of the Super fast media uploads

  • wordpress.com/media/{domain name}
  • intent://media/#Intent;scheme=jetpack;package=com.jetpack.android

To Test:

Preparing

  • Remove other versions of the app from your device (WP and JP)
  • Download the links zip file, unzip and update the test-media-links.html file to include your domain name.
    test-media-links.html.zip
  • Save the update html file and download to the device you will be using to test this PR
  • Download and install JP app from this PR
  • Login

Testing the deep links

  • Open the downloaded test-media-links.html in a browser on your device
  • Tap on the Intent Media link
  • ✅ Verify the app is launched and the media view is visible for the selected site
  • Tap on the wordpress.com/media link
  • ✅ Verify the app is launched and the media view is visible for the selected site
  • Tap on the wordpress.com/media/nonsense link
  • ✅ Verify the app is launched and the media view is visible for the selected site
  • Tap on the wordpress.com/media/.. for your domain link
  • ✅ Verify the app is launched and the media view is visible for your domain

Regression Notes

  1. Potential unintended areas of impact
    N/A

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual testing

  3. What automated tests I added (or what prevented me from doing so)
    N/A


PR Submission Checklist:

  • I have completed the Regression Notes.
  • 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.

UI Changes Testing Checklist:N/A

@zwarm zwarm requested a review from AjeshRPai December 11, 2023 19:20
@zwarm zwarm self-assigned this Dec 11, 2023
@zwarm zwarm added this to the 24.0 milestone Dec 11, 2023
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 11, 2023

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
Versionpr19768-61fc769
Commit61fc769
Direct Downloadwordpress-prototype-build-pr19768-61fc769.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 11, 2023

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
Versionpr19768-61fc769
Commit61fc769
Direct Downloadjetpack-prototype-build-pr19768-61fc769.apk
Note: Google Login is not supported on these builds.

zwarm and others added 5 commits December 11, 2023 16:00
 ↑ Updates: the logic of navigating to media screen in WPMainActivity
- Removes: the logic of launching media from ActivityLauncher.java
+ Adds: the logic of launching media in ActivityNavigator.kt
↑ Updates: the logic in DeepLinkNavigator to use ActivityNavigator.kt
Copy link
Contributor

@AjeshRPai AjeshRPai left a comment

Choose a reason for hiding this comment

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

Hey @zwarm

I have tested and is working as expected. I made some minor changes. I am approving but not merging so that you can take a look at the changes I have made and revert if necessary.

override fun shouldHandleUrl(uri: UriWrapper): Boolean {
return (uri.host == HOST_WORDPRESS_COM &&
uri.pathSegments.firstOrNull() == MEDIA_PATH) || uri.host == MEDIA_PATH
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a typo in the MEDIA_PATH variable, I fixed it with the commit b0b034c

android:host="wordpress.com"
android:pathPattern="/media/..*"
android:scheme="http" />

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @zwarm
When I was testing the changes, wordpress.com/media deeplink was not working for me. I added a path pattern of android:pathPattern="/media/.*" to fix that. Commit 05189a6. Hope thats ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's weird ... we don't support .* for pages or stats or others.

initSelectedSite();
}
mActivityNavigator.viewCurrentBlogMedia(this, getSelectedSite());
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @zwarm

I updated logic to use ActivityNavigator here. I think we should gradually move all the Activity Navigation logic to ActivityNavigator. Commit a0d27d1

Copy link
Contributor Author

@zwarm zwarm Dec 13, 2023

Choose a reason for hiding this comment

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

Great idea - I forgot ActivityNavigator existed.

@@ -114,5 +117,7 @@ class DeepLinkNavigator
object OpenLoginPrologue : NavigateAction()
data class OpenJetpackForDeepLink(val action: String?, val uri: UriWrapper) : NavigateAction()
object OpenJetpackStaticPosterView : NavigateAction()
data class OpenMediaForSite(val site: SiteModel) : NavigateAction()
object OpenMedia : NavigateAction()
Copy link
Contributor

Choose a reason for hiding this comment

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

Updated the logic to use ActivityNavigator here. Commit - 61fc769

@zwarm zwarm merged commit a88d8c4 into trunk Dec 13, 2023
20 checks passed
@zwarm zwarm deleted the issue/add-media-deeplink branch December 13, 2023 12:57
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.

3 participants