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

Issue 29570562: Issue 5827 - Assign new groups even if prior groups are present (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 1 month ago by wspee
Modified:
2 years, 1 month ago
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 11

Patch Set 2 : Revert back to mocking load_notification #

Total comments: 3

Patch Set 3 : Removed return to make in-place editing more apparent #

Unified diffs Side-by-side diffs Delta from patch set Stats (+18 lines, -5 lines) Patch
M sitescripts/notifications/test/notification.py View 1 1 chunk +14 lines, -0 lines 0 comments Download
M sitescripts/notifications/web/notification.py View 1 2 3 chunks +4 lines, -5 lines 0 comments Download

Messages

Total messages: 8
wspee
https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notifications/test/notification.py File sitescripts/notifications/test/notification.py (left): https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notifications/test/notification.py#oldcode136 sitescripts/notifications/test/notification.py:136: def test_notification_variant_merged(self): This "merging" is not possible with the ...
2 years, 1 month ago (2017-10-09 09:12:03 UTC) #1
Vasily Kuznetsov
Hi Winsley, Sorry if you spent much time converting the tests in `test/notifications.py` to use ...
2 years, 1 month ago (2017-10-09 22:29:12 UTC) #2
wspee
https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notifications/test/notification.py File sitescripts/notifications/test/notification.py (right): https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notifications/test/notification.py#newcode35 sitescripts/notifications/test/notification.py:35: self.repo_dir = tempfile.mkdtemp('md_repo') On 2017/10/09 22:29:11, Vasily Kuznetsov wrote: ...
2 years, 1 month ago (2017-10-10 09:03:53 UTC) #3
Vasily Kuznetsov
LGTM
2 years, 1 month ago (2017-10-10 13:41:40 UTC) #4
Felix Dahlke
LGTM with a nit. https://codereview.adblockplus.org/29570562/diff/29572579/sitescripts/notifications/web/notification.py File sitescripts/notifications/web/notification.py (right): https://codereview.adblockplus.org/29570562/diff/29572579/sitescripts/notifications/web/notification.py#newcode55 sitescripts/notifications/web/notification.py:55: return groups Nit: The way ...
2 years, 1 month ago (2017-10-10 14:10:01 UTC) #5
Vasily Kuznetsov
https://codereview.adblockplus.org/29570562/diff/29572579/sitescripts/notifications/web/notification.py File sitescripts/notifications/web/notification.py (right): https://codereview.adblockplus.org/29570562/diff/29572579/sitescripts/notifications/web/notification.py#newcode55 sitescripts/notifications/web/notification.py:55: return groups On 2017/10/10 14:10:01, Felix Dahlke wrote: > ...
2 years, 1 month ago (2017-10-10 14:17:18 UTC) #6
wspee
https://codereview.adblockplus.org/29570562/diff/29572579/sitescripts/notifications/web/notification.py File sitescripts/notifications/web/notification.py (right): https://codereview.adblockplus.org/29570562/diff/29572579/sitescripts/notifications/web/notification.py#newcode55 sitescripts/notifications/web/notification.py:55: return groups On 2017/10/10 14:17:18, Vasily Kuznetsov wrote: > ...
2 years, 1 month ago (2017-10-10 14:46:50 UTC) #7
Felix Dahlke
2 years, 1 month ago (2017-10-10 14:52:23 UTC) #8
LGTM
Sign in to reply to this message.

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