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

Issue 29327106: Issue 3048 - Mark inactive notifications instead of removing them (Closed)

Created:
Sept. 10, 2015, 11:16 a.m. by Felix Dahlke
Modified:
Sept. 11, 2015, 1:09 p.m.
Reviewers:
Sebastian Noack
CC:
Wladimir Palant
Visibility:
Public.

Description

Issue 3048 - Mark inactive notifications instead of removing them

Patch Set 1 #

Patch Set 2 : #

Total comments: 4

Patch Set 3 : Simplify range selection and inactive check #

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

Messages

Total messages: 4
Felix Dahlke
Sept. 10, 2015, 11:38 a.m. (2015-09-10 11:38:40 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29327106/diff/29327112/sitescripts/notifications/parser.py File sitescripts/notifications/parser.py (right): https://codereview.adblockplus.org/29327106/diff/29327112/sitescripts/notifications/parser.py#newcode126 sitescripts/notifications/parser.py:126: (end is not None and current_time > end)): I ...
Sept. 10, 2015, 1:29 p.m. (2015-09-10 13:29:20 UTC) #2
Felix Dahlke
New patch set is up. Do you have other ideas on how to solve the ...
Sept. 11, 2015, 7:04 a.m. (2015-09-11 07:04:43 UTC) #3
Sebastian Noack
Sept. 11, 2015, 11:02 a.m. (2015-09-11 11:02:24 UTC) #4
On 2015/09/11 07:04:43, Felix Dahlke wrote:
> Do you have other ideas on how to solve the problem I'm trying to solve here?
I
> hate the idea of further complicating this code, but couldn't think of another
> way to do it.

Not really, right now. LGTM.

Powered by Google App Engine
This is Rietveld