Create notification messages

The notification mail message is built using the received information from the signal item_modified.

The class is used primarily for encapsulating data.

[Thomas.J.Waldmann, 2013-08-19 21:41:10]

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/_tests/test_noti...
File MoinMoin/util/_tests/test_notifications.py (right):

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/_tests/test_noti...
MoinMoin/util/_tests/test_notifications.py:50: notification.revs = [rev2, rev1]
this is dealing with the implementation of Notification, you should rather
create a new N. via the external api.

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/_tests/test_noti...
MoinMoin/util/_tests/test_notifications.py:54: notification.action =
ACTION_TRASH
same here.

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/_tests/test_noti...
MoinMoin/util/_tests/test_notifications.py:74: assert
notification.get_meta_diff() == dict_diff(dict(), rev1.meta._meta)
heh, it is the same because you call the same fn there, right?

if that fn computes a wrong result, it will ever be the same wrong in both
places.

so, to make sure it works, do you have dict_diff tests?

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/notifications.py
File MoinMoin/util/notifications.py (right):

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/notifications.py...
MoinMoin/util/notifications.py:59: DESTROY_ALL: L_("has been destroyed"),
that is unlikely to work for all languages.

you are assuming that all languages have the same way / order to build the
complete sentence and it is not flexible to cope with other ways/orders.

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/notifications.py...
MoinMoin/util/notifications.py:67: if self.action in (DESTROY_REV, DESTROY_ALL,
):
well, this works, but i rather think of a list in such cases than a tuple.

a list often has elements of same kind. the position has no special meaning
except order. like a list of actions, items, names, ...

a tuple more often has elements of different kinds and position ofter has a
special meaning other than just order. like (x, y, z) giving coordinates in 3
dimensions, like (firstname, lastname, street, zipcode, city)

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/notifications.py...
MoinMoin/util/notifications.py:70: content_diff =
text_diff(self.revs[0].data.readlines(), [])
where do you check that it really IS text?

should the diff computation rather be a method of the specific item class, like
we already have in MoinMoin.items?

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/notifications.py...
MoinMoin/util/notifications.py:90: meta_diff =
dict_diff(self.revs[0].meta._meta, dict())
looks a bit like these 2 cases could get implemented in somehow same way

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/notifications.py...
MoinMoin/util/notifications.py:97: meta_diff = dict_diff(old_meta, new_meta)
even this...

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/notifications.py...
MoinMoin/util/notifications.py:109: revid1 = revid2 = self.revs[0].revid
what does the diff show here?

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/notifications.py...
MoinMoin/util/notifications.py:114: return urljoin(domain, diff_rel_url)
why is that needed?

[ana.balica, 2013-08-21 14:03:26]

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/_tests/test_noti...
File MoinMoin/util/_tests/test_notifications.py (right):

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/_tests/test_noti...
MoinMoin/util/_tests/test_notifications.py:74: assert
notification.get_meta_diff() == dict_diff(dict(), rev1.meta._meta)
On 2013/08/19 21:41:10, Thomas.J.Waldmann wrote:
> so, to make sure it works, do you have dict_diff tests?

Yes, we have dict_diff tests. It's a diff func imported as dict_diff().
https://bitbucket.org/ana-balica/moin-2.0/commits/e3cba78bea697b6fb20930b0db6...

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/notifications.py
File MoinMoin/util/notifications.py (right):

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/notifications.py...
MoinMoin/util/notifications.py:70: content_diff =
text_diff(self.revs[0].data.readlines(), [])
On 2013/08/19 21:41:10, Thomas.J.Waldmann wrote:
> where do you check that it really IS text?

I almost forgot about other Content types.
What about binary, image files? Are we going to show simply the diff url? Or
also include the diffraw image for instance?

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/notifications.py...
MoinMoin/util/notifications.py:109: revid1 = revid2 = self.revs[0].revid
On 2013/08/19 21:41:10, Thomas.J.Waldmann wrote:
> what does the diff show here?

Hm... nothing. And there is no view that can show a diff of a new item (a single
revision).
What URL than should we provide? Or none?

https://codereview.appspot.com/12745048/diff/1/MoinMoin/util/notifications.py...
MoinMoin/util/notifications.py:114: return urljoin(domain, diff_rel_url)
On 2013/08/19 21:41:10, Thomas.J.Waldmann wrote:
> why is that needed?

What exactly? The whole point of the function? urljoining?

[Thomas.J.Waldmann, 2013-08-22 15:59:19]

https://codereview.appspot.com/12745048/diff/15002/MoinMoin/util/_tests/test_...
File MoinMoin/util/_tests/test_notifications.py (right):

https://codereview.appspot.com/12745048/diff/15002/MoinMoin/util/_tests/test_...
MoinMoin/util/_tests/test_notifications.py:59: notification = Notification(app,
self.item_name, [], content=item.content, action=DESTROY_REV)
hmm, so the destroyed metadata can't be seen there, just the data (content)?

[ana.balica, 2013-08-22 21:31:28]

https://codereview.appspot.com/12745048/diff/15002/MoinMoin/util/_tests/test_...
File MoinMoin/util/_tests/test_notifications.py (right):

https://codereview.appspot.com/12745048/diff/15002/MoinMoin/util/_tests/test_...
MoinMoin/util/_tests/test_notifications.py:59: notification = Notification(app,
self.item_name, [], content=item.content, action=DESTROY_REV)
On 2013/08/22 15:59:20, Thomas.J.Waldmann wrote:
> hmm, so the destroyed metadata can't be seen there, just the data (content)?

Metadata can be seen if we pass it in the arguments. Since i don't need it for
testing the content diff, i just skipped it.

[waldi, 2013-09-01 13:47:22]

https://codereview.appspot.com/12745048/diff/34001/MoinMoin/util/notification...
File MoinMoin/util/notifications.py (right):

https://codereview.appspot.com/12745048/diff/34001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:66: user_name=user_name),
Use "**kw" to do the same multiple times.

https://codereview.appspot.com/12745048/diff/34001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:225: subscriber.itemid != u.itemid]
Why do you need a list?

https://codereview.appspot.com/12745048/diff/34001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:226: subscribers_locale = [subscriber.locale for
subscriber in subscribers]
Same.

[Thomas.J.Waldmann, 2013-09-01 14:10:52]

https://codereview.appspot.com/12745048/diff/37001/MoinMoin/util/notification...
File MoinMoin/util/notifications.py (right):

https://codereview.appspot.com/12745048/diff/37001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:87: }
can you try wether something like this works:

def msgs():
    _ = lambda x: x
    messages = dict(
        ACTION_CREATE: _("The '%(item_name)s' item on '%(wiki_name)s' has been
created by %(user_name)s:"),
        ...
    )
    return messages

MESSAGES = msgs()

later use: L_(MESSAGES[key], ...)

or even (if that works): L_(MESSAGES[key], **env)
and first put all that could be needed into env dict.

[Thomas.J.Waldmann, 2013-09-02 09:48:19]

https://codereview.appspot.com/12745048/diff/44001/MoinMoin/util/notification...
File MoinMoin/util/notifications.py (right):

https://codereview.appspot.com/12745048/diff/44001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:46: "The '%(item_name)s' item on '%(wiki_name)s'
has been created by %(user_name)s:"),
I would prefer if each entry was just one line

https://codereview.appspot.com/12745048/diff/44001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:223: for locale in subscribers_locale:
if get_subcribers would return a dict LOCALE -> list/set of subscribers having
that locale, this code here would just iterate over this and not need further
processing.

[waldi, 2013-09-02 12:22:27]

https://codereview.appspot.com/12745048/diff/44001/MoinMoin/util/notification...
File MoinMoin/util/notifications.py (right):

https://codereview.appspot.com/12745048/diff/44001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:98: if self.action in [DESTROY_REV, DESTROY_ALL,
]:
Why list (again)?

https://codereview.appspot.com/12745048/diff/44001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:109: content_diff = [line.strip() for line in
content_diff]
What is the reason that you always use list comprehensions instead of a
generator?

https://codereview.appspot.com/12745048/diff/44001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:207: action = kwargs.get('action', None)
Why? "action=None" as parameter does not work?

https://codereview.appspot.com/12745048/diff/44001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:209: DESTROY_REV, DESTROY_ALL, ] else []
Why lists again?

https://codereview.appspot.com/12745048/diff/44001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:222: subscribers_locale = {subscriber.locale for
subscriber in subscribers}
Why do you need a set here?

[ana.balica, 2013-09-02 15:22:30]

https://codereview.appspot.com/12745048/diff/44001/MoinMoin/util/notification...
File MoinMoin/util/notifications.py (right):

https://codereview.appspot.com/12745048/diff/44001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:98: if self.action in [DESTROY_REV, DESTROY_ALL,
]:
On 2013/09/02 12:22:28, waldi wrote:
> Why list (again)?

The list-tuple argument was raised several times. Please see this cr line 67.

https://codereview.appspot.com/12745048/diff2/1:8001/MoinMoin/util/notificati...

https://codereview.appspot.com/12745048/diff/44001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:109: content_diff = [line.strip() for line in
content_diff]
On 2013/09/02 12:22:28, waldi wrote:
> What is the reason that you always use list comprehensions instead of a
> generator?

Because jinja2 doesn't support iterating over generators. I could create here a
generator, but then i need to cast it to list anyway, since i use it to create
the content diff in the mail templates.

https://codereview.appspot.com/12745048/diff/44001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:222: subscribers_locale = {subscriber.locale for
subscriber in subscribers}
On 2013/09/02 12:22:28, waldi wrote:
> Why do you need a set here?

To eliminate duplication.

https://codereview.appspot.com/12745048/diff/44001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:223: for locale in subscribers_locale:
On 2013/09/02 09:48:19, Thomas.J.Waldmann wrote:
> if get_subcribers would return a dict LOCALE -> list/set of subscribers having
> that locale, this code here would just iterate over this and not need further
> processing.

Good idea, but at the same time that makes the function output very specific
that fits our current needs. It might become inconvenient for some other tasks
which may appear in the future.

[Thomas.J.Waldmann, 2013-09-02 17:15:03]

yeah, maybe true, just was thinking about that it might be a little more
efficient.

but a set of namedtuples is easier...

[Thomas.J.Waldmann, 2013-09-11 16:16:23]

https://codereview.appspot.com/12745048/diff/64001/MoinMoin/util/notification...
File MoinMoin/util/notifications.py (right):

https://codereview.appspot.com/12745048/diff/64001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:93: oldfile, newfile = self.content,
StringIO(u"")
why StringIO ?
self.content is what type?

https://codereview.appspot.com/12745048/diff/64001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:94: elif self.action == ACTION_TRASH:
maybe not your fault, but i notice some inconsistency in naming between
DESTROY_REV/ALL and ACTION_TRASH...

https://codereview.appspot.com/12745048/diff/64001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:96: oldfile, newfile = self.revs[0].data,
StringIO(u"")
why StringIO?
self.revs[0].data is what type?

https://codereview.appspot.com/12745048/diff/64001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:101: oldfile = StringIO(u"")
same question

https://codereview.appspot.com/12745048/diff/64001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:107: item = Item.create(self.item_name,
contenttype=contenttype)
why do you create a new item here?

https://codereview.appspot.com/12745048/diff/64001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:109: return [line.strip() for line in difflines]
why is the strip() needed?

https://codereview.appspot.com/12745048/diff/64001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:212: meta = kwargs.get('meta') if action in
[DESTROY_REV, DESTROY_ALL, ] else revs[0].meta._meta
sometimes, you access rev.meta, sometimes rev.meta._meta (which rather looks
internal) - why?

[ana.balica, 2013-09-13 16:23:57]

https://codereview.appspot.com/12745048/diff/64001/MoinMoin/util/notification...
File MoinMoin/util/notifications.py (right):

https://codereview.appspot.com/12745048/diff/64001/MoinMoin/util/notification...
MoinMoin/util/notifications.py:94: elif self.action == ACTION_TRASH:
On 2013/09/11 16:16:24, Thomas.J.Waldmann wrote:
> maybe not your fault, but i notice some inconsistency in naming between
> DESTROY_REV/ALL and ACTION_TRASH...

Yes, there is an inconsistency. The actions' list (keywords marked with ACTION_
) were matched together, since they are ACTION meta values.
DESTROY_REV/ALL are not ACTIONs in that sense...

[Thomas.J.Waldmann, 2013-09-14 17:47:24]

looks better.