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

sitescripts/notifications/web/notification.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-present 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
@@ skipping 22 matching lines @@
             groups.append({'id': group_id, 'variant': int(version_groups[group_id])})
     return 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']):
             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 _get_active_variant(notifications, groups):
     for group in groups:
         group_id = group['id']
         variant = group['variant']
         if variant == 0:
             continue
         notification = next((x for x in notifications if x['id'] == group_id), None)
         if not notification:
@@ skipping 39 matching lines @@
     }
 
 
 @url_handler('/notification.json')
 def notification(environ, start_response):
     params = urlparse.parse_qs(environ.get('QUERY_STRING', ''))
     version = params.get('lastVersion', [''])[0]
     notifications = load_notifications()
     groups = _determine_groups(version, notifications)
     notifications = [x for x in notifications if not x.get('inactive', False)]
-    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.

+    _assign_groups(groups, notifications)
     response = _create_response(notifications, groups)
     response_headers = [('Content-Type', 'application/json; charset=utf-8'),
                         ('ABP-Notification-Version', response['version'])]
     response_body = json.dumps(response, ensure_ascii=False, indent=2,
                                separators=(',', ': '),
                                sort_keys=True).encode('utf-8')
     start_response('200 OK', response_headers)
     return response_body