Issue 2982 - Return all notifications that have title and message

sitescripts/notifications/web/notification.py

Index: sitescripts/notifications/web/notification.py
===================================================================
--- a/sitescripts/notifications/web/notification.py
+++ b/sitescripts/notifications/web/notification.py
@@ -68,23 +68,36 @@
       notification.pop(key_to_remove, None)
     return notification
 
+def _can_be_shown(notification):
+  return "title" in notification and "en-US" in notification["title"] and \

[Sebastian Noack, 2015/09/04 11:38:12]

Please don't repeat yourself. How about about:

  return all("en-US" in notification.get(x, []) for x in ("title, "message"))

[Sebastian Noack, 2015/09/04 11:47:36]

I just realized: What is if we show a notification only to users of a particular
language in the future? It wouldn't make sense to require an English translation
then.

So probably we should check whether title and message are given in all targeted
languages, if any. Only requiring the default language if there are no targeted
languages or if not all of them have translations.

[Felix Dahlke, 2015/09/07 07:54:36]

Well, we don't support that yet, so I can't really check for that. Still, I
think I'm already going too far in duplicating the language fallback logic here.
This wasn't meant as a means of checking notifications for validity, it was
meant to check whether a notification _could_ be shown. So I'm just checking for
the presence of title and message now, that should suffice.

+    "message" in notification and "en-US" in notification["message"]
+
 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):
+def _get_notifications_to_send(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 = {
+    return _can_be_shown(active_variant) and [active_variant] or []

[Wladimir Palant, 2015/09/04 11:17:04]

Yay on obfuscation!

  return [active_variant] if _can_be_shown(active_variant) else []

[Felix Dahlke, 2015/09/07 07:54:36]

Well, it was the old way to do it, but I really have to drop that habit one of
these days :P Done.

+
+  notifications_to_send = []
+  for notification in notifications:
+    if not _can_be_shown(notification):
+      continue
+    if "variants" in notification:
+      notification = copy.deepcopy(notification)
+      notification.pop("variants", None)

[Wladimir Palant, 2015/09/04 11:17:04]

Second parameter is unnecessary here, you already know that "variants" is in
there.

[Sebastian Noack, 2015/09/04 11:38:12]

If this is true, the del statement would be more appropriate to use here.

[Felix Dahlke, 2015/09/07 07:54:36]

Done.

+    notifications_to_send.append(notification)
+  return notifications_to_send
+
+def _create_response(notifications, groups):
+  return {
     "version": _generate_version(groups),
-    "notifications": notifications
+    "notifications": _get_notifications_to_send(notifications, groups)
   }
-  return response
 
 @url_handler("/notification.json")
 def notification(environ, start_response):