Issue 29325919: Issue 2982 - Return all notifications that have title and message

Issue 29325919: Issue 2982 - Return all notifications that have title and message (Closed)

Created:
Sept. 4, 2015, 8:57 a.m. by Felix Dahlke

Modified:
Sept. 11, 2015, 1:09 p.m.

Reviewers:
Sebastian Noack

CC:
mathias, Wladimir Palant

Visibility:
Public.

Description

Issue 2982 - Return all notifications that have title and message This means we will: 1. Not return any notifications that lack title or message. 2. Return notifications for group "0" in tests, as long as they have title and message.

Patch Set 1 : #

Total comments: 12

Patch Set 2 : Address comments #

Created: Sept. 7, 2015, 7:51 a.m.

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+114 lines, -107 lines)			Patch
	M	sitescripts/notifications/test/notification.py	View	1	13 chunks	+95 lines, -100 lines	0 comments	Download
	M	sitescripts/notifications/web/notification.py	View	1	1 chunk	+19 lines, -7 lines	0 comments	Download

Messages

Total messages: 6

Expand All Messages | Collapse All Messages

Felix Dahlke

https://codereview.adblockplus.org/29325919/diff/29325923/sitescripts/notifications/test/notification.py File sitescripts/notifications/test/notification.py (right): https://codereview.adblockplus.org/29325919/diff/29325923/sitescripts/notifications/test/notification.py#newcode34 sitescripts/notifications/test/notification.py:34: {"id": "1", "title": {"en-US": ""}, "message": {"en-US": ""}} Adding ...

Sept. 4, 2015, 9:08 a.m. (2015-09-04 09:08:40 UTC) #1

Wladimir Palant

https://codereview.adblockplus.org/29325919/diff/29325923/sitescripts/notifications/web/notification.py File sitescripts/notifications/web/notification.py (right): https://codereview.adblockplus.org/29325919/diff/29325923/sitescripts/notifications/web/notification.py#newcode84 sitescripts/notifications/web/notification.py:84: return _can_be_shown(active_variant) and [active_variant] or [] Yay on obfuscation! ...

Sept. 4, 2015, 11:17 a.m. (2015-09-04 11:17:05 UTC) #2

Sebastian Noack

Sept. 4, 2015, 11:38 a.m. (2015-09-04 11:38:12 UTC) #3

Sebastian Noack

https://codereview.adblockplus.org/29325919/diff/29325923/sitescripts/notifications/web/notification.py File sitescripts/notifications/web/notification.py (right): https://codereview.adblockplus.org/29325919/diff/29325923/sitescripts/notifications/web/notification.py#newcode72 sitescripts/notifications/web/notification.py:72: return "title" in notification and "en-US" in notification["title"] and ...

Sept. 4, 2015, 11:47 a.m. (2015-09-04 11:47:36 UTC) #4

Felix Dahlke

New patch set is up https://codereview.adblockplus.org/29325919/diff/29325923/sitescripts/notifications/test/notification.py File sitescripts/notifications/test/notification.py (right): https://codereview.adblockplus.org/29325919/diff/29325923/sitescripts/notifications/test/notification.py#newcode34 sitescripts/notifications/test/notification.py:34: {"id": "1", "title": {"en-US": ...

Sept. 7, 2015, 7:54 a.m. (2015-09-07 07:54:37 UTC) #5

New patch set is up

https://codereview.adblockplus.org/29325919/diff/29325923/sitescripts/notific...
File sitescripts/notifications/test/notification.py (right):

https://codereview.adblockplus.org/29325919/diff/29325923/sitescripts/notific...
sitescripts/notifications/test/notification.py:34: {"id": "1", "title":
{"en-US": ""}, "message": {"en-US": ""}}
On 2015/09/04 11:38:12, Sebastian Noack wrote:
> On 2015/09/04 09:08:40, Felix Dahlke wrote:
> > Adding title and message this way to each notification is arguably a bit
> > redundant, but somehow adding this behind the scenes made this stuff a lot
> > harder to understand, in my opinion. Suggestions on how to reduce
duplication
> > welcome though.
> 
> DUMMY_TEXTS = {"title": {"en-US": ""}, "message": {"en-US": ""}}
> 
> ...
> 
> self.load_notifications_mock.return_value = [
>   dict(DUMMY_TEXTS, id=1),
>   {
>     "id": "a",
>     "variants": [DUMMY_TEXTS]
>   }
> ]

Now that I've gotten rid of the "en-US" checking, I think the duplication isn't
all that bad anymore, let me know if you disagree.

https://codereview.adblockplus.org/29325919/diff/29325923/sitescripts/notific...
File sitescripts/notifications/web/notification.py (right):

https://codereview.adblockplus.org/29325919/diff/29325923/sitescripts/notific...
sitescripts/notifications/web/notification.py:72: return "title" in notification
and "en-US" in notification["title"] and \
On 2015/09/04 11:47:36, Sebastian Noack wrote:
> I just realized: What is if we show a notification only to users of a
particular
> language in the future? It wouldn't make sense to require an English
translation
> then.
> 
> So probably we should check whether title and message are given in all
targeted
> languages, if any. Only requiring the default language if there are no
targeted
> languages or if not all of them have translations.

Well, we don't support that yet, so I can't really check for that. Still, I
think I'm already going too far in duplicating the language fallback logic here.
This wasn't meant as a means of checking notifications for validity, it was
meant to check whether a notification _could_ be shown. So I'm just checking for
the presence of title and message now, that should suffice.

https://codereview.adblockplus.org/29325919/diff/29325923/sitescripts/notific...
sitescripts/notifications/web/notification.py:84: return
_can_be_shown(active_variant) and [active_variant] or []
On 2015/09/04 11:17:04, Wladimir Palant wrote:
> Yay on obfuscation!
> 
>   return [active_variant] if _can_be_shown(active_variant) else []

Well, it was the old way to do it, but I really have to drop that habit one of
these days :P Done.

https://codereview.adblockplus.org/29325919/diff/29325923/sitescripts/notific...
sitescripts/notifications/web/notification.py:92: notification.pop("variants",
None)
On 2015/09/04 11:38:12, Sebastian Noack wrote:
> On 2015/09/04 11:17:04, Wladimir Palant wrote:
> > Second parameter is unnecessary here, you already know that "variants" is in
> > there.
> 
> If this is true, the del statement would be more appropriate to use here.

Done.

LGTM

Expand All Messages | Collapse All Messages