Issue 4883267715072000: Issue 2276 - Handle groups in notification.json requests (Closed)
|
Created:
April 6, 2015, 10:02 p.m. by Felix Dahlke Modified:
June 15, 2015, 3:46 p.m. CC:
Wladimir Palant Visibility:
Public. |
DescriptionIssue 2276 - Handle groups in notification.json requests
Patch Set 1 #Patch Set 2 : Test that translations are mixed properly #Patch Set 3 : Remove sample, return response properly #Patch Set 4 : Refactor #
Total comments: 18
Patch Set 5 : Address comments #
Total comments: 21
Patch Set 6 : Address comments #Patch Set 7 : Extract group parsing #
MessagesTotal messages: 13
Here's my first go at this. I think there's some room for improvement but I fear I can't really see the forest for the trees right now, so I'd appreciate feedback, particularly on how groups are assigned.
New patch set is up, refactored this a bit based on Sebastian's feedback in http://codereview.adblockplus.org/6582297721569280/.
http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... File sitescripts/notifications/test/notification.py (right): http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/test/notification.py:86: notification.load_notifications = lambda: [ You should restore the orginal value. Feel free to use the mock module here as well. http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... File sitescripts/notifications/web/notification.py (right): http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:30: for notification in (x for x in notifications if "variants" in x): Nit: Just continue the loop instead creating a generator. for x in notifications: if "variant" not in x: continue http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:35: break Please move the break below where you set variant to non-zero. http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:37: if sample_size > 0 and base <= selection <= base + sample_size: Nit: The logic of adding base + sample_size is duplicated below. Consider using a variable. http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:52: notification = notification.copy() Note that this will be a shadow copy. To create deep copies use copy.deepcopy() (Should be fine here though) http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:55: notification.pop(key_to_remove, None) I don't mind, but if you prefer to merge the dicts with one operation you can use the dict() constructor like that: dict(notification, **notification["variants"][variant - 1]) http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:67: current_groups = dict(x.split("/") for x in version.split("-")[1:] Please handle the case of multiple present separators: x.count("/") == 1 http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:71: for notification in (x for x in notifications if "variants" in x): See above. http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:78: start_response("200 OK", [("Content-Type", "application/json")]) Please send response charset.
Addressed all comments, new patch set is up. However, I'm still somewhat inclined to refactor _assign_groups - it's the really important part here, and I find it a bit too complex as it is. The idea is to move the selection logic out of that to have something like a weighted random.choice() function. http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... File sitescripts/notifications/test/notification.py (right): http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/test/notification.py:86: notification.load_notifications = lambda: [ On 2015/04/20 14:44:39, Sebastian Noack wrote: > You should restore the orginal value. Feel free to use the mock module here as > well. Done. http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... File sitescripts/notifications/web/notification.py (right): http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:30: for notification in (x for x in notifications if "variants" in x): On 2015/04/20 14:44:39, Sebastian Noack wrote: > Nit: Just continue the loop instead creating a generator. > > for x in notifications: > if "variant" not in x: > continue Done. http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:35: break On 2015/04/20 14:44:39, Sebastian Noack wrote: > Please move the break below where you set variant to non-zero. Done. http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:37: if sample_size > 0 and base <= selection <= base + sample_size: On 2015/04/20 14:44:39, Sebastian Noack wrote: > Nit: The logic of adding base + sample_size is duplicated below. Consider using > a variable. Done. http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:52: notification = notification.copy() On 2015/04/20 14:44:39, Sebastian Noack wrote: > Note that this will be a shadow copy. To create deep copies use copy.deepcopy() > > (Should be fine here though) Done. http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:55: notification.pop(key_to_remove, None) On 2015/04/20 14:44:39, Sebastian Noack wrote: > I don't mind, but if you prefer to merge the dicts with one operation you can > use the dict() constructor like that: > > dict(notification, **notification["variants"][variant - 1]) > I know I specifically asked for a nicer way - but to me the current code is easiest to read at the end of the day. http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:67: current_groups = dict(x.split("/") for x in version.split("-")[1:] On 2015/04/20 14:44:39, Sebastian Noack wrote: > Please handle the case of multiple present separators: x.count("/") == 1 Done. http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:71: for notification in (x for x in notifications if "variants" in x): On 2015/04/20 14:44:39, Sebastian Noack wrote: > See above. Done. http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/site... sitescripts/notifications/web/notification.py:78: start_response("200 OK", [("Content-Type", "application/json")]) On 2015/04/20 14:44:39, Sebastian Noack wrote: > Please send response charset. UTF-8 is default for JSON. However, as discussed with you and Wladimir, doesn't hurt to play it safe here.
LGTM On 2015/04/23 22:15:18, Felix H. Dahlke wrote: > Addressed all comments, new patch set is up. > > However, I'm still somewhat inclined to refactor _assign_groups - it's the > really important part here, and I find it a bit too complex as it is. > > The idea is to move the selection logic out of that to have something like a > weighted random.choice() function. Yeah, the logic there is kinda complicated. Though, I don't have an idea how to make it much easier to understand, other than adding some comments maybe.
Tried to simplify the logic a bit, however, didn't really end up being simpler IMHO. So I'll leave it as it is for now.
Some feedback as requested, feel free to ignore me as the notification system is completely new to me. (Note, I didn't look at the tests.) http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... File sitescripts/notifications/README.md (right): http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/README.md:7: Description / example of the file format would be cool here IMHO. Something like the description of https://issues.adblockplus.org/ticket/2162 http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... File sitescripts/notifications/web/notification.py (right): http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/web/notification.py:29: selection = random.random() Nit: `from random import random` and then we can just call `random()`? http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/web/notification.py:60: response["notifications"] = [notification] 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. http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/web/notification.py:69: params = parse_qs(environ.get("QUERY_STRING", "")) 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". http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/web/notification.py:74: groups = [] 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.
http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... File sitescripts/notifications/web/notification.py (right): http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/web/notification.py:29: selection = random.random() On 2015/06/03 11:09:50, kzar wrote: > Nit: `from random import random` and then we can just call `random()`? 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. http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/web/notification.py:69: params = parse_qs(environ.get("QUERY_STRING", "")) On 2015/06/03 11:09:50, kzar wrote: > 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". 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.
Thanks for having a look, new patch set is up. http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... File sitescripts/notifications/README.md (right): http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/README.md:7: On 2015/06/03 11:09:50, kzar wrote: > Description / example of the file format would be cool here IMHO. Something like > the description of https://issues.adblockplus.org/ticket/2162 Good point, but would you be OK with doing this in a follow-up? We surely should properly specify/document the notification format, but I feel doing that properly is a bit out of scope here. I'll file an issue if you agree. http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... File sitescripts/notifications/web/notification.py (right): http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/web/notification.py:29: selection = random.random() On 2015/06/03 12:17:15, Sebastian Noack wrote: > On 2015/06/03 11:09:50, kzar wrote: > > Nit: `from random import random` and then we can just call `random()`? > > 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. Same here, not incredibly consistent about it however. Made things more consistent by not importing parse_qs either. http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/web/notification.py:60: response["notifications"] = [notification] On 2015/06/03 11:09:50, kzar wrote: > 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. 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. http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/web/notification.py:69: params = parse_qs(environ.get("QUERY_STRING", "")) On 2015/06/03 12:17:15, Sebastian Noack wrote: > On 2015/06/03 11:09:50, kzar wrote: > > 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". > > 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. No particularly strong opinion, but I also think the way it is currently it's actually a bit more straightforward, for just one parameter. http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/web/notification.py:74: groups = [] On 2015/06/03 11:09:50, kzar wrote: > 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. 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?
http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... File sitescripts/notifications/README.md (right): http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/README.md:7: On 2015/06/12 11:02:40, Felix H. Dahlke wrote: > On 2015/06/03 11:09:50, kzar wrote: > > Description / example of the file format would be cool here IMHO. Something > like > > the description of https://issues.adblockplus.org/ticket/2162 > > Good point, but would you be OK with doing this in a follow-up? We surely should > properly specify/document the notification format, but I feel doing that > properly is a bit out of scope here. I'll file an issue if you agree. Well, if adding a README.md here isn't out of scope, documenting it properly shouldn't be either. But up to you. http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... File sitescripts/notifications/web/notification.py (right): http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/web/notification.py:74: groups = [] On 2015/06/12 11:02:40, Felix H. Dahlke wrote: > groups = _determine_groups(notifications, version) > if not groups: > groups = _assign_groups(notifications) I'd vote for this one.
New patch set is up. http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... File sitescripts/notifications/README.md (right): http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/README.md:7: On 2015/06/12 11:15:09, Sebastian Noack wrote: > On 2015/06/12 11:02:40, Felix H. Dahlke wrote: > > On 2015/06/03 11:09:50, kzar wrote: > > > Description / example of the file format would be cool here IMHO. Something > > like > > > the description of https://issues.adblockplus.org/ticket/2162 > > > > Good point, but would you be OK with doing this in a follow-up? We surely > should > > properly specify/document the notification format, but I feel doing that > > properly is a bit out of scope here. I'll file an issue if you agree. > > Well, if adding a README.md here isn't out of scope, documenting it properly > shouldn't be either. But up to you. There are two things we need to document: 1. The notification.json format 2. The format of the notification files in the notifications repository The latter wouldn't be out of scope here since we're changing the format slightly, but the former IMHO is (we're not even changing the notification.json format here), and IMO it's a prerequisite for the latter to make sense. Here's the follow-up: https://issues.adblockplus.org/ticket/2676 http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... File sitescripts/notifications/web/notification.py (right): http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/site... sitescripts/notifications/web/notification.py:74: groups = [] On 2015/06/12 11:15:09, Sebastian Noack wrote: > On 2015/06/12 11:02:40, Felix H. Dahlke wrote: > > groups = _determine_groups(notifications, version) > > if not groups: > > groups = _assign_groups(notifications) > > I'd vote for this one. I prefer that one too, went for it. Let me know what you think Dave.
LGTM https://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sit... File sitescripts/notifications/README.md (right): https://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sit... sitescripts/notifications/README.md:7: On 2015/06/12 12:12:52, Felix Dahlke wrote: > On 2015/06/12 11:15:09, Sebastian Noack wrote: > > On 2015/06/12 11:02:40, Felix H. Dahlke wrote: > > > On 2015/06/03 11:09:50, kzar wrote: > > > > Description / example of the file format would be cool here IMHO. > Something > > > like > > > > the description of https://issues.adblockplus.org/ticket/2162 > > > > > > Good point, but would you be OK with doing this in a follow-up? We surely > > should > > > properly specify/document the notification format, but I feel doing that > > > properly is a bit out of scope here. I'll file an issue if you agree. > > > > Well, if adding a README.md here isn't out of scope, documenting it properly > > shouldn't be either. But up to you. > > There are two things we need to document: > 1. The notification.json format > 2. The format of the notification files in the notifications repository > > The latter wouldn't be out of scope here since we're changing the format > slightly, but the former IMHO is (we're not even changing the notification.json > format here), and IMO it's a prerequisite for the latter to make sense. > > Here's the follow-up: https://issues.adblockplus.org/ticket/2676 Fair enough, follow up issue sounds OK to me. https://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sit... File sitescripts/notifications/web/notification.py (right): https://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sit... sitescripts/notifications/web/notification.py:29: selection = random.random() On 2015/06/12 11:02:40, Felix Dahlke wrote: > On 2015/06/03 12:17:15, Sebastian Noack wrote: > > On 2015/06/03 11:09:50, kzar wrote: > > > Nit: `from random import random` and then we can just call `random()`? > > > > 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. > > Same here, not incredibly consistent about it however. Made things more > consistent by not importing parse_qs either. Acknowledged. https://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sit... sitescripts/notifications/web/notification.py:60: response["notifications"] = [notification] On 2015/06/12 11:02:40, Felix Dahlke wrote: > On 2015/06/03 11:09:50, kzar wrote: > > 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. > > 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. Acknowledged. https://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sit... sitescripts/notifications/web/notification.py:69: params = parse_qs(environ.get("QUERY_STRING", "")) On 2015/06/12 11:02:40, Felix Dahlke wrote: > On 2015/06/03 12:17:15, Sebastian Noack wrote: > > On 2015/06/03 11:09:50, kzar wrote: > > > 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". > > > > 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. > > No particularly strong opinion, but I also think the way it is currently it's > actually a bit more straightforward, for just one parameter. Acknowledged. https://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sit... sitescripts/notifications/web/notification.py:74: groups = [] On 2015/06/12 12:12:52, Felix Dahlke wrote: > On 2015/06/12 11:15:09, Sebastian Noack wrote: > > On 2015/06/12 11:02:40, Felix H. Dahlke wrote: > > > groups = _determine_groups(notifications, version) > > > if not groups: > > > groups = _assign_groups(notifications) > > > > I'd vote for this one. > > I prefer that one too, went for it. Let me know what you think Dave. Sounds good to me
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
