Issue 5827 - Assign new groups even if prior groups are present

sitescripts/notifications/web/notification.py

Index: sitescripts/notifications/web/notification.py
diff --git a/sitescripts/notifications/web/notification.py b/sitescripts/notifications/web/notification.py
index 918df55d2202e0e1125efebb9b994aa79aa13f2e..59cd0094c6f7f64afa5fe059b05f2504ba9efd89 100644
--- a/sitescripts/notifications/web/notification.py
+++ b/sitescripts/notifications/web/notification.py
@@ -34,13 +34,14 @@ def _determine_groups(version, notifications):
     return groups
 
 
-def _assign_groups(notifications):
-    groups = []
+def _assign_groups(groups, notifications):
     selection = random.random()
     start = 0
     for notification in notifications:
         if 'variants' not in notification:
             continue
+        if notification['id'] in [g['id'] for g in groups]:

[Vasily Kuznetsov, 2017/10/09 22:29:12]

This should be a set and probably should be pre-calculated.

+            continue
         group = {'id': notification['id'], 'variant': 0}
         groups.append(group)
         for i, variant in enumerate(notification['variants']):
@@ -111,8 +112,7 @@ def notification(environ, start_response):
     notifications = load_notifications()
     groups = _determine_groups(version, notifications)
     notifications = [x for x in notifications if not x.get('inactive', False)]
-    if not groups:
-        groups = _assign_groups(notifications)
+    groups = _assign_groups(groups, notifications)

[wspee, 2017/10/09 09:12:03]

Ideally I would like to combine _determine_groups and _assign_groups because the
two steps are not really independent.

But then I would be changing a lot of code already and then I would like to
change some more and so on ... based on apple pricing logic I would end up with
a complete rewrite which is probably a bad idea because of [0].

So it's Darwin for now I fear :)

[0] https://www.joelonsoftware.com/2000/04/06/things-you-should-never-do-part-i/

[Vasily Kuznetsov, 2017/10/09 22:29:11]

Since we remove inactive notifications in the middle, notifications parameter in
the two calls is not the same. Seems like you'd have to move this filtering
inside to combine two functions, which would probably make them do too many
unrelated things. I would say the current separation makes some sense.

[wspee, 2017/10/10 09:03:53]

Acknowledged.

     response = _create_response(notifications, groups)
     response_headers = [('Content-Type', 'application/json; charset=utf-8'),
                         ('ABP-Notification-Version', response['version'])]