Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: sitescripts/notifications/web/notification.py

Issue 29570562: Issue 5827 - Assign new groups even if prior groups are present (Closed)
Patch Set: Created Oct. 9, 2017, 8:56 a.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
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-ca
+ 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
Vasily Kuznetsov 2017/10/09 22:29:11 Since we remove inactive notifications in the midd
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'])]

Powered by Google App Engine
This is Rietveld