Issue 2276 - Handle groups in notification.json requests

sitescripts/notifications/web/notification.py

 # 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 copy
 import json
 import random
 import time
-from urlparse import parse_qs
+import urlparse
 
 from sitescripts.notifications.parser import load_notifications
 from sitescripts.web import url_handler
 
+def _determine_groups(version, notifications):
+  version_groups = dict(x.split("/") for x in version.split("-")[1:]
+                        if x.count("/") == 1)
+  groups = []
+  for notification in notifications:
+    if "variants" not in notification:
+      continue
+    group_id = notification["id"]
+    if group_id in version_groups:
+      groups.append({"id": group_id, "variant": int(version_groups[group_id])})
+  return groups
+
 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.

+  start = 0
+  for notification in notifications:
+    if "variants" not in notification:
+      continue
     group = {"id": notification["id"], "variant": 0}
     groups.append(group)
     for i, variant in enumerate(notification["variants"]):
-      if group["variant"] != 0:
+      sample_size = variant["sample"]
+      end = start + sample_size
+      selected = sample_size > 0 and start <= selection <= end
+      start = end
+      if selected:
+        group["variant"] = i + 1
         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())
-  }
+def _get_active_variant(notifications, groups):
   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]
+    if variant == 0:
+      continue
+    notification = next(x for x in notifications if x["id"] == group_id)
+    notification = copy.deepcopy(notification)
+    notification.update(notification["variants"][variant - 1])
+    for key_to_remove in ("sample", "variants"):
+      notification.pop(key_to_remove, None)
+    return notification
+
+def _generate_version(groups):
+  version = time.strftime("%Y%m%d%H%M", time.gmtime())
+  for group in groups:
+    version += "-%s/%s" % (group["id"], group["variant"])
+  return version
+
+def _create_response(notifications, groups):
+  active_variant = _get_active_variant(notifications, groups)
+  if active_variant:
+    notifications = [active_variant]
+  else:
+    notifications = [x for x in notifications if "variants" not in x]
+  response = {
+    "version": _generate_version(groups),
+    "notifications": notifications
+  }
   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", ""))
+  params = urlparse.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])})
+  groups = _determine_groups(version, notifications)
   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.

+  start_response("200 OK",
+                 [("Content-Type", "application/json; charset=utf-8")])
   return response.encode("utf-8")