Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(14)

Issue 4883267715072000: Issue 2276 - Handle groups in notification.json requests (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 7 months ago by Felix Dahlke
Modified:
4 years, 5 months ago
Reviewers:
kzar, Sebastian Noack
CC:
Wladimir Palant
Visibility:
Public.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+415 lines, --2 lines) Patch
A sitescripts/notifications/README.md View 1 chunk +11 lines, -0 lines 0 comments Download
A sitescripts/notifications/test/__init__.py View 0 chunks +-1 lines, --1 lines 0 comments Download
A sitescripts/notifications/test/notification.py View 1 2 3 4 1 chunk +305 lines, -0 lines 0 comments Download
A sitescripts/notifications/web/__init__.py View 0 chunks +-1 lines, --1 lines 0 comments Download
A sitescripts/notifications/web/notification.py View 1 2 3 4 5 6 1 chunk +101 lines, -0 lines 0 comments Download

Messages

Total messages: 13
Felix Dahlke
Here's my first go at this. I think there's some room for improvement but I ...
4 years, 7 months ago (2015-04-06 22:08:51 UTC) #1
Felix Dahlke
New patch set is up, refactored this a bit based on Sebastian's feedback in http://codereview.adblockplus.org/6582297721569280/.
4 years, 7 months ago (2015-04-17 03:01:46 UTC) #2
Sebastian Noack
http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/sitescripts/notifications/test/notification.py File sitescripts/notifications/test/notification.py (right): http://codereview.adblockplus.org/4883267715072000/diff/5689792285114368/sitescripts/notifications/test/notification.py#newcode86 sitescripts/notifications/test/notification.py:86: notification.load_notifications = lambda: [ You should restore the orginal ...
4 years, 7 months ago (2015-04-20 14:44:39 UTC) #3
Felix Dahlke
Addressed all comments, new patch set is up. However, I'm still somewhat inclined to refactor ...
4 years, 6 months ago (2015-04-23 22:15:18 UTC) #4
Sebastian Noack
LGTM On 2015/04/23 22:15:18, Felix H. Dahlke wrote: > Addressed all comments, new patch set ...
4 years, 6 months ago (2015-04-24 10:31:56 UTC) #5
Felix Dahlke
Tried to simplify the logic a bit, however, didn't really end up being simpler IMHO. ...
4 years, 6 months ago (2015-04-26 20:33:02 UTC) #6
kzar
Some feedback as requested, feel free to ignore me as the notification system is completely ...
4 years, 5 months ago (2015-06-03 11:09:49 UTC) #7
Sebastian Noack
http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sitescripts/notifications/web/notification.py File sitescripts/notifications/web/notification.py (right): http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sitescripts/notifications/web/notification.py#newcode29 sitescripts/notifications/web/notification.py:29: selection = random.random() On 2015/06/03 11:09:50, kzar wrote: > ...
4 years, 5 months ago (2015-06-03 12:17:14 UTC) #8
Felix Dahlke
Thanks for having a look, new patch set is up. http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sitescripts/notifications/README.md File sitescripts/notifications/README.md (right): http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sitescripts/notifications/README.md#newcode7 ...
4 years, 5 months ago (2015-06-12 11:02:40 UTC) #9
Sebastian Noack
http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sitescripts/notifications/README.md File sitescripts/notifications/README.md (right): http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sitescripts/notifications/README.md#newcode7 sitescripts/notifications/README.md:7: On 2015/06/12 11:02:40, Felix H. Dahlke wrote: > On ...
4 years, 5 months ago (2015-06-12 11:15:09 UTC) #10
Felix Dahlke
New patch set is up. http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sitescripts/notifications/README.md File sitescripts/notifications/README.md (right): http://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sitescripts/notifications/README.md#newcode7 sitescripts/notifications/README.md:7: On 2015/06/12 11:15:09, Sebastian ...
4 years, 5 months ago (2015-06-12 12:12:52 UTC) #11
kzar
LGTM https://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sitescripts/notifications/README.md File sitescripts/notifications/README.md (right): https://codereview.adblockplus.org/4883267715072000/diff/5661458385862656/sitescripts/notifications/README.md#newcode7 sitescripts/notifications/README.md:7: On 2015/06/12 12:12:52, Felix Dahlke wrote: > On ...
4 years, 5 months ago (2015-06-15 15:17:39 UTC) #12
Sebastian Noack
4 years, 5 months ago (2015-06-15 15:37:45 UTC) #13
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5