Closed
Bug 1161196
Opened 9 years ago
Closed 9 years ago
Accept input of custom explanations for adgroups in Splice
Categories
(Content Services Graveyard :: Tiles: Administration Front-End, defect)
Content Services Graveyard
Tiles: Administration Front-End
Tracking
(Not tracked)
RESOLVED
FIXED
Iteration:
41.1 - May 25
People
(Reporter: oyiptong, Assigned: nanj)
References
Details
(Whiteboard: .?)
No description provided.
Updated•9 years ago
|
Iteration: 40.3 - 11 May → 41.1 - May 25
Reporter | ||
Comment 1•9 years ago
|
||
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
Reporter | ||
Comment 2•9 years ago
|
||
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
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → najiang
Comment 3•9 years ago
|
||
Firefox already does the html sanitization, so the server doesn't really need to.
Reporter | ||
Comment 4•9 years ago
|
||
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•9 years ago
|
||
Ah nod. Worst case simple solution for splice UI is to display it as .textContent as theoretically it should be text.
Comment 6•9 years ago
|
||
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 <njiang028@gmail.com> Date: Thu May 21 13:11:14 2015 -0400 Add dedicated tests for explanation sanity check commit 0bc38e0392354c4dbb1398449fe77cb7139a2052 Author: Nan Jiang <njiang028@gmail.com> Date: Thu May 21 11:24:19 2015 -0400 Fix pip double requirements commit 5d3e9cdf349f84f73cac1d0d4d7a9326c6d2723b Author: Nan Jiang <njiang028@gmail.com> Date: Thu May 21 11:03:14 2015 -0400 add db migration commit 5ce1a8918a7154e9132a0c27b24107832033d93f Merge: 3cd7bf5 e1ec188 Author: Nan Jiang <njiang028@gmail.com> Date: Thu May 21 10:37:22 2015 -0400 merge master commit 3cd7bf5d759d8ba97a813010b5106ea190487f3b Author: Nan Jiang <njiang028@gmail.com> Date: Wed May 20 17:56:35 2015 -0400 Add custom explanation for adgroups Fix bug 1161196 Close #68, #69
Assignee | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 7•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
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•9 years ago
|
||
Yes. There could be %1$S or %2$S or neither or both.
Assignee | ||
Comment 10•9 years ago
|
||
K. Will add this.
You need to log in
before you can comment on or make changes to this bug.
Description
•