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

Track referrer for INSTANCE_UPLOAD action #6474

Merged
merged 4 commits into from
Nov 4, 2024
Merged

Track referrer for INSTANCE_UPLOAD action #6474

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
62b8cf0
Track referrer for INSTANCE_UPLOAD action
grzesiek2010 Oct 26, 2024
5d606e9
Log the entire referrer URI
grzesiek2010 Oct 30, 2024
d04e75c
Use the label parameter to log referrers
grzesiek2010 Oct 30, 2024
d821fa1
Removed redundant referrer initialization
grzesiek2010 Oct 31, 2024
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
15 changes: 13 additions & 2 deletions ...c/main/java/org/odk/collect/android/instancemanagement/send/InstanceUploaderActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import android.app.ProgressDialog;
import android.content.DialogInterface;
import android.net.Uri;
import android.os.Build;
import android.os.Bundle;

import org.odk.collect.android.activities.FormFillingActivity;
Expand Down Expand Up @@ -177,7 +178,7 @@ private void init(Bundle savedInstanceState) {
instanceUploaderTask = new InstanceUploaderTask();

if (url != null) {
instanceUploaderTask.setCompleteDestinationUrl(url + OpenRosaConstants.SUBMISSION);
instanceUploaderTask.setCompleteDestinationUrl(url + OpenRosaConstants.SUBMISSION, getReferrerHost(), true);

if (deleteInstanceAfterUpload != null) {
instanceUploaderTask.setDeleteInstanceAfterSubmission(deleteInstanceAfterUpload);
Expand Down Expand Up @@ -376,7 +377,7 @@ public void updatedCredentials() {
// TODO: is this really needed here? When would the task not have gotten a server set in
// init already?
if (url != null) {
instanceUploaderTask.setCompleteDestinationUrl(url + OpenRosaConstants.SUBMISSION, false);
instanceUploaderTask.setCompleteDestinationUrl(url + OpenRosaConstants.SUBMISSION, getReferrerHost(), false);
}
instanceUploaderTask.setRepositories(instancesRepository, formsRepository, settingsProvider);
instanceUploaderTask.execute(instancesToSend);
Expand All @@ -386,4 +387,14 @@ public void updatedCredentials() {
public void cancelledUpdatingCredentials() {
finish();
}

private String getReferrerHost() {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP_MR1) {
Uri referrerUri = getReferrer();
if (referrerUri != null) {
return referrerUri.getHost();
Copy link
Member

Choose a reason for hiding this comment

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

Do we definitely just want the host here?

Copy link
Member

Choose a reason for hiding this comment

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

Looked a bit deeper into this and it looks like the referrer extra needs to be explicitly set: https://developer.android.com/reference/android/content/Intent#EXTRA_REFERRER It sounds like it's set automatically when the referrer is a web app but not when it's an Android app. We may get all nulls but I still think we should try it.

Can we just return referrerUri.toString?

Copy link
Member

Choose a reason for hiding this comment

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

Oh good:

If the launching Intent contains an Intent.EXTRA_REFERRER, that will be returned as-is; otherwise, if known, an android-app: referrer URI containing the package name that started the Intent will be returned. This may return null if no referrer can be identified -- it is neither explicitly specified, nor is it known which application package was involved.

-- https://developer.android.com/reference/android/app/Activity#getReferrer()

I thought I was losing my mind for a minute there, I could have sworn I saw more certainty that it would be set.

Still, referrerUri.toString seems like the best thing to return.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I changed it to referrerUri.toString if you prefer it.

It sounds like it's set automatically when the referrer is a web app but not when it's an Android app. We may get all nulls but I still think we should try it.

I tested it with Collect Intents Tester and it didn't return nulls.

}
}
return "unknown";
}
}
10 changes: 4 additions & 6 deletions collect_app/src/main/java/org/odk/collect/android/tasks/InstanceUploaderTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public class InstanceUploaderTask extends AsyncTask<Long, Integer, InstanceUploa
// Custom submission URL, username and password that can be sent via intent extras by external
// applications
private String completeDestinationUrl;
private String referrer = "org.odk.collect.android";
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the back and forth, but I just caught this. Why do we set this to "org.odk.collect.android"? Surely it only gets used if completeDestinationUrl is set (which it's set with) so we can just leave it unassigned.

Just want to make sure we're only logging in cases where the custom URL is being used here and that I'm not missing something.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right it was redundant.

private String customUsername;
private String customPassword;
private InstancesRepository instancesRepository;
Expand Down Expand Up @@ -108,7 +109,7 @@ public Outcome doInBackground(Long... instanceIdsToUpload) {
publishProgress(i + 1, instancesToUpload.size());

if (completeDestinationUrl != null) {
Analytics.log(AnalyticsEvents.INSTANCE_UPLOAD_CUSTOM_SERVER);
Analytics.log(AnalyticsEvents.INSTANCE_UPLOAD_CUSTOM_SERVER, "referrer", referrer);
Copy link
Member

Choose a reason for hiding this comment

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

@lognaturel logging like this limits us to viewing a max of 200 distinct values right? As far as I'm aware there isn't another way to log custom data though (other than a user property which isn't really appropriate).

Also assuming we do want to do an event. It needs to be setup on Firebase as far as I can remember.

Copy link
Member

Choose a reason for hiding this comment

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

The 200 limit is right and I'll be absolutely flabbergasted if there are more than 5-10 uniques!!

It needs to be setup on Firebase as far as I can remember.

Ooh, good call. I don't remember that part at all. Would you be ok doing it when you have some time (no big rush)?

Copy link
Member

Choose a reason for hiding this comment

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

There's a limit on how many custom parameters we have, so I think we should use the existing generic "label" here rather than add "referrer".

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, that's a good call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

}

try {
Expand Down Expand Up @@ -202,12 +203,9 @@ public void setRepositories(InstancesRepository instancesRepository, FormsReposi
this.settingsProvider = settingsProvider;
}

public void setCompleteDestinationUrl(String completeDestinationUrl) {
setCompleteDestinationUrl(completeDestinationUrl, true);
}

public void setCompleteDestinationUrl(String completeDestinationUrl, boolean clearPreviousConfig) {
public void setCompleteDestinationUrl(String completeDestinationUrl, String referrer, boolean clearPreviousConfig) {
this.completeDestinationUrl = completeDestinationUrl;
this.referrer = referrer;
if (clearPreviousConfig) {
setTemporaryCredentials();
}
Expand Down