1161196 - Accept input of custom explanations for adgroups in Splice

Status: RESOLVED FIXED

(Content Services Graveyard :: Tiles: Administration Front-End, defect)

Comment 1

[Olivier Yiptong [:oyiptong]]

From Ed's email:

> There's 4 new types of data that Firefox expects for 39:
> 
> frequency caps: https://bugzilla.mozilla.org/show_bug.cgi?id=1159571
> start/stop time: https://bugzilla.mozilla.org/show_bug.cgi?id=1156549
> negative adjacency: https://bugzilla.mozilla.org/show_bug.cgi?id=1159884
> custom explanation: https://bugzilla.mozilla.org/show_bug.cgi?id=1139496
> 
> I figured having a single thread to discuss and decide will be simpler
> than across 4 bugs. Main idea to confirm is that we'll use some
> grouping of values within a suggested tile as sub objects. And we
> should get confirmation on attribute names (we did have camelCase for
> everything except now frecent_sites).
> 
> Following the current documentation format:
> http://gecko.readthedocs.org/en/latest/browser/browser/DirectoryLinksProvider.html#link-object
> 
> {
>  "url": "..",
>  "frecent_sites": ["site1.com", "site2.com"],
>  "frequency_caps": {
>    "daily": 3,
>    "total": 10
>  },
>  "time_limits": {
>    "start": "2014-01-10T20:20:20.600Z",
>    "end": "2014-01-12T00:00:00.000"
>  },
>  "check_blacklist": true,
>  "explanation": "Suggested for %1$S enthusiasts who visit sites like %2$S",
>  "adgroup_name": "Technology"
> }
> 
> frequency_caps: optional object
> frequency_caps.daily: optional integer
> frequency_caps.total: optional integer
> time_limits: optional object
> time_limits.start: optional ISO_8601 string (optional timezone)
> time_liits.end: optional ISO_8601 string (optional timezone)
> check_blacklist: optional boolean
> explanation: optional string (%1 for adgroup, %2 for site)
> adgroup_name: optional string (overrides hardcoded adgroup name)
> 
> I'll have separate bugs to document the decision we make here on format.
> 
> Ed Lee

Comment 2

[Olivier Yiptong [:oyiptong]]

For this bug, there are 2 new columns to be added to the adgroups table, namely:

1. adgroup_name
2. explanation

Explanation is intended to be a template in which the adgroup_name will be inserted.
Neither of the two will contain html and this content needs to be sanitized, to avoid any injection of code.

This is very important because the newtab page runs with chrome privileges.

I recommend using: https://pypi.python.org/pypi/bleach

Comment 3

[Ed Lee :Mardak]

Firefox already does the html sanitization, so the server doesn't really need to.

Comment 4

[Olivier Yiptong [:oyiptong]]

While the content should be sanitzed in Firefox, it would be best to sanitize server-side as well:

The same string is going to be used in a Web UI so we can display on splice.

Comment 5

[Ed Lee :Mardak]

Ah nod. Worst case simple solution for splice UI is to display it as .textContent as theoretically it should be text.

Comment 6

[[github robot]]

Commit pushed to master at https://github.com/mozilla/splice

https://github.com/mozilla/splice/commit/c3f2893a1e2430c686877f4fd51ae6b6e83e57cf
Bug 1161196 - Accept input of custom explanations for adgroups in Splice

Squashed commit of the following:

commit 484d5ddb4a926e70b00efdfbe09abbc3b09e9523
Author: Nan Jiang <[e-mail]>
Date:   Thu May 21 13:11:14 2015 -0400

    Add dedicated tests for explanation sanity check

commit 0bc38e0392354c4dbb1398449fe77cb7139a2052
Author: Nan Jiang <[e-mail]>
Date:   Thu May 21 11:24:19 2015 -0400

    Fix pip double requirements

commit 5d3e9cdf349f84f73cac1d0d4d7a9326c6d2723b
Author: Nan Jiang <[e-mail]>
Date:   Thu May 21 11:03:14 2015 -0400

    add db migration

commit 5ce1a8918a7154e9132a0c27b24107832033d93f
Merge: 3cd7bf5 e1ec188
Author: Nan Jiang <[e-mail]>
Date:   Thu May 21 10:37:22 2015 -0400

    merge master

commit 3cd7bf5d759d8ba97a813010b5106ea190487f3b
Author: Nan Jiang <[e-mail]>
Date:   Wed May 20 17:56:35 2015 -0400

    Add custom explanation for adgroups

Fix bug 1161196
Close #68, #69

Comment 7

[Ed Lee :Mardak]

Just to note this somewhere, the initial explanation should be

A %1$S site suggestion

Where it might be safer to put the category name in instead of %1$S because of "A" vs "An" issues.

Comment 8

[Nan Jiang [:nanj]]

Yes, that sounds like the pluralizing problem.
 
The current valid template is either "...%1$S...%2$S" or "...%2$S...%1$S". Shall we allow people to use "...%1$S..." forms, which don't have "%2$S"?

Comment 9

[Ed Lee :Mardak]

Yes. There could be %1$S or %2$S or neither or both.

Comment 10

[Nan Jiang [:nanj]]

K. Will add this.