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.

Sign up for GitHub

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

Jump to bottom

Update App Banner deep links for iOS universal links #26118

Merged
merged 11 commits into from
Jul 19, 2018
Merged

Update App Banner deep links for iOS universal links #26118

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
90a671b
Update App Banner deep links for iOS universal links
frosty Jul 17, 2018
b3a61e4
AppBanner: Extract iOS deep link method and remove currentPathFragment
frosty Jul 18, 2018
0bf0072
Add AppBanner tests for iOS deep links
frosty Jul 18, 2018
8585878
AppBanner: Handle null routes for iOS deep links
frosty Jul 18, 2018
9dc9335
AppBanner: Remove chai dependency in tests
frosty Jul 18, 2018
9660a35
AppBanner: Slight refactor to deep links and improve tests
frosty Jul 18, 2018
5f5eb76
AppBanner: Extract deep link fragment builder function
frosty Jul 18, 2018
60af3bb
AppBanner: Updated tests
frosty Jul 18, 2018
fc21712
AppBanner: Refactoring deep link building
frosty Jul 18, 2018
3ed9cf0
AppBanner: Improves clarify of tests.
frosty Jul 18, 2018
3f7911e
AppBanner: Fixed getCurrentRoute import
frosty Jul 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions client/blocks/app-banner/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
STATS,
getAppBannerData,
getNewDismissTimes,
getCurrentPathFragment,
getCurrentSection,
isDismissed,
APP_BANNER_DISMISS_TIMES_PREFERENCE,
Expand Down Expand Up @@ -115,7 +116,7 @@ class AppBanner extends Component {
};

getDeepLink() {
const { currentSection } = this.props;
const { currentPathFragment, currentSection } = this.props;

if ( this.isAndroid() ) {
//TODO: update when section deep links are available.
Expand All @@ -131,9 +132,21 @@ class AppBanner extends Component {
}
}

//TODO: update when deferred deep links are available
if ( this.isiOS() ) {
return 'itms://itunes.apple.com/us/app/wordpress/id335703880?mt=8';
switch ( currentSection ) {
Copy link
Contributor

@gwwar gwwar Jul 17, 2018 •
edited
Loading

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', () => {
        //...
	} );
//...

Copy link
Contributor Author

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!

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

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?

Copy link
Contributor Author

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.

Copy link
Member

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 😉

Copy link
Contributor Author

@frosty frosty Jul 17, 2018 •
edited
Loading

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

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

return 'https://apps.wordpress.com/get#' + currentPathFragment;
Copy link
Contributor

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 }`;

Copy link
Contributor Author

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.

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

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Member

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

}

return null;
Expand Down Expand Up @@ -201,6 +214,7 @@ const mapStateToProps = state => {
return {
dismissedUntil: getPreference( state, APP_BANNER_DISMISS_TIMES_PREFERENCE ),
currentSection: getCurrentSection( sectionName, isNotesOpen ),
currentPathFragment: getCurrentPathFragment( state ),
Copy link
Contributor

@gwwar gwwar Jul 17, 2018 •
edited
Loading

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.

Copy link
Contributor Author

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.

fetchingPreferences: isFetchingPreferences( state ),
siteId: getSelectedSiteId( state ),
};
Expand Down
9 changes: 9 additions & 0 deletions client/blocks/app-banner/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

import { get, includes, reduce } from 'lodash';

import getCurrentRoute from 'state/selectors/get-current-route';

export const APP_BANNER_DISMISS_TIMES_PREFERENCE = 'appBannerDismissTimes';
export const EDITOR = 'post-editor';
export const NOTES = 'notifications';
Expand Down Expand Up @@ -61,6 +63,13 @@ export function getCurrentSection( currentSection, isNotesOpen ) {
return null;
}

export function getCurrentPathFragment( state ) {
const route = getCurrentRoute( state );

// Strip off the leading forward slash for use as a fragment
Copy link
Member

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?

Copy link
Contributor Author

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 :)

Copy link
Contributor

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.

Copy link
Contributor

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.

return route.substring( 1 );
}

export function getNewDismissTimes( dismissedSection, currentDismissTimes ) {
const currentTime = Date.now();
const aWeekFromNow = currentTime + ONE_WEEK_IN_MILLISECONDS;
Expand Down