Track referrer for INSTANCE_UPLOAD action #6474

grzesiek2010 · 2024-10-26T16:07:51Z

Why is this the best possible solution? Were any other approaches considered?

getReferrer was added in API 22 so it won't work for API 21 which is the lowest version we support but I don't think this is a problem.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

It doesn't require testing.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

added or modified tests for any new or changed behavior
run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
added a comment above any new strings describing it for translators
added any new strings with date formatting to DateFormatsTest
verified that any code or assets from external sources are properly credited in comments and/or in the about file.
verified that any new UI elements use theme colors. UI Components Style guidelines

seadowg · 2024-10-29T17:28:06Z

.../src/main/java/org/odk/collect/android/instancemanagement/send/InstanceUploaderActivity.java

+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP_MR1) {
+            Uri referrerUri = getReferrer();
+            if (referrerUri != null) {
+                return referrerUri.getHost();


Do we definitely just want the host here?

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?

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.

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.

seadowg · 2024-10-30T16:38:54Z

collect_app/src/main/java/org/odk/collect/android/tasks/InstanceUploaderTask.java

@@ -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);


@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.

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

lognaturel · 2024-10-30T17:26:18Z

Looks great to me! 🙏

I disagree with myself

seadowg · 2024-10-30T18:05:38Z

collect_app/src/main/java/org/odk/collect/android/tasks/InstanceUploaderTask.java

@@ -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);


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".

Ahh, that's a good call.

seadowg · 2024-10-31T08:13:18Z

collect_app/src/main/java/org/odk/collect/android/tasks/InstanceUploaderTask.java

@@ -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";


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.

You are right it was redundant.

grzesiek2010 · 2024-10-31T08:36:02Z

collect_app/src/main/java/org/odk/collect/android/tasks/InstanceUploaderTask.java

@@ -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, "label", referrer != null ? referrer : "");


referrer shouldn't be null here but I added that check just in case to avoid errors as Analytics.log expects non-null strings.

Track referrer for INSTANCE_UPLOAD action

62b8cf0

grzesiek2010 requested a review from seadowg October 26, 2024 17:48

seadowg reviewed Oct 29, 2024

View reviewed changes

Log the entire referrer URI

5d606e9

grzesiek2010 force-pushed the COLLECT-6472 branch from c68977d to 5d606e9 Compare October 30, 2024 15:31

grzesiek2010 requested review from seadowg and lognaturel October 30, 2024 15:34

seadowg reviewed Oct 30, 2024

View reviewed changes

seadowg previously approved these changes Oct 30, 2024

View reviewed changes

seadowg removed the request for review from lognaturel October 30, 2024 18:03

seadowg requested changes Oct 30, 2024

View reviewed changes

Use the label parameter to log referrers

d04e75c

grzesiek2010 force-pushed the COLLECT-6472 branch from e581a6c to d04e75c Compare October 30, 2024 19:45

grzesiek2010 requested review from seadowg and lognaturel October 30, 2024 19:45

seadowg requested changes Oct 31, 2024

View reviewed changes

Removed redundant referrer initialization

d821fa1

grzesiek2010 force-pushed the COLLECT-6472 branch from 741f517 to d821fa1 Compare October 31, 2024 08:34

grzesiek2010 commented Oct 31, 2024

View reviewed changes

grzesiek2010 requested a review from seadowg October 31, 2024 08:36

seadowg approved these changes Nov 4, 2024

View reviewed changes

seadowg merged commit 700594f into getodk:master Nov 4, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track referrer for INSTANCE_UPLOAD action #6474

Track referrer for INSTANCE_UPLOAD action #6474

grzesiek2010 commented Oct 26, 2024 •

edited

Loading

seadowg Oct 29, 2024

lognaturel Oct 29, 2024

lognaturel Oct 29, 2024

grzesiek2010 Oct 30, 2024

seadowg Oct 30, 2024

lognaturel Oct 30, 2024

lognaturel commented Oct 30, 2024

seadowg Oct 30, 2024

lognaturel Oct 30, 2024

grzesiek2010 Oct 30, 2024

seadowg Oct 31, 2024

grzesiek2010 Oct 31, 2024

grzesiek2010 Oct 31, 2024

Track referrer for INSTANCE_UPLOAD action #6474

Track referrer for INSTANCE_UPLOAD action #6474

Conversation

grzesiek2010 commented Oct 26, 2024 • edited Loading

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lognaturel commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzesiek2010 commented Oct 26, 2024 •

edited

Loading