-
Notifications
You must be signed in to change notification settings - Fork 15
Added deep link support #277
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
Conversation
1. AndroidManifest.xml Added a deep link intent filter to MainActivity to handle https://5calls.org/issue/{issueId} URLs: - Added <intent-filter> with android:autoVerify="true" for Android App Links - Configured to handle HTTPS scheme, 5calls.org host, and /issue/ path prefix 2. MainActivity.java Added deep link handling logic: - New field: mPendingDeepLinkIssueId to store the issue ID from the deep link - handleDeepLink(): Parses incoming intent URIs and extracts the issue ID from URLs like https://5calls.org/issue/1051 - onNewIntent(): Handles deep links when the app is already running (singleTop launch mode) - handlePendingDeepLink(): Searches for the issue in the loaded issues list and either opens it or shows an error - Modified onCreate() to call handleDeepLink() on initial launch - Modified onIssuesReceived() callback to call handlePendingDeepLink() after issues are loaded 3. IssuesAdapter.java - Added getAllIssues() getter method to allow MainActivity to search through all loaded issues 4. strings.xml Added two new error messages: - issue_not_found: "This issue isn't relevant anymore" - issue_inactive: "This issue is no longer active"
|
I tried pulling this down onto my dev device and I tried to open https://5calls.org/issue/snap-contingency-funds-governors/, it did prompt me to open with Chrome or 5 Calls (yay!) but then just brought me to the main activity. I attached the debugger and it looks like the code is comparing the issue ID (something like We should instead compare whatever is after |
|
I was wondering about the permalink. So we only care about matching the permalink portion of the deep link intent to existing issues? Or do we want to support potentially both the permalink slug AND the issueId? |
5calls/app/src/main/java/org/a5calls/android/a5calls/controller/MainActivity.java
Outdated
Show resolved
Hide resolved
5calls/app/src/main/java/org/a5calls/android/a5calls/controller/MainActivity.java
Outdated
Show resolved
Hide resolved
5calls/app/src/main/java/org/a5calls/android/a5calls/controller/MainActivity.java
Outdated
Show resolved
Hide resolved
5calls/app/src/main/java/org/a5calls/android/a5calls/controller/MainActivity.java
Show resolved
Hide resolved
Per Slack, just the permalink / slug, we won't link with issue id. |
to handle the permalink slug instead of the issueId also responded to dektar's comments
|
@dektar switched to permalink handling..responded to your comments. For invalid issues, it felt to me like the Snackbar was shown for a short time, but it's already |
dektar
left a comment
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.
Overall comment:
When I tried going to an issue from a link it now goes to the IssueActivity and the correct page, but the contacts failed to load, so the IssueActivity contacts list said "no calls for this issue". Perhaps we have to wait for both the issues and contacts response to complete before forwarding to that activity?
One other nit below.
5calls/app/src/main/java/org/a5calls/android/a5calls/controller/MainActivity.java
Outdated
Show resolved
Hide resolved
Issues Fixed: When users opened the app via a deep link (e.g., from a notification), the IssueActivity would show "no calls for this issue" because contacts weren't loaded yet. Race condition: Issues and contacts load in parallel, but we were launching IssueActivity as soon as issues loaded, without waiting for contacts Path matching issue: Substring matching could cause false matches between similar slugs (national vs state-specific issues) Path format mismatch: Deep link paths didn't match API permalink format (missing slashes) Contacts not populated: Issue objects retrieved for deep linking bypassed the RecyclerView, so their contacts field was never populated
yep, we had to both wait for the contact response, but also the issue objects we fetch for deep linking bypass the recyclerview loading of the contacts field, so that had to be done manually. Felt a bit hacky and my java ability is a bit out of date, so please take a look |
dektar
left a comment
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.
Tiny nit before submit: please add spanish string in strings_es. I got this from Google Translate, which is probably OK to start with: "No se pudo encontrar esta llamada; puede que ya no sea relevante."
This will call Android CI to fail in general if we don't have string translations.
100% Optional: Add tests for the link extracting and matching logic. This codebase doesn't have good tests or test setup so it's OK if you don't do this.
Otherwise LGTM. I can add the Spanish string in a follow-up if you don't have time this weekend.
dektar
left a comment
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 merge and we can fix the test in a follow-up, or you can fix now, LMK.
| public class DeepLinkParsingTest { | ||
|
|
||
| /** | ||
| * Helper method to extract path from URI, mimicking MainActivity.maybeHandleDeepLink logic |
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.
We should refactor this logic into a visible for testing, static method on main activity rather than mimicing logic here. otherwise changes in main activity won't cause this test to fail, and this test only will test itself!
Added a deep link intent filter to MainActivity to handle https://5calls.org/issue/{issueId} URLs:
Added deep link handling logic:
Added two new error messages:
What type of PR is this? (check all applicable)
Description
Related Issues
Were the changes tested?
have not been included