1071039 - Add support for an ID to Suggested Sites

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

We need each Suggested Site to support an optional ID, used to ping Mozilla's metrics server for tracking partnerships. See bug 1068425.

Bug 1012462 added support for Suggested Sites in distributions. We really need Sites in distributions to have this support. It might as well be added to the core Suggested Site system too, since it's optional.

Comment 1

[Nick Alexander :nalexander [he/him]]

Can we get some details here?

1) how is this ID different than the name of the suggestedsite (fxsupport, etc; see [1])
2) how is this ID determined?  Is it expected to be private?  Private to whom?
3) is the ID intended to be consistent across release channels?

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/locales/en-US/chrome/region.properties#42

Comment 2

[Nick Alexander :nalexander [he/him]]

Assigning to myself since I am about to land changes to the generate_*.py code, so I am very familiar with it right now.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #2)
> Assigning to myself since I am about to land changes to the generate_*.py
> code, so I am very familiar with it right now.

bnicholson: there are a few parts to this.  I'll take the build parts; didn't mean to steal the client implementation.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Nick Alexander :nalexander from comment #1)
> Can we get some details here?
> 
> 1) how is this ID different than the name of the suggestedsite (fxsupport,
> etc; see [1])

The ID is assigned by Content Services to track the partnership

> 2) how is this ID determined?  Is it expected to be private?  Private to
> whom?

Content Services makes it. I don't know that it's "private" but it has no other role but tracking the Site/Tile in metrics.

> 3) is the ID intended to be consistent across release channels?

Yes.

Note: Our built-in Sites/Tiles will not have IDs. Only Sites/Tiles in distributions will have IDs.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Just to clarify:
AFAIK suggested sites created by the build system are not going to be tracked via Content Services, so they do not need an ID associated with them. We don't need to touch anything generated using [1]

Suggested sites that are added to Fennec using a Distribution [2] are able to be tracked by Content Services, so we need to add support for an optional ID in the distribution JSON file [3]. It looks like this calls the same loadSites(...) method anyway [4].

In the end, it seems like the Site class needs the work [5]. The constructor and validation need to handle an optional "ID" entry.

[1] http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/generate_suggestedsites.py
[2] https://wiki.mozilla.org/Mobile/Distribution_Files#Suggested_sites
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/SuggestedSites.java#316
[4] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/SuggestedSites.java#223
[5] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/SuggestedSites.java#92

Comment 6

[Lucas Rocha (:lucasr)]

And don't forget to add tests! ;-)

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Add ID support to suggested sites

A hit a few speed bumps working on this test -- namely, bug 1073052 and bug 1022037. I also took a detour with bug 1073257 to keep API consistent for all suggested sites fields.

After all that, here's the simple patch in all it's glory.

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: Update TestSuggestedSites to check IDs

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8495692 [details] [diff] [review]
Add ID support to suggested sites

Review of attachment 8495692 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but please use "tracking id" terminology consistently everywhere.

::: mobile/android/base/db/SuggestedSites.java
@@ +86,5 @@
>          BrowserContract.SuggestedSites.IMAGEURL,
>          BrowserContract.SuggestedSites.BGCOLOR
>      };
>  
> +    private static final String JSON_KEY_ID = "id";

JSON_KEY_ID -> JSON_KEY_TRACKING_ID
id -> trackingid

@@ +93,5 @@
>      private static final String JSON_KEY_IMAGE_URL = "imageurl";
>      private static final String JSON_KEY_BG_COLOR = "bgcolor";
>  
>      private static class Site {
> +        public final String id;

id -> trackingId

@@ +100,5 @@
>          public final String imageUrl;
>          public final String bgColor;
>  
>          public Site(JSONObject json) throws JSONException {
> +            this.id = json.isNull(JSON_KEY_ID) ? null : json.getString(JSON_KEY_ID);

Given that this is an optional ID with a very specific purpose, I'd prefer to name it 'trackingid' for clarity. Calling it 'id' makes it look like a required general-purpose ID.

@@ +110,5 @@
>              validate();
>          }
>  
> +        public Site(String id, String url, String title, String imageUrl, String bgColor) {
> +            this.id = id;

Ditto.

@@ +143,5 @@
>          public JSONObject toJSON() throws JSONException {
>              final JSONObject json = new JSONObject();
>  
> +            // If id is null, the key is not added.
> +            json.put(JSON_KEY_ID, id);

Interesting, I didn't expect this API to behave like this.

::: mobile/android/base/db/TopSitesCursorWrapper.java
@@ +51,5 @@
>              columnIndexes.put(columnNames[i], i);
>          }
>      }
>  
> +    private static final int INDEX_ID = columnIndexes.get(TopSites.TRACKING_ID);

INDEX_TRACKING_ID for clarity.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8495693 [details] [diff] [review]
Update TestSuggestedSites to check IDs

Review of attachment 8495693 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. Please add a test for the expected behavior when the tracking is not defined.

Comment 11

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/398a0d8a790f
https://hg.mozilla.org/integration/fx-team/rev/b9fad08dc0be

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/398a0d8a790f
https://hg.mozilla.org/mozilla-central/rev/b9fad08dc0be