Update App Banner deep links for iOS universal links #26118

frosty · 2018-07-17T16:49:25Z

Disclaimer: I'm not a web developer, and I think there may be better ways to do this! Please let me know if something is wrong or can be improved.

This PR updates the mobile app banner deep link URLs for iOS.

The app banners now point to https://apps.wordpress.com/get, which will redirect the user to the App Store if they don't have the WordPress for iOS app installed.

If the user does have the app installed, the app will be launched and we'll attempt to navigate them into the appropriate section of the app. To accomplish this, we also append a URL fragment to the base URL for each route.

Editor: https://apps.wordpress.com/get#post
Notifications: https://apps.wordpress.com/get#notifications
Reader: https://apps.wordpress.com/get#read for the root of the Reader. If the user is in a specific section of the Reader, we'll attempt to pass through the entire feed URL so we can load the same one in the app. For example, https://apps.wordpress.com/get#read/feeds/123456/posts/123456
Stats: As with the Reader, we'll attempt to append the current path as the fragment, so we can show the same section in the app. For example, https://apps.wordpress.com/get#stats/discover.wordpress.com

To test

Unfortunately the deep linking can't easily be fully tested unless you have a development build of the iOS app. I may be able to put together a special alpha build if you really want to test this, but I think it's sufficient to verify that the URLs we're constructing are correct (I've already tested the launching of the app myself with a development build).

If you don't have the WordPress app for iOS installed, or you don't have a development version, tapping the Open in App button on each of the 4 app banners should launch the App Store (via apps.wordpress.com). The App Store should show the WordPress app listing.
Please verify that the URLs for each of the Open in App buttons match the schemas described above.

matticbot · 2018-07-17T16:49:27Z

Test live: https://calypso.live/?branch=update/app-banner-ios-urls

dmsnell · 2018-07-17T16:57:05Z

client/blocks/app-banner/index.jsx

+				case NOTES:
+					return 'https://apps.wordpress.com/get#notifications';
+				case READER:
+					if ( currentPathFragment.length === 0 ) {


why are we special-casing the Reader with the fragment length check?

The Reader doesn't have its own path when you view it on the web – so the root of the Reader just sits at WordPress.com. Each of the other routes have their own distinct path – /stats, /notifications, /post. The Reader can also be accessed at /read, but this handles the case where you're just at the root and there's no path, and ensures the app knows which section to take the user to.

hm. it's still a bit unclear how this is supposed to function. if Reader is a special exception then I think it begs for a strong explanatory comment otherwise someone will come here and trash the code at some point in the future unless it's obvious why it's there 😉

Definitely adding a comment makes sense. I'll try and explain here a bit more clearly too with a couple of examples:

If you visit the editor in a mobile web browser, the URL is wordpress.com/post, and you'll see the editor app banner. If the app is launched from here, we handle the path /post and display the editor.

If you visit Discover in the Reader in a mobile web browser, the URL is wordpress.com/discover and you'll see the Reader app banner. If the app is launched from here, we handle the path /discover, and display the Discover feed in the app's Reader.

If you visit the Reader (tap the Reader icon in the top bar) in a mobile web browser, the URL is just wordpress.com, and you'll see the Reader app banner. If the app is launched from here, without a special case, we'd just get the path /, and have no way to differentiate it from the logged out WordPress.com URL (which we don't want to deep link into the app). Hence, a special case to interpret this as /read in the app.

A one line comment should be sufficient. We basically want WordPress.com root or /read in this case?

Essentially – I've added an explanatory comment to the code.

dmsnell · 2018-07-17T17:02:14Z

client/blocks/app-banner/index.jsx

+					return 'https://apps.wordpress.com/get#' + currentPathFragment;
+				case STATS:
+					return 'https://apps.wordpress.com/get#' + currentPathFragment;
+			}


a more stable way of dealing with the fragment could be to encode the actual URL in its own transport and let the app disambiguate. I think at least we need some clarification in an explanatory comment why we're passing the fragment and path like this.

for instance, imagine a fully-controlled URL…

const routing = encodeURIComponent( JSON.stringify( { section: 'post', previousFragment: currentPathFragment, } ); return `https://apps.wordpress.com/get?routing=${ routing }`

and we can make a helper for it…

const urlFor = section => `https://apps.wordpress.com/get?${ encodeURIComponent( JSON.stringify( { section, previousFragment: currentPathFragment, } ) ) }`; case EDITOR: return urlFor( 'post' ); case NOTES: return urlFor( 'notifications' ); case READER: return urlFor( 'read' );

something like that

I can certainly test that. The main benefit of passing the path in the way I am currently is that on the app side of things I can just pass the path directly to our routing mechanism for deep linked URLs coming from WordPress.com. It wouldn't be too much extra work to parse the necessary parts out of this type of URL though.

I wouldn't block on this @dmsnell :) though refactors are welcome. The simple mapping here is easy enough to read.

I'm mainly concerned about bugs due to encoding issues in the URL

we're assuming a lot if we push out the currentPathFragment directly into the URL and especially if we're using a fragment as well 🤷‍♂️

just want to make sure that we at least recognize the places this will tend to break and determine how to or how much to mitigate them

Good catch @dmsnell! I totally forgot about that.

The spec is a bit hard to interpret https://tools.ietf.org/html/rfc3986#section-3.5, but you're right that we need to escape.

https://stackoverflow.com/a/2849834

So, combined, the fragment cannot contain #, a raw %, ^, [, ], {, }, , ", < and > according to the RFC.

Thanks for raising this. I've used encodeURIComponent on the fragment now – please let me know if I need to do more than this.

dmsnell · 2018-07-17T17:03:50Z

client/blocks/app-banner/utils.js

+export function getCurrentPathFragment( state ) {
+	const route = getCurrentRoute( state );
+
+	// Strip off the leading forward slash for use as a fragment


an example of the input data could make this comment much more useful to an unfamiliar reader - what does getCurrentRoute() return?

// strip off the leading forward-slash for use as a fragment // e.g. /settings/mysite.wordpress.com -> settings/mysite.wordpress.com

also, why are we stripping the slash?

Definitely agree with the clearer example. If I'm completely honest, there's reason for stripping the leading slash other than I thought it looked nicer. I understand that's not a great reason, and perhaps it'd be best to just leave it in there :)

Seems like a bit of overkill to add this trivial util function before we have a second instance, it's easy enough to strip a leading slash in the deep link function as well.

This is technically a selector too, which would normally live in state/selectors I don't think we need this though. It's enough to modify in mapStateToProps or in the deep link function directly.

gwwar · 2018-07-17T17:18:31Z

client/blocks/app-banner/index.jsx

+						return 'https://apps.wordpress.com/get#read';
+					}
+
+					return 'https://apps.wordpress.com/get#' + currentPathFragment;


Optional, but temperate literals are a bit easier to read than string concat.

eg:

return `https://apps.wordpress.com/get#${ currentPathFragment }`;

Thanks! With my outdated JavaScript knowledge, I didn't know this was a thing :) Definitely looks nicer.

gwwar · 2018-07-17T18:07:48Z

client/blocks/app-banner/index.jsx

 		if ( this.isiOS() ) {
-			return 'itms://itunes.apple.com/us/app/wordpress/id335703880?mt=8';
+			switch ( currentSection ) {


If we want to verify that we return the correct app links, consider adding a unit test. It's fine to pull this switch out into it's own function (makes it easier to test).

//... if ( this.isiOS() ) { return getIOSDeepLinks( currentSection ); } //... } function getIOSDeepLinks( currentSection ) { switch ( currentSection ) { //... }

And in the test file:

//... test( 'iOS Deep links return correct URIs for READER', () => { expect( getIOSDeepLinks( '/' ) ).toEqual( 'https://apps.wordpress.com/get#read' ); expect( getIOSDeepLinks( '/read/feeds/12345/posts/6789' ) ).toEqual( 'https://apps.wordpress.com/get#read/feeds/12345/posts/6789' ); } ); test( 'iOS Deep links return correct URIs for STATS', () => { //... } ); //...

Thanks – I've pulled out a function and added some tests!

gwwar · 2018-07-17T18:15:35Z

client/blocks/app-banner/index.jsx

@@ -201,6 +214,7 @@ const mapStateToProps = state => {
 	return {
 		dismissedUntil: getPreference( state, APP_BANNER_DISMISS_TIMES_PREFERENCE ),
 		currentSection: getCurrentSection( sectionName, isNotesOpen ),
+		currentPathFragment: getCurrentPathFragment( state ),


Consider passing through getCurrentRoute( state ) instead, and letting the getDeepLink function(s) strip out the leading slash. It's much easier to read with that context, instead of needing to click through and see what a current path fragment is.

Note that the current route may be null, so be careful with string operations.

Thanks, I've removed this dependency and added some null checking.

gwwar · 2018-07-17T18:20:06Z

Thanks @frosty ! I left a few notes, but looks pretty close, ping me if you need help with anything.

Unfortunately the deep linking can't easily be fully tested unless you have a development build of the iOS app.

When is app update planned to be released? After this PR gets approval, we'll want to mark when this is safe to land.

gwwar · 2018-07-17T18:24:09Z

For folks 👀 Here are more detailed testing instructions:

Testing instructions

Use your physical Mobile device using this calypso.live branch: https://calypso.live/?branch=update/app-banner-ios-urls
Visit the reader, stats, editor, and notifications
Banners should be visible and behave as expected.
Open notifications again. Click on any part of the banner should not toggle notifications. Open in app and dismiss banner should work as expected.

In Desktop Chrome:

This banner should not be visible on any page in non-mobile browsers.
In order to test it, you can open up DevTools in Chrome and use Toggle device toolbar.
Verify that app banner is shown on stats, reader, and editor pages. It should also be shown on any page when notification panel is open.
Verify that correct text and icons are shown for each page as described in App Prompt: Smart Banner #15228.
Verify that impression tracks event has been recorded upon banner rendering.
Verify that Open in app redirects to app store and that appropriate tracks event/stat bump is recorded. (will be broken since the developer build won't be out).
Verify that No thanks closes the banner and that appropriate tracks event/stat bump is recorded.

Note: If you've dismissed the widget and want to bring it back for testing in current session, you can run the following line in your console:

dispatch( { type: 'PREFERENCES_SET', key: 'appBannerDismissTimes', value: null } );

gwwar · 2018-07-17T18:30:51Z

Some lights smoke testing in emulation mode looks good 👍

sirreal · 2018-07-18T10:42:31Z

client/blocks/app-banner/test/app-banner.jsx

+/**
+ * External dependencies
+ */
+import { expect } from 'chai';


Let's avoid introducing new usage of chai.

Remove this import line.

Jest will provide the global expect –no additional action required

expect( … ).equal( 'some-string' ) becomes expect( … ).toBe( 'some-string' )

Thanks – I've pushed a fix for this.

frosty · 2018-07-18T10:50:42Z

@dmsnell @gwwar I've pushed some updates, and I think I've addressed your comments – but please let me know if there's anything else.

When is app update planned to be released? After this PR gets approval, we'll want to mark when this is safe to land.

So I expect this to go into version 10.6 of WordPress for iOS, which will go into testing on July 30th. However I think it would be okay to land this as soon as it's ready, as the links should still just redirect to the App Store before that app update is available and installed. Once the app enters testing, we'd have a 2 week period to verify / fix anything that may crop up. But I'm happy to delay it if you'd prefer though.

dmsnell · 2018-07-18T12:49:43Z

client/blocks/app-banner/index.jsx

+				break;
+		}
+
+		return `https://apps.wordpress.com/get#${ encodeURIComponent( fragment ) }`;


note that we've exposed a case where we are using an unset variable - if currentSection isn't one of these four things then we'll be trying to encode undefined. maybe something like this would help

default: return 'https://apps.wordpress.com/get';

Thanks for catching that – I'm still in Swift mode where switches must be exhaustive and you'll get an error if they're not.

ahhh. I love languages that don't eschew the kind of automation and bug-detection that computers were designed to provide.

Javascript exists because programming was too easy and people wanted to give programmers the thrill of adrenaline and emergency once again.

With current code the default case will never be reached anyway, since AppBanner will bail early in render if one of those sections is not detected, see:

wp-calypso/client/blocks/app-banner/index.jsx

Line 149 in 82f7c5f

if ( ! includes( ALLOWED_SECTIONS, currentSection ) ) {

Still, that's not immediately obvious and it doesn't hurt to add safeguards here too. 👍

dmsnell · 2018-07-18T12:53:11Z

client/blocks/app-banner/test/app-banner.jsx

+		expect( wrapper.instance().getiOSDeepLink( null, STATS ) ).toBe(
+			'https://apps.wordpress.com/get#%2Fstats'
+		);
+	} );


Thanks for writing these tests. Don't let this be a blocker for the PR but I don't particularly find them that useful - they are mainly just making sure that somebody didn't change the constants defined in the code.

A more valuable test might focus more on the behaviors - probably a good point to question the tests if the descriptions are like "should do the right thing"

So what are the behaviors here we want to test? What could unexpectedly go wrong?

test( 'encodes tricky fragment characters', () => { expect( wrapper.instance().getiOSDeepLink( '?://&#', STATS ).split( '#' )[ 0 ] ).toEqual( '%3F%3A%2F%2F%26%23' ); } );

also, I know there was some discussion about methods vs. functions and I don't want to confuse you, but as you rightly questioned, testing is harder as methods. there's no dependence on the instance here so let's move it out of the class and make it a function in the module scope that the class calls. the tests become more straight-forward

import { getiOSDeepLink } from 'blocks/app-banner'; test( 'properly encodes tricky fragments', () => { expect( getiOSDeepLink( '?://&#', STATS ).split( '#' )[ 1 ] ).toEqual( '%3F%3A%2F%2F%26%23' ); } ); test( 'returns expected link when given unexpected section', () => { expect( getiOSDeepLink( 'test', 'acorn' ).includes( '#' ) ).toBeFalsy(); } );

as a side-note I've tried to narrow the scope of these tests so that we're only testing one thing. for instance, I don't need to test the base URL - it's a hard-coded constant, so my tests ignore that part. if we want to verify that someone doesn't change code we have a couple options:

setup code ownership in Github

use a snapshot which will accomplish the same purpose without forcing our tests to be super specific

test( "base URL isn't accidentally changed", () => { expect( getiOSDeepLink( 'test', STATS ) ).toMatchSnapshot(); } );

dmsnell · 2018-07-18T15:06:19Z

client/blocks/app-banner/index.jsx

+		return baseURI;
+	}
+
+	return `${ baseURI }#${ fragment }`;


fun fact! we can merge these into a single expression

return ( fragment.length > 0 ) ? `${ baseURI }#${ fragment }` : baseURI;

if you like it that is

dmsnell · 2018-07-18T15:17:13Z

client/blocks/app-banner/index.jsx

+			break;
+	}
+
+	return encodeURIComponent( fragment );


just in case you are interested there are two ways we can remove the mutation here

we can return directly…

const hasRoute = … switch ( currentSection ) { case EDITOR: return encodeURIComponent( '/post' ); case NOTES: return encodeURIComponent( '/notifications' ); … default: return ''; }

we can also separate and compose the functionality in various ways. since this is pretty specific behavior and the function doesn't do much else, I'm going to create a function inside of it. could legitimately create another separate and testable function

export function buildDeepLinkFragment( currentRoute, currentSection ) { const hasRoute = …; const getFragment = () => { switch ( currentSection ) { case EDITOR: return '/post'; case NOTES: return '/notifications'; case READER: return hasRoute ? currentRoute : '/read'; case STATS: return hasRoute ? currentRoute : '/stats'; default: return ''; } } return encodeURIComponent( getFragment() ); }

this is mainly a style thing, just for inspiration

Nice – I like the second option here (I wanted to avoid duplicating encodeURIComponent for each return).

frosty · 2018-07-18T15:19:06Z

@dmsnell Do you know why the canary tests are failing? It doesn't look to me like anything I've done?

dmsnell · 2018-07-18T15:22:03Z

client/blocks/app-banner/test/app-banner.jsx

+
+describe( 'iOS deep link fragments', () => {
+	test( 'properly encodes tricky fragments', () => {
+		expect( buildDeepLinkFragment( '?://&#', STATS ) ).toEqual( '%3F%3A%2F%2F%26%23' );


even though I suggested writing this code, I wonder how much this ends up conflating the tests here. we might consider using this instead…

const funnyFragment = '?://&#'; expect( buildDeepLinkFragment( funnyFragment, STATS ) ).toEqual( encodeURIComponent( funnyFragment ) );

that way the test isn't dependent on the implementation of encodeURIComponent() (not that it will change any time soon) and it also indicates in the test code the intended behavior more semantically - we get the expected intention instead of the expected outcome

Thanks! Makes sense!

dmsnell

thanks for all the updates @frosty - the code looks great to me.

for your own reference there's a subtle difference between using export const foo = ( … ) => { … } and export function foo( … ) { … }

in some cases this is a syntax help for small functions, like in a map

[ 1, 2, 3 ].map( a => a * 2 )

so it can lead to clearer syntax

more deeply it changes the way the Javascript assigns the value of this - function is dynamically scoped while the arrow functions are lexically scoped and so my personal preference is to use arrow-functions by default since they don't carry with them any surprising behaviors.

if there's no this in the function it won't matter at all, but look at how the difference exposes itself in the following code

const name = 'bob';

function Person() {
  this.name = 'sue';
  
  this.getName = function() {
    return this.name;
  }
}

const person = new Person();
l( person.getName() ); // returns 'sue'

const getName = person.getName;
l( getName()  ); // returns 'bob'

let's make one change

const name = 'bob';

function Person() {
  this.name = 'sue';
  
  this.getName =() => {
    return this.name;
  }
}

const person = new Person();
l( person.getName() ); // returns 'sue'

const getName = person.getName;
l( getName()  ); // returns 'sue'

so you can see the effect of the dynamic scoping because when we call getName() as a method on person then Javascript sets this to be person itself. when we call it as a function this is implicitly the global or window value, which grabs the outer name

as an arrow function this will always point to the same thing regardless of how it's called.

therefore I use it as a baseline safety practice even though I'm personally aware of the ambiguity surrounding dynamic this assignment, even though I write functions without this - I do it as a habit to remove complexity

on the other hand it brings a drawback noticeable in modules…

function is parsed on the first pass of the JS engine. function definitions are thus scope-hoisted and can be called in lines above where they are declared/defined. const foo = () => {} is a normal variable definition and must appear above the call-site. this can be resolved by using var foo = () => {} since var is hoisted as well, but we avoid var for similar confusing semantics

Copons · 2018-07-19T09:32:22Z

Poor @frosty, he came in for a quick fix, and instead got a Full Fledged @dmsnell Lesson™ on this in JS. 😢

vindl

Nice work @frosty! Tests well and LGTM!

Update App Banner deep links for iOS universal links

90a671b

frosty added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 17, 2018

frosty requested a review from gwwar July 17, 2018 16:49

dmsnell reviewed Jul 17, 2018

View reviewed changes

gwwar requested a review from a team July 17, 2018 17:03

dmsnell reviewed Jul 17, 2018

View reviewed changes

gwwar reviewed Jul 17, 2018

View reviewed changes

frosty added 2 commits July 18, 2018 11:33

AppBanner: Extract iOS deep link method and remove currentPathFragment

b3a61e4

Add AppBanner tests for iOS deep links

0bf0072

sirreal reviewed Jul 18, 2018

View reviewed changes

AppBanner: Handle null routes for iOS deep links

8585878

AppBanner: Remove chai dependency in tests

9dc9335

dmsnell reviewed Jul 18, 2018

View reviewed changes

frosty added 3 commits July 18, 2018 15:37

AppBanner: Slight refactor to deep links and improve tests

9660a35

AppBanner: Extract deep link fragment builder function

5f5eb76

AppBanner: Updated tests

60af3bb

dmsnell reviewed Jul 18, 2018

View reviewed changes

AppBanner: Refactoring deep link building

fc21712

dmsnell reviewed Jul 18, 2018

View reviewed changes

AppBanner: Improves clarify of tests.

3ed9cf0

dmsnell approved these changes Jul 18, 2018

View reviewed changes

AppBanner: Fixed getCurrentRoute import

3f7911e

vindl approved these changes Jul 19, 2018

View reviewed changes

vindl added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 19, 2018

frosty merged commit bb0f1a9 into master Jul 19, 2018

matticbot removed the [Status] Ready to Merge label Jul 19, 2018

frosty deleted the update/app-banner-ios-urls branch July 19, 2018 21:15

SiobhyB mentioned this pull request Aug 30, 2022

[Jetpack App Migration] Update App Banner Branding #67013

Merged

5 tasks

Update App Banner deep links for iOS universal links #26118

Update App Banner deep links for iOS universal links #26118

Conversation

frosty commented Jul 17, 2018

To test

matticbot commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frosty Jul 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwwar Jul 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwwar Jul 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwwar commented Jul 17, 2018

gwwar commented Jul 17, 2018 • edited

Testing instructions

gwwar commented Jul 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frosty commented Jul 18, 2018

dmsnell Jul 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Jul 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frosty commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell left a comment

Choose a reason for hiding this comment

Copons commented Jul 19, 2018

vindl left a comment

Choose a reason for hiding this comment

frosty Jul 17, 2018 •

edited

gwwar Jul 17, 2018 •

edited

gwwar Jul 17, 2018 •

edited

gwwar commented Jul 17, 2018 •

edited

gwwar commented Jul 17, 2018 •

edited

dmsnell Jul 18, 2018 •

edited

dmsnell Jul 18, 2018 •

edited