1155579 - Snippets - Allow multiple countries per snippet

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Giorgos Logiotatidis [:giorgos]]

At the moment Snippets can be optionally targeted to a country. In this case snippets service includes a 'target_geo' key like this: 

{
  'id': 1,
  'text': 'foo bar',
  'icon': '..',
  'url': 'https://...',
  'weight': 100,
  'target_geo': 'GR'
}

and Fennec geolocates the user and takes care of the rest.

We want to change Fennec to allow a single snippet to target multiple countries. Snippet service will return a list like this:

{
  'id': 1,
  'text': 'foo bar',
  'icon': '..',
  'url': 'https://...',
  'weight': 100,
  'target_geo': 'GR'
  'countries': ['GR', 'US']
}

To avoid changing the URI of the endpoint I suggest that we add the key 'countries' along with key 'target_geo'. We will remove the later 6 months after deployment.

Comment 1

[:Margaret Leibovic]

To add support for this, we should update the message filtering logic in `updateBanner` in Snippets.js:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/Snippets.js#197

Comment 2

[Giorgos Logiotatidis [:giorgos]]

Attachment: 1155579-fennec.patch

Comment 3

[:Margaret Leibovic]

Comment on attachment 8596657 [details] [diff] [review]
1155579-fennec.patch

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

Thanks for the patch! Were you able to build and test this?

::: mobile/android/components/Snippets.js
@@ +196,5 @@
>      // Don't add this message to the banner if it's not supposed to be shown in this country.
> +    if ("countries" in message && message.countries.indexOf(gCountryCode) === -1) {
> +      return;
> +    }
> +    else if ("target_geo" in message && message.target_geo != gCountryCode) {

If you want, you could combine these two if statements with an || to have a single return statement.

Comment 4

[Giorgos Logiotatidis [:giorgos]]

(In reply to :Margaret Leibovic from comment #3)
> Thanks for the patch! Were you able to build and test this?

Thanks for approving! I managed to build fennec but I'm having trouble testing this:
 - launch fennec
 - go to about:config
 - change snippet url to my local server
 - reset last lastUpdateTime
 
and lastUpdateTime gets populated again but there are no hits to my local server. Am I missing something?

Also the about:config page auto refreshes every second which makes it really annoying to edit or even view the values. Is there any way to disable that?

Comment 5

[:Margaret Leibovic]

(In reply to Giorgos Logiotatidis [:giorgos] from comment #4)

Sorry for the slow response!

> (In reply to :Margaret Leibovic from comment #3)
> > Thanks for the patch! Were you able to build and test this?
> 
> Thanks for approving! I managed to build fennec but I'm having trouble
> testing this:
>  - launch fennec
>  - go to about:config
>  - change snippet url to my local server
>  - reset last lastUpdateTime
>  
> and lastUpdateTime gets populated again but there are no hits to my local
> server. Am I missing something?

We use an update timer to only ever trigger the snippets code once per 24 hours:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/MobileComponents.manifest#108
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/Snippets.js#429

You should try setting the "browser.snippets.updateInterval" pref to some small value.

> Also the about:config page auto refreshes every second which makes it really
> annoying to edit or even view the values. Is there any way to disable that?

Yeah, I thinks bug 1154646 should have fixed that.

However, if you're making your own build, you can also edit /mobile/android/app/mobile.js to directly set prefs at build time.

It also sucks that we have no unit tests for this... I should just try to make something simple for bug 971107.

Comment 6

[Giorgos Logiotatidis [:giorgos]]

I still get no hits to my local server. Digging deeper I found out that browser.snippets.countryCode is never set either. I tried with the latest Firefox on f-droid same results.

Steps to reproduce:
 - Download firefox from f-droid (also tried with my build and with firefox beta)
 - Open the app
 - Go to about:config
 - Wait a moment, 'app.update.lastUpdateTime.snippets-update-timer' pref appears initial value 0
 - After a while 'app.update.lastUpdateTime.snippets-update-timer' gets a another value, e.g. 1430834774
 - browser.snippets.countryCode and browser.snippets.geoLastUpdate do not exist and my local snippets server shows nothing in the logs.
 - I changed browser.snippets.updateInterval to 60 and restarted Firefox but nothing changed 

Margaret can you please confirm this behavior?

Comment 7

[:Margaret Leibovic]

(In reply to Giorgos Logiotatidis [:giorgos] from comment #6)
> I still get no hits to my local server. Digging deeper I found out that
> browser.snippets.countryCode is never set either. I tried with the latest
> Firefox on f-droid same results.
> 
> Steps to reproduce:
>  - Download firefox from f-droid (also tried with my build and with firefox
> beta)
>  - Open the app
>  - Go to about:config
>  - Wait a moment, 'app.update.lastUpdateTime.snippets-update-timer' pref
> appears initial value 0
>  - After a while 'app.update.lastUpdateTime.snippets-update-timer' gets a
> another value, e.g. 1430834774
>  - browser.snippets.countryCode and browser.snippets.geoLastUpdate do not
> exist and my local snippets server shows nothing in the logs.
>  - I changed browser.snippets.updateInterval to 60 and restarted Firefox but
> nothing changed 
> 
> Margaret can you please confirm this behavior?

I'm not seeing this testing on a release build of Firefox. When I first opened about:config, my country code was set to CA (I've traveled to Canada recently), but after setting browser.snippets.updateInterval to 60 I saw it change to the US.

Are you seeing anything in adb logcat?

You can also use WebIDE to attach a debugger to Snippets.js to step through what's going on:
https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging/Debugging_Firefox_for_Android_with_WebIDE

Do we have data about snippets usage? I assume that we would see a drop in metrics if there is some bug in the way snippets are updated/displayed.

Comment 8

[Giorgos Logiotatidis [:giorgos]]

Attachment: 1155579-fennec.patch

Comment 9

[Giorgos Logiotatidis [:giorgos]]

I managed to debug the problem using the awesome remote debugging feature of WebIDE. Thanks for the tip! :) The problem was that I was running an adblocker :|

With the adblocker turned off, everything works as expected. I updated the patch to remove 'target_geo' entirely since meanwhile I updated the snippets service to send both 'countries' and 'target_geo'. So older Fennec releases will lookup for 'target_geo' and the new ones for 'countries'.

Comment 10

[:Margaret Leibovic]

(In reply to Giorgos Logiotatidis [:giorgos] from comment #9)
> I managed to debug the problem using the awesome remote debugging feature of
> WebIDE. Thanks for the tip! :) The problem was that I was running an
> adblocker :|

I'm happy you figured it out, but that's sad that the snippets server was blocked!

> With the adblocker turned off, everything works as expected. I updated the
> patch to remove 'target_geo' entirely since meanwhile I updated the snippets
> service to send both 'countries' and 'target_geo'. So older Fennec releases
> will lookup for 'target_geo' and the new ones for 'countries'.

Sounds good.

https://hg.mozilla.org/integration/fx-team/rev/6b09f09f41e3

Comment 11

[Giorgos Logiotatidis [:giorgos]]

\o/ Thanks Margaret. Is this going to ride the trains? When is it expected to hit Release?

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6b09f09f41e3

Comment 13

[:Margaret Leibovic]

(In reply to Giorgos Logiotatidis [:giorgos] from comment #11)
> \o/ Thanks Margaret. Is this going to ride the trains? When is it expected
> to hit Release?

This will be in Firefox 40, which ships to release August 11.

This is also a very simple patch, so if you need it sooner, we could probably uplift it.