Issue 2276 - Handle groups in notification.json requests

sitescripts/notifications/web/notification.py

Index: sitescripts/notifications/web/notification.py
===================================================================
new file mode 100644
--- /dev/null
+++ b/sitescripts/notifications/web/notification.py
@@ -0,0 +1,79 @@
+# coding: utf-8
+
+# This file is part of the Adblock Plus web scripts,
+# Copyright (C) 2006-2015 Eyeo GmbH
+#
+# Adblock Plus is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3 as
+# published by the Free Software Foundation.
+#
+# Adblock Plus is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+
+import json
+import random
+import time
+from urlparse import parse_qs
+
+from sitescripts.notifications.parser import load_notifications
+from sitescripts.web import url_handler
+
+def _assign_groups(notifications):
+  groups = []
+  selection = random.random()
+  base = 0
+  for notification in (x for x in notifications if "variants" in x):

[Sebastian Noack, 2015/04/20 14:44:39]

Nit: Just continue the loop instead creating a generator.

for x in notifications:
  if "variant" not in x:
    continue

[Felix Dahlke, 2015/04/23 22:15:18]

Done.

+    group = {"id": notification["id"], "variant": 0}
+    groups.append(group)
+    for i, variant in enumerate(notification["variants"]):
+      if group["variant"] != 0:
+        break

[Sebastian Noack, 2015/04/20 14:44:39]

Please move the break below where you set variant to non-zero.

[Felix Dahlke, 2015/04/23 22:15:18]

Done.

+      sample_size = variant["sample"]
+      if sample_size > 0 and base <= selection <= base + sample_size:

[Sebastian Noack, 2015/04/20 14:44:39]

Nit: The logic of adding base + sample_size is duplicated below. Consider using
a variable.

[Felix Dahlke, 2015/04/23 22:15:18]

Done.

+        group["variant"] = i + 1
+      base += sample_size
+  return groups
+
+def _create_response(notifications, groups):
+  response = {
+    "version": time.strftime("%Y%m%d%H%M", time.gmtime())
+  }
+  for group in groups:
+    group_id = group["id"]
+    variant = group["variant"]
+    response["version"] += "-%s/%s" % (group_id, variant)
+    if variant > 0:
+      notification = next(x for x in notifications if x["id"] == group_id)
+      notification = notification.copy()

[Sebastian Noack, 2015/04/20 14:44:39]

Note that this will be a shadow copy. To create deep copies use copy.deepcopy()

(Should be fine here though)

[Felix Dahlke, 2015/04/23 22:15:18]

Done.

+      notification.update(notification["variants"][variant - 1])
+      for key_to_remove in ("sample", "variants"):
+        notification.pop(key_to_remove, None)

[Sebastian Noack, 2015/04/20 14:44:39]

I don't mind, but if you prefer to merge the dicts with one operation you can
use the dict() constructor like that:

dict(notification, **notification["variants"][variant - 1])

[Felix Dahlke, 2015/04/23 22:15:18]

I know I specifically asked for a nicer way - but to me the current code is
easiest to read at the end of the day.

+      response["notifications"] = [notification]
+  if "notifications" not in response:
+    response["notifications"] = [x for x in notifications
+                                 if "variants" not in x]
+  return json.dumps(response, ensure_ascii=False, indent=2,
+                    separators=(",", ": "), sort_keys=True)
+
+@url_handler("/notification.json")
+def notification(environ, start_response):
+  params = parse_qs(environ.get("QUERY_STRING", ""))
+  version = params.get("lastVersion", [""])[0]
+  current_groups = dict(x.split("/") for x in version.split("-")[1:]

[Felix Dahlke, 2015/04/23 22:15:18]

Done.

+                        if "/" in x)
+  notifications = load_notifications()
+  groups = []
+  for notification in (x for x in notifications if "variants" in x):

[Sebastian Noack, 2015/04/20 14:44:39]

See above.

[Felix Dahlke, 2015/04/23 22:15:18]

Done.

+    group_id = notification["id"]
+    if group_id in current_groups:
+      groups.append({"id": group_id, "variant": int(current_groups[group_id])})
+  if not groups:
+    groups = _assign_groups(notifications)
+  response = _create_response(notifications, groups)
+  start_response("200 OK", [("Content-Type", "application/json")])

[Sebastian Noack, 2015/04/20 14:44:39]

Please send response charset.

[Felix Dahlke, 2015/04/23 22:15:18]

UTF-8 is default for JSON. However, as discussed with you and Wladimir, doesn't
hurt to play it safe here.

+  return response.encode("utf-8")