widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Approve

Some comments while testing:

- On http://alpha.widelands.org/notification/, the maps heading says Wlmaps. That should probably just read Maps.
- I sent myself a PM and got an email immediately.
- I subscribed to the forum and created a new topic.
- When uploading a map I got a 504 gateway timeout the first time. When trying again I got an "Internal server error" after a while, but the map was apparently successful uploaded.

After activating the alpha repo and running /var/www/django_projects/alpha/wlwebsite/code/widelands/manage.py emit_notices I got the two emails I was expecting (map upload and new topic). So it works! Great :)

[pickling]
I think the warning is fine and can be ignored here. Pickling is a way to serialize (i.e. convert to a string representation) a living Python object so that it can be stored somewhere. We store the notices in some sort of on-disk queue for later sending as email (in emit_notices). The design seems interesting, I would expect to save the data into the database using djangos normal means, but apparently another approach was taken here. 

The potential problem is that if we update django, the models living representation might change, since the code changed. Unpickling from this queue will then give half-correct representations that will cause problems. However, this is easily avoided by making sure that the emit queue is empty before we update anything. This should be the case after emit_notices runs successfully.

Code lgtm - 2 nits inlined.

Diff comments:

> 
> === modified file 'notification/models.py'
> --- notification/models.py	2016-12-13 18:28:51 +0000
> +++ notification/models.py	2017-05-01 10:49:01 +0000
> @@ -91,80 +101,17 @@
>          setting.save()
>          return setting
>  
> -
>  def should_send(user, notice_type, medium):
>      return get_notification_setting(user, notice_type, medium).send
>  
> -
> -class NoticeManager(models.Manager):
> -
> -    def notices_for(self, user, archived=False, unseen=None, on_site=None):
> -        """returns Notice objects for the given user.
> -
> -        If archived=False, it only include notices not archived.
> -        If archived=True, it returns all notices for that user.
> -
> -        If unseen=None, it includes all notices.
> -        If unseen=True, return only unseen notices.
> -        If unseen=False, return only seen notices.
> -
> -        """
> -        if archived:
> -            qs = self.filter(user=user)
> -        else:
> -            qs = self.filter(user=user, archived=archived)
> -        if unseen is not None:
> -            qs = qs.filter(unseen=unseen)
> -        if on_site is not None:
> -            qs = qs.filter(on_site=on_site)
> -        return qs
> -
> -    def unseen_count_for(self, user, **kwargs):
> -        """returns the number of unseen notices for the given user but does not
> -        mark them seen."""
> -        return self.notices_for(user, unseen=True, **kwargs).count()
> -
> -
> -class Notice(models.Model):
> -
> -    user = models.ForeignKey(User, verbose_name=_('user'))
> -    message = models.TextField(_('message'))
> -    notice_type = models.ForeignKey(NoticeType, verbose_name=_('notice type'))
> -    added = models.DateTimeField(_('added'), default=datetime.datetime.now)
> -    unseen = models.BooleanField(_('unseen'), default=True)
> -    archived = models.BooleanField(_('archived'), default=False)
> -    on_site = models.BooleanField(_('on site'))
> -
> -    objects = NoticeManager()
> -
> -    def __unicode__(self):
> -        return self.message
> -
> -    def archive(self):
> -        self.archived = True
> -        self.save()
> -
> -    def is_unseen(self):
> -        """returns value of self.unseen but also changes it to false.
> -
> -        Use this in a template to mark an unseen notice differently the
> -        first time it is shown.
> -
> -        """
> -        unseen = self.unseen
> -        if unseen:
> -            self.unseen = False
> -            self.save()
> -        return unseen
> -
> -    class Meta:
> -        ordering = ['-added']
> -        verbose_name = _('notice')
> -        verbose_name_plural = _('notices')
> -
> -    def get_absolute_url(self):
> -        return ('notification_notice', [str(self.pk)])
> -    get_absolute_url = models.permalink(get_absolute_url)
> +def get_observers_for(notice_type):
> +    """ Returns the list of users which wants to get a message (email) for this
> +    type of notice."""
> +    settings = NoticeSetting.objects.filter(notice_type__label=notice_type).filter(send=True)

return [s.user for s in NoticeSetting.objects.filter(notice_type__label=notice_type).filter(send=True)]

is a bit shorter.

> +    users = []
> +    for s in settings:
> +        users.append(s.user)
> +    return users
>  
>  
>  class NoticeQueueBatch(models.Model):
> @@ -262,66 +209,65 @@
>      if extra_context is None:
>          extra_context = {}
>  
> -    notice_type = NoticeType.objects.get(label=label)
> -
> -    current_site = Site.objects.get_current()
> -    notices_url = u"http://%s%s"; % (
> -        unicode(current_site),
> -        reverse('notification_notices'),
> -    )
> -
> -    current_language = get_language()
> -
> -    formats = (
> -        'short.txt',
> -        'full.txt',
> -        'notice.html',
> -        'full.html',
> -    )  # TODO make formats configurable
> -
> -    for user in users:
> -        recipients = []
> -        # get user language for user from language store defined in
> -        # NOTIFICATION_LANGUAGE_MODULE setting
> -        try:
> -            language = get_notification_language(user)
> -        except LanguageStoreNotAvailable:
> -            language = None
> -
> -        if language is not None:
> -            # activate the user's language
> -            activate(language)
> -
> -        # update context with user specific translations
> -        context = Context({
> -            'user': user,
> -            'notice': ugettext(notice_type.display),
> -            'notices_url': notices_url,
> -            'current_site': current_site,
> -        })
> -        context.update(extra_context)
> -
> -        # get prerendered format messages
> -        messages = get_formatted_messages(formats, label, context)
> -
> -        # Strip newlines from subject
> -        subject = ''.join(render_to_string('notification/email_subject.txt', {
> -            'message': messages['short.txt'],
> -        }, context).splitlines())
> -
> -        body = render_to_string('notification/email_body.txt', {
> -            'message': messages['full.txt'],
> -        }, context)
> -
> -        notice = Notice.objects.create(user=user, message=messages['notice.html'],
> -                                       notice_type=notice_type, on_site=on_site)
> -        if should_send(user, notice_type, '1') and user.email:  # Email
> -            recipients.append(user.email)
> -        send_mail(subject, body, settings.DEFAULT_FROM_EMAIL, recipients)
> -
> -    # reset environment to original language
> -    activate(current_language)
> -
> +    # FrankU: This try statement is added to pass notice types

Could you add an example here which app needs this?

> +    # which are deleted but used by third party apps to create a notice
> +    try:
> +        notice_type = NoticeType.objects.get(label=label)
> +
> +        current_site = Site.objects.get_current()
> +        notices_url = u"http://%s%s"; % (
> +            unicode(current_site),
> +            reverse('notification_notices'),
> +        )
> +
> +        current_language = get_language()
> +
> +        formats = (
> +            'short.txt',
> +            'full.txt',
> +        )  # TODO make formats configurable
> +
> +        for user in users:
> +            recipients = []
> +            # get user language for user from language store defined in
> +            # NOTIFICATION_LANGUAGE_MODULE setting
> +            try:
> +                language = get_notification_language(user)
> +            except LanguageStoreNotAvailable:
> +                language = None
> +
> +            if language is not None:
> +                # activate the user's language
> +                activate(language)
> +
> +            # update context with user specific translations
> +            context = Context({
> +                'user': user,
> +                'notices_url': notices_url,
> +                'current_site': current_site,
> +            })
> +            context.update(extra_context)
> +
> +            # get prerendered format messages
> +            messages = get_formatted_messages(formats, label, context)
> +
> +            # Strip newlines from subject
> +            subject = ''.join(render_to_string('notification/email_subject.txt', {
> +                'message': messages['short.txt'],
> +            }, context).splitlines())
> +
> +            body = render_to_string('notification/email_body.txt', {
> +                'message': messages['full.txt'],
> +            }, context)
> +
> +            if should_send(user, notice_type, '1') and user.email:  # Email
> +                recipients.append(user.email)
> +            send_mail(subject, body, settings.DEFAULT_FROM_EMAIL, recipients)
> +
> +        # reset environment to original language
> +        activate(current_language)
> +    except NoticeType.DoesNotExist:
> +        pass
>  
>  def send(*args, **kwargs):
>      """A basic interface around both queue and send_now.


-- 
https://code.launchpad.net/~widelands-dev/widelands-website/notifications_cleanup/+merge/323457
Your team Widelands Developers is subscribed to branch lp:widelands-website.