Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Delta Between Two Patch Sets: sitescripts/notifications/web/notification.py

Issue 4883267715072000: Issue 2276 - Handle groups in notification.json requests (Closed)
Left Patch Set: Address comments Created April 23, 2015, 10:12 p.m.
Right Patch Set: Extract group parsing Created June 12, 2015, 12:11 p.m.
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
Left: Side by side diff | Download
Right: Side by side diff | Download
« no previous file with change/comment | « sitescripts/notifications/web/__init__.py ('k') | no next file » | no next file with change/comment »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
LEFTRIGHT
1 # coding: utf-8 1 # coding: utf-8
2 2
3 # This file is part of the Adblock Plus web scripts, 3 # This file is part of the Adblock Plus web scripts,
4 # Copyright (C) 2006-2015 Eyeo GmbH 4 # Copyright (C) 2006-2015 Eyeo GmbH
5 # 5 #
6 # Adblock Plus is free software: you can redistribute it and/or modify 6 # Adblock Plus is free software: you can redistribute it and/or modify
7 # it under the terms of the GNU General Public License version 3 as 7 # it under the terms of the GNU General Public License version 3 as
8 # published by the Free Software Foundation. 8 # published by the Free Software Foundation.
9 # 9 #
10 # Adblock Plus is distributed in the hope that it will be useful, 10 # Adblock Plus is distributed in the hope that it will be useful,
11 # but WITHOUT ANY WARRANTY; without even the implied warranty of 11 # but WITHOUT ANY WARRANTY; without even the implied warranty of
12 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the 12 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13 # GNU General Public License for more details. 13 # GNU General Public License for more details.
14 # 14 #
15 # You should have received a copy of the GNU General Public License 15 # You should have received a copy of the GNU General Public License
16 # along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>. 16 # along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.
17 17
18 import copy 18 import copy
19 import json 19 import json
20 import random 20 import random
21 import time 21 import time
22 from urlparse import parse_qs 22 import urlparse
23 23
24 from sitescripts.notifications.parser import load_notifications 24 from sitescripts.notifications.parser import load_notifications
25 from sitescripts.web import url_handler 25 from sitescripts.web import url_handler
26
27 def _determine_groups(version, notifications):
28 version_groups = dict(x.split("/") for x in version.split("-")[1:]
29 if x.count("/") == 1)
30 groups = []
31 for notification in notifications:
32 if "variants" not in notification:
33 continue
34 group_id = notification["id"]
35 if group_id in version_groups:
36 groups.append({"id": group_id, "variant": int(version_groups[group_id])})
37 return groups
26 38
27 def _assign_groups(notifications): 39 def _assign_groups(notifications):
28 groups = [] 40 groups = []
29 selection = random.random() 41 selection = random.random()
kzar 2015/06/03 11:09:50 Nit: `from random import random` and then we can j
Sebastian Noack 2015/06/03 12:17:15 I slightly prefer to import just the module, looki
Felix Dahlke 2015/06/12 11:02:40 Same here, not incredibly consistent about it howe
kzar 2015/06/15 15:17:38 Acknowledged.
30 start = 0 42 start = 0
31 for notification in notifications: 43 for notification in notifications:
32 if "variants" not in notification: 44 if "variants" not in notification:
33 continue 45 continue
34 group = {"id": notification["id"], "variant": 0} 46 group = {"id": notification["id"], "variant": 0}
35 groups.append(group) 47 groups.append(group)
36 for i, variant in enumerate(notification["variants"]): 48 for i, variant in enumerate(notification["variants"]):
37 sample_size = variant["sample"] 49 sample_size = variant["sample"]
38 end = start + sample_size 50 end = start + sample_size
39 selected = sample_size > 0 and start <= selection <= end 51 selected = sample_size > 0 and start <= selection <= end
40 start = end 52 start = end
41 if selected: 53 if selected:
42 group["variant"] = i + 1 54 group["variant"] = i + 1
43 break 55 break
44 return groups 56 return groups
45 57
46 def _create_response(notifications, groups): 58 def _get_active_variant(notifications, groups):
47 response = {
48 "version": time.strftime("%Y%m%d%H%M", time.gmtime())
49 }
50 for group in groups: 59 for group in groups:
51 group_id = group["id"] 60 group_id = group["id"]
52 variant = group["variant"] 61 variant = group["variant"]
53 response["version"] += "-%s/%s" % (group_id, variant) 62 if variant == 0:
54 if variant > 0: 63 continue
55 notification = next(x for x in notifications if x["id"] == group_id) 64 notification = next(x for x in notifications if x["id"] == group_id)
56 notification = copy.deepcopy(notification) 65 notification = copy.deepcopy(notification)
57 notification.update(notification["variants"][variant - 1]) 66 notification.update(notification["variants"][variant - 1])
58 for key_to_remove in ("sample", "variants"): 67 for key_to_remove in ("sample", "variants"):
59 notification.pop(key_to_remove, None) 68 notification.pop(key_to_remove, None)
60 response["notifications"] = [notification] 69 return notification
kzar 2015/06/03 11:09:50 Won't you overwrite a previous notification in the
Felix Dahlke 2015/06/12 11:02:40 That's intentional - if the user is in a test, the
kzar 2015/06/15 15:17:37 Acknowledged.
61 if "notifications" not in response: 70
62 response["notifications"] = [x for x in notifications 71 def _generate_version(groups):
63 if "variants" not in x] 72 version = time.strftime("%Y%m%d%H%M", time.gmtime())
73 for group in groups:
74 version += "-%s/%s" % (group["id"], group["variant"])
75 return version
76
77 def _create_response(notifications, groups):
78 active_variant = _get_active_variant(notifications, groups)
79 if active_variant:
80 notifications = [active_variant]
81 else:
82 notifications = [x for x in notifications if "variants" not in x]
83 response = {
84 "version": _generate_version(groups),
85 "notifications": notifications
86 }
64 return json.dumps(response, ensure_ascii=False, indent=2, 87 return json.dumps(response, ensure_ascii=False, indent=2,
65 separators=(",", ": "), sort_keys=True) 88 separators=(",", ": "), sort_keys=True)
66 89
67 @url_handler("/notification.json") 90 @url_handler("/notification.json")
68 def notification(environ, start_response): 91 def notification(environ, start_response):
69 params = parse_qs(environ.get("QUERY_STRING", "")) 92 params = urlparse.parse_qs(environ.get("QUERY_STRING", ""))
kzar 2015/06/03 11:09:50 Nit: IMHO it's easier to do `params = dict(parse_q
Sebastian Noack 2015/06/03 12:17:15 Given that only one param is retrieved here, I thi
Felix Dahlke 2015/06/12 11:02:40 No particularly strong opinion, but I also think t
kzar 2015/06/15 15:17:37 Acknowledged.
70 version = params.get("lastVersion", [""])[0] 93 version = params.get("lastVersion", [""])[0]
71 current_groups = dict(x.split("/") for x in version.split("-")[1:]
72 if x.count("/") == 1)
73 notifications = load_notifications() 94 notifications = load_notifications()
74 groups = [] 95 groups = _determine_groups(version, notifications)
kzar 2015/06/03 11:09:50 Wouldn't it be better to merge current_groups with
Felix Dahlke 2015/06/12 11:02:40 I can see why you don't like this, but I'm not sur
Sebastian Noack 2015/06/12 11:15:09 I'd vote for this one.
Felix Dahlke 2015/06/12 12:12:52 I prefer that one too, went for it. Let me know wh
kzar 2015/06/15 15:17:37 Sounds good to me
75 for notification in notifications:
76 if "variants" not in notification:
77 continue
78 group_id = notification["id"]
79 if group_id in current_groups:
80 groups.append({"id": group_id, "variant": int(current_groups[group_id])})
81 if not groups: 96 if not groups:
82 groups = _assign_groups(notifications) 97 groups = _assign_groups(notifications)
83 response = _create_response(notifications, groups) 98 response = _create_response(notifications, groups)
84 start_response("200 OK", 99 start_response("200 OK",
85 [("Content-Type", "application/json; charset=utf-8")]) 100 [("Content-Type", "application/json; charset=utf-8")])
86 return response.encode("utf-8") 101 return response.encode("utf-8")
LEFTRIGHT

Powered by Google App Engine
This is Rietveld