-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Update App Banner deep links for iOS universal links #26118
Conversation
client/blocks/app-banner/index.jsx
Outdated
case NOTES: | ||
return 'https://apps.wordpress.com/get#notifications'; | ||
case READER: | ||
if ( currentPathFragment.length === 0 ) { |
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.
why are we special-casing the Reader with the fragment length check?
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.
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.
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.
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 😉
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.
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.
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.
A one line comment should be sufficient. We basically want WordPress.com root or /read
in this case?
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.
Essentially – I've added an explanatory comment to the code.
client/blocks/app-banner/index.jsx
Outdated
return 'https://apps.wordpress.com/get#' + currentPathFragment; | ||
case STATS: | ||
return 'https://apps.wordpress.com/get#' + currentPathFragment; | ||
} |
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.
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
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 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.
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 wouldn't block on this @dmsnell :) though refactors are welcome. The simple mapping here is easy enough to read.
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'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
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.
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.
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.
Thanks for raising this. I've used encodeURIComponent
on the fragment now – please let me know if I need to do more than this.
client/blocks/app-banner/utils.js
Outdated
export function getCurrentPathFragment( state ) { | ||
const route = getCurrentRoute( state ); | ||
|
||
// Strip off the leading forward slash for use as a fragment |
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.
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?
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.
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 :)
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.
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.
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.
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.
client/blocks/app-banner/index.jsx
Outdated
return 'https://apps.wordpress.com/get#read'; | ||
} | ||
|
||
return 'https://apps.wordpress.com/get#' + currentPathFragment; |
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.
Optional, but temperate literals are a bit easier to read than string concat.
eg:
return `https://apps.wordpress.com/get#${ currentPathFragment }`;
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.
Thanks! With my outdated JavaScript knowledge, I didn't know this was a thing :) Definitely looks nicer.
client/blocks/app-banner/index.jsx
Outdated
if ( this.isiOS() ) { | ||
return 'itms://itunes.apple.com/us/app/wordpress/id335703880?mt=8'; | ||
switch ( currentSection ) { |
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.
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', () => {
//...
} );
//...
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.
Thanks – I've pulled out a function and added some tests!
client/blocks/app-banner/index.jsx
Outdated
@@ -201,6 +214,7 @@ const mapStateToProps = state => { | |||
return { | |||
dismissedUntil: getPreference( state, APP_BANNER_DISMISS_TIMES_PREFERENCE ), | |||
currentSection: getCurrentSection( sectionName, isNotesOpen ), | |||
currentPathFragment: getCurrentPathFragment( state ), |
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.
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.
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.
Thanks, I've removed this dependency and added some null checking.
Thanks @frosty ! I left a few notes, but looks pretty close, ping me if you need help with anything.
When is app update planned to be released? After this PR gets approval, we'll want to mark when this is safe to land. |
For folks 👀 Here are more detailed testing instructions: Testing instructions
In Desktop Chrome:
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 } ); |
/** | ||
* External dependencies | ||
*/ | ||
import { expect } from 'chai'; |
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.
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' )
becomesexpect( … ).toBe( 'some-string' )
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.
Thanks – I've pushed a fix for this.
@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.
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. |
client/blocks/app-banner/index.jsx
Outdated
break; | ||
} | ||
|
||
return `https://apps.wordpress.com/get#${ encodeURIComponent( fragment ) }`; |
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.
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';
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.
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.
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.
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.
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.
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 ) ) { |
expect( wrapper.instance().getiOSDeepLink( null, STATS ) ).toBe( | ||
'https://apps.wordpress.com/get#%2Fstats' | ||
); | ||
} ); |
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.
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();
} );
client/blocks/app-banner/index.jsx
Outdated
return baseURI; | ||
} | ||
|
||
return `${ baseURI }#${ fragment }`; |
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.
fun fact! we can merge these into a single expression
return ( fragment.length > 0 )
? `${ baseURI }#${ fragment }`
: baseURI;
if you like it that is
client/blocks/app-banner/index.jsx
Outdated
break; | ||
} | ||
|
||
return encodeURIComponent( fragment ); |
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.
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
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.
Nice – I like the second option here (I wanted to avoid duplicating encodeURIComponent
for each return).
@dmsnell Do you know why the canary tests are failing? It doesn't look to me like anything I've done? |
|
||
describe( 'iOS deep link fragments', () => { | ||
test( 'properly encodes tricky fragments', () => { | ||
expect( buildDeepLinkFragment( '?://&#', STATS ) ).toEqual( '%3F%3A%2F%2F%26%23' ); |
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.
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
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.
Thanks! Makes sense!
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.
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
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.
Nice work @frosty! Tests well and LGTM!
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.
https://apps.wordpress.com/get#post
https://apps.wordpress.com/get#notifications
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
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).