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()

[kzar, 2015/06/03 11:09:50]

Nit: `from random import random` and then we can just call `random()`?

[Sebastian Noack, 2015/06/03 12:17:15]

I slightly prefer to import just the module, looking up the function where it is
called, at least for symbols from corelib modules used only once. That way it's
more obvious where the function is coming from. But I leave it up to Felix.

[Felix Dahlke, 2015/06/12 11:02:40]

Same here, not incredibly consistent about it however. Made things more
consistent by not importing parse_qs either.

[kzar, 2015/06/15 15:17:38]

Acknowledged.

   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"]):
       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
   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 = copy.deepcopy(notification)
-      notification.update(notification["variants"][variant - 1])
-      for key_to_remove in ("sample", "variants"):
-        notification.pop(key_to_remove, None)
-      response["notifications"] = [notification]

[kzar, 2015/06/03 11:09:50]

Won't you overwrite a previous notification in the response here if there is
one? Shouldn't we append if the key already exists? (setdefault might be useful
for that https://docs.python.org/2/library/stdtypes.html#dict.setdefault )

If it's OK to rewrite previous notifications or for some reason I missed there
will always be one notification we could probably simplify this logic. We could
break from the `for group in groups` loop here and have an else clause to set
the default response["notifications"]. That way we could avoid the `if
"notifications" not in response` check.

[Felix Dahlke, 2015/06/12 11:02:40]

That's intentional - if the user is in a test, they shouldn't get any other
notifications.

We can't break here, since we do two things in this loop: We populate
response["version"] and we set response["notifications"]. I guess that's what
makes this hard to follow, so I've split this up and refactored a bit - let me
know if things are more clear this way.

[kzar, 2015/06/15 15:17:37]

Acknowledged.

-  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", ""))

[kzar, 2015/06/03 11:09:50]

Nit: IMHO it's easier to do `params = dict(parse_qsl(environ.get("QUERY_STRING",
"")))` and then you don't have to mess around with the default value of `[""]`
or always appending [0] when accessing a parameter. Debatable either way and not
so important here as you only need to access "lastVersion".

[Sebastian Noack, 2015/06/03 12:17:15]

Given that only one param is retrieved here, I think it's better to stick to
parse_qs(). Otherwise we just trade in the complexity below for some complexity
here. Also I think it's more obvious what the code is doing if we explicitly
pick the first value.

[Felix Dahlke, 2015/06/12 11:02:40]

No particularly strong opinion, but I also think the way it is currently it's
actually a bit more straightforward, for just one parameter.

[kzar, 2015/06/15 15:17:37]

Acknowledged.

+  params = urlparse.parse_qs(environ.get("QUERY_STRING", ""))
   version = params.get("lastVersion", [""])[0]
-  current_groups = dict(x.split("/") for x in version.split("-")[1:]
-                        if x.count("/") == 1)
   notifications = load_notifications()
-  groups = []

[kzar, 2015/06/03 11:09:50]

Wouldn't it be better to merge current_groups with the result of _assign_groups
here? _assign_groups could even take current_groups as a parameter I suppose,
then you could just add to it for notifications that already have a variant set.

I think it would be simpler + nice to move the logic for group assignment to one
place, instead of it being in the handler here.

[Felix Dahlke, 2015/06/12 11:02:40]

I can see why you don't like this, but I'm not sure moving this logic to
_assign_groups is better. What happens here logically is:

1. Determine which groups the user is in (by parsing lastVersion above, and with
the for loop below)
2. If he isn't in any groups, put him into 0-1 groups (that's _assign_groups)

Maybe I misunderstood your point, but IMHO merging these two things together
makes them even harder to understand. But I agree that it's not great as it is.

What I believe would make things clearer:

groups = _determine_groups(notifications, version)
if not groups:
  groups = _assign_groups(notifications)

Or shorter (but possibly too cryptic):

groups = (_determine_groups(notifications, version) or
          _assign_groups(notifications))

_determine_groups (not sure the naming is brilliant) would then parse the groups
from the lastVersion parameter and run the for loop below, putting the user in
the groups he's in (that actually exist).

What do you think?

[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 what you think Dave.

[kzar, 2015/06/15 15:17:37]

Sounds good to me

-  for notification in notifications:
-    if "variants" not in notification:
-      continue
-    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; charset=utf-8")])
   return response.encode("utf-8")