Closed Bug 1155579 Opened 5 years ago Closed 5 years ago

Snippets - Allow multiple countries per snippet

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: giorgos, Assigned: giorgos, Mentored)

References

Details

(Whiteboard: [lang=js][good next bug])

Attachments

(1 file, 1 obsolete file)

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.
See Also: → 922792
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
Mentor: margaret.leibovic
Whiteboard: [lang=js][good next bug]
Attached patch 1155579-fennec.patch (obsolete) — Splinter Review
Attachment #8596657 - Flags: review?(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.
Attachment #8596657 - Flags: review?(margaret.leibovic) → review+
Assignee: nobody → 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?
Flags: needinfo?(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.
Flags: needinfo?(margaret.leibovic)
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?
Flags: needinfo?(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.
Flags: needinfo?(margaret.leibovic) → needinfo?(giorgos)
Attachment #8596657 - Attachment is obsolete: true
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'.
Flags: needinfo?(giorgos)
(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
\o/ Thanks Margaret. Is this going to ride the trains? When is it expected to hit Release?
https://hg.mozilla.org/mozilla-central/rev/6b09f09f41e3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
(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.
You need to log in before you can comment on or make changes to this bug.