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

Expose URI that was deeplinked through a parameter #10

Merged
merged 1 commit into from
Jul 6, 2015

Conversation

MichaelEvans
Copy link
Contributor

No description provided.

@MichaelEvans
Copy link
Contributor Author

Addresses #7

@felipecsl
Copy link
Collaborator

I'd like to see a test around this change to guarantee that the generated intent has this new key in the bundle. Please let me know if you need directions on how to do it

@@ -187,6 +187,7 @@ private void generateDeepLinkActivity() throws IOException {
.endControlFlow()
.addStatement("parameterMap.put(queryParameter, uri.getQueryParameter(queryParameter))")
.endControlFlow()
.addStatement("parameterMap.put(DeepLink.URI, hostPath)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have an intact URI with the scheme, so we should replace hostPath with uri.toString().

@MichaelEvans
Copy link
Contributor Author

@felipecsl Yeah, if you could give me some pointers on how I could test this, I'd gladly write some or it. I wanted to submit one, but I wasn't sure how best to test that.

@felipecsl
Copy link
Collaborator

@MichaelEvans looks like none of the existing test are suitable to cover this test scenario. I'll set up some Robolectric tests that will make this easier. Hang tight!

Update intent tests to check URI param
@MichaelEvans
Copy link
Contributor Author

@felipecsl added some assertions to the tests from #11 to verify the behavior

@cdeonier
Copy link
Collaborator

cdeonier commented Jul 6, 2015

felipecsl added a commit that referenced this pull request Jul 6, 2015
Expose URI that was deeplinked through a parameter
@felipecsl felipecsl merged commit d5552e8 into airbnb:master Jul 6, 2015
@felipecsl
Copy link
Collaborator

yay! thanks @MichaelEvans

@MichaelEvans
Copy link
Contributor Author

woot! thanks!

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

Successfully merging this pull request may close these issues.

3 participants