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

Untangle http:// and wordpress:// deeplinking URLs #11550

Merged
merged 3 commits into from Mar 30, 2020

Conversation

maxme
Copy link
Contributor

@maxme maxme commented Mar 27, 2020

Fixes an issue that made wordpress:// URLs non-functional.

This was probably introduced in: #9957

Note that I'm not sure why the issue is happening, according to the documentation this should work fine, and other urls like: http://viewpost should also work as well (which is an unwanted side effect).

Test

Before the patch, run:

adb shell am start \
 -W -a android.intent.action.VIEW \
 -c android.intent.category.BROWSABLE \
 -d "wordpress://viewpost?blogId=124661775\&postId=1194"

The app won't open.

After the patch, run the same command:

adb shell am start \
 -W -a android.intent.action.VIEW \
 -c android.intent.category.BROWSABLE \
 -d "wordpress://viewpost?blogId=124661775\&postId=1194"

The app will open and show the specified post in the reader.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • 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.

@maxme maxme added the Core label Mar 27, 2020
@maxme maxme added this to the 14.6 milestone Mar 27, 2020
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Mar 27, 2020

You can test the changes on this Pull Request by downloading the APK here.

@hypest
Copy link
Contributor

hypest commented Mar 30, 2020

Test results:

Testing the wordpress custom scheme

wordpress://viewpost?blogId=124661775\&postId=1194
wordpress://stats
wordpress://notifications
wordpress://read
wordpress://post

Testing a normal link to a post

✅ Opens in the app in read-specific-post mode https://stefanosuser.wordpress.com/2020/02/20/comments-issue/

Tests from #9957

✅ Should not open the app: https://wordpress.com/post/
✅ Should not open the app: https://public-api.wordpress.com/nope/
✅ Should not open the app: https://public-api.wordpress.com/bar/?redirect_to=https%3A%2F%2FCHANGEME.wordpress.com%2Fpost
✅ Should open the app's editor on previously selected site: https://wordpress.com/post/wrong-hostname
✅ Should open the app's editor with selected site: https://wordpress.com/post/stefanosuser.wordpress.com
✅ Should open the app's editor with selected site: https://public-api.wordpress.com/mbar/?redirect_to=https%3A%2F%2Fwordpress.com%2Fpost%2Fstefanosuser.wordpress.com
✅ Should "open" the app, but then redirect to the browser: https://public-api.wordpress.com/mbar/?redirect_to=https%3A%2F%2Fwordpress.com%2Fnope

Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix @maxme ! LGTM!

@hypest
Copy link
Contributor

hypest commented Mar 30, 2020

Feel free to merge @maxme after updating from develop to resolve the merge conflict 👍

Edit: merged it after seeing the update happened.

@hypest hypest merged commit 63c6399 into develop Mar 30, 2020
@hypest hypest deleted the issue/untangle-deeplinking-urls branch March 30, 2020 13:37
@adityabhaskar
Copy link
Contributor

@maxme Tangentially related: Is it possible to send an intent to open a post for editing in the WordPress app? Something on the lines of:
wordpress://post?blogId=[site_id]&postId=[post_id] or https://wordpress.com/post/[blog_id]/[post_id].

At the moment they both just open the post editor for a blank new post in the currently selected site.

The reference code starts here:

@maxme
Copy link
Contributor Author

maxme commented May 12, 2020

@adityabhaskar That's a good idea and this would be possible if we changed that code a bit to capture the blog id and then open the editor for this specific site (similar to what is done with the "Share with" feature).

@adityabhaskar
Copy link
Contributor

@maxme Should I create a new issue with this and tag you in it?

@maxme
Copy link
Contributor Author

maxme commented May 13, 2020

Yes please!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants