Closed Bug 1139496 Opened 9 years ago Closed 9 years ago

Allow server provided explanation / ad group name to be displayed on Suggested Tiles

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
normal
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 41
Iteration:
41.1 - May 25
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: kghim, Assigned: emtwo)

References

(Blocks 1 open bug)

Details

(Whiteboard: .004)

Attachments

(4 files, 2 obsolete files)

Allow ad group name to be displayed on Suggested Tiles' explanation text if needed. For example, if the ad group name is "Comic Books," the explanation text would be something like "Suggested because you like Comic Books"

Best practices:
- Use standard Moz Categories if possible. Listed here: https://docs.google.com/a/mozilla.com/spreadsheets/d/1wlPZrU07y035hXDKcHp0KtmszTdRf7jWuz3jo1kpqyk/edit#gid=0
- If not, use short and localizable categories
- Try avoiding custom categories with indistinguishable interests
Blocks: 1126191
Whiteboard: .?
Blocks: 1140185
No longer blocks: 1126191
Instead of just an adgroup name that gets inserted, we could have the server provide the whole localized string. This would allow us to have different types of messaging that still reuse the underlying site matching mechanism:

[firefox for android tile]
Because you downloaded Firefox
Summary: Allow ad group name to be displayed on Suggested Tiles → Allow server provided explanation / ad group name to be displayed on Suggested Tiles
Points: --- → 3
Whiteboard: .? → .001
Whiteboard: .? → .001

.001 makes this bug pretty much the highest priority item. I don't think this work should be done before we finish what we need to do for 38.
Whiteboard: .001 → .004
Blocks: 1156537
Assignee: nobody → msamuel
Flags: needinfo?(oyiptong)
Flags: needinfo?(oyiptong)
Blocks: 1160372
Making it a 5, because its going to require tests
Points: 3 → 5
Component: Tiles: Administration Front-End → Tiles
Depends on: 1161196
Iteration: --- → 40.3 - 11 May
Component: Tiles → New Tab Page
Product: Content Services → Firefox
No longer blocks: 1160372
Iteration: 40.3 - 11 May → 41.1 - May 25
Taking out some code I was testing with.
Attachment #8605535 - Attachment is obsolete: true
Attachment #8605535 - Flags: review?(adw)
Attachment #8605536 - Flags: review?(adw)
Attachment #8605536 - Attachment is patch: true
Comment on attachment 8605536 [details] [diff] [review]
v2: Allow server provided explanation / ad group name to be displayed on Suggested Tile

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

Cool.

::: browser/base/content/newtab/sites.js
@@ +118,5 @@
>  
> +  _newTabString: function(str, substrArr) {
> +    let regExp = /%[0-9]\$S/g;
> +    let matches;
> +    while ((matches = regExp.exec(str)) !== null) {

Nit: while ((matches = regExp.exec(str))) {
Attachment #8605536 - Flags: review?(adw) → review+
Comment on attachment 8605536 [details] [diff] [review]
v2: Allow server provided explanation / ad group name to be displayed on Suggested Tile

emtwo, I forgot to mention we need to update the .rst explaining the data from the server and giving an example:

explanation and adgroup_name

See bug 1156549 comment 16 for an example comment. And request f? :bsmedberg
See https://hg.mozilla.org/mozilla-central/rev/cd19fda2d5f8 for updated rst for time_limits
Attachment #8605536 - Flags: feedback-
f? per https://wiki.mozilla.org/Firefox/Data_Collection

There's no additional data being collected, but the server provides two extra strings relating to the explanation below the suggested tile.
Attachment #8606109 - Flags: feedback?(benjamin)
Attachment #8605536 - Attachment is obsolete: true
Attachment #8606109 - Attachment description: v2.1 → Part 1: v2.1: Allow server provided explanation / ad group name to be displayed on Suggested Tile
Comment on attachment 8606754 [details] [diff] [review]
Part 2: v1: Sanitize explanation & adgroup name sent by server

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

::: browser/modules/DirectoryLinksProvider.jsm
@@ +617,5 @@
>          if (name == undefined) {
>            return;
>          }
> +        let sanitizeFlags =
> +          ParserUtils.SanitizerCidEmbedsOnly|ParserUtils.SanitizerDropForms|ParserUtils.SanitizerDropNonCSSPresentation;

Style nit: please put each of these flags on its own line.
Attachment #8606754 - Flags: review?(adw) → review+
Attachment #8606109 - Flags: feedback?(benjamin) → feedback+
https://hg.mozilla.org/mozilla-central/rev/bafcb7152768
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Depends on: 1167243
Verified on Nightly - 41.0a1 (2015-05-26).

verification steps:
1. created fresh profile
2. browsed to achieve 8 history sites showing up in newtab
3. one of history sites is cmm.com (which is in the "News" list of targeted sites)
4. observed "News App" suggested tile from Firefox Marketplace targeted to News sites
5. Text snippet under the tile says "Suggested for News visitors"
Status: RESOLVED → VERIFIED
Verified on Nightly - 41.0a1 (2015-05-26).


Note explanation text in json (full version: https://people.mozilla.org/~mzhilyaev/suggested1.json)
{
  "adgroup_name": "Mozilla enthusiast",
  ....
  "explanation": "A %1$S suggestion for those who visit %2$S (I'm so wordy!)",
  ....
}

And the corresponding display of explanation text in the attached screen shot:
https://people.mozilla.org/~mzhilyaev/1138817.sh2.png

The explanation string shows properly replaced '%1$S' with adgroup_name and '%2$S' with targeted site.
The verification step for comment 17 requires going to about:config and setting

browser.newtabpage.directory.source

to

https://people.mozilla.org/~mzhilyaev/suggested1.json
Comment on attachment 8610021 [details] [diff] [review]
for aurora (Mardak will land)

Approval Request Comment: See bug 1140185 comment 11
Attachment #8610021 - Flags: approval-mozilla-aurora?
Comment on attachment 8610021 [details] [diff] [review]
for aurora (Mardak will land)

approval-mozilla-aurora+ granted in bug 1140185 comment 14
Attachment #8610021 - Flags: approval-mozilla-aurora?
Confirming the fix on latest Aurora, build ID: 20150528004000.

Testing was performed on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit.
QA Contact: cornel.ionce
Depends on: 1263256
You need to log in before you can comment on or make changes to this bug.