964677 - Json snippets should not have 'target_geo' key when country is not defined

Status: RESOLVED FIXED

(Snippets :: Service, defect)

Description

[Giorgos Logiotatidis [:giorgos]]

If a json snippet doesn't have country defined, do not include 'target_geo' key in it's json representation.

What's happening now:
[{"url": "http://nightly.mozilla.org/", "text": "Thanks for using Nightly!", "target_geo": "", "id": 1, "icon": "data:image/png;base64,iVBORw0KGgoAAAANSUh... }]

What's expected:
[{"url": "http://nightly.mozilla.org/", "text": "Thanks for using Nightly!", "id": 1, "icon": "data:image/png;base64,iVBORw0KGgoAAAANSUh... }]

Comment 1

[Osmose [:osmose, :mkelly]]

Is there a particular reason why this is bad?

Comment 2

[[github robot]]

Commits pushed to master at https://github.com/mozilla/snippets-service

https://github.com/mozilla/snippets-service/commit/3fd058871b3d962c7913f495d959e4057e1eb8f5
[fix bug 964677] Do not include 'target_geo' in json response if no country.

https://github.com/mozilla/snippets-service/commit/07e214ae7cd08bb8313853aea2aab4c0e92cde14
Merge pull request #50 from glogiotatidis/master

[fix bug 964677] Do not include 'target_geo' in json response if no country.

Comment 3

[Giorgos Logiotatidis [:giorgos]]

For the record:

While testing snippets for android, I realized that a snippet will not show up unless we have a matching country. Digging into fennec's code I found out that if the target_geo key is present it tries to match it to the user's country. Removing it for snippets that do not have country defined will make this work properly.

https://mxr.mozilla.org/mozilla-central/source/mobile/android/components/Snippets.js#173

I decided to make the change in snippet's service, instead of filling a bug against fennec, because we follow the same pattern for desktop snippets too i.e. when country is not defined we do not include it in the data- attributes

https://github.com/mozilla/snippets-service/blob/master/snippets/base/models.py#L225

Comment 4

[Giorgos Logiotatidis [:giorgos]]

This is now fixed and on production.